[Mesa-dev] [PATCH v3] i965: expose SRGB visuals and turn on EGL_KHR_gl_colorspace
Tapani Pälli
tapani.palli at intel.com
Wed Nov 8 06:37:53 UTC 2017
On 11/08/2017 07:25 AM, Kenneth Graunke wrote:
> On Thursday, November 2, 2017 1:48:34 AM PST Tapani Pälli wrote:
>> Patch exposes sRGB visuals and adds DRI integer query support for
>> __DRI2_RENDERER_HAS_FRAMEBUFFER_SRGB. Further changes make sure that
>> we mark if the app explicitly wanted sRGB and for these framebuffers
>> we don't turn sRGB off in intel_gles3_srgb_workaround. This way we
>> keep compatibility for existing applications relying on default sRGB
>> and ony add more visual support.
>>
>> With this change, following dEQP tests start to pass:
>>
>> dEQP-EGL.functional.wide_color.window_8888_colorspace_srgb
>> dEQP-EGL.functional.wide_color.pbuffer_8888_colorspace_srgb
>>
>> v2: some code cleanup (Emil Velikov)
>> update num_formats correctly (reported by deveee at gmail.com)
>>
>> v3: cleanup, remove redundant is_srgb
>> rename explicit_srgb as 'need_srgb' to follow style better
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102264
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102354
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102503
>> ---
>> src/mesa/drivers/dri/i965/brw_context.c | 16 ++++++++++------
>> src/mesa/drivers/dri/i965/intel_fbo.h | 5 +++++
>> src/mesa/drivers/dri/i965/intel_screen.c | 13 ++++++++++++-
>> 3 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
>> index 037e349fdb..0b8134ae9f 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.c
>> +++ b/src/mesa/drivers/dri/i965/brw_context.c
>> @@ -1142,8 +1142,8 @@ intelUnbindContext(__DRIcontext * driContextPriv)
>> *
>> * Unfortunately, renderbuffer setup happens before a context is created. So
>> * in intel_screen.c we always set up sRGB, and here, if you're a GLES2/3
>> - * context (without an sRGB visual, though we don't have sRGB visuals exposed
>> - * yet), we go turn that back off before anyone finds out.
>> + * context (without an sRGB visual), we go turn that back off before anyone
>> + * finds out.
>> */
>> static void
>> intel_gles3_srgb_workaround(struct brw_context *brw,
>> @@ -1154,15 +1154,19 @@ intel_gles3_srgb_workaround(struct brw_context *brw,
>> if (_mesa_is_desktop_gl(ctx) || !fb->Visual.sRGBCapable)
>> return;
>>
>> - /* Some day when we support the sRGB capable bit on visuals available for
>> - * GLES, we'll need to respect that and not disable things here.
>> - */
>> - fb->Visual.sRGBCapable = false;
>> for (int i = 0; i < BUFFER_COUNT; i++) {
>> struct gl_renderbuffer *rb = fb->Attachment[i].Renderbuffer;
>> +
>> + /* Check if sRGB was specifically asked for. */
>> + struct intel_renderbuffer *irb = intel_get_renderbuffer(fb, i);
>> + if (irb && irb->need_srgb)
>> + return;
>> +
>> if (rb)
>> rb->Format = _mesa_get_srgb_format_linear(rb->Format);
>> }
>> + /* Disable sRGB from framebuffers that are not compatible. */
>> + fb->Visual.sRGBCapable = false;
>> }
>>
>> GLboolean
>> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.h b/src/mesa/drivers/dri/i965/intel_fbo.h
>> index 1e2494286b..608a1c4e7d 100644
>> --- a/src/mesa/drivers/dri/i965/intel_fbo.h
>> +++ b/src/mesa/drivers/dri/i965/intel_fbo.h
>> @@ -116,6 +116,11 @@ struct intel_renderbuffer
>> * for the duration of a mapping.
>> */
>> bool singlesample_mt_is_tmp;
>> +
>> + /**
>> + * Set to true when application specifically asked for a sRGB visual.
>> + */
>> + bool need_srgb;
>> };
>>
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
>> index 10064c3236..90303df899 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>> @@ -1384,6 +1384,9 @@ brw_query_renderer_integer(__DRIscreen *dri_screen,
>> 0, BRW_CONTEXT_MEDIUM_PRIORITY) == 0)
>> value[0] |= __DRI2_RENDERER_HAS_CONTEXT_PRIORITY_MEDIUM;
>> return 0;
>> + case __DRI2_RENDERER_HAS_FRAMEBUFFER_SRGB:
>> + value[0] = screen->mesa_format_supports_render[MESA_FORMAT_B8G8R8A8_SRGB];
>
> I think it would be better to do:
>
> value[0] = true;
>
> as we may add other sRGB formats in the future. In particular, I'm
> thinking about Mario's 10-bit visual series. We'd probably want to
> expose this property regardless of whether we expose B8G8B8A8_SRGB,
> R8G8B8A8_SRGB, B10G10R10A2_SRGB, or whatever...and checking one of
> them would be confusing.
>
> Plus, it's always true, so there's not much use in checking it...
Yes, this makes sense.
>> + return 0;
>> default:
>> return driQueryRendererIntegerCommon(dri_screen, param, value);
>> }
>> @@ -1544,13 +1547,18 @@ intelCreateBuffer(__DRIscreen *dri_screen,
>> fb->Visual.sRGBCapable = true;
>> }
>>
>> + /* mesaVis->sRGBCapable was set, user is asking for sRGB */
>> + bool srgb_cap_set = mesaVis->redBits >= 8 && mesaVis->sRGBCapable;
>> +
>> /* setup the hardware-based renderbuffers */
>> rb = intel_create_winsys_renderbuffer(screen, rgbFormat, num_samples);
>> _mesa_attach_and_own_rb(fb, BUFFER_FRONT_LEFT, &rb->Base.Base);
>> + rb->need_srgb = srgb_cap_set;
>>
>> if (mesaVis->doubleBufferMode) {
>> rb = intel_create_winsys_renderbuffer(screen, rgbFormat, num_samples);
>> _mesa_attach_and_own_rb(fb, BUFFER_BACK_LEFT, &rb->Base.Base);
>> + rb->need_srgb = srgb_cap_set;
>> }
>>
>> /*
>> @@ -1911,6 +1919,9 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
>> MESA_FORMAT_B8G8R8A8_UNORM,
>> MESA_FORMAT_B8G8R8X8_UNORM,
>>
>> + MESA_FORMAT_B8G8R8A8_SRGB,
>> + MESA_FORMAT_B8G8R8X8_SRGB,
>> +
>
> I'm worried that exposing B8G8R8X8_SRGB is not going to work. See the
> explanation in commit 61cdb7665f7bd147533cdc5977750d990c2eafd5. Unless
> I'm mistaken, I don't think anything has been done to fix that problem,
> so trying to use such a visual with DRI3 would just crash.
>
> It would certainly be nice to expose it RGBX sRGB visuals, and not only
> RGBA...but I think more work is required. Are there any tests...?
Right, I've seen 61cdb7665f7bd147533cdc5977750d990c2eafd5 but forgot
about it. I checked and dEQP tests 8888 but not 8880 for srgb
(dEQP-EGL.functional.wide_color.window_8888_colorspace_srgb), will
remove BGRX.
> If you drop BGRX, and take the brw_query_renderer_integer suggestion,
> this patch would be:
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
Thanks for the review Ken, I will apply the changes and run some testing
before pushing.
> In the longer term, I think I'd like to drop the hack to make ordinary
> visuals sRGB capable in OpenGL. Let sRGB-capable visuals be sRGB, and
> non-sRGB-capable visuals be linear. This would let us drop the
> intel_gles3_srgb_workaround hack too.
>
> I think we added that hack for Left 4 Dead 2 near when it was first
> released, but it seems to work fine without the hack these days. With
> the new GLES behavior, and push to make things compatible in OpenGL 4.x,
> and dEQP and CTS testing, I think other vendors have made a lot of sRGB
> related changes. I know NVIDIA has changed their behavior for a number
> of things. I haven't checked this specifically, though.
>
> Removing that would make it much more obvious how things work. We've
> also seen that our current behavior surprises some app developers (i.e.
> SuperTuxKart).
>
> That said, I'd prefer to land your patch as is, and make that change
> later. That way, we can let users report any issues with this patch
> (exposing new sRGB-capable visuals). Then we can drop the hacks, and
> see if that works out. And if it doesn't, we could revert the
> hack-dropping, without also reverting sRGB-capable visuals.
>
>> /* The 32-bit RGBA format must not precede the 32-bit BGRA format.
>> * Likewise for RGBX and BGRX. Otherwise, the GLX client and the GLX
>> * server may disagree on which format the GLXFBConfig represents,
>> @@ -1950,7 +1961,7 @@ intel_screen_make_configs(__DRIscreen *dri_screen)
>> if (intel_loader_get_cap(dri_screen, DRI_LOADER_CAP_RGBA_ORDERING))
>> num_formats = ARRAY_SIZE(formats);
>> else
>> - num_formats = 3;
>> + num_formats = ARRAY_SIZE(formats) - 2; /* all - RGBA_ORDERING formats */
>>
>> /* Generate singlesample configs without accumulation buffer. */
>> for (unsigned i = 0; i < num_formats; i++) {
More information about the mesa-dev
mailing list