[PATCH 1/3] drm/i915: Drop return value from intel_fill_fb_ggtt_view

[PATCH 1/3] drm/i915: Drop return value from intel_fill_fb_ggtt_view

[Daniel Vetter]

It can't fail and there's even a WARN_ON suggesting that if it would,
it would be a disaster.

Correct this to make things less confusing.

Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 184725770ae7..2c9d822b29b6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2243,7 +2243,7 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height,
 					       fb_format_modifier, 0));
 }
 
-static int
+static void
 intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
 			const struct drm_plane_state *plane_state)
 {
@@ -2253,10 +2253,10 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
 	*view = i915_ggtt_view_normal;
 
 	if (!plane_state)
-		return 0;
+		return;
 
 	if (!intel_rotation_90_or_270(plane_state->rotation))
-		return 0;
+		return;
 
 	*view = i915_ggtt_view_rotated;
 
@@ -2283,8 +2283,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
 		info->size_uv = info->width_pages_uv * info->height_pages_uv *
 				PAGE_SIZE;
 	}
-
-	return 0;
 }
 
 static unsigned int intel_linear_alignment(struct drm_i915_private *dev_priv)
@@ -2340,9 +2338,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
-	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
-	if (ret)
-		return ret;
+	intel_fill_fb_ggtt_view(&view, fb, plane_state);
 
 	/* Note that the w/a also requires 64 PTE of padding following the
 	 * bo. We currently fill all unused PTE with the shadow page and so
@@ -2406,12 +2402,10 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
 {
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct i915_ggtt_view view;
-	int ret;
 
 	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
 
-	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
-	WARN_ONCE(ret, "Couldn't get view from plane state!");
+	intel_fill_fb_ggtt_view(&view, fb, plane_state);
 
 	i915_gem_object_unpin_fence(obj);
 	i915_gem_object_unpin_from_display_plane(obj, &view);
-- 
2.5.1

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

[PATCH 2/3] drm/i915: Stuff rotation params into view union

[Daniel Vetter]

We don't need 2 separate unions.

Note that this was done intentinoally

Author: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Date:   Wed May 6 14:35:38 2015 +0300

    drm/i915: Add a partial GGTT view type

on Tvrtko's request, but without a clear justification. Rotated views
are also not checking for matching paramters in i915_ggtt_view_equal,
which seems like a bug. But this patch here doesn't change that.

Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c  | 4 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.h  | 5 +----
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index bead61e0de1b..15dfefd58d08 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3283,7 +3283,7 @@ static struct sg_table *
 intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
 			  struct drm_i915_gem_object *obj)
 {
-	struct intel_rotation_info *rot_info = &ggtt_view->rotation_info;
+	struct intel_rotation_info *rot_info = &ggtt_view->params.rotation_info;
 	unsigned int size_pages = rot_info->size >> PAGE_SHIFT;
 	unsigned int size_pages_uv;
 	struct sg_page_iter sg_iter;
@@ -3515,7 +3515,7 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 	if (view->type == I915_GGTT_VIEW_NORMAL) {
 		return obj->base.size;
 	} else if (view->type == I915_GGTT_VIEW_ROTATED) {
-		return view->rotation_info.size;
+		return view->params.rotation_info.size;
 	} else if (view->type == I915_GGTT_VIEW_PARTIAL) {
 		return view->params.partial.size << PAGE_SHIFT;
 	} else {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 9fbb07d6eaad..2e1f6493c9e7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -156,13 +156,10 @@ struct i915_ggtt_view {
 			u64 offset;
 			unsigned int size;
 		} partial;
+		struct intel_rotation_info rotation_info;
 	} params;
 
 	struct sg_table *pages;
-
-	union {
-		struct intel_rotation_info rotation_info;
-	};
 };
 
 extern const struct i915_ggtt_view i915_ggtt_view_normal;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2c9d822b29b6..57459fedf216 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2247,7 +2247,7 @@ static void
 intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
 			const struct drm_plane_state *plane_state)
 {
-	struct intel_rotation_info *info = &view->rotation_info;
+	struct intel_rotation_info *info = &view->params.rotation_info;
 	unsigned int tile_height, tile_pitch;
 
 	*view = i915_ggtt_view_normal;
@@ -2909,7 +2909,7 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 	offset = (unsigned char *)vma->node.start;
 
 	if (plane == 1) {
-		offset += vma->ggtt_view.rotation_info.uv_start_page *
+		offset += vma->ggtt_view.params.rotation_info.uv_start_page *
 			  PAGE_SIZE;
 	}
 
-- 
2.5.1

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

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

[Daniel Vetter]

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;
-- 
2.5.1

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

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

[Tvrtko Ursulin]

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

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

[Chris Wilson]

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@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.

On your todo list is reviewing the patches that eliminate
intel_plane_obj_offset.
:-p
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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@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.

*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).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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@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.
>
> *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

Regards,

Tvrtko


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

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

[Tvrtko Ursulin]

On 14/10/15 16:33, 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@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.
>
> On your todo list is reviewing the patches that eliminate
> intel_plane_obj_offset.
> :-p

Have I missed something? I thought I reviewed what you sent so far.

Regards,

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

Re: [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@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.
> >
> >*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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Oct 14, 2015 at 04:56:44PM +0100, Tvrtko Ursulin wrote:
> 
> On 14/10/15 16:33, 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@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.
> >
> >On your todo list is reviewing the patches that eliminate
> >intel_plane_obj_offset.
> >:-p
> 
> Have I missed something? I thought I reviewed what you sent so far.

It's the next step of the vma rewrite which was in the previous pile.
Once we are using the VMA as the pin/unpin cookie, storiing it in the
plane->state and using the VMA directly is a natural consequence. It
just requires playing along with atomic.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Stuff rotation params into view union

[Ville Syrjälä]

On Wed, Oct 14, 2015 at 04:51:05PM +0200, Daniel Vetter wrote:
> We don't need 2 separate unions.
> 
> Note that this was done intentinoally
> 
> Author: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Date:   Wed May 6 14:35:38 2015 +0300
> 
>     drm/i915: Add a partial GGTT view type
> 
> on Tvrtko's request, but without a clear justification. Rotated views
> are also not checking for matching paramters in i915_ggtt_view_equal,
> which seems like a bug. But this patch here doesn't change that.
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c  | 4 ++--
>  drivers/gpu/drm/i915/i915_gem_gtt.h  | 5 +----
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  3 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index bead61e0de1b..15dfefd58d08 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3283,7 +3283,7 @@ static struct sg_table *
>  intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view,
>  			  struct drm_i915_gem_object *obj)
>  {
> -	struct intel_rotation_info *rot_info = &ggtt_view->rotation_info;
> +	struct intel_rotation_info *rot_info = &ggtt_view->params.rotation_info;
>  	unsigned int size_pages = rot_info->size >> PAGE_SHIFT;
>  	unsigned int size_pages_uv;
>  	struct sg_page_iter sg_iter;
> @@ -3515,7 +3515,7 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>  	if (view->type == I915_GGTT_VIEW_NORMAL) {
>  		return obj->base.size;
>  	} else if (view->type == I915_GGTT_VIEW_ROTATED) {
> -		return view->rotation_info.size;
> +		return view->params.rotation_info.size;
>  	} else if (view->type == I915_GGTT_VIEW_PARTIAL) {
>  		return view->params.partial.size << PAGE_SHIFT;
>  	} else {
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 9fbb07d6eaad..2e1f6493c9e7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -156,13 +156,10 @@ struct i915_ggtt_view {
>  			u64 offset;
>  			unsigned int size;
>  		} partial;
> +		struct intel_rotation_info rotation_info;
>  	} params;
>  
>  	struct sg_table *pages;
> -
> -	union {
> -		struct intel_rotation_info rotation_info;
> -	};

So, I basically just sent the same patch, except I made the union
anonymous, and I renamed 'rotation_info' to 'rotated' to match the type.

>  };
>  
>  extern const struct i915_ggtt_view i915_ggtt_view_normal;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2c9d822b29b6..57459fedf216 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2247,7 +2247,7 @@ static void
>  intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>  			const struct drm_plane_state *plane_state)
>  {
> -	struct intel_rotation_info *info = &view->rotation_info;
> +	struct intel_rotation_info *info = &view->params.rotation_info;
>  	unsigned int tile_height, tile_pitch;
>  
>  	*view = i915_ggtt_view_normal;
> @@ -2909,7 +2909,7 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>  	offset = (unsigned char *)vma->node.start;
>  
>  	if (plane == 1) {
> -		offset += vma->ggtt_view.rotation_info.uv_start_page *
> +		offset += vma->ggtt_view.params.rotation_info.uv_start_page *
>  			  PAGE_SIZE;
>  	}
>  
> -- 
> 2.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ville Syrjälä]

On Wed, Oct 14, 2015 at 04:51:06PM +0200, 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));

Again I have basically the same patch, except in mine the union is
anoymous so mine needs to handle partial and rotated sepeartely.

>  	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;
> -- 
> 2.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Daniel Vetter]

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@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 rebased my pile and noticed that these three still cleanly apply. So
pushed them, thanks for the review.
-Daniel

> 
> 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx