[Mesa-dev] [PATCH] i965/gen9: use an unreserved surface alignment value

[Pohjolainen, Topi]

On Wed, Jun 24, 2015 at 05:57:13PM -0700, Anuj Phogat wrote:
> On Wed, Jun 24, 2015 at 3:51 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> > From: Nanley Chery <nanley.g.chery at intel.com>
> >
> > Although the horizontal and vertical alignment fields are ignored here,
> > 0 is a reserved value for them and may cause undefined behavior. Change
> > the default value to an abitrary valid one.
> >
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/gen8_surface_state.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > index b2d1a57..22ae960 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > @@ -93,7 +93,7 @@ vertical_alignment(const struct brw_context *brw,
> >     if (brw->gen > 8 &&
> >         (mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE ||
> >          surf_type == BRW_SURFACE_1D))
> > -      return 0;
> > +      return GEN8_SURFACE_VALIGN_4;
> >
> >     switch (mt->align_h) {
> >     case 4:
> > @@ -118,7 +118,7 @@ horizontal_alignment(const struct brw_context *brw,
> >     if (brw->gen > 8 &&
> >         (mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE ||
> >          gen9_use_linear_1d_layout(brw, mt)))
> > -      return 0;
> > +      return GEN8_SURFACE_HALIGN_4;
> >
> >     switch (mt->align_w) {
> >     case 4:
> > --
> > 2.4.4
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> Good find Nanley. We had no known issues with value 0 but it's
> always nice to avoid undefined behavior :).

Right, I thought about this when I reviewed the original. The spec says
it is ignored in these cases and hence the reserved value seemed fine. Now
that we put something meaningful there, somebody is going to compare the
spec and wonder why we set it to 4. If we added also a comment here that
says this is just an arbitrary (non-reserved) value and really ignored
by the hardware, it would prevent misunderstandings. What do you guys think?

> 
> Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev