Message-ID: <CAKMK7uFofJ2f4ozx=QMkfYHLAy0euOf6S0wou5qYC--QiEe5Qw@mail.gmail.com>
Date:   Wed, 18 Nov 2020 11:23:40 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     "Jonas Mark (BT-FIR/ENG1-Grb)" <Mark.Jonas@...bosch.com>
Cc:     Thomas Zimmermann <tzimmermann@...e.de>,
        "RUAN Tingquan (BT-FIR/ENG1-Zhu)" <Tingquan.Ruan@...bosch.com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        David Airlie <airlied@...ux.ie>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        NXP Linux Team <linux-imx@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] drm: imx: Move fbdev setup to before output polling

On Wed, Nov 18, 2020 at 10:47 AM Jonas Mark (BT-FIR/ENG1-Grb)
<Mark.Jonas@...bosch.com> wrote:
>
> Hi Thomas and Daniel,
>
> Thank you very much for your feedback. We appreciate it.
>
> > >>> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
> > >>> b/drivers/gpu/drm/imx/imx-drm-core.c
> > >>> index 9bf5ad6d18a2..2665040e11c7 100644
> > >>> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > >>> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > >>> @@ -240,14 +240,18 @@ static int imx_drm_bind(struct device *dev)
> > >>>                legacyfb_depth = 16;
> > >>>        }
> > >>>
> > >>> +     /*
> > >>> +      * The generic fbdev has to be setup before enabling output polling.
> > >>> +      * Otherwise the fbdev client is not ready to handle delayed events.
> > >>> +      */
> > >>> +     drm_fbdev_generic_setup(drm, legacyfb_depth);
> > >>> +
> > >>>        drm_kms_helper_poll_init(drm);
> > >>>
> > >>>        ret = drm_dev_register(drm, 0);
> > >>>        if (ret)
> > >>>                goto err_poll_fini;
> > >>>
> > >>> -     drm_fbdev_generic_setup(drm, legacyfb_depth);
> > >>> -
> > >>
> > >> This does not work well. fbdev is supposed to be another regular DRM
> > >> client. It has to be enabled after registering the DRM device.
> > >>
> > >> I'd rather improve fbdev or the driver to handle this gracefully.
> > >
> > > Yeah I'm not understanding the point here. Once fbcon is running, you
> > > have a screen. Any fbdev userspace client  also should do a modeset
> > > first. And if they dont and it's expected uapi for fbdev chardev that
> > > the display boots up enabled, then we need to fix that in the fbdev
> > > helpers, not through clever reordering in drivers so that a
> > > side-effect causes a modeset.
> > >
> > > Note that this is a bit tricky since fbdev shouldn't take over the
> > > screen by default, so we'd need to delay this until first open of
> > > /dev/fb0. And we should probably also delay the hotplug handling until
> > > the first open. fbcon also fake-opens the fbdev file, so it's the same
> > > code path.
> >
> > As far as I understand the commit message, the problem is that the display
> > blanks out after registering the driver. And fbdev somewhat mitigates this by
> > doing an early modeset. Users with fbdev disabled (most of them in
> > embedded, I guess) would still run into the issue until userspace makes a
> > modeset.
> >
> > Mark, if that's the case, an option might be to pick up the device settings
> > instead of calling drm_mode_config_reset(). The driver would then continue
> > to display whatever is on the screen.
>
> We are started using fbdev in our embedded application with Linux 3.10, later
> updated to 4.14 and are now in the process of updating to 5.4. So far the uapi
> appeared to us as if we could rely on an already enabled fbdev. That is, none
> of our applications does a modeset.
>
> When switching to 5.4 we noticed that the fbdev uapi changed. That is, the
> LCD is switched off until it is explicitly enabled. It could be enabled by
> writing to /sys/class/graphics/fb0/blank.
>
> You are right, we are not using fbcon. fbcon will still enable the LCD but in our
> embedded domain we have it disabled because we must not show a console.
>
> Do we understand your proposal correctly to replace the call to
> drm_mode_config_reset() in imx_drm_bind() [imx-drm-core.c] with picking up
> the device settings instead?
>
> https://elixir.bootlin.com/linux/v5.10-rc4/source/drivers/gpu/drm/imx/imx-drm-core.c#L231
>
> We are a little clueless right now: How do we pick up the device settings?

Nope, not what I had in mind.

Instead intercept the fb_ops->open call and in there if it's a
userspace open (user parameter of the callback tells you that) and kms
is not in use, then try to light up the display for the fbdev
userspace to use. drm fbdev helpers already have that callback as
drm_fbdev_fb_open(). I think you could try and just call
drm_fbdev_client_hotplug directly, that should do the trick. Or maybe
calling drm_fb_helper_dpms is the better option, not sure. fbmem.c
seems to not store any blanking state at all, so this is probably all
ill-defined.

Important part is to do this only for the user fb_open case, since
fbcon will do its own thing too.

Plus I guess we need to document that this is the uapi we're having
for fbdev clients, so ideally this should be cc'ed widely so we can
get some acks from former fbdev maintainers.

Also ideally we'd have an igt for this uapi to make sure it never
breaks again. Something like:
1. open the kms driver for this, make sure display is completely off.
2. close kms file
3. open fbdev file
4. check (through opening kms side again) that the display has been enabled.

See https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#validating-changes-with-igt
for some details on our validation testing, there's already a very
basic fbdev testcase there.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists