On Thu, Jul 02, 2015 at 12:58:33PM -0700, Matt Turner wrote:
> On Thu, Jul 2, 2015 at 12:57 PM, Matt Turner <matts...@gmail.com> wrote:
> > On Wed, Jul 1, 2015 at 4:03 PM, Ben Widawsky
> > <benjamin.widaw...@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 <b...@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;
> > }
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
