anv/query: ensure visibility of reset before copying query results

Submitted by Iago Toral Quiroga on April 30, 2019, 12:43 p.m.

Details

Message ID 20190430124319.16747-1-itoral@igalia.com
State New
Headers show
Series "anv/query: ensure visibility of reset before copying query results" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga April 30, 2019, 12:43 p.m.
Specifically, vkCmdCopyQueryPoolResults is required to see the effect
of a previous vkCmdResetQueryPool. This may not work currently when
query execution is still on going, as some of the queries may become
available asynchronously after the reset.

Fixes new CTS tests:
dEQP-VK.query_pool.statistics_query.reset_before_copy.*
---

Jason, do you have any better ideas?

 src/intel/vulkan/genX_query.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/intel/vulkan/genX_query.c b/src/intel/vulkan/genX_query.c
index 146435c3f8f..08b013f6351 100644
--- a/src/intel/vulkan/genX_query.c
+++ b/src/intel/vulkan/genX_query.c
@@ -383,6 +383,19 @@  void genX(CmdResetQueryPool)(
    ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
    ANV_FROM_HANDLE(anv_query_pool, pool, queryPool);
 
+   /* From the Vulkan spec:
+    *
+    *    "vkCmdCopyQueryPoolResults is guaranteed to see the effect of
+    *     previous uses of vkCmdResetQueryPool in the same queue, without
+    *     any additional synchronization. Thus, the results will always
+    *     reflect the most recent use of the query."
+    *
+    * So we need to make sure that any on-going queries are finished by
+    * the time we emit the reset.
+    */
+   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_CS_STALL_BIT;
+   genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
+
    for (uint32_t i = 0; i < queryCount; i++) {
       anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), sdm) {
          sdm.Address = anv_query_address(pool, firstQuery + i);

Comments

Let me check the new tests and see if where the problem is.
Thanks for letting us know!

-Lionel

On 30/04/2019 13:43, Iago Toral Quiroga wrote:
> Specifically, vkCmdCopyQueryPoolResults is required to see the effect
> of a previous vkCmdResetQueryPool. This may not work currently when
> query execution is still on going, as some of the queries may become
> available asynchronously after the reset.
>
> Fixes new CTS tests:
> dEQP-VK.query_pool.statistics_query.reset_before_copy.*
> ---
>
> Jason, do you have any better ideas?
>
>   src/intel/vulkan/genX_query.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/src/intel/vulkan/genX_query.c b/src/intel/vulkan/genX_query.c
> index 146435c3f8f..08b013f6351 100644
> --- a/src/intel/vulkan/genX_query.c
> +++ b/src/intel/vulkan/genX_query.c
> @@ -383,6 +383,19 @@ void genX(CmdResetQueryPool)(
>      ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
>      ANV_FROM_HANDLE(anv_query_pool, pool, queryPool);
>   
> +   /* From the Vulkan spec:
> +    *
> +    *    "vkCmdCopyQueryPoolResults is guaranteed to see the effect of
> +    *     previous uses of vkCmdResetQueryPool in the same queue, without
> +    *     any additional synchronization. Thus, the results will always
> +    *     reflect the most recent use of the query."
> +    *
> +    * So we need to make sure that any on-going queries are finished by
> +    * the time we emit the reset.
> +    */
> +   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_CS_STALL_BIT;
> +   genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
> +
>      for (uint32_t i = 0; i < queryCount; i++) {
>         anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), sdm) {
>            sdm.Address = anv_query_address(pool, firstQuery + i);
Experimenting a bit with the visibility of PIPE_CONTROL writes & MI_* 
commands I realized those are not coherent.
Essentially anything written with a PIPE_CONTROL will be visible to MI_* 
commands only after some time.
In my experiment I had to insert 2 MI instructions after the 
PIPE_CONTROL write to actually have the following MI_COPY_MEM_MEM 
command pick up what was written by the pipe control.

So my recommendation would be to always write the availability with the 
same part of the command streamer (PIPE_CONTROL).
So that the sequences of writes to the same location are not subject to 
complicated synchronization rules.

Essentially just make emit_query_availability() take an additional 
boolean and then use it in CmdResetQueryPool().

-Lionel

On 30/04/2019 18:18, Lionel Landwerlin wrote:
> Let me check the new tests and see if where the problem is.
> Thanks for letting us know!
>
> -Lionel
>
> On 30/04/2019 13:43, Iago Toral Quiroga wrote:
>> Specifically, vkCmdCopyQueryPoolResults is required to see the effect
>> of a previous vkCmdResetQueryPool. This may not work currently when
>> query execution is still on going, as some of the queries may become
>> available asynchronously after the reset.
>>
>> Fixes new CTS tests:
>> dEQP-VK.query_pool.statistics_query.reset_before_copy.*
>> ---
>>
>> Jason, do you have any better ideas?
>>
>>   src/intel/vulkan/genX_query.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/src/intel/vulkan/genX_query.c 
>> b/src/intel/vulkan/genX_query.c
>> index 146435c3f8f..08b013f6351 100644
>> --- a/src/intel/vulkan/genX_query.c
>> +++ b/src/intel/vulkan/genX_query.c
>> @@ -383,6 +383,19 @@ void genX(CmdResetQueryPool)(
>>      ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
>>      ANV_FROM_HANDLE(anv_query_pool, pool, queryPool);
>>   +   /* From the Vulkan spec:
>> +    *
>> +    *    "vkCmdCopyQueryPoolResults is guaranteed to see the effect of
>> +    *     previous uses of vkCmdResetQueryPool in the same queue, 
>> without
>> +    *     any additional synchronization. Thus, the results will always
>> +    *     reflect the most recent use of the query."
>> +    *
>> +    * So we need to make sure that any on-going queries are finished by
>> +    * the time we emit the reset.
>> +    */
>> +   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_CS_STALL_BIT;
>> +   genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
>> +
>>      for (uint32_t i = 0; i < queryCount; i++) {
>>         anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), 
>> sdm) {
>>            sdm.Address = anv_query_address(pool, firstQuery + i);
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
I've uploaded this MR : 
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/776

Which actually highlights a bigger issue with our queries.

-Lionel

On 01/05/2019 11:58, Lionel Landwerlin wrote:
> Experimenting a bit with the visibility of PIPE_CONTROL writes & MI_* 
> commands I realized those are not coherent.
> Essentially anything written with a PIPE_CONTROL will be visible to 
> MI_* commands only after some time.
> In my experiment I had to insert 2 MI instructions after the 
> PIPE_CONTROL write to actually have the following MI_COPY_MEM_MEM 
> command pick up what was written by the pipe control.
>
> So my recommendation would be to always write the availability with 
> the same part of the command streamer (PIPE_CONTROL).
> So that the sequences of writes to the same location are not subject 
> to complicated synchronization rules.
>
> Essentially just make emit_query_availability() take an additional 
> boolean and then use it in CmdResetQueryPool().
>
> -Lionel
>
> On 30/04/2019 18:18, Lionel Landwerlin wrote:
>> Let me check the new tests and see if where the problem is.
>> Thanks for letting us know!
>>
>> -Lionel
>>
>> On 30/04/2019 13:43, Iago Toral Quiroga wrote:
>>> Specifically, vkCmdCopyQueryPoolResults is required to see the effect
>>> of a previous vkCmdResetQueryPool. This may not work currently when
>>> query execution is still on going, as some of the queries may become
>>> available asynchronously after the reset.
>>>
>>> Fixes new CTS tests:
>>> dEQP-VK.query_pool.statistics_query.reset_before_copy.*
>>> ---
>>>
>>> Jason, do you have any better ideas?
>>>
>>>   src/intel/vulkan/genX_query.c | 13 +++++++++++++
>>>   1 file changed, 13 insertions(+)
>>>
>>> diff --git a/src/intel/vulkan/genX_query.c 
>>> b/src/intel/vulkan/genX_query.c
>>> index 146435c3f8f..08b013f6351 100644
>>> --- a/src/intel/vulkan/genX_query.c
>>> +++ b/src/intel/vulkan/genX_query.c
>>> @@ -383,6 +383,19 @@ void genX(CmdResetQueryPool)(
>>>      ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
>>>      ANV_FROM_HANDLE(anv_query_pool, pool, queryPool);
>>>   +   /* From the Vulkan spec:
>>> +    *
>>> +    *    "vkCmdCopyQueryPoolResults is guaranteed to see the effect of
>>> +    *     previous uses of vkCmdResetQueryPool in the same queue, 
>>> without
>>> +    *     any additional synchronization. Thus, the results will 
>>> always
>>> +    *     reflect the most recent use of the query."
>>> +    *
>>> +    * So we need to make sure that any on-going queries are 
>>> finished by
>>> +    * the time we emit the reset.
>>> +    */
>>> +   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_CS_STALL_BIT;
>>> +   genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
>>> +
>>>      for (uint32_t i = 0; i < queryCount; i++) {
>>>         anv_batch_emit(&cmd_buffer->batch, GENX(MI_STORE_DATA_IMM), 
>>> sdm) {
>>>            sdm.Address = anv_query_address(pool, firstQuery + i);
>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev