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

[Jason Ekstrand]

On Tue, Mar 28, 2017 at 9:15 AM, Chad Versace <chadversary at chromium.org>
wrote:

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

I don't really care all that much.  In my brain one is a "does it have this
bit or does it have this bit or does it have that bit" vs. "does it have
one of these bits".  It really doesn't matter...


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

That's fine.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170328/ad7d2501/attachment-0001.html>