[Mesa-dev] [PATCH] [v2] i965/chv|skl: Apply sampler bypass w/a

[Matt Turner]

On Wed, Jul 8, 2015 at 5:04 PM, Ben Widawsky
<benjamin.widawsky at intel.com> wrote:
> Certain compressed formats require this setting. The docs don't go into much
> detail as to why it's needed exactly.
>
> This patch introduces no piglit regressions on gen9 (bsw is untested). Note that
> the SKL "regressions" are fixed tests, and the egl_khr_gl_colorspace tests are
> WTF. The patch also fixes nothing I can find.
> http://otc-mesa-ci.jf.intel.com/job/Leeroy/127820/
>
> v2:
> Reworded commit message (Matt); Added piglit results link.
> Restructured condition (Matt)
> Moved check out to function (Nanley). I left the setting of the bit in the
>   surface state open coded because it seems to go better with the existing code.
>
> Cc: Matt Turner <mattst88 at gmail.com>
> Cc: Nanley Chery <nanleychery at gmail.com>
> Cc: Jordan Justen <jordan.l.justen at intel.com> (aux-hiz needs this too)
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_defines.h        |  1 +
>  src/mesa/drivers/dri/i965/gen8_surface_state.c | 29 ++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> index 19489ab..3668967 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -276,6 +276,7 @@
>  #define GEN8_SURFACE_TILING_W                       (1 << 12)
>  #define GEN8_SURFACE_TILING_X                       (2 << 12)
>  #define GEN8_SURFACE_TILING_Y                       (3 << 12)
> +#define GEN8_SURFACE_SAMPLER_L2_BYPASS_DISABLE      (1 << 9)
>  #define BRW_SURFACE_RC_READ_WRITE      (1 << 8)
>  #define BRW_SURFACE_MIPLAYOUT_SHIFT    10
>  #define BRW_SURFACE_MIPMAPLAYOUT_BELOW   0
> diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> index bd3eb00..9bbe8ae 100644
> --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> @@ -132,6 +132,27 @@ horizontal_alignment(const struct brw_context *brw,
>     }
>  }
>
> +static bool
> +sampler_l2_bypass_disable(struct brw_context *brw, unsigned format)
> +{
> +   /* From the CHV PRM, Volume 2d, page 321 (RENDER_SURFACE_STATE dword 0
> +    * bit 9 "Sampler L2 Bypass Mode Disable" Programming Notes):
> +    *
> +    *    This bit must be set for the following surface types: BC2_UNORM
> +    *    BC3_UNORM BC5_UNORM BC5_SNORM BC7_UNORM
> +    */
> +   if ((brw->gen >= 9 || brw->is_cherryview) &&
> +       (format == BRW_SURFACEFORMAT_BC2_UNORM ||
> +        format == BRW_SURFACEFORMAT_BC3_UNORM ||
> +        format == BRW_SURFACEFORMAT_BC5_UNORM ||
> +        format == BRW_SURFACEFORMAT_BC5_SNORM ||
> +        format == BRW_SURFACEFORMAT_BC7_UNORM)) {
> +      return true;
> +   }
> +
> +   return false;
> +}
> +
>  static uint32_t *
>  allocate_surface_state(struct brw_context *brw, uint32_t *out_offset, int index)
>  {
> @@ -238,6 +259,10 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
>        surf[0] |= BRW_SURFACE_CUBEFACE_ENABLES;
>     }
>
> +   if (sampler_l2_bypass_disable(brw, format)) {
> +      surf[0] |= GEN8_SURFACE_SAMPLER_L2_BYPASS_DISABLE;
> +   }
> +
>     if (_mesa_is_array_texture(target) || target == GL_TEXTURE_CUBE_MAP)
>        surf[0] |= GEN8_SURFACE_IS_ARRAY;
>
> @@ -465,6 +490,10 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
>               horizontal_alignment(brw, mt, surf_type) |
>               surface_tiling_mode(tiling);
>
> +   if (sampler_l2_bypass_disable(brw, format)) {
> +      surf[0] |= GEN8_SURFACE_SAMPLER_L2_BYPASS_DISABLE;
> +   }

I don't think any of the formats this workaround applies to can be
render targets, can they?

All BC* RT entries in the brw_surface_formats.c table are 'x'
(unsupported) and I don't think new hardware is going to implement a
compressor for these. :)

Remove the change to gen8_update_renderbuffer_surface, and inline the
function in its one actual use in gen8_emit_texture_surface_state(),
and it gets a

Reviewed-by: Matt Turner <mattst88 at gmail.com>

> +
>     surf[1] = SET_FIELD(mocs, GEN8_SURFACE_MOCS) | mt->qpitch >> 2;
>
>     surf[2] = SET_FIELD(width - 1, GEN7_SURFACE_WIDTH) |
> --
> 2.4.4
>