<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Jun 20, 2016 at 3:53 PM, Chad Versace <<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Sat 11 Jun 2016, Jason Ekstrand wrote: > --- > src/intel/isl/Makefile.am | 12 ++++++++ > src/intel/isl/Makefile.sources | 13 +++++++-- > src/intel/isl/isl.c | 28 +++++++++++++++++++ > src/intel/isl/isl_priv.h | 24 ++++++++++++++++ > src/intel/isl/isl_surface_state.c | 58 ++++++++++++++++++++++++++++++++++++--- > 5 files changed, 129 insertions(+), 6 deletions(-) > > diff --git a/src/intel/isl/Makefile.am b/src/intel/isl/Makefile.am > index 74f863a..ae367a9 100644 > --- a/src/intel/isl/Makefile.am > +++ b/src/intel/isl/Makefile.am > @@ -22,6 +22,9 @@ > include Makefile.sources > > ISL_GEN_LIBS = \ > + <a href="http://libisl-gen4.la" rel="noreferrer" target="_blank">libisl-gen4.la</a> \ > + <a href="http://libisl-gen5.la" rel="noreferrer" target="_blank">libisl-gen5.la</a> \ > + <a href="http://libisl-gen6.la" rel="noreferrer" target="_blank">libisl-gen6.la</a> \ > <a href="http://libisl-gen7.la" rel="noreferrer" target="_blank">libisl-gen7.la</a> \ > <a href="http://libisl-gen75.la" rel="noreferrer" target="_blank">libisl-gen75.la</a> \ > <a href="http://libisl-gen8.la" rel="noreferrer" target="_blank">libisl-gen8.la</a> \ > @@ -52,6 +55,15 @@ libisl_la_LIBADD = $(ISL_GEN_LIBS) > > libisl_la_SOURCES = $(ISL_FILES) $(ISL_GENERATED_FILES) > > +libisl_gen4_la_SOURCES = $(ISL_GEN4_FILES) > +libisl_gen4_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=40 > + > +libisl_gen5_la_SOURCES = $(ISL_GEN5_FILES) > +libisl_gen5_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=50 > + > +libisl_gen6_la_SOURCES = $(ISL_GEN6_FILES) > +libisl_gen6_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=60 > + > libisl_gen7_la_SOURCES = $(ISL_GEN7_FILES) > libisl_gen7_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=70 > > diff --git a/src/intel/isl/Makefile.sources b/src/intel/isl/Makefile.sources > index 89b1418..aa20ed4 100644 > --- a/src/intel/isl/Makefile.sources > +++ b/src/intel/isl/Makefile.sources > @@ -2,12 +2,21 @@ ISL_FILES = \ > isl.c \ > isl.h \ > isl_format.c \ > + isl_priv.h \ > + isl_storage_image.c > + > +ISL_GEN4_FILES = \ > isl_gen4.c \ > isl_gen4.h \ > + isl_surface_state.c > + > +ISL_GEN5_FILES = \ > + isl_surface_state.c > + > +ISL_GEN6_FILES = \ > isl_gen6.c \ > isl_gen6.h \ > - isl_priv.h \ > - isl_storage_image.c > + isl_surface_state.c > > ISL_GEN7_FILES = \ > isl_gen7.c \ > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c > index 77b570d..7343a55 100644 > --- a/src/intel/isl/isl.c > +++ b/src/intel/isl/isl.c > @@ -1191,6 +1191,20 @@ isl_surf_fill_state_s(const struct isl_device *dev, void *state, > } > > switch (ISL_DEV_GEN(dev)) { > + case 4: > + if (ISL_DEV_IS_G4X(dev)) { > + isl_gen4_surf_fill_state_s(dev, state, info); > + } else { > + /* G45 surface state is the same as gen5 */ > + isl_gen5_surf_fill_state_s(dev, state, info); > + } > + break; </div></div>The above can't be correct, because... #define GEN_IS_G4X ((GEN_VERSIONx10) == 45) I think you meant this: if (ISL_DEV_IS_G4X(dev)) { /* G45 surface state is the same as gen5 */ isl_gen5_surf_fill_state_s(dev, state, info); } else { isl_gen4_surf_fill_state_s(dev, state, info); } </blockquote><div> </div><div>You are correct. The reason it didn't trigger a bug is that the only difference bertween gen4 and 4.5 is that x/y offsets are introduced in 4.5. Since we never use ISL for filling out surfaces with x/y offsets on gen4.5 with this series, I never caught it. Thanks! </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + case 5: > + isl_gen5_surf_fill_state_s(dev, state, info); > + break; > + case 6: > + isl_gen6_surf_fill_state_s(dev, state, info); > + break; > case 7: > if (ISL_DEV_IS_HASWELL(dev)) { > isl_gen75_surf_fill_state_s(dev, state, info); > @@ -1214,6 +1228,20 @@ isl_buffer_fill_state_s(const struct isl_device *dev, void *state, > const struct isl_buffer_fill_state_info *restrict info) > { > switch (ISL_DEV_GEN(dev)) { > + case 4: > + if (ISL_DEV_IS_G4X(dev)) { > + isl_gen4_buffer_fill_state_s(state, info); > + } else { > + /* G45 surface state is the same as gen5 */ > + isl_gen5_buffer_fill_state_s(state, info); > + } > + break; Same as above. </blockquote><div> </div><div>My local fix just changes buffers to use the gen5 version for 4, 4.5, and 5 </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + case 5: > + isl_gen5_buffer_fill_state_s(state, info); > + break; > + case 6: > + isl_gen6_buffer_fill_state_s(state, info); > + break; > case 7: > if (ISL_DEV_IS_HASWELL(dev)) { > isl_gen75_buffer_fill_state_s(state, info); > diff --git a/src/intel/isl/isl_priv.h b/src/intel/isl/isl_priv.h > index d98e707..3a7af1a 100644 > --- a/src/intel/isl/isl_priv.h > +++ b/src/intel/isl/isl_priv.h > @@ -136,6 +136,18 @@ isl_extent3d_el_to_sa(enum isl_format fmt, struct isl_extent3d extent_el) > } > > void > +isl_gen4_surf_fill_state_s(const struct isl_device *dev, void *state, > + const struct isl_surf_fill_state_info *restrict info); > + > +void > +isl_gen5_surf_fill_state_s(const struct isl_device *dev, void *state, > + const struct isl_surf_fill_state_info *restrict info); > + > +void > +isl_gen6_surf_fill_state_s(const struct isl_device *dev, void *state, > + const struct isl_surf_fill_state_info *restrict info); > + > +void > isl_gen7_surf_fill_state_s(const struct isl_device *dev, void *state, > const struct isl_surf_fill_state_info *restrict info); > > @@ -150,6 +162,18 @@ isl_gen9_surf_fill_state_s(const struct isl_device *dev, void *state, > const struct isl_surf_fill_state_info *restrict info); > > void > +isl_gen4_buffer_fill_state_s(void *state, > + const struct isl_buffer_fill_state_info *restrict info); > + > +void > +isl_gen5_buffer_fill_state_s(void *state, > + const struct isl_buffer_fill_state_info *restrict info); > + > +void > +isl_gen6_buffer_fill_state_s(void *state, > + const struct isl_buffer_fill_state_info *restrict info); > + > +void > isl_gen7_buffer_fill_state_s(void *state, > const struct isl_buffer_fill_state_info *restrict info); > > diff --git a/src/intel/isl/isl_surface_state.c b/src/intel/isl/isl_surface_state.c > index fb3fd99..53ff56f 100644 > --- a/src/intel/isl/isl_surface_state.c > +++ b/src/intel/isl/isl_surface_state.c > @@ -78,11 +78,13 @@ static const uint8_t isl_to_gen_tiling[] = { > }; > #endif > > +#if GEN_GEN >= 7 > static const uint32_t isl_to_gen_multisample_layout[] = { > [ISL_MSAA_LAYOUT_NONE] = MSFMT_MSS, > [ISL_MSAA_LAYOUT_INTERLEAVED] = MSFMT_DEPTH_STENCIL, > [ISL_MSAA_LAYOUT_ARRAY] = MSFMT_MSS, > }; > +#endif </div></div>Wow. The hardware enum names are awful. WTF is MSFMT_MSS? <div><div class="h5"> > static uint8_t > get_surftype(enum isl_surf_dim dim, isl_surf_usage_flags_t usage) > @@ -112,7 +114,7 @@ get_surftype(enum isl_surf_dim dim, isl_surf_usage_flags_t usage) > * Get the values to pack into RENDER_SUFFACE_STATE.SurfaceHorizontalAlignment > * and SurfaceVerticalAlignment. > */ > -static struct isl_extent3d > +static inline struct isl_extent3d > get_image_alignment(const struct isl_surf *surf) > { > if (GEN_GEN >= 9) { > @@ -199,6 +201,23 @@ isl_genX(surf_fill_state_s)(const struct isl_device *dev, void *state, > s.Width = info->surf->logical_level0_px.width - 1; > s.Height = info->surf->logical_level0_px.height - 1; > > + /* In the gen6 PRM Volume 1 Part 1: Graphics Core, Section 7.18.3.7.1 > + * (Surface Arrays For all surfaces other than separate stencil buffer): > + * > + * "[DevSNB] Errata: Sampler MSAA Qpitch will be 4 greater than the value > + * calculated in the equation above , for every other odd Surface Height > + * starting from 1 i.e. 1,5,9,13" > + * > + * Since this Qpitch errata only impacts the sampler, we have to adjust the > + * input for the rendering surface to achieve the same qpitch. For the > + * affected heights, we increment the height by 1 for the rendering > + * surface. > + */ > + if (GEN_GEN == 6 && (info->view->usage & ISL_SURF_USAGE_RENDER_TARGET_BIT) && > + info->surf->samples > 1 && > + (info->surf->logical_level0_px.height % 4) == 1) > + s.Height++; > + </div></div>Ouch, that's a surprising workaround. </blockquote><div> </div><div>Yes, yes it is. We'll have to add a similar hack to isl's gen6 layout code. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > switch (s.SurfaceType) { > case SURFTYPE_1D: > case SURFTYPE_2D: > @@ -251,7 +270,9 @@ isl_genX(surf_fill_state_s)(const struct isl_device *dev, void *state, > unreachable("bad SurfaceType"); > } > > +#if GEN_GEN >= 7 > s.SurfaceArray = info->surf->dim != ISL_SURF_DIM_3D; > +#endif > > if (info->view->usage & ISL_SURF_USAGE_RENDER_TARGET_BIT) { > /* For render target surfaces, the hardware interprets field > @@ -279,9 +300,13 @@ isl_genX(surf_fill_state_s)(const struct isl_device *dev, void *state, > s.MipTailStartLOD = 15; > #endif > > +#if GEN_GEN >= 6 > const struct isl_extent3d image_align = get_image_alignment(info->surf); > s.SurfaceVerticalAlignment = isl_to_gen_valign[image_align.height]; > +#if GEN_GEN >= 7 > s.SurfaceHorizontalAlignment = isl_to_gen_halign[image_align.width]; > +#endif > +#endif > > if (info->surf->tiling == ISL_TILING_W) { > /* From the Broadwell PRM documentation for this field: > @@ -333,9 +358,13 @@ isl_genX(surf_fill_state_s)(const struct isl_device *dev, void *state, > #endif > } > > +#if GEN_GEN >= 6 > + s.NumberofMultisamples = ffs(info->surf->samples) - 1; > +#if GEN_GEN >= 7 > s.MultisampledSurfaceStorageFormat = > isl_to_gen_multisample_layout[info->surf->msaa_layout]; > - s.NumberofMultisamples = ffs(info->surf->samples) - 1; > +#endif > +#endif > > #if (GEN_GEN >= 8 || GEN_IS_HASWELL) > s.ShaderChannelSelectRed = info->view->channel_select[0]; > @@ -345,11 +374,14 @@ isl_genX(surf_fill_state_s)(const struct isl_device *dev, void *state, > #endif > > s.SurfaceBaseAddress = info->address; > + > +#if GEN_GEN >= 6 > s.MOCS = info->mocs; > +#endif > > #if GEN_GEN >= 8 > s.AuxiliarySurfaceMode = AUX_NONE; > -#else > +#elif GEN_GEN >= 7 > s.MCSEnable = false; > #endif > > @@ -424,15 +456,31 @@ isl_genX(buffer_fill_state_s)(void *state, > struct GENX(RENDER_SURFACE_STATE) s = { 0 }; > > s.SurfaceType = SURFTYPE_BUFFER, > - s.SurfaceArray = false, > s.SurfaceFormat = info->format, > + > +#if GEN_GEN >= 6 > s.SurfaceVerticalAlignment = isl_to_gen_valign[4], > +#if GEN_GEN >= 7 > s.SurfaceHorizontalAlignment = isl_to_gen_halign[4], > + s.SurfaceArray = false, > +#endif > +#endif > + > +#if GEN_GEN >= 7 > s.Height = ((num_elements - 1) >> 7) & 0x3fff, > s.Width = (num_elements - 1) & 0x7f, > s.Depth = ((num_elements - 1) >> 21) & 0x3ff, > +#else > + s.Height = ((num_elements - 1) >> 7) & 0x1fff, > + s.Width = (num_elements - 1) & 0x7f, > + s.Depth = ((num_elements - 1) >> 20) & 0x7f, > +#endif > + > s.SurfacePitch = info->stride - 1, > + > +#if GEN_GEN >= 6 > s.NumberofMultisamples = MULTISAMPLECOUNT_1, > +#endif > > #if (GEN_GEN >= 8) > s.TileMode = LINEAR, > @@ -447,7 +495,9 @@ isl_genX(buffer_fill_state_s)(void *state, > #endif > > s.SurfaceBaseAddress = info->address, > +#if GEN_GEN >= 6 > s.MOCS = info->mocs, > +#endif > > #if (GEN_GEN >= 8 || GEN_IS_HASWELL) > s.ShaderChannelSelectRed = SCS_RED, </div></div>I'm really pleased with how this function looks after adding support for all gens. You've been able to populate non-buffer surface state in a single function for all gens, and it's very readable. It's much more readable than having various functions per gen, like i965. Before I began reviewing this series, I didn't expect this would be possible. Waiting to hear your reply on the G4X comment. </blockquote></div> </div></div>