[Mesa-dev] [PATCH 1/2] radv: don't overwrite results in VkGetQueryPoolResults() when queries are not available

[Samuel Pitoiset]

On 3/22/19 5:18 PM, Samuel Pitoiset wrote:
>
> On 3/22/19 1:03 PM, Samuel Iglesias Gonsálvez wrote:
>> If the query is not available and VK_QUERY_RESULT_WAIT_BIT and
>> VK_QUERY_RESULT_PARTIAL_BIT are both not set, the spec doesn't
>> allow to modify its result.
>>
>>  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 | 52 +++++++++++++++++++++++--------------
>>   1 file changed, 33 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/amd/vulkan/radv_query.c b/src/amd/vulkan/radv_query.c
>> index e808aa65170..8578680f09d 100644
>> --- a/src/amd/vulkan/radv_query.c
>> +++ b/src/amd/vulkan/radv_query.c
>> @@ -1148,10 +1148,12 @@ VkResult radv_GetQueryPoolResults(
>>               }
>>                 if (flags & VK_QUERY_RESULT_64_BIT) {
>> -                *(uint64_t*)dest = *(uint64_t*)src;
>> +                if (available || (flags & VK_QUERY_RESULT_PARTIAL_BIT))
>> +                    *(uint64_t*)dest = *(uint64_t*)src;
>>                   dest += 8;
>>               } else {
>> -                *(uint32_t*)dest = *(uint32_t*)src;
>> +                if (available || (flags & VK_QUERY_RESULT_PARTIAL_BIT))
>> +                    *(uint32_t*)dest = *(uint32_t*)src;
>>                   dest += 4;
>>               }
>>               break;
>> @@ -1183,10 +1185,12 @@ VkResult radv_GetQueryPoolResults(
>>               }
>>                 if (flags & VK_QUERY_RESULT_64_BIT) {
>> -                *(uint64_t*)dest = sample_count;
>> +                if (available || (flags & VK_QUERY_RESULT_PARTIAL_BIT))
>> +                    *(uint64_t*)dest = sample_count;
>>                   dest += 8;
>>               } else {
>> -                *(uint32_t*)dest = sample_count;
>> +                if (available || (flags & VK_QUERY_RESULT_PARTIAL_BIT))
>> +                    *(uint32_t*)dest = sample_count;
>>                   dest += 4;
>>               }
>>               break;
>> @@ -1203,18 +1207,26 @@ VkResult radv_GetQueryPoolResults(
>>               if (flags & VK_QUERY_RESULT_64_BIT) {
>>                   uint64_t *dst = (uint64_t*)dest;
>>                   dest += util_bitcount(pool->pipeline_stats_mask) * 8;
>> -                for(int i = 0; i < 11; ++i)
>> -                    if(pool->pipeline_stats_mask & (1u << i))
>> -                        *dst++ = stop[pipeline_statistics_indices[i]] -
>> - start[pipeline_statistics_indices[i]];
>> +                for(int i = 0; i < 11; ++i) {
>> +                    if(pool->pipeline_stats_mask & (1u << i)) {
>> +                        if (available || (flags & 
>> VK_QUERY_RESULT_PARTIAL_BIT))
>> +                            *dst = 
>> stop[pipeline_statistics_indices[i]] -
>> + start[pipeline_statistics_indices[i]];
>> +                        dst++;
>> +                    }
>> +                }
>>                 } else {
>>                   uint32_t *dst = (uint32_t*)dest;
>>                   dest += util_bitcount(pool->pipeline_stats_mask) * 4;
>> -                for(int i = 0; i < 11; ++i)
>> -                    if(pool->pipeline_stats_mask & (1u << i))
>> -                        *dst++ = stop[pipeline_statistics_indices[i]] -
>> - start[pipeline_statistics_indices[i]];
>> +                for(int i = 0; i < 11; ++i) {
>> +                    if(pool->pipeline_stats_mask & (1u << i)) {
>> +                        if (available || (flags & 
>> VK_QUERY_RESULT_PARTIAL_BIT))
>> +                            *dst = 
>> stop[pipeline_statistics_indices[i]] -
>> + start[pipeline_statistics_indices[i]];
>> +                        dst++;
>> +                    }
>> +                }
>>               }
>>               break;
>>           }
>> @@ -1235,23 +1247,25 @@ VkResult radv_GetQueryPoolResults(
>>                       available = 0;
>>               }
>>   -            if (!available && !(flags & 
>> VK_QUERY_RESULT_PARTIAL_BIT)) {
>> +            if (!available && !(flags & VK_QUERY_RESULT_PARTIAL_BIT))
>>                   result = VK_NOT_READY;
>> -                break;
>> -            }
>
> Removing that break is useless?
After reading patch 2, I understand. This change should be moved to next 
patch.
>
>>                 num_primitives_written = src64[3] - src64[1];
>>               primitive_storage_needed = src64[2] - src64[0];
>>                 if (flags & VK_QUERY_RESULT_64_BIT) {
>> -                *(uint64_t *)dest = num_primitives_written;
>> +                if (available || (flags & VK_QUERY_RESULT_PARTIAL_BIT))
>> +                    *(uint64_t *)dest = num_primitives_written;
>>                   dest += 8;
>> -                *(uint64_t *)dest = primitive_storage_needed;
>> +                if (available || (flags & VK_QUERY_RESULT_PARTIAL_BIT))
>> +                    *(uint64_t *)dest = primitive_storage_needed;
>>                   dest += 8;
>>               } else {
>> -                *(uint32_t *)dest = num_primitives_written;
>> +                if (available || (flags & VK_QUERY_RESULT_PARTIAL_BIT))
>> +                    *(uint32_t *)dest = num_primitives_written;
>>                   dest += 4;
>> -                *(uint32_t *)dest = primitive_storage_needed;
>> +                if (available || (flags & VK_QUERY_RESULT_PARTIAL_BIT))
>> +                    *(uint32_t *)dest = primitive_storage_needed;
>>                   dest += 4;
>>               }
>>               break;