<html><head></head><body><div>On Wed, 2018-01-17 at 13:14 -0800, Jason Ekstrand wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 17, 2018 at 4:59 AM, Iago Toral Quiroga <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><span class="">From the Vulkan spec with KHX extensions:<br>
<br>
"If queries are used while executing a render pass instance that has<br>
multiview enabled, the query uses N consecutive query indices<br>
in the query pool (starting at query) where N is the number of bits<br>
set in the view mask in the subpass the query is used in.<br>
<br>
How the numerical results of the query are distributed among the<br>
queries is implementation-dependent. For example, some implementations<br>
may write each view's results to a distinct query, while other<br>
implementations may write the total result to the first query and write<br>
zero to the other queries. However, the sum of the results in all the<br>
queries must accurately reflect the total result of the query summed<br>
over all views. Applications can sum the results from all the queries to<br>
compute the total result."<br>
<br>
In our case we only really emit a single query (in the first query index)<br>
that stores the aggregated result for all views, but we still need to manage<br>
availability for all the other query indices involved, even if we don't<br>
actually use them.<br>
<br>
This is relevant when clients call vkGetQueryPoolResults and pass all N<br>
queries to retrieve the results. In that scenario, without this patch,<br>
we will never see queries other than the first being available since we<br>
never emit them.<br>
<br>
</span>v2: we need the same tratment for timestamp queries.<br>
<br>
v3 (Jason):<br>
- Better and if instead of an early return.<br>
- We can't write to this memory in the CPU, we should use<br>
MI_STORE_DATA_IMM and emit_query_availability (Jason).<br>
<span class=""><br>
Fixes test failures in some work-in-progress CTS multiview+query tests.<br>
---<br>
</span> src/intel/vulkan/genX_query.c | 54 +++++++++++++++++++++++++++++++++++++++++++<br>
1 file changed, 54 insertions(+)<br>
<br>
diff --git a/src/intel/vulkan/genX_query.c b/src/intel/vulkan/genX_query.c<br>
index 7683d0d1e3..8db3d7b561 100644<br>
--- a/src/intel/vulkan/genX_query.c<br>
+++ b/src/intel/vulkan/genX_query.c<br>
@@ -322,6 +322,30 @@ emit_query_availability(struct anv_cmd_buffer *cmd_buffer,<br>
}<br>
}<br>
<br>
+/**<br>
+ * Goes through a series of consecutive query indices in the given pool,<br>
+ * sets all elements to the provided value and emits the queries.<br>
+ */<br>
+static void<br>
+emit_preset_queries(struct anv_cmd_buffer *cmd_buffer,<br>
+ struct anv_query_pool *pool,<br>
+ uint32_t first_index, uint32_t num_queries, uint64_t value)<br></blockquote><div><br></div><div>I don't know that having a value passed in is all that useful. We may as well make it emit_zero_query or emit_null_query.</div></div></div></div></blockquote><div><br></div><div>Ok.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+{<br>
+ const uint32_t num_elements = pool->stride / sizeof(uint64_t); <br>
+<br>
+ for (uint32_t i = 0; i < num_queries; i++) {<br>
+ uint32_t slot = (first_index + i) * pool->stride;<br></blockquote><div><br></div><div>I think you probably want to start with i = 1 instead of i = 0. write_availability below will set the first uint64_t for you; this loop only needs to set the others.</div></div></div></div></blockquote><div><br></div><div>This is looping through the queries that need to be emitted as 0, the loop below handles writing 0's to all their value elements, first_index is the index of the first query that needs to be set to 0 and num_elements represents the number of queries (starting at first_index that need to be set to 0), so I think the loop is fine. The loop below is the one that takes care of setting a single query to 0, and that one starts at 1 like you suggest, so I think this is fine, right?</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>With those two adjustments made, this would be</div><div><br></div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div><div> </div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+ for (uint32_t j = 1; j < num_elements; j++) {<br>
+ anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), sdi) {<br>
+ <a href="http://sdi.Address.bo" rel="noreferrer" target="_blank">sdi.Address.bo</a> = &pool->bo;<br>
+ sdi.Address.offset = slot + j * sizeof(uint64_t);<br>
+ sdi.ImmediateData = value;<br>
+ }<br>
+ }<br>
+ emit_query_availability(cmd_buffer, &pool->bo, slot);<br>
+ }<br>
+}<br>
+<br>
void genX(CmdResetQueryPool)(<br>
VkCommandBuffer commandBuffer,<br>
VkQueryPool queryPool,<br>
@@ -462,6 +486,21 @@ void genX(CmdEndQuery)(<br>
<span class=""> default:<br>
unreachable("");<br>
}<br>
+<br>
+ /* When multiview is active the spec requires that N consecutive query<br>
+ * indices are used, where N is the number of active views in the subpass.<br>
+ * The spec allows that we only write the results to one of the queries<br>
+ * but we still need to manage result availability for all the query indices.<br>
+ * Since we only emit a single query for all active views in the<br>
+ * first index, mark the other query indices as being already available<br>
+ * with result 0.<br>
+ */<br>
</span>+ if (cmd_buffer->state.subpass && cmd_buffer->state.subpass->view_mask) {<br>
+ const uint32_t num_queries =<br>
+ _mesa_bitcount(cmd_buffer->state.subpass->view_mask);<br>
+ if (num_queries > 1)<br>
+ emit_preset_queries(cmd_buffer, pool, query + 1, num_queries - 1, 0LL);<br>
+ }<br>
}<br>
<br>
#define TIMESTAMP 0x2358<br>
@@ -504,6 +543,21 @@ void genX(CmdWriteTimestamp)(<br>
<span class=""> }<br>
<br>
emit_query_availability(cmd_<wbr>buffer, &pool->bo, offset);<br>
+<br>
+ /* When multiview is active the spec requires that N consecutive query<br>
+ * indices are used, where N is the number of active views in the subpass.<br>
+ * The spec allows that we only write the results to one of the queries<br>
+ * but we still need to manage result availability for all the query indices.<br>
+ * Since we only emit a single query for all active views in the<br>
+ * first index, mark the other query indices as being already available<br>
+ * with result 0.<br>
+ */<br>
</span>+ if (cmd_buffer->state.subpass && cmd_buffer->state.subpass->view_mask) {<br>
+ const uint32_t num_queries =<br>
+ _mesa_bitcount(cmd_buffer->state.subpass->view_mask);<br>
+ if (num_queries > 1)<br>
+ emit_preset_queries(cmd_buffer, pool, query + 1, num_queries - 1, 0LL);<br>
<span class="">+ }<br>
}<br>
<br>
#if GEN_GEN > 7 || GEN_IS_HASWELL<br>
--<br>
</span>2.14.1<br>
<br>
</blockquote></div><br></div></div>
</blockquote></body></html>