On Wed, Jan 17, 2018 at 1:27 AM, Iago Toral <ito...@igalia.com> wrote:
> On Tue, 2018-01-16 at 08:07 -0800, Jason Ekstrand wrote:
>
> On Mon, Jan 8, 2018 at 4:57 AM, Iago Toral Quiroga <ito...@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.
>
> Fixes test failures in some work-in-progress CTS multiview+query tests.
> ---
> src/intel/vulkan/genX_query.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/src/intel/vulkan/genX_query.c b/src/intel/vulkan/genX_query.c
> index 7683d0d1e3..231c605b6b 100644
> --- a/src/intel/vulkan/genX_query.c
> +++ b/src/intel/vulkan/genX_query.c
> @@ -462,6 +462,24 @@ 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->vi
> ew_mask)
> + return;
>
>
> I think this would be better as just an if instead of an early return.
>
>
> +
> + uint32_t num_queries = _mesa_bitcount(cmd_buffer->sta
> te.subpass->view_mask);
> + for (uint32_t i = 1; i < num_queries; i++) {
> + uint64_t *slot = pool->bo.map + (query + i) * pool->stride;
> + slot[0] = 1;
> + memset(&slot[1], 0, sizeof(uint64_t) * pool->stride);
>
>
> We can't set this from the CPU, we need to emit an MI_STORE_DATA_IMM and
> call emit_query_availability.
>
>
> Oh ok, I'll do that then. Thanks for the feedback! Out of curiosity, what
> is the problem with setting this on the CPU side? I was thinking that since
> we are not emitting these queries at all the GPU is not going to touch the
> memory for them anyway...
>
Because they might re-use the query pool and a given slot may be written
from the CPU or the GPU depending on how they construct the command buffer
and they may switch back and forth for a given query. Also, touching it
from the CPU happens immediately instead of when the command executes. The
combination of these two things can lead to some very bizarre results.
> + }
> }
>
> #define TIMESTAMP 0x2358
> @@ -504,6 +522,24 @@ 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->vi
> ew_mask)
> + return;
> +
> + uint32_t num_queries = _mesa_bitcount(cmd_buffer->sta
> te.subpass->view_mask);
> + for (uint32_t i = 1; i < num_queries; i++) {
> + uint64_t *slot = pool->bo.map + (query + i) * pool->stride;
> + slot[0] = 1;
> + memset(&slot[1], 0, sizeof(uint64_t) * pool->stride);
>
>
> Same comments here.
>
>
> + }
> }
>
> #if GEN_GEN > 7 || GEN_IS_HASWELL
> --
> 2.11.0
>
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
