On Mon, Dec 3, 2018 at 11:26 AM Lionel Landwerlin < lionel.g.landwer...@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.landwer...@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108909 > Fixes: 37f9788e9a8e44 ("anv: flush pipeline before query result copies") > Cc: mesa-sta...@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 <ja...@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 > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
