[Mesa-dev] [PATCH v3] anv/query: implement multiview interactions
Jason Ekstrand
jason at jlekstrand.net
Thu Jan 18 15:13:50 UTC 2018
On January 17, 2018 23:06:23 Iago Toral <itoral at 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 at 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 at 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
>> >
>> >
>> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180118/4d5ec894/attachment-0001.html>
More information about the mesa-dev
mailing list