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.pohjolai...@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 <nanleych...@gmail.com> > >> wrote: > >> > From: Nanley Chery <nanley.g.ch...@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.ch...@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@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! _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev