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

[Jason Ekstrand]

On Fri, Mar 24, 2017 at 11:10 AM, Chad Versace <chadversary at chromium.org>
wrote:

> 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.
>

Fine with me.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170324/a3378e62/attachment-0001.html>