[Intel-gfx] [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()

[Thomas Zimmermann]

Hi

Am 21.02.23 um 11:27 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann at suse.de> writes:
> 
>> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
>> calling fbdev implementation. Avoids a possible stale mutex with
>> generic fbdev code.
>>
>> As indicated by its name, drm_fb_helper_prepare() prepares struct
>> drm_fb_helper before setting up the fbdev support with a call to
>> drm_fb_helper_init(). In legacy fbdev emulation, this happens next
>> to each other. If successful, drm_fb_helper_fini() later tear down
>> the fbdev device and also unprepare via drm_fb_helper_unprepare().
>>
>> Generic fbdev emulation prepares struct drm_fb_helper immediately
>> after allocating the instance. It only calls drm_fb_helper_init()
>> as part of processing a hotplug event. If the hotplug-handling fails,
>> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
>> and the next hotplug event runs on stale data.
>>
>> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
>> into the fbdev implementations. Call it right before freeing the
>> fb-helper instance.
>>
>> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
> 
> I think this should be Fixes: 032116bbe152 ("drm/fbdev-generic: Minimize
> client unregistering") instead? Because commit 4825797c36da just added a
> wrapper function for mutex_destroy(&fb_helper->lock), but it was commit
> 032116bbe152 that made drm_fbdev_cleanup() to call that helper function.

Good point. After looking through the recent fbdev commits, I'll use 
commit 643231b28380 ("drm/fbdev-generic: Minimize hotplug error 
handling") for the tag. This is the one that added the call to 
drm_fb_helper_fini() to the client's hotplug handler. And _fini() 
currently does the _unprepare(), when it shouldn't.

> 
>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Javier Martinez Canillas <javierm at redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Maxime Ripard <mripard at kernel.org>
>> Cc: David Airlie <airlied at gmail.com>
>> Cc: Daniel Vetter <daniel at ffwll.ch>
>> Cc: dri-devel at lists.freedesktop.org
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
> 
> The change itself looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
> 

Thanks a lot.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20230221/d4db2154/attachment.sig>