[Intel-gfx] [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly

[Chris Wilson]

On Wed, Oct 14, 2015 at 04:55:47PM +0100, Tvrtko Ursulin wrote:
> 
> On 14/10/15 16:35, Chris Wilson wrote:
> >On Wed, Oct 14, 2015 at 04:08:25PM +0100, Tvrtko Ursulin wrote:
> >>
> >>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 at 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 at linux.intel.com>
> >>>Signed-off-by: Daniel Vetter <daniel.vetter at 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.
> >
> >*by default (since it requires Y-tiling of the frontbuffer and that
> >makes other accesses very slow and needing workarounds to prevent
> >overall degredation, for some workloads in particular).
> 
> Even if compiled to use Y tiling it still goes back to X tiled when
> 90/270 rotation is attempted. I reported that here:
> https://bugs.freedesktop.org/show_bug.cgi?id=91562#c14

Which says the kernel rejected the setcrtc with the primary rotation
property set to 2 (either ROTATE_90 or 270, I forget which).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre