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

[Matt Turner]

On Thu, Jul 2, 2015 at 12:57 PM, Matt Turner <mattst88 at gmail.com> wrote:
> 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 ||

Bah, would be nice to so BC5_UNORM and then BC5_SNORM to better match
the comment.

>        format == BRW_SURFACEFORMAT_BC7_UNORM)
>       surf[0] |= GEN8_SURFACE_SAMPLER_L2_BYPASS_DISABLE;
>    }