[Mesa-dev] [RFC] i965: Restore vbo after color resolve during brw_try_draw_prims()

[Ben Widawsky]

Topi, you are one of my favorite people. I love how you prove how circuitous and
difficult meta can be, without actually saying it explicitly.

On Wed, Jan 27, 2016 at 01:44:10PM +0200, Topi Pohjolainen wrote:
> Part of brw_try_draw_prims() is a check to validate textures
> (brw_validate_textures()). In case of textures that currently have
> only level zero but are marked for mipmap generation, i965 driver
> will decide to replace the underlying buffer with a larger one
> capable of holding also the additional levels. This results into
> blit from the original buffer to the newly allocated (see
> intel_miptree_copy_teximage()). This blit is currently handled with
> blitter engine and hence it won't effect the ongoing draw operation.
> However, this blit in turn may trigger color resolve on the source
> buffer. In principle, this should be possible with fast cleared
> buffers but I only started hitting it when I enabled lossless
> compression (that reguires similar resolve to fast cleared buffers).
> 
> Now, the color resolve is a meta operation and uses the same drawing
> path we are already in middle of. After quite a bit of debugging I
> realized that the resolve will modify the current vbo setup but it
> won't restore it afterwards resulting in the original draw call
> using wrong vertex data.
> When brw_try_draw_prims() gets called, the vbo logic in the Mesa
> core (see vbo_draw_arrays()) has just bound the vbo (see
> vbo_bind_arrays() and recalculate_input_bindings()). Color resolve
> operation will overwrite the vbo setup by calling vbo_bind_arrays()
> against the resolve rectangle (see brw_draw_rectlist()). Once the
> color resolve is done the vbo setup is left to the resolve rectangle
> state and the original drawing call yields bogus results.
> 
> This patch aims to restore the original state after the color
> resolve by calling vbo_bind_arrays() yet again after the vertex
> array state in the core context have been restored.
> 
> Now having said all this, I'd also like to state that I'm quite
> uncomfortable with the nested meta operations. Ths original draw
> call in this case is in fact a meta operation itself. It is a blit
> from level zero to level one when generating the additional mipmap
> levels (see _mesa_meta_GenerateMipmap()). Imagine the complexity
> if the blit in the middle from buffer to another would go to meta
> path also instead of blitter.
> 

To be accurate, there isn't really added complexity, only added risk. In
general, the complexity remains the same no matter how many levels deep you
recurse, it's just the more different ways you use meta, the more likely you are
to hit some state you forget to save or restore. Okay, I'm just picking nits.

> I would very tempted to try to move all the resolves to happen
> before a meta operation is started.
> Additionally I still feel that work I did earlier in the spring/
> summer time moving meta operations to use direct state upload
> bypassing the core context would make sense.
> 

Tracking resolves, from the hardware perspective should be pretty easy, at least
on paper. It's any time you require a part of the hardware that doesn't
understand the underlying format - sampler operation because of API, blitter
operation because of what you found, etc.

I think certainly as a debug feature, the idea of always doing the resolve is
a sure thing. I'm still undecided though if you just solved a really hard to
track down bug, or you have a real point about how to proceed always. There are
certainly cases where not doing a resolve is possible, and optimal.

> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> index d338659..affe1bc 100644
> --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> @@ -896,6 +896,9 @@ brw_meta_resolve_color(struct brw_context *brw,
>  
>     _mesa_meta_end(ctx);
>  
> +   /* Restore in case we were called in the middle of brw_try_draw_prims() */
> +   vbo_bind_arrays(ctx);
> +

I don't claim to understand meta well enough to call this reviewed-by (mostly, I
don't understand mesa core well enough). But from your description this totally
sounds like a thing we need to do. So maybe call this:
Acked-by: Ben Widawsky <benjamin.widawsky at intel.com>

>     /* We're typically called from intel_update_state() and we're supposed to
>      * return with the state all updated to what it was before
>      * brw_meta_resolve_color() was called.  The meta rendering will have
> -- 
> 2.5.0
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev