[Mesa-dev] [PATCH 6/9] isl: Validate the calculated row pitch (v2)

[Chad Versace]

On Thu 23 Mar 2017, Jason Ekstrand wrote:
> On Wed, Mar 22, 2017 at 6:04 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.
> > ---
> >  src/intel/Makefile.isl.am |  3 ++
> >  src/intel/isl/isl.c       | 72 ++++++++++++++++++++++++++++++
> > +++++++++++++----
> >  2 files changed, 69 insertions(+), 6 deletions(-)




> > +
> > +   if (row_pitch == 0)
> > +      return false;
> > +
> > +   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");
> > +      goto done;
> > +   }
> > +
> > +   if (((surf_info->usage & ISL_SURF_USAGE_RENDER_TARGET_BIT) ||
> > +        (surf_info->usage & ISL_SURF_USAGE_TEXTURE_BIT)) &&
> >
> 
> We also want to handle STORAGE_BIT

Done. I added it locally.

> > +       !pitch_in_range(row_pitch, RENDER_SURFACE_STATE_
> > SurfacePitch_bits(gen_10x)))
> > +      return false;
> > +
> > +   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(gen_10x)))
> > +      return false;
> > +
> > +   if ((surf_info->usage & ISL_SURF_USAGE_DEPTH_BIT) &&
> > +       !pitch_in_range(row_pitch, _3DSTATE_DEPTH_BUFFER_
> > SurfacePitch_bits(gen_10x)))
> > +      return false;
> > +
> > +   if ((surf_info->usage & ISL_SURF_USAGE_HIZ_BIT) &&
> > +       !pitch_in_range(row_pitch, _3DSTATE_HIER_DEPTH_BUFFER_
> > SurfacePitch_bits(gen_10x)))
> > +      return false;
> > +
> > +   if (surf_info->usage & ISL_SURF_USAGE_STENCIL_BIT)
> > +      isl_finishme("validate row pitch of stencil surfaces");
> >
> 
> How hard is this?  I assume it's not trivial or you would have done it.

It's not trivial. There's corner cases, especially for older gens.  But
it's not *too* hard. It's just that I have no confidence that I could
write it correctly on the first try.

I wanted to land all the obvious pitch validations asap. Where obvious
means:

   if ((usage & bits) && !pitch_in_range())
      return false;

Since stencil pitch validation is the only non-obvious validation,
I wanted to do it as a follow-up. Because there WILL be errors in v1 of
the patch. And I didn't want this patch to be blocked while I debugged
the stencil validation failures and argued over the corner cases.