[Mesa-dev] [RFC PATCH] mesa/st/cb_clear: in st_Clear also validate the render state (needed by virgl)

[Gert Wollny]

Am Freitag, den 04.05.2018, 08:21 -0400 schrieb Ilia Mirkin:
> On Fri, May 4, 2018 at 7:36 AM, Gert Wollny <gert.wollny at collabora.co
> m> wrote:
> > Am Donnerstag, den 03.05.2018, 19:52 +0200 schrieb Gert Wollny:
> > > Am Donnerstag, den 03.05.2018, 13:24 -0400 schrieb Ilia Mirkin:
> > > > 
> > > > The api call is "clear", not "glClear in the context of
> > > > whatever
> > > > random GL state there might be". When the gallium clear API is
> > > > invoked, the bound framebuffer needs to be cleared. This is how
> > > > the
> > > > API works, this is how all drivers implement it. It's basically
> > > > memset(). It doesn't care about rasterizer discard or anything
> > > > else.
> > > > 
> > > 
> > > I stand corrected and sorry for the noise.
> > 
> > Actually no:
> > 
> > The OpenGL standard section 14.1 says:
> 
> Not sure what this has to do with anything. We're not talking about
> glClear(). We're talking about the gallium clear API. Here's how it's
> specified:
> 
> http://gallium.readthedocs.io/en/latest/context.html

Thanks for the pointers, am am reading it and I am looking through the
code, but I don't really see how to solve this properly. 

> What you need to do in the virgl_clear() function is to make that
> happen. The fact that you're messing around with GL state on the
> driver end isn't the gallium api's concern, nor is it the gallium
> api's users'. My recommendation is instead of virgl_clear working
> around virglrenderer's limitations by sending extra state (such as to
> force-disable raster discard), to instead have virglrenderer track it
> properly, 

virglrenderer passes all information it gets through to the underlying
OpenGL implementation, in that sense it tracks the state. Having a
closer look at some simple example pseudo code: 

  ...
  glEnable(GL_RASTERIZER_DISCARD); 
    -> the gallium state tracker sets the flag of this state
    -> mesa keeps another variable ctx->RasterDiscard to hold this 
       state 
  glClear(...)
    -> mesa does an early exit because RasterizerDisable is true. 

  glDrawSomething(...);  
    -> before this draw gallium issues bind_rasterizer_state which 
       sends the state to the virglrenderer and it now has 
       RASTERIZER disabled. If mesa is the host backend, then 
       the host ctx->RasterDiscard is now true.
 
  glDisable(GL_RASTERIZER_DISABLE); 
     -> the according flags in the guest gallium state tracker and mesa
 
        ctx->RasterDiscard are cleared, but the state change is not 
        passed to virgl and hence also not to virglrenderer on the 
        host. 

  glClear(); 
     -> the guest gallium state tracker issues the clear call 
        virgl_clear which transmits the clear command to virglrenderer 
        but since the host state was not updated, the command is 
        ignored.

> and if raster discard has been enabled, to first disable it
> before calling glClear() (and then re-enable it).

I tried that, but there were still seven tests out of 17 are failing,
so there is probably some other state change involved. In fact the test
suite in question combines RASTERIZER_DISABLE with SCISSOR_TEST, which
is also communicated by bind_rasterizer_state and also taken into
account by glClear.

Now, from where I stand it is gallium as the state tracker that keeps
track of these states, and virgl (and respectively virglrenderer) only
see the actual state changes when they are passed in by
bind_rasterizer_state, which is always called from gallium.

For that reason I think that a render state validation within st_Clear
makes sense, because it makes sure that the states like  SCISSOR_TEST
or DISCARD_RASTERIZER are properly send to the driver, before clear()
is executed that makes use of these states. And while the workaround
for DISCARD_RASTERIZER is possible because if it is set then the host
should actually never see the clear command, SCISSOR_TEST is different,
its state can not be guessed on the host.

Anyway, I happily take every hint on how this can be fixed directly in
virgl_clear or virgl in general, and thank you for taking the time to
answer me,

best,
Gert