[Mesa-dev] [PATCH] anv/query: flush render target before copying results

[Jason Ekstrand]

On Mon, Dec 3, 2018 at 11:26 AM Lionel Landwerlin <
lionel.g.landwerlin at intel.com> wrote:

> This change tracks render target writes in the pipeline and applies a
> render target flush before copying the query results to make sure the
> preceding operations have landed in memory before the command streamer
> initiates the copy.
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108909
> Fixes: 37f9788e9a8e44 ("anv: flush pipeline before query result copies")
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/intel/vulkan/anv_private.h     |  7 +++++++
>  src/intel/vulkan/genX_blorp_exec.c |  1 +
>  src/intel/vulkan/genX_cmd_buffer.c | 14 ++++++++++++++
>  src/intel/vulkan/genX_gpu_memcpy.c |  1 +
>  src/intel/vulkan/genX_query.c      | 10 +++++++++-
>  5 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index 62c563294f5..aff076a55d9 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -1747,6 +1747,13 @@ enum anv_pipe_bits {
>      * we would have to CS stall on every flush which could be bad.
>      */
>     ANV_PIPE_NEEDS_CS_STALL_BIT               = (1 << 21),
> +
> +   /* This bit does not exist directly in PIPE_CONTROL. It means that
> render
> +    * target operations are ongoing. Some operations like copies on the
> +    * command streamer might need to be aware of this to trigger the
> +    * appropriate stall before they can proceed with the copy.
> +    */
> +   ANV_PIPE_RENDER_TARGET_WRITES              = (1 << 22),
>  };
>
>  #define ANV_PIPE_FLUSH_BITS ( \
> diff --git a/src/intel/vulkan/genX_blorp_exec.c
> b/src/intel/vulkan/genX_blorp_exec.c
> index 2035017ce0e..c573e890946 100644
> --- a/src/intel/vulkan/genX_blorp_exec.c
> +++ b/src/intel/vulkan/genX_blorp_exec.c
> @@ -263,4 +263,5 @@ genX(blorp_exec)(struct blorp_batch *batch,
>     cmd_buffer->state.gfx.vb_dirty = ~0;
>     cmd_buffer->state.gfx.dirty = ~0;
>     cmd_buffer->state.push_constants_dirty = ~0;
> +   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_RENDER_TARGET_WRITES;
>  }
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index c7e5ef9596e..fb70cd2e386 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -1766,6 +1766,12 @@ genX(cmd_buffer_apply_pipe_flushes)(struct
> anv_cmd_buffer *cmd_buffer)
>              pipe.StallAtPixelScoreboard = true;
>        }
>
> +      /* If a render target flush was emitted, then we can toggle off the
> bit
> +       * saying that render target writes are ongoing.
> +       */
> +      if (bits & ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT)
> +         bits &= ~(ANV_PIPE_RENDER_TARGET_WRITES);
> +
>        bits &= ~(ANV_PIPE_FLUSH_BITS | ANV_PIPE_CS_STALL_BIT);
>     }
>
> @@ -2777,6 +2783,8 @@ void genX(CmdDraw)(
>        prim.StartInstanceLocation    = firstInstance;
>        prim.BaseVertexLocation       = 0;
>     }
> +
> +   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_RENDER_TARGET_WRITES;
>  }
>
>  void genX(CmdDrawIndexed)(
> @@ -2816,6 +2824,8 @@ void genX(CmdDrawIndexed)(
>        prim.StartInstanceLocation    = firstInstance;
>        prim.BaseVertexLocation       = vertexOffset;
>     }
> +
> +   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_RENDER_TARGET_WRITES;
>  }
>
>  /* Auto-Draw / Indirect Registers */
> @@ -2949,6 +2959,8 @@ void genX(CmdDrawIndirect)(
>
>        offset += stride;
>     }
> +
> +   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_RENDER_TARGET_WRITES;
>  }
>
>  void genX(CmdDrawIndexedIndirect)(
> @@ -2988,6 +3000,8 @@ void genX(CmdDrawIndexedIndirect)(
>
>        offset += stride;
>     }
> +
> +   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_RENDER_TARGET_WRITES;
>  }
>
>  static VkResult
> diff --git a/src/intel/vulkan/genX_gpu_memcpy.c
> b/src/intel/vulkan/genX_gpu_memcpy.c
> index 81522986550..1bee1c6dc17 100644
> --- a/src/intel/vulkan/genX_gpu_memcpy.c
> +++ b/src/intel/vulkan/genX_gpu_memcpy.c
> @@ -302,4 +302,5 @@ genX(cmd_buffer_so_memcpy)(struct anv_cmd_buffer
> *cmd_buffer,
>     }
>
>     cmd_buffer->state.gfx.dirty |= ANV_CMD_DIRTY_PIPELINE;
> +   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_RENDER_TARGET_WRITES;
>

This is a bit odd since there are no RT writes going on.... Still, probably
accurate enough so meh.


>  }
> diff --git a/src/intel/vulkan/genX_query.c b/src/intel/vulkan/genX_query.c
> index 4831c4ea334..130c9661f15 100644
> --- a/src/intel/vulkan/genX_query.c
> +++ b/src/intel/vulkan/genX_query.c
> @@ -730,7 +730,15 @@ void genX(CmdCopyQueryPoolResults)(
>     ANV_FROM_HANDLE(anv_buffer, buffer, destBuffer);
>
>     if ((flags & VK_QUERY_RESULT_WAIT_BIT) ||
> -       (cmd_buffer->state.pending_pipe_bits & ANV_PIPE_FLUSH_BITS)) {
> +       (cmd_buffer->state.pending_pipe_bits &
> +        (ANV_PIPE_FLUSH_BITS | ANV_PIPE_RENDER_TARGET_WRITES))) {
> +      /* If render target writes are ongoing, request a render target
> cache
> +       * flush.
> +       */
> +      if (cmd_buffer->state.pending_pipe_bits &
> ANV_PIPE_RENDER_TARGET_WRITES) {
> +         cmd_buffer->state.pending_pipe_bits |=
> +            ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT;
> +      }
>

You could also just put the above bit above the whole if and you wouldn't
need to explicitly check ANV_PIPE_RENDER_TARGET_WRITE there.  I'm not sure
that's actually better; it just looks like less code.  Either way,

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

Thanks for sorting this out!

--Jason


>        cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_CS_STALL_BIT;
>        genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
>     }
> --
> 2.20.0.rc1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181203/d2093b90/attachment-0001.html>