[Intel-gfx] [PATCH v4 01/25] drm/i915/fbdev: Call intel_unpin_fb_obj() on release

[Chris Wilson]

On Wed, Apr 27, 2016 at 01:20:52PM +0100, Tvrtko Ursulin wrote:
> 
> On 26/04/16 07:54, Chris Wilson wrote:
> >When releasing the intel_fbdev, we should unpin the framebuffer that we
> >pinned during construction.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  drivers/gpu/drm/i915/intel_drv.h     | 1 +
> >  drivers/gpu/drm/i915/intel_fbdev.c   | 7 ++++++-
> >  3 files changed, 8 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index b7cb632a2a31..87ce7852482b 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -2309,7 +2309,7 @@ err_pm:
> >  	return ret;
> >  }
> >
> >-static void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> >+void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> >  {
> >  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >  	struct i915_ggtt_view view;
> >diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >index b9f1304439e2..54f7000e2fe9 100644
> >--- a/drivers/gpu/drm/i915/intel_drv.h
> >+++ b/drivers/gpu/drm/i915/intel_drv.h
> >@@ -1161,6 +1161,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
> >  				    struct drm_modeset_acquire_ctx *ctx);
> >  int intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
> >  			       unsigned int rotation);
> >+void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
> >  struct drm_framebuffer *
> >  __intel_framebuffer_create(struct drm_device *dev,
> >  			   struct drm_mode_fb_cmd2 *mode_cmd,
> >diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> >index af561542a5a1..1c3ad121f1b9 100644
> >--- a/drivers/gpu/drm/i915/intel_fbdev.c
> >+++ b/drivers/gpu/drm/i915/intel_fbdev.c
> >@@ -287,7 +287,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  out_destroy_fbi:
> >  	drm_fb_helper_release_fbi(helper);
> >  out_unpin:
> >-	i915_gem_object_ggtt_unpin(obj);
> >+	intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0));
> >  out_unlock:
> >  	mutex_unlock(&dev->struct_mutex);
> >  	return ret;
> >@@ -551,6 +551,11 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> >
> >  	if (ifbdev->fb) {
> >  		drm_framebuffer_unregister_private(&ifbdev->fb->base);
> >+
> >+		mutex_lock(&dev->struct_mutex);
> >+		intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0));
> >+		mutex_unlock(&dev->struct_mutex);
> >+
> >  		drm_framebuffer_remove(&ifbdev->fb->base);
> 
> I think I've asked before - why this isn't the last fb reference so
> the obj unref would do the unpin automatically?

And I thought I answered that it was bad practice to leave it to the
core as then we can't detect a leak, and that later code I have will
complain about the pin_display leak here. With user pinning gone, every
pin should now be accountable and anything remaining should be a WARN.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre