[Mesa-dev] [PATCH] anv: Improve flushing around STATE_BASE_ADDRESS

[Iago Toral]

On Tue, 2017-01-31 at 10:33 -0800, Jason Ekstrand wrote:
> It is not clear from the docs exactly how pipelined
> STATE_BASE_ADDRESS
> actually is.  Previously, we knew we needed to flush prior to re-
> emitting
> STATE_BASE_ADDRESS on gen8+ but we had never confirmed it on gen7 so
> we
> left it alone and avoided the flush.  Recently, Mark found hangs on
> gen7
> which appear to be STATE_BASE_ADDRESS related which this patch fixes.
> The theory is that SURFACE_STATE structures are cached in texture
> cache
> but relative addressing nature of things doesn't work nicely with the
> cache.  This means that you need to invalidate the texture cache when
> you change STATE_BASE_ADDRESS and interesting things can happen if
> there
> are any EU threads in-flight when surface state base address changes.

If we already knew this was necessary in gen8 and it solves hangs in
gen7 too I guess it should be correct.

> While we're here, we also add data cache flushing in order to ensure
> that any compute shaders running are finished before we change the
> surface state base address.  It's unknown whether or not this would
> actually be a problem but, given how hard these things are to debug,
> we
> might as well make sure.

I suppose this is okay too. It is a bit annoying that the docs are not
more clear about these things and we end up having to code defensively
to avoid running into hard to debug scenarios... Maybe add a FIXME
indicating this situation so we know we have not verified that this is
actually needed.

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

> Reported-by: Mark Janes <mark.a.janes at intel.com>
> Tested-by: Mark Janes <mark.a.janes at intel.com>
> ---
>  src/intel/vulkan/genX_cmd_buffer.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index f7894a0..7b43c6f 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -55,8 +55,6 @@ genX(cmd_buffer_emit_state_base_address)(struct
> anv_cmd_buffer *cmd_buffer)
>  {
>     struct anv_device *device = cmd_buffer->device;
>  
> -/* XXX: Do we need this on more than just BDW? */
> -#if (GEN_GEN >= 8)
>     /* Emit a render target cache flush.
>      *
>      * This isn't documented anywhere in the PRM.  However, it seems
> to be
> @@ -65,9 +63,10 @@ genX(cmd_buffer_emit_state_base_address)(struct
> anv_cmd_buffer *cmd_buffer)
>      * clear depth, reset state base address, and then go render
> stuff.
>      */
>     anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) {
> +      pc.DCFlushEnable = true;
>        pc.RenderTargetCacheFlushEnable = true;
> +      pc.CommandStreamerStallEnable = true;
>     }
> -#endif
>  
>     anv_batch_emit(&cmd_buffer->batch, GENX(STATE_BASE_ADDRESS), sba)
> {
>        sba.GeneralStateBaseAddress = (struct anv_address) { NULL, 0
> };