[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 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.

> 
> 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