Re: [PATCH] drm/i915: use the correct obj when preparing the sprite plane

Paulo Zanoni
2014-10-30T19:33:27Z

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

--
Paulo Zanoni