[Mesa-dev] [PATCH 1/3] panfrost: Make sure a clear does not re-use a pre-existing batch

[Boris Brezillon]

On Sun, 22 Sep 2019 15:24:10 +0200
Boris Brezillon <boris.brezillon at collabora.com> wrote:

> On Sun, 22 Sep 2019 08:38:30 -0400
> Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:
> 
> > > > To be clear, if we have a batch and do the following operations:
> > > > 
> > > > 	clear red
> > > > 	draw 1
> > > > 	clear green
> > > > 	draw 2
> > > > 	flush
> > > > 
> > > > All we should see is #2 on a green background, which this patch handles
> > > > by the second clear invalidating all the clears/draws that came before
> > > > it (provided there is no flush in between). 
> > > > 
> > > > I might just be tripped up by the "freeze" name. That really means throw
> > > > away / free here, I guess?    
> > > 
> > > Nope. Freeze means "stop queuing new draws to this batch". I guess we
> > > could free the batch as well if the result of the previous draws/clear
> > > are really overwritten by this new clear, but that implies checking the
> > > new clear flags to make sure they target the same depth/stencil/color
> > > combination. On the other hand, I'm wondering if it's really the
> > > driver's job to try to optimize silly things the apps might do. I mean,
> > > the sequence you describe does not look like a wise thing to do since
> > > the "clear red+draw 1" end up being overwritten by "clear green + draw
> > > 2".    
> > 
> > I'm quite confused how this patch works, then.
> > 
> > A few thoughts: if the app clears all buffers in the middle, then yes
> > it's silly and yes we may as well optimize it out. (Should that be a thing GL
> > drivers have to do? I mean, if the other drivers are too...)
> > 
> > If the sequence is more like:
> > 
> > 	clear all buffers
> > 	draw 1
> > 	clear color buffer (preserve depth stencil)
> > 	draw 2
> > 	flush
> > 
> > That second clear should really be done by drawing a full screen quad,
> > just like if we were wallpapering, except loading its colour from a
> > uniform instead of a texture.
> > 
> > Similarly, a depth-only clear mid-frame can be emulated by drawing a
> > full-screen quad with the gl_Position.zw components juryrigged to the
> > desired depth components, and disabling colour draws by setting the
> > colour mask to 0x0. That also means you can skip having any shader at
> > all (literally set the shader pointer to 0x0) so that's faster.
> > 
> > Finally, a stencil-only clear can occur similarly playing tricks with
> > the stencil test parameters.
> > 
> > I suspect u_blitter or mesa/st is capable of doing these sorts of tricks
> > on our behalf, but I have not researched it extensively.  
> 
> AFAIU, mesa/st does not transform the clear into a quad-draw for
> depth/stencil only clears, it only does that for the color(s)-masked
> case. That's certainly something we can do in Panfrost if we want to
> avoid creating a new batch in such situations though.

One more thing: optimization of the above scenario is probably
something we'll want to have at some point, but I think the current
patch is worth applying in the meantime. All this patch does is
enforcing ordering of clears/draws to make sure the end result matches
users expectation.

> 
> > 
> > In any case, for a partial clear mid-frame, we would much rather do:
> > 
> > 	clear all buffers
> > 	draw 1
> > 	draw fullscreen quad (disable Z/S writes)
> > 	draw 2
> > 	flush
> > 
> > than what it sounds like this patch does
> > 
> > 	clear all buffers
> > 	draw 1
> > 	flush
> > 
> > 	clear all buffers
> > 	wallpaper
> > 	draw 2
> > 	flush
> > 
> > Please do correct me if I've misunderstood the logic here.  
> 
> 
> There's no flush introduced by this patch, it just adds a dep between
> batch 2 and 1:
> 
> batch1:	clear all buffers
>  	draw 1
> 
> batch2: clear all buffers
> 	wallpaper
> 	draw 2
> 
> 	flush (actually flushes batch 1 and 2)
> 
> with batch2 --depends-on--> batch1