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@xxxxxxxxxxxxxxxxxx>
> >---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
