[PATCH] drm/i915: unbind fbs from crtcs during driver unload

[PATCH] drm/i915: unbind fbs from crtcs during driver unload

[Imre Deak]

So far during driver unload we called drm_framebuffer_cleanup() for the
fbdev fb, which only removes the fb from the drm fb list regardless of
its reference count, but leaves the fb bound on an active crtc. Since
the fb's backing storage was freed this could mean we scan some random
memory content out afterwards. It's not a big issue since the fb is
allocated from stolen memory and afaik there is no other user for that
than i915. It's still cleaner to properly unbind the fb and disable the
crtc, which is what drm_framebuffer_remove() does.

Note that after

commit 88891eb1e9eca0ba619518bed31580f91e9cf84d
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Mon Feb 10 18:00:38 2014 +0100

we call drm_framebuffer_cleanup() only after dropping the last reference
on the fb, but that won't happen since we don't unbind the fb. This
results in a drm core warn about a leaked fb.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index cf46273..3a53ee3 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -265,7 +265,7 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 	drm_fb_helper_fini(&ifbdev->helper);
 
 	drm_framebuffer_unregister_private(&ifbdev->fb->base);
-	drm_framebuffer_unreference(&ifbdev->fb->base);
+	drm_framebuffer_remove(&ifbdev->fb->base);
 }
 
 int intel_fbdev_init(struct drm_device *dev)
-- 
1.8.3.2

Re: [PATCH] drm/i915: unbind fbs from crtcs during driver unload

[Daniel Vetter]

On Wed, Feb 12, 2014 at 12:01:10AM +0200, Imre Deak wrote:
> So far during driver unload we called drm_framebuffer_cleanup() for the
> fbdev fb, which only removes the fb from the drm fb list regardless of
> its reference count, but leaves the fb bound on an active crtc. Since
> the fb's backing storage was freed this could mean we scan some random
> memory content out afterwards. It's not a big issue since the fb is
> allocated from stolen memory and afaik there is no other user for that
> than i915. It's still cleaner to properly unbind the fb and disable the
> crtc, which is what drm_framebuffer_remove() does.
> 
> Note that after
> 
> commit 88891eb1e9eca0ba619518bed31580f91e9cf84d
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Mon Feb 10 18:00:38 2014 +0100
> 
> we call drm_framebuffer_cleanup() only after dropping the last reference
> on the fb, but that won't happen since we don't unbind the fb. This
> results in a drm core warn about a leaked fb.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Yeah, makes more sense. Apparently I really don't know my own fb lifetime
rules :( Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index cf46273..3a53ee3 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -265,7 +265,7 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>  	drm_fb_helper_fini(&ifbdev->helper);
>  
>  	drm_framebuffer_unregister_private(&ifbdev->fb->base);
> -	drm_framebuffer_unreference(&ifbdev->fb->base);
> +	drm_framebuffer_remove(&ifbdev->fb->base);
>  }
>  
>  int intel_fbdev_init(struct drm_device *dev)
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch