On Mon, Jun 20, 2016 at 3:53 PM, Chad Versace <chad.vers...@intel.com>
wrote:
> 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 = \
> > + libisl-gen4.la \
> > + libisl-gen5.la \
> > + libisl-gen6.la \
> > libisl-gen7.la \
> > libisl-gen75.la \
> > libisl-gen8.la \
> > @@ -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;
>
> 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);
> }
>
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!
>
>
>
> > + 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.
>
My local fix just changes buffers to use the gen5 version for 4, 4.5, and 5
>
> > + 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
>
> Wow. The hardware enum names are awful. WTF is MSFMT_MSS?
>
> > 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++;
> > +
>
> Ouch, that's a surprising workaround.
>
Yes, yes it is. We'll have to add a similar hack to isl's gen6 layout code.
>
> > 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,
>
> 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.
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
