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

[Kenneth Graunke]

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...

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.

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>

> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170512/9ac521ba/attachment.sig>