<html> <head> <meta http-equiv="Content-Type" content="text/html"/> </head> <body> <div style="color: black;"> On January 17, 2018 23:06:23 Iago Toral <itoral@igalia.com> wrote: > On Wed, 2018-01-17 at 13:14 -0800, Jason Ekstrand wrote: >> On Wed, Jan 17, 2018 at 4:59 AM, Iago Toral Quiroga <itoral@igalia.co >> m> wrote: >> > From the Vulkan spec with KHX extensions: >> > >> > >> > >> > "If queries are used while executing a render pass instance that >> > has >> > >> > multiview enabled, the query uses N consecutive query indices >> > >> > in the query pool (starting at query) where N is the number of >> > bits >> > >> > set in the view mask in the subpass the query is used in. >> > >> > >> > >> > How the numerical results of the query are distributed among the >> > >> > queries is implementation-dependent. For example, some >> > implementations >> > >> > may write each view's results to a distinct query, while other >> > >> > implementations may write the total result to the first query >> > and write >> > >> > zero to the other queries. However, the sum of the results in >> > all the >> > >> > queries must accurately reflect the total result of the query >> > summed >> > >> > over all views. Applications can sum the results from all the >> > queries to >> > >> > compute the total result." >> > >> > >> > >> > In our case we only really emit a single query (in the first query >> > index) >> > >> > that stores the aggregated result for all views, but we still need >> > to manage >> > >> > availability for all the other query indices involved, even if we >> > don't >> > >> > actually use them. >> > >> > >> > >> > This is relevant when clients call vkGetQueryPoolResults and pass >> > all N >> > >> > queries to retrieve the results. In that scenario, without this >> > patch, >> > >> > we will never see queries other than the first being available >> > since we >> > >> > never emit them. >> > >> > >> > >> > v2: we need the same tratment for timestamp queries. >> > >> > >> > >> > v3 (Jason): >> > >> > - Better and if instead of an early return. >> > >> > - We can't write to this memory in the CPU, we should use >> > >> > MI_STORE_DATA_IMM and emit_query_availability (Jason). >> > >> > >> > >> > Fixes test failures in some work-in-progress CTS multiview+query >> > tests. >> > >> > --- >> > >> > src/intel/vulkan/genX_query.c | 54 >> > +++++++++++++++++++++++++++++++++++++++++++ >> > >> > 1 file changed, 54 insertions(+) >> > >> > >> > >> > diff --git a/src/intel/vulkan/genX_query.c >> > b/src/intel/vulkan/genX_query.c >> > >> > index 7683d0d1e3..8db3d7b561 100644 >> > >> > --- a/src/intel/vulkan/genX_query.c >> > >> > +++ b/src/intel/vulkan/genX_query.c >> > >> > @@ -322,6 +322,30 @@ emit_query_availability(struct anv_cmd_buffer >> > *cmd_buffer, >> > >> > } >> > >> > } >> > >> > >> > >> > +/** >> > >> > + * Goes through a series of consecutive query indices in the given >> > pool, >> > >> > + * sets all elements to the provided value and emits the queries. >> > >> > + */ >> > >> > +static void >> > >> > +emit_preset_queries(struct anv_cmd_buffer *cmd_buffer, >> > >> > + struct anv_query_pool *pool, >> > >> > + uint32_t first_index, uint32_t num_queries, >> > uint64_t value) >> >> 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. > > Ok. >> > +{ >> > >> > + const uint32_t num_elements = pool->stride / sizeof(uint64_t); >> > >> > + >> > >> > + for (uint32_t i = 0; i < num_queries; i++) { >> > >> > + uint32_t slot = (first_index + i) * pool->stride; slot_offset might be a better name. >> 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. > > 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? Your right. I didn't read clearly enough. What you have is correct. Rb stands. >> With those two adjustments made, this would be >> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> >> >> > + for (uint32_t j = 1; j < num_elements; j++) { >> > >> > + anv_batch_emit(&cmd_buffer->batch, >> > GENX(MI_STORE_DATA_IMM), sdi) { >> > >> > + sdi.Address.bo = &pool->bo; >> > >> > + sdi.Address.offset = slot + j * sizeof(uint64_t); >> > >> > + sdi.ImmediateData = value; >> > >> > + } >> > >> > + } >> > >> > + emit_query_availability(cmd_buffer, &pool->bo, slot); >> > >> > + } >> > >> > +} >> > >> > + >> > >> > void genX(CmdResetQueryPool)( >> > >> > VkCommandBuffer commandBuffer, >> > >> > VkQueryPool queryPool, >> > >> > @@ -462,6 +486,21 @@ void genX(CmdEndQuery)( >> > >> > default: >> > >> > unreachable(""); >> > >> > } >> > >> > + >> > >> > + /* When multiview is active the spec requires that N >> > consecutive query >> > >> > + * indices are used, where N is the number of active views in >> > the subpass. >> > >> > + * The spec allows that we only write the results to one of the >> > queries >> > >> > + * but we still need to manage result availability for all the >> > query indices. >> > >> > + * Since we only emit a single query for all active views in >> > the >> > >> > + * first index, mark the other query indices as being already >> > available >> > >> > + * with result 0. >> > >> > + */ >> > >> > + if (cmd_buffer->state.subpass && cmd_buffer->state.subpass- >> > >view_mask) { >> > >> > + const uint32_t num_queries = >> > >> > + _mesa_bitcount(cmd_buffer->state.subpass->view_mask); >> > >> > + if (num_queries > 1) >> > >> > + emit_preset_queries(cmd_buffer, pool, query + 1, >> > num_queries - 1, 0LL); >> > >> > + } >> > >> > } >> > >> > >> > >> > #define TIMESTAMP 0x2358 >> > >> > @@ -504,6 +543,21 @@ void genX(CmdWriteTimestamp)( >> > >> > } >> > >> > >> > >> > emit_query_availability(cmd_buffer, &pool->bo, offset); >> > >> > + >> > >> > + /* When multiview is active the spec requires that N >> > consecutive query >> > >> > + * indices are used, where N is the number of active views in >> > the subpass. >> > >> > + * The spec allows that we only write the results to one of the >> > queries >> > >> > + * but we still need to manage result availability for all the >> > query indices. >> > >> > + * Since we only emit a single query for all active views in >> > the >> > >> > + * first index, mark the other query indices as being already >> > available >> > >> > + * with result 0. >> > >> > + */ >> > >> > + if (cmd_buffer->state.subpass && cmd_buffer->state.subpass- >> > >view_mask) { >> > >> > + const uint32_t num_queries = >> > >> > + _mesa_bitcount(cmd_buffer->state.subpass->view_mask); >> > >> > + if (num_queries > 1) >> > >> > + emit_preset_queries(cmd_buffer, pool, query + 1, >> > num_queries - 1, 0LL); >> > >> > + } >> > >> > } >> > >> > >> > >> > #if GEN_GEN > 7 || GEN_IS_HASWELL >> > >> > -- >> > >> > 2.14.1 >> > >> > >> > </div> </body> </html>