| Message ID | 1414692121-13150-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 30, 2014 at 04:02:01PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > 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 <gustavo.padovan@collabora.co.uk> > 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 <imre.deak@intel.com> > Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > 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
2014-10-30 17:10 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > On Thu, Oct 30, 2014 at 04:02:01PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> 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 <gustavo.padovan@collabora.co.uk> >> 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 <imre.deak@intel.com> >> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> 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
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) {