[Mesa-stable] [Mesa-dev] [PATCH v3] radv: make sure to emit cache flushes before starting a query

[Samuel Pitoiset]

On 03/01/2018 10:21 AM, Alex Smith wrote:
> Hi Samuel,
> 
> On 28 February 2018 at 20:47, Samuel Pitoiset <samuel.pitoiset at gmail.com 
> <mailto:samuel.pitoiset at gmail.com>> wrote:
> 
>     If the query pool has been previously resetted using the compute
>     shader path.
> 
>     v3: set pending_reset_query only for the compute shader path
>     v2: handle multiple commands buffers with same pool
> 
>     Fixes: a41e2e9cf5 ("radv: allow to use a compute shader for
>     resetting the query pool")
>     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105292
>     <https://bugs.freedesktop.org/show_bug.cgi?id=105292>
>     Cc: "18.0" <mesa-stable at lists.freedesktop.org
>     <mailto:mesa-stable at lists.freedesktop.org>>
>     Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com
>     <mailto:samuel.pitoiset at gmail.com>>
>     ---
>       src/amd/vulkan/radv_cmd_buffer.c |  7 +++++++
>       src/amd/vulkan/radv_private.h    |  5 +++++
>       src/amd/vulkan/radv_query.c      | 28 +++++++++++++++++++++-------
>       3 files changed, 33 insertions(+), 7 deletions(-)
> 
>     diff --git a/src/amd/vulkan/radv_cmd_buffer.c
>     b/src/amd/vulkan/radv_cmd_buffer.c
>     index 2b41baea3d..cfdc531acd 100644
>     --- a/src/amd/vulkan/radv_cmd_buffer.c
>     +++ b/src/amd/vulkan/radv_cmd_buffer.c
>     @@ -1930,6 +1930,13 @@ VkResult radv_BeginCommandBuffer(
> 
>              cmd_buffer->status = RADV_CMD_BUFFER_STATUS_RECORDING;
> 
>     +       /* Force cache flushes before starting a new query in case the
>     +        * corresponding pool has been resetted from a different command
>     +        * buffer. This is because we have to flush caches between
>     reset and
>     +        * begin if the compute shader path has been used.
>     +        */
>     +       cmd_buffer->pending_reset_query = true;
> 
> 
> Since this just ends up calling si_emit_cache_flush, doesn't flush_bits 
> need to be set accordingly for it to actually do anything? If the reset 
> is done in a previous command buffer, I think the flush would already 
> have been done in EndCommandBuffer on that?

You are right, this is just useless because we always flushes caches in 
EndCommandBuffer(). Thanks for pointing this out. Will remove that hunk.

> 
> Thanks,
> Alex
> 
>     +
>              return result;
>       }
> 
>     diff --git a/src/amd/vulkan/radv_private.h
>     b/src/amd/vulkan/radv_private.h
>     index c72df5a737..b76d2eb5cb 100644
>     --- a/src/amd/vulkan/radv_private.h
>     +++ b/src/amd/vulkan/radv_private.h
>     @@ -1003,6 +1003,11 @@ struct radv_cmd_buffer {
>              uint32_t gfx9_fence_offset;
>              struct radeon_winsys_bo *gfx9_fence_bo;
>              uint32_t gfx9_fence_idx;
>     +
>     +       /**
>     +        * Whether a query pool has been resetted and we have to
>     flush caches.
>     +        */
>     +       bool pending_reset_query;
>       };
> 
>       struct radv_image;
>     diff --git a/src/amd/vulkan/radv_query.c b/src/amd/vulkan/radv_query.c
>     index ace745e4e6..b1393a2ec7 100644
>     --- a/src/amd/vulkan/radv_query.c
>     +++ b/src/amd/vulkan/radv_query.c
>     @@ -1058,17 +1058,23 @@ void radv_CmdResetQueryPool(
>       {
>              RADV_FROM_HANDLE(radv_cmd_buffer, cmd_buffer, commandBuffer);
>              RADV_FROM_HANDLE(radv_query_pool, pool, queryPool);
>     -       struct radv_cmd_state *state = &cmd_buffer->state;
>     +       uint32_t flush_bits = 0;
> 
>     -       state->flush_bits |= radv_fill_buffer(cmd_buffer, pool->bo,
>     -                                             firstQuery * pool->stride,
>     -                                             queryCount *
>     pool->stride, 0);
>     +       flush_bits |= radv_fill_buffer(cmd_buffer, pool->bo,
>     +                                      firstQuery * pool->stride,
>     +                                      queryCount * pool->stride, 0);
> 
>              if (pool->type == VK_QUERY_TYPE_TIMESTAMP ||
>                  pool->type == VK_QUERY_TYPE_PIPELINE_STATISTICS) {
>     -               state->flush_bits |= radv_fill_buffer(cmd_buffer,
>     pool->bo,
>     -                                                   
>       pool->availability_offset + firstQuery * 4,
>     -                                                     queryCount *
>     4, 0);
>     +               flush_bits |= radv_fill_buffer(cmd_buffer, pool->bo,
>     +                                             
>     pool->availability_offset + firstQuery * 4,
>     +                                              queryCount * 4, 0);
>     +       }
>     +
>     +       if (flush_bits) {
>     +               /* Only need to flush caches for the compute shader
>     path. */
>     +               cmd_buffer->pending_reset_query = true;
>     +               cmd_buffer->state.flush_bits |= flush_bits;
>              }
>       }
> 
>     @@ -1086,6 +1092,14 @@ void radv_CmdBeginQuery(
> 
>              radv_cs_add_buffer(cmd_buffer->device->ws, cs, pool->bo, 8);
> 
>     +       if (cmd_buffer->pending_reset_query) {
>     +               /* Make sure to flush caches if the query pool has been
>     +                * previously resetted using the compute shader path.
>     +                */
>     +               si_emit_cache_flush(cmd_buffer);
>     +               cmd_buffer->pending_reset_query = false;
>     +       }
>     +
>              switch (pool->type) {
>              case VK_QUERY_TYPE_OCCLUSION:
>                      radeon_check_space(cmd_buffer->device->ws, cs, 7);
>     --
>     2.16.2
> 
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>     <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
> 
>