linux-kernel - Re: [PATCH] drm: imx: Move fbdev setup to before output polling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68af913c-9f4e-73b5-a2cb-8692902a2847@suse.de>
Date:   Wed, 18 Nov 2020 10:12:52 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Daniel Vetter <daniel@...ll.ch>
Cc:     Leo Ruan <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>,
        Mark Jonas <mark.jonas@...bosch.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

Hi

Am 18.11.20 um 09:59 schrieb Daniel Vetter:
> On Wed, Nov 18, 2020 at 9:10 AM Thomas Zimmermann <tzimmermann@...e.de> wrote:
>>
>> Hi
>>
>> Am 17.11.20 um 16:52 schrieb Mark Jonas:
>>> From: Leo Ruan <tingquan.ruan@...bosch.com>
>>>
>>> The generic fbdev has to be setup before enabling output polling.
>>> Otherwise the fbdev client is not ready to handle delayed events.
>>>
>>> Since f53705fd, the generic fbdev is setup after the output polling
>>> init. During fbdev setup, when fbdev probes attached outputs and a
>>> status changes from unknown to connected, the delayed event is
>>> marked and the output_poll_work thread is scheduled without delay.
>>> If output_poll_execute() is runs immediately, the delayed event
>>> is handled without actually polling the output because the fbdev is not
>>> registered yet. So the delayed event is lost. This leads to a dark
>>> screen until a KMS application (or fbcon) sets the screen mode.
>>>
>>> This patch fixes the issue by moving the setup of generic fbdev before
>>> initializing and enabling output polling.
>>>
>>> Signed-off-by: Leo Ruan <tingquan.ruan@...bosch.com>
>>> Signed-off-by: Mark Jonas <mark.jonas@...bosch.com>
>>> ---
>>>   drivers/gpu/drm/imx/imx-drm-core.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> 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.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>>        return 0;
>>>
>>>   err_poll_fini:
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Download attachment "OpenPGP_0x680DC11D530B7A23.asc" of type "application/pgp-keys" (7436 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)