[Mesa-dev] [PATCH v3] i965: expose SRGB visuals and turn on EGL_KHR_gl_colorspace
Kenneth Graunke
kenneth at whitecape.org
Wed Nov 8 05:25:03 UTC 2017
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...
> + 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...?
If you drop BGRX, and take the brw_query_renderer_integer suggestion,
this patch would be:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
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++) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171107/d713d113/attachment.sig>
More information about the mesa-dev
mailing list