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

[Pohjolainen, Topi]

On Thu, Jun 25, 2015 at 08:40:33AM -0700, Nanley Chery wrote:
> On Thu, Jun 25, 2015 at 12:37 AM, Pohjolainen, Topi
> <topi.pohjolainen at intel.com> wrote:
> > 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?
> >
> There's enough space to insert "Set to an arbitrary non-reserved
> value." in both of the comments preceding the conditional without
> adding an extra line. I wouldn't mind including it.

Sounds great, thanks!