| Message ID | 1366727228-6207-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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);
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(-)