[PATCH] drm/i915: release cursor when crtc is destroyed

[PATCH] drm/i915: release cursor when crtc is destroyed

[Mika Kuoppala]

crtc is holding a reference to a cursor bo and it needs
to be released when crtc is destroyed so that we don't leak
the cursor bo.

v2: Enhance set and move cursor so that disabled
cursor is handled correctly (Ville Syrjälä)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 74156e2..f5cdd91 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6554,7 +6554,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	intel_crtc->cursor_width = width;
 	intel_crtc->cursor_height = height;
 
-	intel_crtc_update_cursor(crtc, true);
+	intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 
 	return 0;
 fail_unpin:
@@ -6573,7 +6573,7 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	intel_crtc->cursor_x = x;
 	intel_crtc->cursor_y = y;
 
-	intel_crtc_update_cursor(crtc, true);
+	intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
 
 	return 0;
 }
@@ -7087,6 +7087,8 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 		kfree(work);
 	}
 
+	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
+
 	drm_crtc_cleanup(crtc);
 
 	kfree(intel_crtc);
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: release cursor when crtc is destroyed

[Daniel Vetter]

On Tue, Apr 23, 2013 at 05:27:08PM +0300, Mika Kuoppala wrote:
> crtc is holding a reference to a cursor bo and it needs
> to be released when crtc is destroyed so that we don't leak
> the cursor bo.
> 
> v2: Enhance set and move cursor so that disabled
> cursor is handled correctly (Ville Syrjälä)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Oh, nice catch!

Could we somehow test this in an igt? I'm thinking of the following
sequence:
- Check how many objects there are in debugfs (maybe that needs a slightly
  saner interface than what we currently have in i915_gem_objects).
- Setup a mode and provoke the leak (we could augment the tests with
  sprites and similar stuff).
- Check whether the object count dropped back to the old value or not. If
  not, fail the test.

We need to check the object count both before&afterwards to account for
pinned kernel objects (which might chance depending upon kernel version
and similar things).

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 74156e2..f5cdd91 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6554,7 +6554,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	intel_crtc->cursor_width = width;
>  	intel_crtc->cursor_height = height;
>  
> -	intel_crtc_update_cursor(crtc, true);
> +	intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
>  
>  	return 0;
>  fail_unpin:
> @@ -6573,7 +6573,7 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  	intel_crtc->cursor_x = x;
>  	intel_crtc->cursor_y = y;
>  
> -	intel_crtc_update_cursor(crtc, true);
> +	intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
>  
>  	return 0;
>  }
> @@ -7087,6 +7087,8 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  		kfree(work);
>  	}
>  
> +	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
> +
>  	drm_crtc_cleanup(crtc);
>  
>  	kfree(intel_crtc);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> 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

Re: [PATCH] drm/i915: release cursor when crtc is destroyed

[Chris Wilson]

On Tue, Apr 23, 2013 at 04:56:09PM +0200, Daniel Vetter wrote:
> On Tue, Apr 23, 2013 at 05:27:08PM +0300, Mika Kuoppala wrote:
> > crtc is holding a reference to a cursor bo and it needs
> > to be released when crtc is destroyed so that we don't leak
> > the cursor bo.
> > 
> > v2: Enhance set and move cursor so that disabled
> > cursor is handled correctly (Ville Syrjälä)
> > 
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> Oh, nice catch!
> 
> Could we somehow test this in an igt? I'm thinking of the following
> sequence:
> - Check how many objects there are in debugfs (maybe that needs a slightly
>   saner interface than what we currently have in i915_gem_objects).
> - Setup a mode and provoke the leak (we could augment the tests with
>   sprites and similar stuff).
> - Check whether the object count dropped back to the old value or not. If
>   not, fail the test.

It's a leak upon module unload, so presumably you want to use kmemleak
instead.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: release cursor when crtc is destroyed

[Daniel Vetter]

On Tue, Apr 23, 2013 at 4:58 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Apr 23, 2013 at 04:56:09PM +0200, Daniel Vetter wrote:
>> On Tue, Apr 23, 2013 at 05:27:08PM +0300, Mika Kuoppala wrote:
>> > crtc is holding a reference to a cursor bo and it needs
>> > to be released when crtc is destroyed so that we don't leak
>> > the cursor bo.
>> >
>> > v2: Enhance set and move cursor so that disabled
>> > cursor is handled correctly (Ville Syrjälä)
>> >
>> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>
>> Oh, nice catch!
>>
>> Could we somehow test this in an igt? I'm thinking of the following
>> sequence:
>> - Check how many objects there are in debugfs (maybe that needs a slightly
>>   saner interface than what we currently have in i915_gem_objects).
>> - Setup a mode and provoke the leak (we could augment the tests with
>>   sprites and similar stuff).
>> - Check whether the object count dropped back to the old value or not. If
>>   not, fail the test.
>
> It's a leak upon module unload, so presumably you want to use kmemleak
> instead.

Hm, right, I think the cursor ref will survive a crtc off with the
current patch from Mika. Should we bother to rectify this?

Also I've just wondered where exactly we clean up the display state
and drop the fb reference ... Is that just by accident due module
unload only happening with fbcon at the helm and us clearing up the
fbcon fb manually?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: release cursor when crtc is destroyed

[Daniel Vetter]

On Tue, Apr 23, 2013 at 05:27:08PM +0300, Mika Kuoppala wrote:
> crtc is holding a reference to a cursor bo and it needs
> to be released when crtc is destroyed so that we don't leak
> the cursor bo.
> 
> v2: Enhance set and move cursor so that disabled
> cursor is handled correctly (Ville Syrjälä)
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

I've wondered a bit whether we shouldn't shove the last part into
crtc_off, but that would constitute a bit an api change. Queued for -next,
thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 74156e2..f5cdd91 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6554,7 +6554,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	intel_crtc->cursor_width = width;
>  	intel_crtc->cursor_height = height;
>  
> -	intel_crtc_update_cursor(crtc, true);
> +	intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
>  
>  	return 0;
>  fail_unpin:
> @@ -6573,7 +6573,7 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  	intel_crtc->cursor_x = x;
>  	intel_crtc->cursor_y = y;
>  
> -	intel_crtc_update_cursor(crtc, true);
> +	intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
>  
>  	return 0;
>  }
> @@ -7087,6 +7087,8 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  		kfree(work);
>  	}
>  
> +	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
> +
>  	drm_crtc_cleanup(crtc);
>  
>  	kfree(intel_crtc);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> 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