| Message ID | 1444840154-7804-15-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/10/15 17:29, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > In case we have multiple different rotated views into the same object, > each one may need its own vma due to being of different sizes. So don't > treat all rotated views as equal. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index caa182f..68de734 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > if (a->type != b->type) > return false; > + if (a->type == I915_GGTT_VIEW_ROTATED) > + return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); > if (a->type == I915_GGTT_VIEW_PARTIAL) > return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); > return true; This is where anonymous union causes problems since you have to build a switch statement before memcmp while the original goal was for memcmp to elegantly avoid that. Regards, Tvrtko
On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote: > > On 14/10/15 17:29, ville.syrjala@linux.intel.com wrote: > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >In case we have multiple different rotated views into the same object, > >each one may need its own vma due to being of different sizes. So don't > >treat all rotated views as equal. > > > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >--- > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > >index caa182f..68de734 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > > > if (a->type != b->type) > > return false; > >+ if (a->type == I915_GGTT_VIEW_ROTATED) > >+ return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); > > if (a->type == I915_GGTT_VIEW_PARTIAL) > > return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); > > return true; > > This is where anonymous union causes problems since you have to build a > switch statement before memcmp while the original goal was for memcmp to > elegantly avoid that. Yup, mentioned the same on irc. I much prefer we do the memcmp on type != NORMAL, to avoid mistakes when adding more views in the future. -Daniel
On Thu, Oct 15, 2015 at 02:02:24PM +0200, Daniel Vetter wrote: > On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote: > > > > On 14/10/15 17:29, ville.syrjala@linux.intel.com wrote: > > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > >In case we have multiple different rotated views into the same object, > > >each one may need its own vma due to being of different sizes. So don't > > >treat all rotated views as equal. > > > > > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >--- > > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > > >index caa182f..68de734 100644 > > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > > >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > > > > > if (a->type != b->type) > > > return false; > > >+ if (a->type == I915_GGTT_VIEW_ROTATED) > > >+ return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); > > > if (a->type == I915_GGTT_VIEW_PARTIAL) > > > return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); > > > return true; > > > > This is where anonymous union causes problems since you have to build a > > switch statement before memcmp while the original goal was for memcmp to > > elegantly avoid that. > > Yup, mentioned the same on irc. I much prefer we do the memcmp on type != > NORMAL, to avoid mistakes when adding more views in the future. I just dislike the .partial. extra stuff all over. But whatever, it's a minor detail in my book. We can go with the non-anon union if people prefer that.
On Thu, Oct 15, 2015 at 03:06:11PM +0300, Ville Syrjälä wrote: > On Thu, Oct 15, 2015 at 02:02:24PM +0200, Daniel Vetter wrote: > > On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote: > > > > > > On 14/10/15 17:29, ville.syrjala@linux.intel.com wrote: > > > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > >In case we have multiple different rotated views into the same object, > > > >each one may need its own vma due to being of different sizes. So don't > > > >treat all rotated views as equal. > > > > > > > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >--- > > > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > > > >index caa182f..68de734 100644 > > > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > > > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > > > >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > > > > > > > if (a->type != b->type) > > > > return false; > > > >+ if (a->type == I915_GGTT_VIEW_ROTATED) > > > >+ return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); > > > > if (a->type == I915_GGTT_VIEW_PARTIAL) > > > > return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); > > > > return true; > > > > > > This is where anonymous union causes problems since you have to build a > > > switch statement before memcmp while the original goal was for memcmp to > > > elegantly avoid that. > > > > Yup, mentioned the same on irc. I much prefer we do the memcmp on type != > > NORMAL, to avoid mistakes when adding more views in the future. > > I just dislike the .partial. extra stuff all over. But whatever, it's a > minor detail in my book. We can go with the non-anon union if people > prefer that. Here it would be if (a->type != NORMA)) return !memcmp(&a->params, &b->params); Seems at least in this case cleaner. But yet everywhere else you'll get a params., but that can be alleviated a bit with local variables. -Daniel > > -- > Ville Syrjälä > Intel OTC
On Wed, Oct 14, 2015 at 07:29:06PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > In case we have multiple different rotated views into the same object, > each one may need its own vma due to being of different sizes. So don't > treat all rotated views as equal. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index caa182f..68de734 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > if (a->type != b->type) > return false; > + if (a->type == I915_GGTT_VIEW_ROTATED) > + return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); We should take my "[PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly" instead since being more strict here means intel_plane_obj_offset won't find the prepared view any more. Another reasons to just track the vma in the plane_state. -Daniel > if (a->type == I915_GGTT_VIEW_PARTIAL) > return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); > return true; > -- > 2.4.9 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 15, 2015 at 02:24:51PM +0200, Daniel Vetter wrote: > On Thu, Oct 15, 2015 at 03:06:11PM +0300, Ville Syrjälä wrote: > > On Thu, Oct 15, 2015 at 02:02:24PM +0200, Daniel Vetter wrote: > > > On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote: > > > > > > > > On 14/10/15 17:29, ville.syrjala@linux.intel.com wrote: > > > > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > >In case we have multiple different rotated views into the same object, > > > > >each one may need its own vma due to being of different sizes. So don't > > > > >treat all rotated views as equal. > > > > > > > > > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > >--- > > > > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > > > > >index caa182f..68de734 100644 > > > > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > > > > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > > > > >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > > > > > > > > > if (a->type != b->type) > > > > > return false; > > > > >+ if (a->type == I915_GGTT_VIEW_ROTATED) > > > > >+ return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); > > > > > if (a->type == I915_GGTT_VIEW_PARTIAL) > > > > > return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); > > > > > return true; > > > > > > > > This is where anonymous union causes problems since you have to build a > > > > switch statement before memcmp while the original goal was for memcmp to > > > > elegantly avoid that. > > > > > > Yup, mentioned the same on irc. I much prefer we do the memcmp on type != > > > NORMAL, to avoid mistakes when adding more views in the future. > > > > I just dislike the .partial. extra stuff all over. But whatever, it's a > > minor detail in my book. We can go with the non-anon union if people > > prefer that. > > Here it would be > if (a->type != NORMA)) > return !memcmp(&a->params, &b->params); > > Seems at least in this case cleaner. But yet everywhere else you'll get a > params., but that can be alleviated a bit with local variables. We can go with your patches that preserve the named union. I can then rebase my stuff on top.
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index caa182f..68de734 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, if (a->type != b->type) return false; + if (a->type == I915_GGTT_VIEW_ROTATED) + return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); if (a->type == I915_GGTT_VIEW_PARTIAL) return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); return true;