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-sta...@lists.freedesktop.org>
> > Signed-off-by: Nanley Chery <nanley.g.ch...@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 <kenn...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
