[virglrenderer-devel] [PATCH] vrend: use full colormask before a clear, restore previous colormask afterwards

[Elie Tournier]

On Fri, Feb 09, 2018 at 03:52:54PM -0800, Gurchetan Singh wrote:
I add some comments bellow.
With this,
Reviewed-by: Elie Tournier <elie.tournier at collabora.com>
> Consider this series of events:
> 
> glColorMask(GL_FALSE, GL_TRUE, GL_TRUE, GL_TRUE);
> glClearColor(0.125f, 0.25f, 0.5f, 1.0f);
> glClear(GL_COLOR_BUFFER_BIT);
> glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
> glClearColor(0.1f, 0.1f, 0.1f, 1.0f);
> glClear(GL_COLOR_BUFFER_BIT);
> 
> With virgl, this may render an incorrect result.  This is because
> there our two paths for clears in Gallium -- one for full clears
> (st->pipe->clear) and another for color-masked/scissored clears
> (clear_with_quad).  We only encode the color mask in the
> virgl_encode_blend_state in the clear_with_quad case.
> 
> We should set the colormask the case of full clears as well, since
> we need to update the GL state on the host driver.
> 
> With this patch, the number of dEQP GLES2 failures on Virgl with a
> nvidia host driver goes from 260 to 136 with no regressions.
Almost same result on my system with Mesa 17.3 on the host.
Goes from 300 to 172.

> Some representative test cases are:
> 
> dEQP-GLES2.functional.color_clear.masked_scissored_rgb
> dEQP-GLES2.functional.color_clear.masked_scissored_rgba
> dEQP-GLES2.functional.depth_stencil_clear.depth_stencil_scissored
> dEQP-GLES2.functional.fragment_ops.random.0
> dEQP-GLES2.functional.fragment_ops.interaction.basic_shader.0
> ---
>  src/vrend_renderer.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> index ec37fc8..e36f509 100644
> --- a/src/vrend_renderer.c
> +++ b/src/vrend_renderer.c
> @@ -2413,6 +2413,16 @@ void vrend_clear(struct vrend_context *ctx,
>        } else {
>           glClearColor(color->f[0], color->f[1], color->f[2], color->f[3]);
>        }
> +
> +      /* This function is virglrenderer's implementation of Gallium's pipe->clear(), so we can
> +         assume a full mask at this point. */
The wording let us think that we can hit some issue in some corner case.
Can we change this comment to something like:
"We clear all glColorMask when doing a full clear (st->pipe->clear) since it is not effected by it. Then restore it afterwards."
Do not hesitate to suggest something else, my english is not the best. ;)
> +      if (ctx->sub->hw_blend_state.independent_blend_enable) {
> +         /* ARB_draw_buffers_blend is required for this */
> +         int i;
> +         for (i = 0; i < PIPE_MAX_COLOR_BUFS; i++)
> +            glColorMaskIndexedEXT(i, GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
> +      } else
> +         glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
>     }
>  
>     if (buffers & PIPE_CLEAR_DEPTH) {
> @@ -2460,6 +2470,25 @@ void vrend_clear(struct vrend_context *ctx,
>     if (buffers & PIPE_CLEAR_DEPTH)
>        if (!ctx->sub->dsa_state.depth.writemask)
>           glDepthMask(GL_FALSE);
> +
> +   // Restore previous color mask
> +   if (buffers & PIPE_CLEAR_COLOR) {
> +      if (ctx->sub->hw_blend_state.independent_blend_enable) {
> +         /* ARB_draw_buffers_blend is required for this */
> +         int i;
> +         for (i = 0; i < PIPE_MAX_COLOR_BUFS; i++) {
> +            struct pipe_blend_state *blend = &ctx->sub->hw_blend_state;
> +            glColorMaskIndexedEXT(i, blend->rt[i].colormask & PIPE_MASK_R ? GL_TRUE : GL_FALSE,
> +                                  blend->rt[i].colormask & PIPE_MASK_G ? GL_TRUE : GL_FALSE,
> +                                  blend->rt[i].colormask & PIPE_MASK_B ? GL_TRUE : GL_FALSE,
> +                                  blend->rt[i].colormask & PIPE_MASK_A ? GL_TRUE : GL_FALSE);
> +         }
> +      } else
> +         glColorMask(ctx->sub->hw_blend_state.rt[0].colormask & PIPE_MASK_R ? GL_TRUE : GL_FALSE,
> +                     ctx->sub->hw_blend_state.rt[0].colormask & PIPE_MASK_G ? GL_TRUE : GL_FALSE,
> +                     ctx->sub->hw_blend_state.rt[0].colormask & PIPE_MASK_B ? GL_TRUE : GL_FALSE,
> +                     ctx->sub->hw_blend_state.rt[0].colormask & PIPE_MASK_A ? GL_TRUE : GL_FALSE);
Can we add some braces on the else statment? (Personal preference)
> +   }
>  }
>  
>  static void vrend_update_scissor_state(struct vrend_context *ctx)
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel