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

Thu Jan 18 07:06:22 UTC 2018

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;
> 
> 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?
> 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/dbf6bf9e/attachment-0001.html>