[Mesa-dev] [PATCH v3 2/3] mesa/st: enable EXT_sRGB_write_control for drivers that support it
Gert Wollny
gert.wollny at collabora.com
Fri Oct 19 16:45:34 UTC 2018
Am Freitag, den 19.10.2018, 08:25 -0400 schrieb Ilia Mirkin:
> I don't think a PIPE_CAP is the answer here... I think you're on the
> right track with format checks and whatnot.
The problem is that with an GLES host, the checked sRGB format might be
supported by virgl, but not EXT_sRGB_write_control, so the guest will
still expose the extension, but then one of the dEQP tests fails. If I
use host caps in this case to disable the the extension in the guest I
can avoid exposing the not properly working extension, and for this I
need the caps.
I will dig a bit to figure out whether this is actually problem in
virglrenderer, or how Gallium handles enabling and disabling
FRAMEBUFFER_SRGB, but I'm not that optimistic.
Best,
Gert
> On Fri, Oct 19, 2018 at 2:54 AM Gert Wollny
> <gert.wollny at collabora.com> wrote:
> >
> > Considering how virgl has to deal with host capabilities the
> > extension should be enabled via a CAP, so I'll rework this patch
> > another time.
> >
> > Sorry for being so noisy,
> > Gert
> >
> >
> > Am Donnerstag, den 18.10.2018, 18:39 +0200 schrieb Gert Wollny:
> > > From: Gert Wollny <gert.wollny at collabora.com>
> > >
> > > With this patch the extension EXT_sRGB_write_control is enabled
> > > for
> > > gallium drivers that support sRGB formats as render targets.
> > >
> > > Tested (and pass) on r600 (evergreen) and softpipe:
> > >
> > > dEQP-
> > > GLES31.functional.fbo.srgb_write_control.framebuffer_srgb_enabled
> > > *
> > >
> > > with "MESA_GLES_VERSION_OVERRIDE=3.2" (the tests needlessly check
> > > for
> > > this), and
> > >
> > > dEQP-
> > > GLES31.functional.fbo.srgb_write_control.framebuffer_srgb_unsuppo
> > > rted
> > > _enum
> > >
> > > when extension is manually disabled via MESA_EXTENSION_OVERRIDE
> > >
> > > v2: - always enable the extension when sRGB is supported (Ilia
> > > Mirkin).
> > > - Correct handling by moving extension initialization to the
> > > place where gallium/st actually takes care of this. This
> > > also
> > > fixes properly disabling the extension via
> > > MESA_EXTENSION_OVERRIDE
> > > - reinstate check for desktop GL and add check for the
> > > extension
> > > when creating the framebuffer
> > >
> > > v3: Only create sRGB renderbuffers based on Visual.srgbCapable
> > > when
> > > on desktop GL.
> > >
> > > Signed-off-by: Gert Wollny <gert.wollny at collabora.com>
> > > ---
> > > src/mesa/state_tracker/st_extensions.c | 6 +++++
> > > src/mesa/state_tracker/st_manager.c | 37 ++++++++++++++++--
> > > ----
> > > ----
> > > 2 files changed, 29 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/mesa/state_tracker/st_extensions.c
> > > b/src/mesa/state_tracker/st_extensions.c
> > > index 798ee60875..401609d728 100644
> > > --- a/src/mesa/state_tracker/st_extensions.c
> > > +++ b/src/mesa/state_tracker/st_extensions.c
> > > @@ -1169,6 +1169,12 @@ void st_init_extensions(struct pipe_screen
> > > *screen,
> > > void_formats, 32,
> > > PIPE_BIND_RENDER_TARGET);
> > >
> > > + extensions->EXT_sRGB_write_control =
> > > + screen->is_format_supported(screen,
> > > PIPE_FORMAT_R8G8B8A8_SRGB,
> > > + PIPE_TEXTURE_2D, 0, 0,
> > > + (PIPE_BIND_DISPLAY_TARGET |
> > > + PIPE_BIND_RENDER_TARGET));
> > > +
> > > if (extensions->AMD_framebuffer_multisample_advanced) {
> > > /* AMD_framebuffer_multisample_advanced */
> > > /* This can be greater than storage samples. */
> > > diff --git a/src/mesa/state_tracker/st_manager.c
> > > b/src/mesa/state_tracker/st_manager.c
> > > index ceb48dd490..df898beb23 100644
> > > --- a/src/mesa/state_tracker/st_manager.c
> > > +++ b/src/mesa/state_tracker/st_manager.c
> > > @@ -295,7 +295,7 @@ st_framebuffer_update_attachments(struct
> > > st_framebuffer *stfb)
> > > */
> > > static boolean
> > > st_framebuffer_add_renderbuffer(struct st_framebuffer *stfb,
> > > - gl_buffer_index idx)
> > > + gl_buffer_index idx, bool
> > > prefer_srgb)
> > > {
> > > struct gl_renderbuffer *rb;
> > > enum pipe_format format;
> > > @@ -318,7 +318,7 @@ st_framebuffer_add_renderbuffer(struct
> > > st_framebuffer *stfb,
> > > break;
> > > default:
> > > format = stfb->iface->visual->color_format;
> > > - if (stfb->Base.Visual.sRGBCapable)
> > > + if (prefer_srgb)
> > > format = util_format_srgb(format);
> > > sw = FALSE;
> > > break;
> > > @@ -436,6 +436,7 @@ st_framebuffer_create(struct st_context *st,
> > > struct st_framebuffer *stfb;
> > > struct gl_config mode;
> > > gl_buffer_index idx;
> > > + bool prefer_srgb = false;
> > >
> > > if (!stfbi)
> > > return NULL;
> > > @@ -457,14 +458,15 @@ st_framebuffer_create(struct st_context
> > > *st,
> > > * format such that util_format_srgb(visual->color_format)
> > > can be
> > > supported
> > > * by the pipe driver. We still need to advertise the
> > > capability
> > > here.
> > > *
> > > - * For GLES, however, sRGB framebuffer write is controlled
> > > only
> > > by the
> > > - * capability of the framebuffer. There is
> > > GL_EXT_sRGB_write_control to
> > > - * give applications the control back, but sRGB write is
> > > still
> > > enabled by
> > > - * default. To avoid unexpected results, we should not
> > > advertise
> > > the
> > > - * capability. This could change when we add support for
> > > - * EGL_KHR_gl_colorspace.
> > > + * For GLES, however, sRGB framebuffer write is initially
> > > only
> > > controlled
> > > + * by the capability of the framebuffer, with
> > > GL_EXT_sRGB_write_control
> > > + * control is given back to the applications, but
> > > GL_FRAMEBUFFER_SRGB is
> > > + * still enabled by default since this is the behaviour when
> > > + * EXT_sRGB_write_control is not available.
> > > */
> > > - if (_mesa_is_desktop_gl(st->ctx)) {
> > > + if (_mesa_is_desktop_gl(st->ctx) ||
> > > + st->ctx->Extensions.EXT_sRGB_write_control)
> > > + {
> > > struct pipe_screen *screen = st->pipe->screen;
> > > const enum pipe_format srgb_format =
> > > util_format_srgb(stfbi->visual->color_format);
> > > @@ -475,8 +477,14 @@ st_framebuffer_create(struct st_context *st,
> > > PIPE_TEXTURE_2D, stfbi-
> > > > visual->samples,
> > >
> > > stfbi->visual->samples,
> > > (PIPE_BIND_DISPLAY_TARGET
> > > |
> > > - PIPE_BIND_RENDER_TARGET))
> > > )
> > > + PIPE_BIND_RENDER_TARGET))
> > > ) {
> > > mode.sRGBCapable = GL_TRUE;
> > > + /* Since GL_FRAMEBUFFER_SRGB is enabled by default on
> > > GLES
> > > we must not
> > > + * create renderbuffers with an sRGB format derived
> > > from
> > > the
> > > + * visual->color_format, but we still want this for
> > > desktop
> > > GL.
> > > + */
> > > + prefer_srgb = _mesa_is_desktop_gl(st->ctx);
> > > + }
> > > }
> > >
> > > _mesa_initialize_window_framebuffer(&stfb->Base, &mode);
> > > @@ -487,13 +495,13 @@ st_framebuffer_create(struct st_context
> > > *st,
> > >
> > > /* add the color buffer */
> > > idx = stfb->Base._ColorDrawBufferIndexes[0];
> > > - if (!st_framebuffer_add_renderbuffer(stfb, idx)) {
> > > + if (!st_framebuffer_add_renderbuffer(stfb, idx, prefer_srgb))
> > > {
> > > free(stfb);
> > > return NULL;
> > > }
> > >
> > > - st_framebuffer_add_renderbuffer(stfb, BUFFER_DEPTH);
> > > - st_framebuffer_add_renderbuffer(stfb, BUFFER_ACCUM);
> > > + st_framebuffer_add_renderbuffer(stfb, BUFFER_DEPTH, false);
> > > + st_framebuffer_add_renderbuffer(stfb, BUFFER_ACCUM, false);
> > >
> > > stfb->stamp = 0;
> > > st_framebuffer_update_attachments(stfb);
> > > @@ -1189,7 +1197,8 @@ st_manager_add_color_renderbuffer(struct
> > > st_context *st,
> > > return FALSE;
> > > }
> > >
> > > - if (!st_framebuffer_add_renderbuffer(stfb, idx))
> > > + if (!st_framebuffer_add_renderbuffer(stfb, idx,
> > > + stfb-
> > > > Base.Visual.sRGBCapable))
> > >
> > > return FALSE;
> > >
> > > st_framebuffer_update_attachments(stfb);
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list