[Mesa-dev] [PATCH v5 2/5] isl: Validate the calculated row pitch (v4)

[Chad Versace]

On Sat 25 Mar 2017, Jason Ekstrand wrote:
> 
> 
> On March 24, 2017 7:29:05 PM Chad Versace <chadversary at chromium.org> wrote:
> 
> > Validate that isl_surf::row_pitch fits in the below bitfields,
> > if applicable based on isl_surf::usage.
> > 
> >     RENDER_SURFACE_STATE::SurfacePitch
> >     RENDER_SURFACE_STATE::AuxiliarySurfacePitch
> >     3DSTATE_DEPTH_BUFFER::SurfacePitch
> >     3DSTATE_HIER_DEPTH_BUFFER::SurfacePitch
> > 
> > v2:
> >   -Add a Makefile dependency on generated header genX_bits.h.
> > v3:
> >   - Test ISL_SURF_USAGE_STORAGE_BIT too. [for jekstrand]
> >   - Drop explicity dependency on generated header. [for emil]
> > v4:
> >   - Rebase for new gen_bits_header.py script.
> >   - Replace gen_10x with gen_device_info*.
> > ---
> >  src/intel/isl/isl.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 65 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> > index 81f40b6a6fb..4777113de84 100644



> > +   if (dim_layout == ISL_DIM_LAYOUT_GEN9_1D) {
> > +      /* SurfacePitch is ignored for this layout.How should we validate it? */
> > +      isl_finishme("validate row pitch for ISL_DIM_LAYOUT_GEN9_1D");
> 
> We validate qpitch if anything.  But the regular pitch isn't just ignored,
> it's meaningless.  I think we can drop the finishme.

Done.

> > +   if (((surf_info->usage & ISL_SURF_USAGE_RENDER_TARGET_BIT) ||
> > +        (surf_info->usage & ISL_SURF_USAGE_TEXTURE_BIT) ||
> > +        (surf_info->usage & ISL_SURF_USAGE_STORAGE_BIT)) &&
> > +       !pitch_in_range(row_pitch,
> > RENDER_SURFACE_STATE_SurfacePitch_bits(dev->info)))
> > +      return false;
> 
> You know... "(thing & foo) || (thing & bar)" is equivalent to "thing & (foo
> | bar)".  Not that you need to change but sometimes its a but nicer.

I know :). I think my style looks nicer, and the compiler should emit
the same code. But I changed it to use '|' locally so that it matches
the style in other Intel code.

> > +
> > +   if (((surf_info->usage & ISL_SURF_USAGE_CCS_BIT) ||
> > +        (surf_info->usage & ISL_SURF_USAGE_MCS_BIT)) &&
> > +       !pitch_in_range(row_pitch_tiles,
> > RENDER_SURFACE_STATE_AuxiliarySurfacePitch_bits(dev->info)))
> > +      return false;
> > +
> > +   if ((surf_info->usage & ISL_SURF_USAGE_DEPTH_BIT) &&
> > +       !pitch_in_range(row_pitch,
> > _3DSTATE_DEPTH_BUFFER_SurfacePitch_bits(dev->info)))
> > +      return false;
> > +
> > +   if ((surf_info->usage & ISL_SURF_USAGE_HIZ_BIT) &&
> > +       !pitch_in_range(row_pitch,
> > _3DSTATE_HIER_DEPTH_BUFFER_SurfacePitch_bits(dev->info)))
> 
> On gen8+, if the surface has the TEXTURE bit set we should check it against
> AuxiliarySurfacePitch as well.

Good catch.

I've already test the branch as-is, so I hesitate to add this fixup. It
can come as an immediate follow-up patch.