Re: [Mesa-dev] [PATCH] anv/query: ensure visibility of reset before copying query results

2019-05-01 Thread Lionel Landwerlin 
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



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] anv/query: ensure visibility of reset before copying query results

2019-05-01 Thread Lionel Landwerlin 
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

Re: [Mesa-dev] [PATCH] anv/query: ensure visibility of reset before copying query results

2019-04-30 Thread Lionel Landwerlin 

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] [PATCH] anv/query: ensure visibility of reset before copying query results

2019-04-30 Thread Iago Toral Quiroga 
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);
-- 
2.17.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev