[Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane
From: Paulo Zanoni Commit "drm/i915: create a prepare phase for sprite plane updates" changed the old_obj pointer we use when committing sprite planes, which caused a WARN() and a BUG() to be triggered. This patch should revert the code back to the previous behavior, fixing the regression. Regression introduced by: commit ec82cb793c9224e0692eed904f43490cf70e8258 Author: Gustavo Padovan Date: Fri Oct 24 14:51:32 2014 +0100 drm/i915: create a prepare phase for sprite plane updates Credits to Imre Deak for pointing out the exact lines that were wrong. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634 Testcase: igt/pm_rpm/legacy-planes Testcase: igt/pm_rpm/legacy-planes-dpms Testcase: igt/pm_rpm/universal-planes Testcase: igt/pm_rpm/universal-planes-dpms Credits-to: Imre Deak Cc: Gustavo Padovan Cc: Ville Syrjälä Signed-off-by: Paulo Zanoni --- drivers/gpu/drm/i915/intel_sprite.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 8b80d68..1c874309 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1197,10 +1197,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane, struct drm_device *dev = plane->dev; struct drm_crtc *crtc = state->crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_plane *intel_plane = to_intel_plane(plane); enum pipe pipe = intel_crtc->pipe; struct drm_framebuffer *fb = state->fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); + struct drm_i915_gem_object *old_obj = intel_plane->obj; int ret; if (old_obj != obj) { -- 2.1.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane
From: Paulo Zanoni Commit "drm/i915: create a prepare phase for sprite plane updates" changed the old_obj pointer we use when committing sprite planes, which caused a WARN() and a BUG() to be triggered. Later, commit "drm/i915: use intel_fb_obj() macros to assign gem objects" introduced the same problem to function intel_commit_sprite_plane(). Regression introduced by: commit ec82cb793c9224e0692eed904f43490cf70e8258 Author: Gustavo Padovan Date: Fri Oct 24 14:51:32 2014 +0100 drm/i915: create a prepare phase for sprite plane updates and: commit 77cde95217484e845743818691df026cec2534f4 Author: Gustavo Padovan Date: Fri Oct 24 14:51:33 2014 +0100 drm/i915: use intel_fb_obj() macros to assign gem objects Credits to Imre Deak for pointing out the exact lines that were wrong. v2: Also fix intel_commit_sprite_plane() (Ville) Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634 Testcase: igt/pm_rpm/legacy-planes Testcase: igt/pm_rpm/legacy-planes-dpms Testcase: igt/pm_rpm/universal-planes Testcase: igt/pm_rpm/universal-planes-dpms Credits-to: Imre Deak Cc: Gustavo Padovan Cc: Ville Syrjälä Signed-off-by: Paulo Zanoni --- drivers/gpu/drm/i915/intel_sprite.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 64076555..7d9c340 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1264,10 +1264,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane, struct drm_device *dev = plane->dev; struct drm_crtc *crtc = state->crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_plane *intel_plane = to_intel_plane(plane); enum pipe pipe = intel_crtc->pipe; struct drm_framebuffer *fb = state->fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); + struct drm_i915_gem_object *old_obj = intel_plane->obj; int ret; if (old_obj != obj) { @@ -1302,7 +1303,7 @@ intel_commit_sprite_plane(struct drm_plane *plane, enum pipe pipe = intel_crtc->pipe; struct drm_framebuffer *fb = state->fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); + struct drm_i915_gem_object *old_obj = intel_plane->obj; int crtc_x, crtc_y; unsigned int crtc_w, crtc_h; uint32_t src_x, src_y, src_w, src_h; -- 2.1.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane
On Thu, Oct 30, 2014 at 04:02:01PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni > > Commit "drm/i915: create a prepare phase for sprite plane updates" > changed the old_obj pointer we use when committing sprite planes, > which caused a WARN() and a BUG() to be triggered. This patch should > revert the code back to the previous behavior, fixing the regression. > > Regression introduced by: > commit ec82cb793c9224e0692eed904f43490cf70e8258 > Author: Gustavo Padovan > Date: Fri Oct 24 14:51:32 2014 +0100 > drm/i915: create a prepare phase for sprite plane updates > > Credits to Imre Deak for pointing out the exact lines that were wrong. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634 > Testcase: igt/pm_rpm/legacy-planes > Testcase: igt/pm_rpm/legacy-planes-dpms > Testcase: igt/pm_rpm/universal-planes > Testcase: igt/pm_rpm/universal-planes-dpms > Credits-to: Imre Deak > Cc: Gustavo Padovan > Cc: Ville Syrjälä > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/intel_sprite.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index 8b80d68..1c874309 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1197,10 +1197,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane, > struct drm_device *dev = plane->dev; > struct drm_crtc *crtc = state->crtc; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_plane *intel_plane = to_intel_plane(plane); > enum pipe pipe = intel_crtc->pipe; > struct drm_framebuffer *fb = state->fb; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > + struct drm_i915_gem_object *old_obj = intel_plane->obj; You need to change intel_commit_sprite_plane() too. Othwerwise something like this could happen: 1. .update_plane(fb=A) // note plane->fb=NULL here -> prepare() -> pin(A) -> obj=A -> commit() -> nop ... plane->fb=A 2. .crtc_disable() -> disable() -> unpin(A) -> obj=NULL 3. .update_plane(fb=B) // note plane->fb=A here still -> prepare() -> pin(B) -> commit() -> unpin(A) -> obj=B ... plane->fb=B So we still get the double unpin of A. I take it our basic plane tests didn't catch this? > int ret; > > if (old_obj != obj) { > -- > 2.1.1 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane
2014-10-30 17:10 GMT-02:00 Ville Syrjälä : > On Thu, Oct 30, 2014 at 04:02:01PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni >> >> Commit "drm/i915: create a prepare phase for sprite plane updates" >> changed the old_obj pointer we use when committing sprite planes, >> which caused a WARN() and a BUG() to be triggered. This patch should >> revert the code back to the previous behavior, fixing the regression. >> >> Regression introduced by: >> commit ec82cb793c9224e0692eed904f43490cf70e8258 >> Author: Gustavo Padovan >> Date: Fri Oct 24 14:51:32 2014 +0100 >> drm/i915: create a prepare phase for sprite plane updates >> >> Credits to Imre Deak for pointing out the exact lines that were wrong. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634 >> Testcase: igt/pm_rpm/legacy-planes >> Testcase: igt/pm_rpm/legacy-planes-dpms >> Testcase: igt/pm_rpm/universal-planes >> Testcase: igt/pm_rpm/universal-planes-dpms >> Credits-to: Imre Deak >> Cc: Gustavo Padovan >> Cc: Ville Syrjälä >> Signed-off-by: Paulo Zanoni >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c >> b/drivers/gpu/drm/i915/intel_sprite.c >> index 8b80d68..1c874309 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -1197,10 +1197,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane, >> struct drm_device *dev = plane->dev; >> struct drm_crtc *crtc = state->crtc; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> + struct intel_plane *intel_plane = to_intel_plane(plane); >> enum pipe pipe = intel_crtc->pipe; >> struct drm_framebuffer *fb = state->fb; >> struct drm_i915_gem_object *obj = intel_fb_obj(fb); >> - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); >> + struct drm_i915_gem_object *old_obj = intel_plane->obj; > > You need to change intel_commit_sprite_plane() too. Othwerwise something > like this could happen: > > 1. .update_plane(fb=A) >// note plane->fb=NULL here >-> prepare() -> pin(A) -> obj=A >-> commit() -> nop >... >plane->fb=A > > 2. .crtc_disable() >-> disable() -> unpin(A) -> obj=NULL > > 3. .update_plane(fb=B) >// note plane->fb=A here still >-> prepare() -> pin(B) >-> commit() -> unpin(A) -> obj=B >... >plane->fb=B > > So we still get the double unpin of A. > > I take it our basic plane tests didn't catch this? I didn't run any, nor associated any of the recent bug reports with that. Feel free to write tests if you can think of an easy way to exploit the bug. I also won't mind if someone else writes/sends v2 of this patch :) > >> int ret; >> >> if (old_obj != obj) { >> -- >> 2.1.1 > > -- > Ville Syrjälä > Intel OTC -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane
On Mon, Nov 10, 2014 at 02:47:30PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni > > Commit "drm/i915: create a prepare phase for sprite plane updates" > changed the old_obj pointer we use when committing sprite planes, > which caused a WARN() and a BUG() to be triggered. Later, commit > "drm/i915: use intel_fb_obj() macros to assign gem objects" introduced > the same problem to function intel_commit_sprite_plane(). > > Regression introduced by: > commit ec82cb793c9224e0692eed904f43490cf70e8258 > Author: Gustavo Padovan > Date: Fri Oct 24 14:51:32 2014 +0100 > drm/i915: create a prepare phase for sprite plane updates > and: > commit 77cde95217484e845743818691df026cec2534f4 > Author: Gustavo Padovan > Date: Fri Oct 24 14:51:33 2014 +0100 > drm/i915: use intel_fb_obj() macros to assign gem objects > > Credits to Imre Deak for pointing out the exact lines that were wrong. > > v2: Also fix intel_commit_sprite_plane() (Ville) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634 > Testcase: igt/pm_rpm/legacy-planes > Testcase: igt/pm_rpm/legacy-planes-dpms > Testcase: igt/pm_rpm/universal-planes > Testcase: igt/pm_rpm/universal-planes-dpms > Credits-to: Imre Deak > Cc: Gustavo Padovan > Cc: Ville Syrjälä > Signed-off-by: Paulo Zanoni Yep, I believe that should do it. Reviewed-by: Ville Syrjälä As a side note if someone is looking for stuff to do, then the pin/unpin logic might be good thing to look at. We're currently a bit inconsistent whether we have the buffer pinned when the plane is disabled, or just otherwise invisible, or when the crtc itself is disabled. And I guess cooking up some tests to poke at planes with disabled crtcs might be in order too, as well as all kinds of variations on the crtc_enable->plane_enable->crtc_disable->plane_disable theme. > --- > drivers/gpu/drm/i915/intel_sprite.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index 64076555..7d9c340 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1264,10 +1264,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane, > struct drm_device *dev = plane->dev; > struct drm_crtc *crtc = state->crtc; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_plane *intel_plane = to_intel_plane(plane); > enum pipe pipe = intel_crtc->pipe; > struct drm_framebuffer *fb = state->fb; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > + struct drm_i915_gem_object *old_obj = intel_plane->obj; > int ret; > > if (old_obj != obj) { > @@ -1302,7 +1303,7 @@ intel_commit_sprite_plane(struct drm_plane *plane, > enum pipe pipe = intel_crtc->pipe; > struct drm_framebuffer *fb = state->fb; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > + struct drm_i915_gem_object *old_obj = intel_plane->obj; > int crtc_x, crtc_y; > unsigned int crtc_w, crtc_h; > uint32_t src_x, src_y, src_w, src_h; > -- > 2.1.1 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane
On Mon, Nov 10, 2014 at 07:15:04PM +0200, Ville Syrjälä wrote: > On Mon, Nov 10, 2014 at 02:47:30PM -0200, Paulo Zanoni wrote: > > From: Paulo Zanoni > > > > Commit "drm/i915: create a prepare phase for sprite plane updates" > > changed the old_obj pointer we use when committing sprite planes, > > which caused a WARN() and a BUG() to be triggered. Later, commit > > "drm/i915: use intel_fb_obj() macros to assign gem objects" introduced > > the same problem to function intel_commit_sprite_plane(). > > > > Regression introduced by: > > commit ec82cb793c9224e0692eed904f43490cf70e8258 > > Author: Gustavo Padovan > > Date: Fri Oct 24 14:51:32 2014 +0100 > > drm/i915: create a prepare phase for sprite plane updates > > and: > > commit 77cde95217484e845743818691df026cec2534f4 > > Author: Gustavo Padovan > > Date: Fri Oct 24 14:51:33 2014 +0100 > > drm/i915: use intel_fb_obj() macros to assign gem objects > > > > Credits to Imre Deak for pointing out the exact lines that were wrong. > > > > v2: Also fix intel_commit_sprite_plane() (Ville) > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85634 > > Testcase: igt/pm_rpm/legacy-planes > > Testcase: igt/pm_rpm/legacy-planes-dpms > > Testcase: igt/pm_rpm/universal-planes > > Testcase: igt/pm_rpm/universal-planes-dpms > > Credits-to: Imre Deak > > Cc: Gustavo Padovan > > Cc: Ville Syrjälä > > Signed-off-by: Paulo Zanoni > > Yep, I believe that should do it. > > Reviewed-by: Ville Syrjälä Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx