On 03/12/2018 22:08, Jason Ekstrand wrote:
On Mon, Dec 3, 2018 at 11:26 AM Lionel Landwerlin
<lionel.g.landwer...@intel.com <mailto: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
<mailto: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
<mailto: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.
I could add another bit like "side buffers writes" which would call for
a pixel scoreboard stall only.
}
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,
Will do.
Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net
<mailto: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
