[Mesa-dev] [PATCH v3 2/3] mesa/st: enable EXT_sRGB_write_control for drivers that support it

[Gert Wollny]

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_unsupported
> _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);