On 5/9/22 20:12, Thomas Zimmermann wrote:

[snip]

quoted

quoted

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.

I see, that makes sense. Then something like the following instead?

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d265a73313c9..ce0d89c49e42 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c

@@ -627,12 +627,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
         cancel_work_sync(&fb_helper->resume_work);
         cancel_work_sync(&fb_helper->damage_work);
  
-       info = fb_helper->fbdev;
-       if (info) {
-               if (info->cmap.len)
-                       fb_dealloc_cmap(&info->cmap);
-               framebuffer_release(info);
-       }
         fb_helper->fbdev = NULL;
  
         mutex_lock(&kernel_fb_helper_lock);

@@ -2111,7 +2105,11 @@ static void drm_fbdev_release(struct drm_fb_helper *fb_helper)
   */
  static void drm_fbdev_fb_destroy(struct fb_info *info)
  {
+       if (info->cmap.len)
+               fb_dealloc_cmap(&info->cmap);
+
         drm_fbdev_release(info->par);
+       framebuffer_release(info);