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

[Kenneth Graunke]

On Tuesday, January 31, 2017 10:33:41 AM PST 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

This description doesn't match the actual change you're making.  We
already invalidate the texture cache after STATE_BASE_ADDRESS, on all
generations.  You're making it flush the render cache.  According to
the comment already in that function, it sounds like the render cache
may also include surface states, so...same explanation, just
s/texture/render/g.  It seems like a fine thing to do.

> you change STATE_BASE_ADDRESS and interesting things can happen if there
> are any EU threads in-flight when surface state base address changes.
> 
> 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.

That's kind of a separate change from applying the flushes on Gen7/7.5.
It also seems like a fine thing to do.  Might make sense to split into
two patches.

At minimum, assuming you update the commit message,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> 
> 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 };
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170131/ae5c9b99/attachment.sig>