<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Mar 24, 2017 at 11:10 AM, Chad Versace <span dir="ltr"><<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu 23 Mar 2017, Jason Ekstrand wrote:<br>
> On Wed, Mar 22, 2017 at 6:04 PM, Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>><br>
> wrote:<br>
><br>
> > Validate that isl_surf::row_pitch fits in the below bitfields,<br>
> > if applicable based on isl_surf::usage.<br>
> ><br>
> >     RENDER_SURFACE_STATE::<wbr>SurfacePitch<br>
> >     RENDER_SURFACE_STATE::<wbr>AuxiliarySurfacePitch<br>
> >     3DSTATE_DEPTH_BUFFER::<wbr>SurfacePitch<br>
> >     3DSTATE_HIER_DEPTH_BUFFER::<wbr>SurfacePitch<br>
> ><br>
> > v2: Add a Makefile dependency on generated header genX_bits.h.<br>
> > ---<br>
> >  src/intel/<a href="http://Makefile.isl.am" rel="noreferrer" target="_blank">Makefile.isl.am</a> |  3 ++<br>
> >  src/intel/isl/isl.c       | 72 ++++++++++++++++++++++++++++++<br>
> > +++++++++++++----<br>
> >  2 files changed, 69 insertions(+), 6 deletions(-)<br>
<br>
<br>
<br>
<br>
</span><span class="">> > +<br>
> > +   if (row_pitch == 0)<br>
> > +      return false;<br>
> > +<br>
> > +   if (dim_layout == ISL_DIM_LAYOUT_GEN9_1D) {<br>
> > +      /* SurfacePitch is ignored for this layout.How should we validate<br>
> > it? */<br>
> > +      isl_finishme("validate row pitch for ISL_DIM_LAYOUT_GEN9_1D");<br>
> > +      goto done;<br>
> > +   }<br>
> > +<br>
> > +   if (((surf_info->usage & ISL_SURF_USAGE_RENDER_TARGET_<wbr>BIT) ||<br>
> > +        (surf_info->usage & ISL_SURF_USAGE_TEXTURE_BIT)) &&<br>
> ><br>
><br>
> We also want to handle STORAGE_BIT<br>
<br>
</span>Done. I added it locally.<br>
<br>
> > +       !pitch_in_range(row_pitch, RENDER_SURFACE_STATE_<br>
<span class="">> > SurfacePitch_bits(gen_10x)))<br>
> > +      return false;<br>
> > +<br>
> > +   if (((surf_info->usage & ISL_SURF_USAGE_CCS_BIT) ||<br>
> > +        (surf_info->usage & ISL_SURF_USAGE_MCS_BIT)) &&<br>
> > +       !pitch_in_range(row_pitch_<wbr>tiles, RENDER_SURFACE_STATE_<br>
> > AuxiliarySurfacePitch_bits(<wbr>gen_10x)))<br>
> > +      return false;<br>
> > +<br>
> > +   if ((surf_info->usage & ISL_SURF_USAGE_DEPTH_BIT) &&<br>
> > +       !pitch_in_range(row_pitch, _3DSTATE_DEPTH_BUFFER_<br>
> > SurfacePitch_bits(gen_10x)))<br>
> > +      return false;<br>
> > +<br>
> > +   if ((surf_info->usage & ISL_SURF_USAGE_HIZ_BIT) &&<br>
> > +       !pitch_in_range(row_pitch, _3DSTATE_HIER_DEPTH_BUFFER_<br>
> > SurfacePitch_bits(gen_10x)))<br>
> > +      return false;<br>
> > +<br>
> > +   if (surf_info->usage & ISL_SURF_USAGE_STENCIL_BIT)<br>
> > +      isl_finishme("validate row pitch of stencil surfaces");<br>
> ><br>
><br>
> How hard is this?  I assume it's not trivial or you would have done it.<br>
<br>
</span>It's not trivial. There's corner cases, especially for older gens.  But<br>
it's not *too* hard. It's just that I have no confidence that I could<br>
write it correctly on the first try.<br>
<br>
I wanted to land all the obvious pitch validations asap. Where obvious<br>
means:<br>
<br>
   if ((usage & bits) && !pitch_in_range())<br>
      return false;<br>
<br>
Since stencil pitch validation is the only non-obvious validation,<br>
I wanted to do it as a follow-up. Because there WILL be errors in v1 of<br>
the patch. And I didn't want this patch to be blocked while I debugged<br>
the stencil validation failures and argued over the corner cases.<br>
</blockquote></div><br></div><div class="gmail_extra">Fine with me.<br></div></div>