Re: [Mesa-dev] [PATCH 2/2] radv: write availability status vkGetQueryPoolResults() when the data is not available

2019-03-29 Thread Samuel Iglesias Gonsálvez 
On Thu, 2019-03-28 at 16:21 +0100, Samuel Pitoiset wrote:
> On 3/25/19 8:16 AM, Samuel Iglesias Gonsálvez wrote:
> > On Fri, 2019-03-22 at 17:21 +0100, Samuel Pitoiset wrote:
> > > Does this fix anything known?
> > > 
> > I am writing CTS tests for VK_EXT_host_query_reset extension and I
> > found this bug while testing them on RADV.
> > 
> > > Does that rule also apply for CmdCopyQueryPoolResults()? If so,
> > > we
> > > might
> > > need to fix it (I haven't looked yet).
> > > 
> > The rule is slightly different on CmdCopyQueryPoolResults():
> > 
> > "Similarly, if VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is set and
> > VK_QUERY_RESULT_WAIT_BIT is not set, the availability is guaranteed
> > to
> > reflect the most recent use of the query on the same queue,
> > assuming
> > that the query is not being simultaneously used by other queues. As
> > with vkGetQueryPoolResults, implementations must guarantee that if
> > they
> > return a non-zero availability value, then the numerical results
> > are
> > valid."
> > 
> > So if VK_QUERY_RESULT_WITH_AVAILABILITY_BIT we need to still set
> > the
> > availability state.
> > 
> > I skimmed the implementation of this function on RADV, it seems it
> > is
> > missing setting the availability value for all the queries except
> > for
> > VK_QUERY_TYPE_TIMESTAMP.
> > 
> > Could you take care of this?
> Yes, do you have tests for CmdCopyQueryPoolResults()?

There are no tests for this case on CTS. I modified manually one test
and it seems I was wrong: RADV is setting the availability value. So
you don't need to do anything for the moment.

I'm going to add in my TODO list to add tests for this in CTS when time
allows, as it is not fully covered right now.

Thanks,

Sam

> > > With the comment on patch 1, series is:
> > > 
> > > Reviewed-by: Samuel Pitoiset 
> > > 
> > OK, thanks! It seems I did a wrong squash of patches on patch 1. I
> > will
> > fix it and push both patches to master.
> > 
> > Sam
> > 
> > > On 3/22/19 1:03 PM, Samuel Iglesias Gonsálvez wrote:
> > > > If VK_QUERY_RESULT_WITH_AVAILABILY_BIT is set and
> > > > VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT are
> > > > both
> > > > not
> > > > set, we need return to VK_NOT_READY only and set the
> > > > availability
> > > > status field for each query.
> > > > 
> > > >   From Vulkan spec:
> > > > 
> > > > "If VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT
> > > > are
> > > > both
> > > > not set then no result values are written to pData for queries
> > > > that
> > > > are
> > > > in the unavailable state at the time of the call, and
> > > > vkGetQueryPoolResults returns VK_NOT_READY. However,
> > > > availability
> > > > state
> > > > is still written to pData for those queries if
> > > > VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is set."
> > > > 
> > > > Signed-off-by: Samuel Iglesias Gonsálvez 
> > > > ---
> > > >src/amd/vulkan/radv_query.c | 15 +++
> > > >1 file changed, 3 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/src/amd/vulkan/radv_query.c
> > > > b/src/amd/vulkan/radv_query.c
> > > > index 8578680f09d..63a2ab773a8 100644
> > > > --- a/src/amd/vulkan/radv_query.c
> > > > +++ b/src/amd/vulkan/radv_query.c
> > > > @@ -1141,11 +1141,8 @@ VkResult radv_GetQueryPoolResults(
> > > > available = *(uint64_t *)src !=
> > > > TIMESTAMP_NOT_READY;
> > > > }
> > > >
> > > > -   if (!available && !(flags &
> > > > VK_QUERY_RESULT_PARTIAL_BIT)) {
> > > > +   if (!available && !(flags &
> > > > VK_QUERY_RESULT_PARTIAL_BIT))
> > > > result = VK_NOT_READY;
> > > > -   break;
> > > > -
> > > > -   }
> > > >
> > > > if (flags & VK_QUERY_RESULT_64_BIT) {
> > > > if (available || (flags &
> > > > VK_QUERY_RESULT_PARTIAL_BIT))
> > > > @@ -1178,11 +1175,8 @@ VkResult radv_GetQueryPoolResults(
> > > > }
> > > > }
> > > >
> > > > -   if (!available && !(flags &
> > > > VK_QUERY_RESULT_PARTIAL_BIT)) {
> > > > +   if (!available && !(flags &
> > > > VK_QUERY_RESULT_PARTIAL_BIT))
> > > > result = VK_NOT_READY;
> > > > -   break;
> > > > -
> > > > -   }
> > > >
> > > > if (flags & VK_QUERY_RESULT_64_BIT) {
> > > > if (available || (flags &
> > > > VK_QUERY_RESULT_PARTIAL_BIT))
> > > > @@ -1196,11 +1190,8 @@ VkResult radv_GetQueryPoolResults(
> > > > break;
> > > > }
> > > > case VK_QUERY_TYPE_PIPELINE_STATISTICS: {
> > > > -   if (!available && !(flags &
> > > > VK_QUERY_RESULT_PARTIAL_BIT)) {
> > > > +   if

Re: [Mesa-dev] [PATCH 2/2] radv: write availability status vkGetQueryPoolResults() when the data is not available

2019-03-28 Thread Samuel Pitoiset 


On 3/25/19 8:16 AM, Samuel Iglesias Gonsálvez wrote:

On Fri, 2019-03-22 at 17:21 +0100, Samuel Pitoiset wrote:

Does this fix anything known?


I am writing CTS tests for VK_EXT_host_query_reset extension and I
found this bug while testing them on RADV.


Does that rule also apply for CmdCopyQueryPoolResults()? If so, we
might
need to fix it (I haven't looked yet).


The rule is slightly different on CmdCopyQueryPoolResults():

"Similarly, if VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is set and
VK_QUERY_RESULT_WAIT_BIT is not set, the availability is guaranteed to
reflect the most recent use of the query on the same queue, assuming
that the query is not being simultaneously used by other queues. As
with vkGetQueryPoolResults, implementations must guarantee that if they
return a non-zero availability value, then the numerical results are
valid."

So if VK_QUERY_RESULT_WITH_AVAILABILITY_BIT we need to still set the
availability state.

I skimmed the implementation of this function on RADV, it seems it is
missing setting the availability value for all the queries except for
VK_QUERY_TYPE_TIMESTAMP.

Could you take care of this?

Yes, do you have tests for CmdCopyQueryPoolResults()?



With the comment on patch 1, series is:

Reviewed-by: Samuel Pitoiset 


OK, thanks! It seems I did a wrong squash of patches on patch 1. I will
fix it and push both patches to master.

Sam


On 3/22/19 1:03 PM, Samuel Iglesias Gonsálvez wrote:

If VK_QUERY_RESULT_WITH_AVAILABILY_BIT is set and
VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT are both
not
set, we need return to VK_NOT_READY only and set the availability
status field for each query.

  From Vulkan spec:

"If VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT are
both
not set then no result values are written to pData for queries that
are
in the unavailable state at the time of the call, and
vkGetQueryPoolResults returns VK_NOT_READY. However, availability
state
is still written to pData for those queries if
VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is set."

Signed-off-by: Samuel Iglesias Gonsálvez 
---
   src/amd/vulkan/radv_query.c | 15 +++
   1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/src/amd/vulkan/radv_query.c
b/src/amd/vulkan/radv_query.c
index 8578680f09d..63a2ab773a8 100644
--- a/src/amd/vulkan/radv_query.c
+++ b/src/amd/vulkan/radv_query.c
@@ -1141,11 +1141,8 @@ VkResult radv_GetQueryPoolResults(
available = *(uint64_t *)src !=
TIMESTAMP_NOT_READY;
}
   
-			if (!available && !(flags &

VK_QUERY_RESULT_PARTIAL_BIT)) {
+   if (!available && !(flags &
VK_QUERY_RESULT_PARTIAL_BIT))
result = VK_NOT_READY;
-   break;
-
-   }
   
   			if (flags & VK_QUERY_RESULT_64_BIT) {

if (available || (flags &
VK_QUERY_RESULT_PARTIAL_BIT))
@@ -1178,11 +1175,8 @@ VkResult radv_GetQueryPoolResults(
}
}
   
-			if (!available && !(flags &

VK_QUERY_RESULT_PARTIAL_BIT)) {
+   if (!available && !(flags &
VK_QUERY_RESULT_PARTIAL_BIT))
result = VK_NOT_READY;
-   break;
-
-   }
   
   			if (flags & VK_QUERY_RESULT_64_BIT) {

if (available || (flags &
VK_QUERY_RESULT_PARTIAL_BIT))
@@ -1196,11 +1190,8 @@ VkResult radv_GetQueryPoolResults(
break;
}
case VK_QUERY_TYPE_PIPELINE_STATISTICS: {
-   if (!available && !(flags &
VK_QUERY_RESULT_PARTIAL_BIT)) {
+   if (!available && !(flags &
VK_QUERY_RESULT_PARTIAL_BIT))
result = VK_NOT_READY;
-   break;
-
-   }
   
   			const uint64_t *start = (uint64_t*)src;

const uint64_t *stop = (uint64_t*)(src +
pipelinestat_block_size);

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

Re: [Mesa-dev] [PATCH 2/2] radv: write availability status vkGetQueryPoolResults() when the data is not available

2019-03-25 Thread Samuel Iglesias Gonsálvez 
On Fri, 2019-03-22 at 17:21 +0100, Samuel Pitoiset wrote:
> Does this fix anything known?
> 

I am writing CTS tests for VK_EXT_host_query_reset extension and I
found this bug while testing them on RADV.

> Does that rule also apply for CmdCopyQueryPoolResults()? If so, we
> might 
> need to fix it (I haven't looked yet).
> 

The rule is slightly different on CmdCopyQueryPoolResults():

"Similarly, if VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is set and
VK_QUERY_RESULT_WAIT_BIT is not set, the availability is guaranteed to
reflect the most recent use of the query on the same queue, assuming
that the query is not being simultaneously used by other queues. As
with vkGetQueryPoolResults, implementations must guarantee that if they
return a non-zero availability value, then the numerical results are
valid."

So if VK_QUERY_RESULT_WITH_AVAILABILITY_BIT we need to still set the
availability state.

I skimmed the implementation of this function on RADV, it seems it is
missing setting the availability value for all the queries except for
VK_QUERY_TYPE_TIMESTAMP.

Could you take care of this?

> With the comment on patch 1, series is:
> 
> Reviewed-by: Samuel Pitoiset 
> 

OK, thanks! It seems I did a wrong squash of patches on patch 1. I will
fix it and push both patches to master.

Sam

> On 3/22/19 1:03 PM, Samuel Iglesias Gonsálvez wrote:
> > If VK_QUERY_RESULT_WITH_AVAILABILY_BIT is set and
> > VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT are both
> > not
> > set, we need return to VK_NOT_READY only and set the availability
> > status field for each query.
> > 
> >  From Vulkan spec:
> > 
> > "If VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT are
> > both
> > not set then no result values are written to pData for queries that
> > are
> > in the unavailable state at the time of the call, and
> > vkGetQueryPoolResults returns VK_NOT_READY. However, availability
> > state
> > is still written to pData for those queries if
> > VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is set."
> > 
> > Signed-off-by: Samuel Iglesias Gonsálvez 
> > ---
> >   src/amd/vulkan/radv_query.c | 15 +++
> >   1 file changed, 3 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/amd/vulkan/radv_query.c
> > b/src/amd/vulkan/radv_query.c
> > index 8578680f09d..63a2ab773a8 100644
> > --- a/src/amd/vulkan/radv_query.c
> > +++ b/src/amd/vulkan/radv_query.c
> > @@ -1141,11 +1141,8 @@ VkResult radv_GetQueryPoolResults(
> > available = *(uint64_t *)src !=
> > TIMESTAMP_NOT_READY;
> > }
> >   
> > -   if (!available && !(flags &
> > VK_QUERY_RESULT_PARTIAL_BIT)) {
> > +   if (!available && !(flags &
> > VK_QUERY_RESULT_PARTIAL_BIT))
> > result = VK_NOT_READY;
> > -   break;
> > -
> > -   }
> >   
> > if (flags & VK_QUERY_RESULT_64_BIT) {
> > if (available || (flags &
> > VK_QUERY_RESULT_PARTIAL_BIT))
> > @@ -1178,11 +1175,8 @@ VkResult radv_GetQueryPoolResults(
> > }
> > }
> >   
> > -   if (!available && !(flags &
> > VK_QUERY_RESULT_PARTIAL_BIT)) {
> > +   if (!available && !(flags &
> > VK_QUERY_RESULT_PARTIAL_BIT))
> > result = VK_NOT_READY;
> > -   break;
> > -
> > -   }
> >   
> > if (flags & VK_QUERY_RESULT_64_BIT) {
> > if (available || (flags &
> > VK_QUERY_RESULT_PARTIAL_BIT))
> > @@ -1196,11 +1190,8 @@ VkResult radv_GetQueryPoolResults(
> > break;
> > }
> > case VK_QUERY_TYPE_PIPELINE_STATISTICS: {
> > -   if (!available && !(flags &
> > VK_QUERY_RESULT_PARTIAL_BIT)) {
> > +   if (!available && !(flags &
> > VK_QUERY_RESULT_PARTIAL_BIT))
> > result = VK_NOT_READY;
> > -   break;
> > -
> > -   }
> >   
> > const uint64_t *start = (uint64_t*)src;
> > const uint64_t *stop = (uint64_t*)(src +
> > pipelinestat_block_size);


signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] radv: write availability status vkGetQueryPoolResults() when the data is not available

2019-03-22 Thread Samuel Pitoiset 

Does this fix anything known?

Does that rule also apply for CmdCopyQueryPoolResults()? If so, we might 
need to fix it (I haven't looked yet).


With the comment on patch 1, series is:

Reviewed-by: Samuel Pitoiset 

On 3/22/19 1:03 PM, Samuel Iglesias Gonsálvez wrote:

If VK_QUERY_RESULT_WITH_AVAILABILY_BIT is set and
VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT are both not
set, we need return to VK_NOT_READY only and set the availability
status field for each query.

 From Vulkan spec:

"If VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT are both
not set then no result values are written to pData for queries that are
in the unavailable state at the time of the call, and
vkGetQueryPoolResults returns VK_NOT_READY. However, availability state
is still written to pData for those queries if
VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is set."

Signed-off-by: Samuel Iglesias Gonsálvez 
---
  src/amd/vulkan/radv_query.c | 15 +++
  1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/src/amd/vulkan/radv_query.c b/src/amd/vulkan/radv_query.c
index 8578680f09d..63a2ab773a8 100644
--- a/src/amd/vulkan/radv_query.c
+++ b/src/amd/vulkan/radv_query.c
@@ -1141,11 +1141,8 @@ VkResult radv_GetQueryPoolResults(
available = *(uint64_t *)src != 
TIMESTAMP_NOT_READY;
}
  
-			if (!available && !(flags & VK_QUERY_RESULT_PARTIAL_BIT)) {

+   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
result = VK_NOT_READY;
-   break;
-
-   }
  
  			if (flags & VK_QUERY_RESULT_64_BIT) {

if (available || (flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
@@ -1178,11 +1175,8 @@ VkResult radv_GetQueryPoolResults(
}
}
  
-			if (!available && !(flags & VK_QUERY_RESULT_PARTIAL_BIT)) {

+   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
result = VK_NOT_READY;
-   break;
-
-   }
  
  			if (flags & VK_QUERY_RESULT_64_BIT) {

if (available || (flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
@@ -1196,11 +1190,8 @@ VkResult radv_GetQueryPoolResults(
break;
}
case VK_QUERY_TYPE_PIPELINE_STATISTICS: {
-   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT)) {
+   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
result = VK_NOT_READY;
-   break;
-
-   }
  
  			const uint64_t *start = (uint64_t*)src;

const uint64_t *stop = (uint64_t*)(src + 
pipelinestat_block_size);

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

[Mesa-dev] [PATCH 2/2] radv: write availability status vkGetQueryPoolResults() when the data is not available

2019-03-22 Thread Samuel Iglesias Gonsálvez 
If VK_QUERY_RESULT_WITH_AVAILABILY_BIT is set and
VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT are both not
set, we need return to VK_NOT_READY only and set the availability
status field for each query.

From Vulkan spec:

"If VK_QUERY_RESULT_WAIT_BIT and VK_QUERY_RESULT_PARTIAL_BIT are both
not set then no result values are written to pData for queries that are
in the unavailable state at the time of the call, and
vkGetQueryPoolResults returns VK_NOT_READY. However, availability state
is still written to pData for those queries if
VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is set."

Signed-off-by: Samuel Iglesias Gonsálvez 
---
 src/amd/vulkan/radv_query.c | 15 +++
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/src/amd/vulkan/radv_query.c b/src/amd/vulkan/radv_query.c
index 8578680f09d..63a2ab773a8 100644
--- a/src/amd/vulkan/radv_query.c
+++ b/src/amd/vulkan/radv_query.c
@@ -1141,11 +1141,8 @@ VkResult radv_GetQueryPoolResults(
available = *(uint64_t *)src != 
TIMESTAMP_NOT_READY;
}
 
-   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT)) {
+   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
result = VK_NOT_READY;
-   break;
-
-   }
 
if (flags & VK_QUERY_RESULT_64_BIT) {
if (available || (flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
@@ -1178,11 +1175,8 @@ VkResult radv_GetQueryPoolResults(
}
}
 
-   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT)) {
+   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
result = VK_NOT_READY;
-   break;
-
-   }
 
if (flags & VK_QUERY_RESULT_64_BIT) {
if (available || (flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
@@ -1196,11 +1190,8 @@ VkResult radv_GetQueryPoolResults(
break;
}
case VK_QUERY_TYPE_PIPELINE_STATISTICS: {
-   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT)) {
+   if (!available && !(flags & 
VK_QUERY_RESULT_PARTIAL_BIT))
result = VK_NOT_READY;
-   break;
-
-   }
 
const uint64_t *start = (uint64_t*)src;
const uint64_t *stop = (uint64_t*)(src + 
pipelinestat_block_size);
-- 
2.20.1

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