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

[Matt Turner]

On Wed, Jul 1, 2015 at 4:03 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 fixes 0 piglit failures with a GBM gpu piglit run.

That's a really weird way of saying that.

>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>
> I had this one sitting around for almost 2 months. I'm not sure why I didn't
> send it out sooner. It seems like it's needed.
>
> ---
>  src/mesa/drivers/dri/i965/brw_defines.h        |  1 +
>  src/mesa/drivers/dri/i965/gen8_surface_state.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> index 66b9abc..f55fd49 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 ca5ed17..a245379 100644
> --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> @@ -264,6 +264,19 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
>        surf[0] |= BRW_SURFACE_CUBEFACE_ENABLES;
>     }
>
> +   if (brw->is_cherryview || brw->gen >= 9) {
> +      /* "This bit must be set for the following surface types: BC2_UNORM
> +       * BC3_UNORM BC5_UNORM BC5_SNORM BC7_UNORM"

Don't do naked quotes -- Use the normal style.

> +       */
> +      switch (format) {
> +      case BRW_SURFACEFORMAT_BC2_UNORM:
> +      case BRW_SURFACEFORMAT_BC3_UNORM:
> +      case BRW_SURFACEFORMAT_BC5_SNORM:

Missing BRW_SURFACEFORMAT_BC5_UNORM.

> +      case BRW_SURFACEFORMAT_BC7_UNORM:
> +         surf[0] |= GEN8_SURFACE_SAMPLER_L2_BYPASS_DISABLE;

It wouldn't surprise me if static analysis tools complain about the
missing break.

Add a break or make it an if statement (which would be two lines
shorter). All together, how about

   /* 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 (format == BRW_SURFACEFORMAT_BC2_UNORM ||
       format == BRW_SURFACEFORMAT_BC3_UNORM ||
       format == BRW_SURFACEFORMAT_BC5_SNORM ||
       format == BRW_SURFACEFORMAT_BC5_UNORM ||
       format == BRW_SURFACEFORMAT_BC7_UNORM)
      surf[0] |= GEN8_SURFACE_SAMPLER_L2_BYPASS_DISABLE;
   }