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

[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 <samuel.pitoiset at gmail.com>
>>
> 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 <siglesias at igalia.com>
>>> ---
>>>    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);