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

[Ben Widawsky]

On Thu, Jul 02, 2015 at 12:58:33PM -0700, Matt Turner wrote:
> 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.
> 

Thanks for catching the missing case.

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