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

Ville Syrjälä
2014-10-30T19:10:09Z

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

--
Ville Syrjälä
Intel OTC