<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 26, 2018 at 4:08 AM, Iago Toral Quiroga <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Storage images require to patch push constant stateto work, which happens during<br>
binding table emision. In the scenario where our pipeline and descriptors are<br>
not dirty, we don't re-emit the binding table, however, if our push constant<br>
state is dirty, we will re-emit the push constant state, trashing storage<br>
image setup.<br>
<br>
While that scenario is probably not very likely to happen in practice, there<br>
are some CTS tests that trigger this by clearing storage images and buffers<br>
and dispatching a compute shader in a loop. The clearing of the images<br>
and buffers will trigger a blorp execution which will dirty our push constant<br>
state, however, because we don't alter the descriptors or the compute dispatch<br>
at all in the loop (we are basically execution the same program in a loop),<br>
our pipeline and descriptor state is not dirty. If the shader uses a storage<br>
image, then any iteration after the first will re-emit push constant state<br>
without re-emitting binding tables and the storage image will not be properly<br>
setup any more.<br></blockquote><div><br></div><div>I don't see why that is a problem. The only thing flush_descriptor_sets does is fill out the binding and sampler tables and fill in the push constant data for storage images/buffers. The actual HW packets are filled out by flush_push_constants and emit_descriptor_pointers. Yes, blorp trashes our descriptor pointers but the descriptor sets should be fine. For push constants, it does emit 3DSTATE_CONSTANT_* but it doesn't actually modify anv_cmd_state::push_constants.</div><div><br></div><div>Are secondary command buffers involved? I could see something funny going on with those.</div><div><br></div><div>--Jason</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Fixes multiple failures in some new CTS tests.<br>
---<br>
src/intel/vulkan/genX_cmd_<wbr>buffer.c | 9 ++++++++-<br>
1 file changed, 8 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/src/intel/vulkan/genX_cmd_<wbr>buffer.c b/src/intel/vulkan/genX_cmd_<wbr>buffer.c<br>
index 97b321ccaeb..6e48aaedb9b 100644<br>
--- a/src/intel/vulkan/genX_cmd_<wbr>buffer.c<br>
+++ b/src/intel/vulkan/genX_cmd_<wbr>buffer.c<br>
@@ -2554,7 +2554,8 @@ genX(cmd_buffer_flush_state)(<wbr>struct anv_cmd_buffer *cmd_buffer)<br>
* 3DSTATE_BINDING_TABLE_POINTER_<wbr>* for the push constants to take effect.<br>
*/<br>
uint32_t dirty = 0;<br>
- if (cmd_buffer->state.<wbr>descriptors_dirty)<br>
+ if (cmd_buffer->state.<wbr>descriptors_dirty ||<br>
+ cmd_buffer->state.push_<wbr>constants_dirty)<br>
dirty = flush_descriptor_sets(cmd_<wbr>buffer);<br>
<br>
if (dirty || cmd_buffer->state.push_<wbr>constants_dirty) {<br>
@@ -2988,7 +2989,13 @@ genX(cmd_buffer_flush_compute_<wbr>state)(struct anv_cmd_buffer *cmd_buffer)<br>
anv_batch_emit_batch(&cmd_<wbr>buffer->batch, &pipeline->batch);<br>
}<br>
<br>
+ /* Storage images require push constant data, which is setup during the<br>
+ * binding table emision. If we have dirty push constants, we need to<br>
+ * re-emit the binding table so we get the push constant storage image setup<br>
+ * done, otherwise we trash it when we emit push constants below.<br>
+ */<br>
if ((cmd_buffer->state.<wbr>descriptors_dirty & VK_SHADER_STAGE_COMPUTE_BIT) ||<br>
+ (cmd_buffer->state.push_<wbr>constants_dirty & VK_SHADER_STAGE_COMPUTE_BIT) ||<br>
cmd_buffer->state.compute.<wbr>pipeline_dirty) {<br>
/* FIXME: figure out descriptors for gen7 */<br>
result = flush_compute_descriptor_set(<wbr>cmd_buffer);<br>
<span class="HOEnZb"><font color="#888888">-- <br>
2.14.1<br>
<br>
</font></span></blockquote></div><br></div></div>