* Re: [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
2015-10-14 15:08 ` Tvrtko Ursulin
@ 2015-10-14 15:33 ` Chris Wilson
2015-10-14 15:56 ` Tvrtko Ursulin
2015-10-14 15:35 ` Chris Wilson
2015-11-19 15:42 ` Daniel Vetter
2 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2015-10-14 15:33 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniel Vetter, Intel Graphics Development, 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 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
2015-10-14 15:33 ` Chris Wilson
@ 2015-10-14 15:56 ` Tvrtko Ursulin
2015-10-14 16:13 ` Chris Wilson
0 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2015-10-14 15:56 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Intel Graphics Development,
Daniel Vetter
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
2015-10-14 15:56 ` Tvrtko Ursulin
@ 2015-10-14 16:13 ` Chris Wilson
0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2015-10-14 16:13 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
2015-10-14 15:08 ` Tvrtko Ursulin
2015-10-14 15:33 ` Chris Wilson
@ 2015-10-14 15:35 ` Chris Wilson
2015-10-14 15:55 ` Tvrtko Ursulin
2015-11-19 15:42 ` Daniel Vetter
2 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2015-10-14 15:35 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniel Vetter, Intel Graphics Development, 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.
*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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
2015-10-14 15:35 ` Chris Wilson
@ 2015-10-14 15:55 ` Tvrtko Ursulin
2015-10-14 16:10 ` Chris Wilson
0 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2015-10-14 15:55 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Intel Graphics Development,
Daniel Vetter
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
2015-10-14 15:55 ` Tvrtko Ursulin
@ 2015-10-14 16:10 ` Chris Wilson
0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2015-10-14 16:10 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
2015-10-14 15:08 ` Tvrtko Ursulin
2015-10-14 15:33 ` Chris Wilson
2015-10-14 15:35 ` Chris Wilson
@ 2015-11-19 15:42 ` Daniel Vetter
2 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2015-11-19 15:42 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniel Vetter, Intel Graphics Development, 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
^ permalink raw reply [flat|nested] 13+ messages in thread