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

Submitted by Neil Roberts on Dec. 11, 2015, 12:32 p.m.

Details

Message ID 1449837138-3358-3-git-send-email-neil@linux.intel.com
State Accepted
Commit 839793680f99b8387bee9489733d5071c10f3ace
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Neil Roberts Dec. 11, 2015, 12:32 p.m.
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@lists.freedesktop.org>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Suggested-by: Ilia Mirkin <imirkin@alum.mit.edu>
---
 src/mesa/drivers/dri/i965/intel_screen.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

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;
    }
 

Comments

On Fri, Dec 11, 2015 at 7:32 AM, Neil Roberts <neil@linux.intel.com> wrote:
> 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@lists.freedesktop.org>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Suggested-by: Ilia Mirkin <imirkin@alum.mit.edu>

FWIW this all seems reasonable to me, but based on personal
experience, format handling is some of the more subtle code in a
driver, with many different parts relying on cooperative behaviour
between them, so someone more familiar with the specifics will have to
review.

Cheers,

  -ilia

> ---
>  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;
>     }
>
> --
> 1.9.3
>
Neil Roberts <neil@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@lists.freedesktop.org>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Suggested-by: Ilia Mirkin <imirkin@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@bitplanet.net>

>     }
>  
> -- 
> 1.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Fri, Dec 11, 2015 at 1:02 PM, Kristian Høgsberg <hoegsberg@gmail.com> wrote:
> Neil Roberts <neil@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@lists.freedesktop.org>
>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
>> Suggested-by: Ilia Mirkin <imirkin@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

Well, for some reason, i965 doesn't expose *any* srgb-capable visuals.
But previoiusly all the 8-bit color ones used to be srgb-capable
anyways, whereas after BGRX was added, suddenly requesting an
alpha-less visual would silently lose you srgb, with no way to get it
back other than to guess that alpha = srgb.

The application shouldn't require a visual without srgb advertised to
produce srgb-capable framebuffers, so that's an application bug.
However the situation with srgb vs non-srgb was also a little
confusing, so I think it's worth straightening it out.

  -ilia
On Friday, December 11, 2015 12:32:18 PM Neil Roberts wrote:
> 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@lists.freedesktop.org>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Suggested-by: Ilia Mirkin <imirkin@alum.mit.edu>
> ---
>  src/mesa/drivers/dri/i965/intel_screen.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)

The whole series is:
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

We definitely should have the same behavior regardless of whether the
config has an alpha channel.  So, this is good.
Hi

These three commits have stopped Plasma5's kwin  working on my skylake
system

Reverting back to 7752bbc44e78e982de3cd4c34862adc38a338234 fixed it for me

I can send you more details / raise a bug if you like

Cheers

Mike

On Sat, 12 Dec 2015 at 20:56 Kenneth Graunke <kenneth@whitecape.org> wrote:

> On Friday, December 11, 2015 12:32:18 PM Neil Roberts wrote:
> > 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@lists.freedesktop.org>
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > Suggested-by: Ilia Mirkin <imirkin@alum.mit.edu>
> > ---
> >  src/mesa/drivers/dri/i965/intel_screen.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
>
> The whole series is:
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
>
> We definitely should have the same behavior regardless of whether the
> config has an alpha channel.  So, this is good.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Dec 13, 2015 2:06 PM, "Mike Lothian" <mike@fireburn.co.uk> wrote:
>
> Hi
>
> These three commits have stopped Plasma5's kwin  working on my skylake
system

As another data point, it also breaks XWayland+DRI3 on my BDW. I can't even
run glsgears without it segfaulting.

> Reverting back to 7752bbc44e78e982de3cd4c34862adc38a338234 fixed it for me
>
> I can send you more details / raise a bug if you like
>
> Cheers
>
> Mike
>
> On Sat, 12 Dec 2015 at 20:56 Kenneth Graunke <kenneth@whitecape.org>
wrote:
>>
>> On Friday, December 11, 2015 12:32:18 PM Neil Roberts wrote:
>> > 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@lists.freedesktop.org>
>> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
>> > Suggested-by: Ilia Mirkin <imirkin@alum.mit.edu>
>> > ---
>> >  src/mesa/drivers/dri/i965/intel_screen.c | 13 ++++++-------
>> >  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> The whole series is:
>> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
>>
>> We definitely should have the same behavior regardless of whether the
>> config has an alpha channel.  So, this is good.
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>