2014-10-30 17:10 GMT-02:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>: > On Thu, Oct 30, 2014 at 04:02:01PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> >> 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@xxxxxxxxxxxxxxx> >> 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@xxxxxxxxx> >> Cc: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
