[Mesa-dev] [PATCH 2/2] i965/formats: Update the RGB_DXT1 mappings

[Nanley Chery]

On Fri, May 12, 2017 at 08:39:40AM -0700, Kenneth Graunke wrote:
> On Thursday, May 11, 2017 4:46:27 PM PDT Nanley Chery wrote:
> > The DXT1_RGB* format does not provide the correct behavior for OpenGL in
> > the case where color_0 <= color_1. BC1_RGB_UNORM with a alpha set to 1
> > provides the behavior which matches the spec.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100925
> > Cc: <mesa-stable at lists.freedesktop.org>
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_surface_formats.c | 15 ++-------------
> >  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> Two things I was worried about:
> 
> 1. Where's the swizzling code in this patch?
> 
>   ->  it turns out that we already swizzle A -> 1 when the mesa format
>       does not have alpha, but the hardware format does.  So, it's
>       already there, we don't need any new code :)
> 
> 2. Will it impact performance?
> 
>   ->  Might be nice to check...but the compressed data is the same size
>       for both formats, so it's just how it interprets it...doubtful.
> 
>       This will cause recompiles on Ivybridge and older hardware, due
>       to the implicit texture swizzling.  Sad, but not sure we can do
>       anything about it.  Could change the default to RGB1 if that's
>       better...
> 

Oh, I didn't know about about those recompiles. I'm not sure how they
work.

> It might be nice to copy more of the findings you posted in bugzilla
> into the commit messages - people are more likely to find your commit
> message when blaming this code than to look at the bug.
> 

I added more detail to the commit messages and will send out a v2.

> It looks like Windows does this same workaround on Gen7.5+.  I'm not
> clear what they do on Gen7-.
> 
> Great find!  Both are:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> 

Thank you!

> > diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c b/src/mesa/drivers/dri/i965/brw_surface_formats.c
> > index 7b17e11125..b176a21c22 100644
> > --- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
> > +++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
> > @@ -94,14 +94,14 @@ brw_isl_format_for_mesa_format(mesa_format mesa_format)
> >        [MESA_FORMAT_L_SRGB8] = ISL_FORMAT_L8_UNORM_SRGB,
> >        [MESA_FORMAT_L8A8_SRGB] = ISL_FORMAT_L8A8_UNORM_SRGB,
> >        [MESA_FORMAT_A8L8_SRGB] = 0,
> > -      [MESA_FORMAT_SRGB_DXT1] = ISL_FORMAT_DXT1_RGB_SRGB,
> > +      [MESA_FORMAT_SRGB_DXT1] = ISL_FORMAT_BC1_UNORM_SRGB,
> >        [MESA_FORMAT_SRGBA_DXT1] = ISL_FORMAT_BC1_UNORM_SRGB,
> >        [MESA_FORMAT_SRGBA_DXT3] = ISL_FORMAT_BC2_UNORM_SRGB,
> >        [MESA_FORMAT_SRGBA_DXT5] = ISL_FORMAT_BC3_UNORM_SRGB,
> >  
> >        [MESA_FORMAT_RGB_FXT1] = ISL_FORMAT_FXT1,
> >        [MESA_FORMAT_RGBA_FXT1] = ISL_FORMAT_FXT1,
> > -      [MESA_FORMAT_RGB_DXT1] = ISL_FORMAT_DXT1_RGB,
> > +      [MESA_FORMAT_RGB_DXT1] = ISL_FORMAT_BC1_UNORM,
> >        [MESA_FORMAT_RGBA_DXT1] = ISL_FORMAT_BC1_UNORM,
> >        [MESA_FORMAT_RGBA_DXT3] = ISL_FORMAT_BC2_UNORM,
> >        [MESA_FORMAT_RGBA_DXT5] = ISL_FORMAT_BC3_UNORM,
> > @@ -541,17 +541,6 @@ translate_tex_format(struct brw_context *brw,
> >         */
> >        return ISL_FORMAT_R32G32B32A32_FLOAT;
> >  
> > -   case MESA_FORMAT_SRGB_DXT1:
> > -      if (brw->gen == 4 && !brw->is_g4x) {
> > -         /* Work around missing SRGB DXT1 support on original gen4 by just
> > -          * skipping SRGB decode.  It's not worth not supporting sRGB in
> > -          * general to prevent this.
> > -          */
> > -         WARN_ONCE(true, "Demoting sRGB DXT1 texture to non-sRGB\n");
> > -         mesa_format = MESA_FORMAT_RGB_DXT1;
> > -      }
> > -      return brw_isl_format_for_mesa_format(mesa_format);
> > -
> >     case MESA_FORMAT_RGBA_ASTC_4x4:
> >     case MESA_FORMAT_RGBA_ASTC_5x4:
> >     case MESA_FORMAT_RGBA_ASTC_5x5:
> > 
> 
> Nice to be rid of this!  Using an RGBA format and setting alpha to 1 is
> a lot better than not doing sRGB decode.

Agreed.