Re: [Mesa-dev] [PATCH] i965: handle stencil_bits parameter for MESA_FORMAT_B8G8R8X8_UNORM format.

[Neil Roberts]

Ilia Mirkin  writes:

> I suspect that something like this may be the right thing for the
> intel driver... no reason not to expose sRGB-capable visuals when it
> so happens that alpha == 0. Also probably the same treatment should be
> done for BGR565... sRGB encoding will matter even more there, and from
> the looks of it, all gens support that.
>
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
> b/src/mesa/drivers/dri/i965/intel_screen.c
> index cc90efe..75f2cce 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -1003,9 +1003,10 @@ intelCreateBuffer(__DRIscreen * driScrnPriv,
>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 {
> +   else if (mesaVis->alphaBits == 0) {
> +  rgbFormat = MESA_FORMAT_B8G8R8X8_SRGB;
> +  fb->Visual.sRGBCapable = true;
> +   } else {
>rgbFormat = MESA_FORMAT_B8G8R8A8_SRGB;
>fb->Visual.sRGBCapable = true;
> }

Thanks for the suggestion. I assume you didn't want to make this into a
proper patch yourself so I went ahead and did it here:

http://patchwork.freedesktop.org/patch/67844/

Along with the other two patches it doesn't cause any regressions in our
CI system.

I left the 565 format alone because there is no enum for
MESA_FORMAT_B5G6R5_SRGB and it seemed like it's better treated as a
separate issue.

We should also fix the fact that the visuals aren't marked as
sRGB-capable even though they are, but again I think that is a separate
issue.

Regards,
- Neil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: handle stencil_bits parameter for MESA_FORMAT_B8G8R8X8_UNORM format.

[Ilia Mirkin]

On Wed, Dec 9, 2015 at 2:15 PM, Ilia Mirkin  wrote:
> On Wed, Dec 9, 2015 at 11:23 AM, Ilia Mirkin  wrote:
>> On Wed, Dec 9, 2015 at 11:18 AM, Deve  wrote:
>>> This patch indeed seems to not have a sense. I just added it to the bug
>>> report as a suggestion that it works for me after this modification. Emil
>>> Velikov said that I should send it to the mailing list.
>>>
>>> Here is how it works in Supertuxkart:
>>> We create rtt with following parameters:
>>>
>>> DepthStencilTexture = generateRTT(res, GL_DEPTH24_STENCIL8,
>>> GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8);
>>>
>>> Then, during rendering scene, we do:
>>>
>>> glEnable(GL_FRAMEBUFFER_SRGB);
>>> glBindFramebuffer(GL_FRAMEBUFFER, 0);
>>
>> OK, so this is the "winsys" framebuffer (GL has some term for it,
>> sorry, I don't remember what it is... perhaps it's even winsys). This
>> is created based on parameters of your selected GLX visual. For
>> example, when I run glxinfo, I see (on nouveau; the list on intel will
>> be different but comparable):
>>
>> 480 GLX Visuals
>> visual  x   bf lv rg d st  colorbuffer  sr ax dp st accumbuffer  ms  cav
>>   id dep cl sp  sz l  ci b ro  r  g  b  a F gb bf th cl  r  g  b  a ns b eat
>> 
>> 0x021 24 tc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None
>> 0x022 24 dc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None
>> ...
>> 0x343 24 tc  0  32  0 r  . .   8  8  8  8 .  s  0  0  0  0  0  0  0  0 0 None
>> 0x344 24 tc  0  32  0 r  . .   8  8  8  8 .  s  0  0  0 16 16 16 16  0 0 Slow
>> 0x345 24 tc  0  32  0 r  y .   8  8  8  8 .  s  0  0  0  0  0  0  0  0 0 None
>> 0x346 24 tc  0  32  0 r  y .   8  8  8  8 .  s  0  0  0 16 16 16 16  0 0 Slow
>>
>> Note how some of them have srgb, others don't (and have various
>> differences in their various other properties). EGL has something
>> similar I believe, but tbh I don't remember the specifics. That's the
>> mesaVis->sRGBCapable bit below. If you need an sRGB-capable visual,
>> are you sure you're picking one? This would be somewhere well before
>> any actual rendering takes place.
>>
>> If you're not sure whether you need an sRGB-capable visual or not, try
>> making sure you pick a *non-srgb* visual in a working configuration
>> and see if it breaks.
>
> Right, so looking at your code with in more detail, you probably just
> require a stencil visual, which, in combination with this patch, makes
> you skip this one.
>
> However I noticed that i965/hsw and probably others don't expose *any*
> sRGB-capable GLX visuals/fb configs! Pretty sure that's not good...
> but GLX, sRGB, and visuals are not exactly my strong point, hopefully
> someone a bit more clued in can comment.

I suspect that something like this may be the right thing for the
intel driver... no reason not to expose sRGB-capable visuals when it
so happens that alpha == 0. Also probably the same treatment should be
done for BGR565... sRGB encoding will matter even more there, and from
the looks of it, all gens support that.

diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
b/src/mesa/drivers/dri/i965/intel_screen.c
index cc90efe..75f2cce 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1003,9 +1003,10 @@ intelCreateBuffer(__DRIscreen * driScrnPriv,
   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 {
+   else if (mesaVis->alphaBits == 0) {
+  rgbFormat = MESA_FORMAT_B8G8R8X8_SRGB;
+  fb->Visual.sRGBCapable = true;
+   } else {
   rgbFormat = MESA_FORMAT_B8G8R8A8_SRGB;
   fb->Visual.sRGBCapable = true;
}
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: handle stencil_bits parameter for MESA_FORMAT_B8G8R8X8_UNORM format.

[Ilia Mirkin]

On Wed, Dec 9, 2015 at 11:18 AM, Deve  wrote:
> This patch indeed seems to not have a sense. I just added it to the bug
> report as a suggestion that it works for me after this modification. Emil
> Velikov said that I should send it to the mailing list.
>
> Here is how it works in Supertuxkart:
> We create rtt with following parameters:
>
> DepthStencilTexture = generateRTT(res, GL_DEPTH24_STENCIL8,
> GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8);
>
> Then, during rendering scene, we do:
>
> glEnable(GL_FRAMEBUFFER_SRGB);
> glBindFramebuffer(GL_FRAMEBUFFER, 0);

OK, so this is the "winsys" framebuffer (GL has some term for it,
sorry, I don't remember what it is... perhaps it's even winsys). This
is created based on parameters of your selected GLX visual. For
example, when I run glxinfo, I see (on nouveau; the list on intel will
be different but comparable):

480 GLX Visuals
visual  x   bf lv rg d st  colorbuffer  sr ax dp st accumbuffer  ms  cav
  id dep cl sp  sz l  ci b ro  r  g  b  a F gb bf th cl  r  g  b  a ns b eat

0x021 24 tc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None
0x022 24 dc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None
...
0x343 24 tc  0  32  0 r  . .   8  8  8  8 .  s  0  0  0  0  0  0  0  0 0 None
0x344 24 tc  0  32  0 r  . .   8  8  8  8 .  s  0  0  0 16 16 16 16  0 0 Slow
0x345 24 tc  0  32  0 r  y .   8  8  8  8 .  s  0  0  0  0  0  0  0  0 0 None
0x346 24 tc  0  32  0 r  y .   8  8  8  8 .  s  0  0  0 16 16 16 16  0 0 Slow

Note how some of them have srgb, others don't (and have various
differences in their various other properties). EGL has something
similar I believe, but tbh I don't remember the specifics. That's the
mesaVis->sRGBCapable bit below. If you need an sRGB-capable visual,
are you sure you're picking one? This would be somewhere well before
any actual rendering takes place.

If you're not sure whether you need an sRGB-capable visual or not, try
making sure you pick a *non-srgb* visual in a working configuration
and see if it breaks.

Cheers,

  -ilia

> (...)
> render();
> (...)
> glDisable(GL_FRAMEBUFFER_SRGB);
>
> It looks that glEnable(GL_FRAMEBUFFER_SRGB) doesn't work anymore. It's
> because of following lines in intel_screen.c in intelCreateBuffer()
> function:
>
>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;
>   fb->Visual.sRGBCapable = true;
>}
>
> Previously MESA_FORMAT_B8G8R8X8_UNORM was not available, and thus
> MESA_FORMAT_B8G8R8A8_UNORM was handled as last case (using
> MESA_FORMAT_B8G8R8A8_SRGB format). Now it uses MESA_FORMAT_B8G8R8X8_UNORM
> format.
>
> Any ideas how it should be handled?
>
> Regards,
> Deve
>
> W dniu 09.12.2015 o 03:00, Ilia Mirkin pisze:
>
>> On Mon, Dec 7, 2015 at 5:32 PM, Dawid Gan  wrote:
>>>
>>> This format has been added in commit:
>>> 28090b30dd6b5977de085f48c620574214b6b4ba
>>> But it was handled in the same way as MESA_FORMAT_B8G8R8A8_UNORM format.
>>> It was causing the screen in Supertuxkart to be darker than expected,
>>> see:
>>> https://bugs.freedesktop.org/show_bug.cgi?id=92759
>>>
>>> Cc: Boyan Ding 
>>> Cc: "11.0 11.1" 
>>> Fixes: 28090b30dd6 "i965: Add XRGB format to
>>> intel_screen_make_configs"
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92759
>>> ---
>>>   src/mesa/drivers/dri/i965/intel_screen.c | 9 +
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
>>> b/src/mesa/drivers/dri/i965/intel_screen.c
>>> index cc90efe..75d5a65 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>>> @@ -1237,6 +1237,9 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
>>>stencil_bits[2] = 8;
>>>num_depth_stencil_bits = 3;
>>>}
>>> +  } else if (formats[i] == MESA_FORMAT_B8G8R8X8_UNORM) {
>>> + depth_bits[1] = 24;
>>> + stencil_bits[1] = 0;
>>
>>
>> Why would you want depth without stencil when using BGRX? I don't see
>> how the two are connected... Are you sure you're picking the right
>> visual?
>>
>>-ilia
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: handle stencil_bits parameter for MESA_FORMAT_B8G8R8X8_UNORM format.

[Deve]

This patch indeed seems to not have a sense. I just added it to the bug 
report as a suggestion that it works for me after this modification. 
Emil Velikov said that I should send it to the mailing list.


Here is how it works in Supertuxkart:
We create rtt with following parameters:

DepthStencilTexture = generateRTT(res, GL_DEPTH24_STENCIL8, 
GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8);


Then, during rendering scene, we do:

glEnable(GL_FRAMEBUFFER_SRGB);
glBindFramebuffer(GL_FRAMEBUFFER, 0);
(...)
render();
(...)
glDisable(GL_FRAMEBUFFER_SRGB);

It looks that glEnable(GL_FRAMEBUFFER_SRGB) doesn't work anymore. It's 
because of following lines in intel_screen.c in intelCreateBuffer() 
function:


   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;
  fb->Visual.sRGBCapable = true;
   }

Previously MESA_FORMAT_B8G8R8X8_UNORM was not available, and thus 
MESA_FORMAT_B8G8R8A8_UNORM was handled as last case (using 
MESA_FORMAT_B8G8R8A8_SRGB format). Now it uses 
MESA_FORMAT_B8G8R8X8_UNORM format.


Any ideas how it should be handled?

Regards,
Deve

W dniu 09.12.2015 o 03:00, Ilia Mirkin pisze:

On Mon, Dec 7, 2015 at 5:32 PM, Dawid Gan  wrote:

This format has been added in commit:
28090b30dd6b5977de085f48c620574214b6b4ba
But it was handled in the same way as MESA_FORMAT_B8G8R8A8_UNORM format.
It was causing the screen in Supertuxkart to be darker than expected, see:
https://bugs.freedesktop.org/show_bug.cgi?id=92759

Cc: Boyan Ding 
Cc: "11.0 11.1" 
Fixes: 28090b30dd6 "i965: Add XRGB format to intel_screen_make_configs"
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92759
---
  src/mesa/drivers/dri/i965/intel_screen.c | 9 +
  1 file changed, 9 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index cc90efe..75d5a65 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1237,6 +1237,9 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
   stencil_bits[2] = 8;
   num_depth_stencil_bits = 3;
   }
+  } else if (formats[i] == MESA_FORMAT_B8G8R8X8_UNORM) {
+ depth_bits[1] = 24;
+ stencil_bits[1] = 0;


Why would you want depth without stencil when using BGRX? I don't see
how the two are connected... Are you sure you're picking the right
visual?

   -ilia


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: handle stencil_bits parameter for MESA_FORMAT_B8G8R8X8_UNORM format.

[Ilia Mirkin]

On Wed, Dec 9, 2015 at 11:23 AM, Ilia Mirkin  wrote:
> On Wed, Dec 9, 2015 at 11:18 AM, Deve  wrote:
>> This patch indeed seems to not have a sense. I just added it to the bug
>> report as a suggestion that it works for me after this modification. Emil
>> Velikov said that I should send it to the mailing list.
>>
>> Here is how it works in Supertuxkart:
>> We create rtt with following parameters:
>>
>> DepthStencilTexture = generateRTT(res, GL_DEPTH24_STENCIL8,
>> GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8);
>>
>> Then, during rendering scene, we do:
>>
>> glEnable(GL_FRAMEBUFFER_SRGB);
>> glBindFramebuffer(GL_FRAMEBUFFER, 0);
>
> OK, so this is the "winsys" framebuffer (GL has some term for it,
> sorry, I don't remember what it is... perhaps it's even winsys). This
> is created based on parameters of your selected GLX visual. For
> example, when I run glxinfo, I see (on nouveau; the list on intel will
> be different but comparable):
>
> 480 GLX Visuals
> visual  x   bf lv rg d st  colorbuffer  sr ax dp st accumbuffer  ms  cav
>   id dep cl sp  sz l  ci b ro  r  g  b  a F gb bf th cl  r  g  b  a ns b eat
> 
> 0x021 24 tc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None
> 0x022 24 dc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None
> ...
> 0x343 24 tc  0  32  0 r  . .   8  8  8  8 .  s  0  0  0  0  0  0  0  0 0 None
> 0x344 24 tc  0  32  0 r  . .   8  8  8  8 .  s  0  0  0 16 16 16 16  0 0 Slow
> 0x345 24 tc  0  32  0 r  y .   8  8  8  8 .  s  0  0  0  0  0  0  0  0 0 None
> 0x346 24 tc  0  32  0 r  y .   8  8  8  8 .  s  0  0  0 16 16 16 16  0 0 Slow
>
> Note how some of them have srgb, others don't (and have various
> differences in their various other properties). EGL has something
> similar I believe, but tbh I don't remember the specifics. That's the
> mesaVis->sRGBCapable bit below. If you need an sRGB-capable visual,
> are you sure you're picking one? This would be somewhere well before
> any actual rendering takes place.
>
> If you're not sure whether you need an sRGB-capable visual or not, try
> making sure you pick a *non-srgb* visual in a working configuration
> and see if it breaks.

Right, so looking at your code with in more detail, you probably just
require a stencil visual, which, in combination with this patch, makes
you skip this one.

However I noticed that i965/hsw and probably others don't expose *any*
sRGB-capable GLX visuals/fb configs! Pretty sure that's not good...
but GLX, sRGB, and visuals are not exactly my strong point, hopefully
someone a bit more clued in can comment.

Cheers,

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: handle stencil_bits parameter for MESA_FORMAT_B8G8R8X8_UNORM format.

[Ilia Mirkin]

On Mon, Dec 7, 2015 at 5:32 PM, Dawid Gan  wrote:
> This format has been added in commit:
> 28090b30dd6b5977de085f48c620574214b6b4ba
> But it was handled in the same way as MESA_FORMAT_B8G8R8A8_UNORM format.
> It was causing the screen in Supertuxkart to be darker than expected, see:
> https://bugs.freedesktop.org/show_bug.cgi?id=92759
>
> Cc: Boyan Ding 
> Cc: "11.0 11.1" 
> Fixes: 28090b30dd6 "i965: Add XRGB format to intel_screen_make_configs"
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92759
> ---
>  src/mesa/drivers/dri/i965/intel_screen.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
> b/src/mesa/drivers/dri/i965/intel_screen.c
> index cc90efe..75d5a65 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -1237,6 +1237,9 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
>   stencil_bits[2] = 8;
>   num_depth_stencil_bits = 3;
>   }
> +  } else if (formats[i] == MESA_FORMAT_B8G8R8X8_UNORM) {
> + depth_bits[1] = 24;
> + stencil_bits[1] = 0;

Why would you want depth without stencil when using BGRX? I don't see
how the two are connected... Are you sure you're picking the right
visual?

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev