[Intel-gfx] [PATCH] drm/i915: use the correct obj when preparing the sprite plane

[Paulo Zanoni]

2014-10-30 17:10 GMT-02:00 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> On Thu, Oct 30, 2014 at 04:02:01PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at 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 at 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 at intel.com>
>> Cc: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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