<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Mar 28, 2017 at 9:15 AM, Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sat 25 Mar 2017, Jason Ekstrand wrote: > > > On March 24, 2017 7:29:05 PM Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> 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. </blockquote><div> </div><div>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... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > + > > + 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. </blockquote></div> </div><div class="gmail_extra">That's fine. </div></div>