[Mesa-dev] [PATCH 36/64] isl: Add support for filling out surface states all the way back to gen4
Chad Versace
chad.versace at intel.com
Mon Jun 20 22:53:26 UTC 2016
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);
}
> + 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.
> + 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.
> 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.
More information about the mesa-dev
mailing list