[Mesa-dev] [PATCH 3/3] i965: Use MESA_FORMAT_B8G8R8X8_SRGB for RGB visuals

[Kristian Høgsberg]

Neil Roberts <neil at linux.intel.com> writes:

> Previously if the visual didn't have an alpha channel then it would
> pick a format that is not sRGB-capable. I don't think there's any
> reason not to always have an sRGB-capable visual. Since 28090b30 there
> are now visuals advertised without an alpha channel which means that
> games that don't request alpha bits in the config would end up without
> an sRGB-capable visual. This was breaking supertuxkart which assumes
> the winsys buffer is always sRGB-capable.
>
> The previous code always used an RGBA format if the visual config
> itself was marked as sRGB-capable regardless of whether the visual has
> alpha bits. I think we don't actually advertise any sRGB-capable
> visuals (but we just use sRGB formats anyway) so it shouldn't make any
> difference. However this patch also changes it to use RGBX if an
> sRGB-capable visual is requested without alpha bits for consistency.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92759
> Cc: "11.0 11.1" <mesa-stable at lists.freedesktop.org>
> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> Suggested-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>  src/mesa/drivers/dri/i965/intel_screen.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> index cc90efe..825a7c1 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -999,14 +999,13 @@ intelCreateBuffer(__DRIscreen * driScrnPriv,
>        fb->Visual.samples = num_samples;
>     }
>  
> -   if (mesaVis->redBits == 5)
> +   if (mesaVis->redBits == 5) {
>        rgbFormat = MESA_FORMAT_B5G6R5_UNORM;
> -   else if (mesaVis->sRGBCapable)
> -      rgbFormat = MESA_FORMAT_B8G8R8A8_SRGB;
> -   else if (mesaVis->alphaBits == 0)
> -      rgbFormat = MESA_FORMAT_B8G8R8X8_UNORM;
> -   else {
> -      rgbFormat = MESA_FORMAT_B8G8R8A8_SRGB;
> +   } else {
> +      if (mesaVis->alphaBits == 0)
> +         rgbFormat = MESA_FORMAT_B8G8R8X8_SRGB;
> +      else
> +         rgbFormat = MESA_FORMAT_B8G8R8A8_SRGB;
>        fb->Visual.sRGBCapable = true;

I agree with Ilia that this code is notoriously subtle and prone to
breaking applications. While I don't fully understand why we use
MESA_FORMAT_B8G8R8A8_SRGB for !mesaVis->sRGBCapable, the change you're
making makes sense. If we can get a Tested-by from the reporter, I'll go
out on a limb and add

Reviewed-by: Kristian Høgsberg <krh at bitplanet.net>

>     }
>  
> -- 
> 1.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev