[Mesa-dev] [PATCH v3] anv/query: implement multiview interactions

[Jason Ekstrand]

On Wed, Jan 17, 2018 at 4:59 AM, Iago Toral Quiroga <itoral at igalia.com>
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.


> +{
> +   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;
>

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.

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/20180117/ea3cb7fa/attachment-0001.html>