From aed656cd7a5714cc09723476308b5d1dc9c636bc Mon Sep 17 00:00:00 2001 From: Hao Li Date: Thu, 22 Apr 2021 10:10:12 +0000 Subject: [PATCH] Clear resolve buffer to 0 for resolving unavailable queries - Add vkCmdFillBuffer in ResolveQuerySet to clear the buffer to 0s for these unavailable queries if the buffer has been initialized or fully used which won't been initialized with 0s again. - Because vkCmdFillBuffer has driver issue on Intel Windows, Skip some affected cases. - Remove unsafe api checking from Occlusion Query. Bug: dawn:434 Change-Id: Ib34f81d93b0de8f08f0eeebf3c8a967eeb5ecefb Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/48320 Reviewed-by: Corentin Wallez Reviewed-by: Austin Eng Commit-Queue: Corentin Wallez --- src/dawn_native/CommandEncoder.cpp | 8 ----- src/dawn_native/vulkan/CommandBufferVk.cpp | 28 ++++++++++++++---- src/tests/end2end/QueryTests.cpp | 16 +++++----- .../validation/UnsafeAPIValidationTests.cpp | 29 ------------------- 4 files changed, 31 insertions(+), 50 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index e460eab10b..921d5f4176 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -370,14 +370,6 @@ namespace dawn_native { if (descriptor->occlusionQuerySet != nullptr) { DAWN_TRY(device->ValidateObject(descriptor->occlusionQuerySet)); - // Occlusion query has not been implemented completely. Disallow it as unsafe until - // the implementaion is completed. - if (device->IsToggleEnabled(Toggle::DisallowUnsafeAPIs)) { - return DAWN_VALIDATION_ERROR( - "Occlusion query is disallowed because it has not been implemented " - "completely."); - } - if (descriptor->occlusionQuerySet->GetQueryType() != wgpu::QueryType::Occlusion) { return DAWN_VALIDATION_ERROR("The type of query set must be Occlusion"); } diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 16d81005b0..d154026a21 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -800,13 +800,29 @@ namespace dawn_native { namespace vulkan { QuerySet* querySet = ToBackend(cmd->querySet.Get()); Buffer* destination = ToBackend(cmd->destination.Get()); - // TODO(hao.x.li@intel.com): Clear the resolve region of the buffer to 0 if at - // least one query is unavailable for the resolving and the resolve buffer has - // been initialized or fully used. + // vkCmdCopyQueryPoolResults only can retrieve available queries because + // VK_QUERY_RESULT_WAIT_BIT is set, for these unavailable queries, we need to + // clear the resolving region of the buffer to 0s if the buffer has been + // initialized or fully used. + auto startIt = querySet->GetQueryAvailability().begin() + cmd->firstQuery; + auto endIt = querySet->GetQueryAvailability().begin() + cmd->firstQuery + + cmd->queryCount; + bool hasUnavailableQueries = std::find(startIt, endIt, false) != endIt; + if (hasUnavailableQueries && + (destination->IsDataInitialized() || + destination->IsFullBufferRange(cmd->destinationOffset, + cmd->queryCount * sizeof(uint64_t)))) { + destination->TransitionUsageNow(recordingContext, + wgpu::BufferUsage::CopyDst); + device->fn.CmdFillBuffer(commands, destination->GetHandle(), + cmd->destinationOffset, + cmd->queryCount * sizeof(uint64_t), 0u); + } else { + destination->EnsureDataInitializedAsDestination( + recordingContext, cmd->destinationOffset, + cmd->queryCount * sizeof(uint64_t)); + } - destination->EnsureDataInitializedAsDestination( - recordingContext, cmd->destinationOffset, - cmd->queryCount * sizeof(uint64_t)); destination->TransitionUsageNow(recordingContext, wgpu::BufferUsage::QueryResolve); diff --git a/src/tests/end2end/QueryTests.cpp b/src/tests/end2end/QueryTests.cpp index c09a921bd6..861a3f1852 100644 --- a/src/tests/end2end/QueryTests.cpp +++ b/src/tests/end2end/QueryTests.cpp @@ -302,9 +302,10 @@ TEST_P(OcclusionQueryTests, Rewrite) { // Test resolving occlusion query correctly if the queries are written sparsely, which also tests // the query resetting at the start of render passes on Vulkan backend. TEST_P(OcclusionQueryTests, ResolveSparseQueries) { - // TODO(hao.x.li@intel.com): Clear the resolve region of the buffer to 0 if there is at least - // one query not written and the resolve buffer has been initialized or fully used. - DAWN_SKIP_TEST_IF(IsVulkan()); + // TODO(hao.x.li@intel.com): Fails on Intel Windows Vulkan due to a driver issue that + // vkCmdFillBuffer and vkCmdCopyQueryPoolResults are not executed in order, skip it util + // the issue is fixed. + DAWN_SKIP_TEST_IF(IsWindows() && IsVulkan() && IsIntel()); // TODO(hao.x.li@intel.com): Investigate why it's failed on D3D12 on Nvidia when running with // the previous occlusion tests. Expect resolve to 0 for these unwritten queries but the @@ -366,10 +367,6 @@ TEST_P(OcclusionQueryTests, ResolveSparseQueries) { // Test resolving occlusion query to 0 if all queries are not written TEST_P(OcclusionQueryTests, ResolveWithoutWritten) { - // TODO(hao.x.li@intel.com): Clear the resolve region of the buffer to 0 if there is at least - // one query not written and the resolve buffer has been initialized or fully used. - DAWN_SKIP_TEST_IF(IsVulkan()); - // TODO(hao.x.li@intel.com): Investigate why it's failed on D3D12 on Nvidia when running with // the previous occlusion tests. Expect resolve to 0 but the occlusion result of the previous // tests is got. @@ -707,6 +704,11 @@ TEST_P(TimestampQueryTests, ResolveFromAnotherEncoder) { // Test resolving timestamp query correctly if the queries are written sparsely TEST_P(TimestampQueryTests, ResolveSparseQueries) { + // TODO(hao.x.li@intel.com): Fails on Intel Windows Vulkan due to a driver issue that + // vkCmdFillBuffer and vkCmdCopyQueryPoolResults are not executed in order, skip it util + // the issue is fixed. + DAWN_SKIP_TEST_IF(IsWindows() && IsVulkan() && IsIntel()); + constexpr uint32_t kQueryCount = 4; wgpu::QuerySet querySet = CreateQuerySetForTimestamp(kQueryCount); diff --git a/src/tests/unittests/validation/UnsafeAPIValidationTests.cpp b/src/tests/unittests/validation/UnsafeAPIValidationTests.cpp index 8c6986bfd7..29816c8f87 100644 --- a/src/tests/unittests/validation/UnsafeAPIValidationTests.cpp +++ b/src/tests/unittests/validation/UnsafeAPIValidationTests.cpp @@ -201,32 +201,3 @@ TEST_F(UnsafeAPIValidationTest, DynamicStorageBuffer) { ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&desc)); } } - -// Check that occlusion query is disallowed as part of unsafe APIs. -TEST_F(UnsafeAPIValidationTest, OcclusionQueryDisallowed) { - DummyRenderPass renderPass(device); - - // Control case: BeginRenderPass without occlusionQuerySet is allowed. - { - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); - - pass.EndPass(); - encoder.Finish(); - } - - // Error case: BeginRenderPass with occlusionQuerySet is disallowed. - { - wgpu::QuerySetDescriptor descriptor; - descriptor.type = wgpu::QueryType::Occlusion; - descriptor.count = 1; - wgpu::QuerySet querySet = device.CreateQuerySet(&descriptor); - renderPass.occlusionQuerySet = querySet; - - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); - - pass.EndPass(); - ASSERT_DEVICE_ERROR(encoder.Finish()); - } -}