Re: [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

On 14/10/15 15:51, Daniel Vetter wrote:
> The rotated view depends upon the rotation paramters, but thus far we
> didn't bother checking for those. This seems to have been an issue
> ever since this was introduce in
>
> commit fe14d5f4e5468c5b80a24f1a64abcbe116143670
> Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Date:   Wed Dec 10 17:27:58 2014 +0000
>
>      drm/i915: Infrastructure for supporting different GGTT views per object
>
> But userspace is allowed to reuse framebuffer backing storage with
> different framebuffers with different pixel formats/stride/whatever.
> And e.g. SNA indeed does this. Hence we must check for all the
> paramters to match, not just that it's rotated.
>
> v2: intel_plane_obj_offset also needs to construct the full view, to
> avoid fallout since they don't fully match.
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.h  |  2 +-
>   drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
>   2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 2e1f6493c9e7..8a36f4fcc676 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
>
>   	if (a->type != b->type)
>   		return false;
> -	if (a->type == I915_GGTT_VIEW_PARTIAL)
> +	if (a->type != I915_GGTT_VIEW_NORMAL)
>   		return !memcmp(&a->params, &b->params, sizeof(a->params));
>   	return true;
>   }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 57459fedf216..2a5987ce576c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>   				     struct drm_i915_gem_object *obj,
>   				     unsigned int plane)
>   {
> -	const struct i915_ggtt_view *view = &i915_ggtt_view_normal;
> +	struct i915_ggtt_view view;
>   	struct i915_vma *vma;
>   	unsigned char *offset;
>
> -	if (intel_rotation_90_or_270(intel_plane->base.state->rotation))
> -		view = &i915_ggtt_view_rotated;
> +	intel_fill_fb_ggtt_view(&view, intel_plane->base.fb,
> +				intel_plane->base.state);
>
> -	vma = i915_gem_obj_to_ggtt_view(obj, view);
> +	vma = i915_gem_obj_to_ggtt_view(obj, &view);
>   	if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n",
> -		view->type))
> +		view.type))
>   		return -1;
>
>   	offset = (unsigned char *)vma->node.start;
>

As we discussed on IRC I had wrong assumptions when developing this. 
Luckily SNA is not using hardware 90/270 yet so we are safe there. And 
Android probably doesn't reuse the fb obj or it would have been 
reported. But I'll check.

So thanks for the cleanup! For all three:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Just a shame this means so much more computation in 
intel_plane_obj_offset, really highlights that vma should be stored in 
the state, if it is possible.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx