| Message ID | 1444834266-12689-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
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;
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(-)