Hi Javier Am 09.05.22 um 18:33 schrieb Javier Martinez Canillas:
On 5/9/22 17:51, Andrzej Hajda wrote: [snip]+Regarding drm: What about drm_fb_helper_fini? It calls also framebuffer_release and is called often from _remove paths (checked intel/radeon/nouveau). I guess it should be fixed as well. Do you plan to fix it?I think you are correct. Maybe we need something like the following? diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index d265a73313c9..b09598f7af28 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -631,7 +631,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper) if (info) { if (info->cmap.len) fb_dealloc_cmap(&info->cmap); - framebuffer_release(info);What about cmap? I am not an fb expert, but IMO cmap can be accessed from userspace as well.I actually thought about the same but then remembered what Daniel said in [0] (AFAIU at least) that these should be done in .remove() so the current code looks like matches that and only framebuffer_release() should be moved. For vesafb a previous patch proposed to also move a release_region() call to .fb_destroy() and Daniel also said that it was iffy and shouldn't be done [1]. But I'm also not fb expert so happy to move fb_dealloc_cmap() as well if that is the correct thing to do.
The cmap data structure is software state that can be accessed via icotl as long as the devfile is open. Drivers update the hardware from it. See [1]. Moving that cleanup into fb_destroy seems appropriate to me.
Best regards Thomas[1] https://elixir.bootlin.com/linux/v5.17.6/source/drivers/video/fbdev/core/fbcmap.c#L231
[0]: https://www.spinics.net/lists/dri-devel/msg346257.html [1]: https://www.spinics.net/lists/dri-devel/msg346261.html
-- 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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
