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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 10, 2014 at 02:47:30PM -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. Later, commit
> "drm/i915: use intel_fb_obj() macros to assign gem objects" introduced
> the same problem to function intel_commit_sprite_plane().
> 
> 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
> and:
>     commit 77cde95217484e845743818691df026cec2534f4
>     Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>     Date:   Fri Oct 24 14:51:33 2014 +0100
>         drm/i915: use intel_fb_obj() macros to assign gem objects
> 
> Credits to Imre Deak for pointing out the exact lines that were wrong.
> 
> v2: Also fix intel_commit_sprite_plane() (Ville)
> 
> 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>

Yep, I believe that should do it.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

As a side note if someone is looking for stuff to do, then the pin/unpin
logic might be good thing to look at. We're currently a bit inconsistent
whether we have the buffer pinned when the plane is disabled, or just
otherwise invisible, or when the crtc itself is disabled. And I guess
cooking up some tests to poke at planes with disabled crtcs might be in
order too, as well as all kinds of variations on the
crtc_enable->plane_enable->crtc_disable->plane_disable theme.

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 64076555..7d9c340 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1264,10 +1264,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) {
> @@ -1302,7 +1303,7 @@ intel_commit_sprite_plane(struct drm_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 crtc_x, crtc_y;
>  	unsigned int crtc_w, crtc_h;
>  	uint32_t src_x, src_y, src_w, src_h;
> -- 
> 2.1.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx