Add additional buffer usage for indirect usage transition at backends
This patch adds an additional buffer usage for indirect usage transition
at the backends (Vulkan, D3D12) so that when recording the commands in
the command encoder at the front end we can avoid setting this internal
buffer usage in the usage tracker to signal that this buffer won't be
used as an indirect buffer because it needs to be used as a storage
buffer in a compute pass for validation or other purposes instead.
Fixed: chromium:42240167
Test: dawn_end2end_tests
Change-Id: Ia2d3dbda0dda9d42d2248561b3926509f12785b4
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/201817
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Jiawei Shao <jiawei.shao@intel.com>
diff --git a/src/dawn/native/Buffer.h b/src/dawn/native/Buffer.h
index d5aa0b6..7f130e6 100644
--- a/src/dawn/native/Buffer.h
+++ b/src/dawn/native/Buffer.h
@@ -57,7 +57,7 @@
static constexpr wgpu::BufferUsage kReadOnlyBufferUsages =
wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::Index |
wgpu::BufferUsage::Vertex | wgpu::BufferUsage::Uniform | kReadOnlyStorageBuffer |
- wgpu::BufferUsage::Indirect;
+ kIndirectBufferForFrontendValidation | kIndirectBufferForBackendResourceTracking;
static constexpr wgpu::BufferUsage kMappableBufferUsages =
wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite;
diff --git a/src/dawn/native/ComputePassEncoder.cpp b/src/dawn/native/ComputePassEncoder.cpp
index ad81066..fb76d7f 100644
--- a/src/dawn/native/ComputePassEncoder.cpp
+++ b/src/dawn/native/ComputePassEncoder.cpp
@@ -371,12 +371,7 @@
}
SyncScopeUsageTracker scope;
- scope.BufferUsedAs(indirectBuffer, wgpu::BufferUsage::Indirect);
mUsageTracker.AddReferencedBuffer(indirectBuffer);
- // TODO(crbug.com/dawn/1166): If validation is enabled, adding |indirectBuffer|
- // is needed for correct usage validation even though it will only be bound for
- // storage. This will unecessarily transition the |indirectBuffer| in
- // the backend.
Ref<BufferBase> indirectBufferRef = indirectBuffer;
@@ -399,10 +394,18 @@
// If we have created a new scratch dispatch indirect buffer in
// TransformIndirectDispatchBuffer(), we need to track it in mUsageTracker.
if (indirectBufferRef.Get() != indirectBuffer) {
- // |indirectBufferRef| was replaced with a scratch buffer. Add it to the
- // synchronization scope.
- scope.BufferUsedAs(indirectBufferRef.Get(), wgpu::BufferUsage::Indirect);
+ // |indirectBufferRef| was replaced with a scratch buffer, so we just need to track
+ // it for backend resource tracking and not for frontend validation.
+ scope.BufferUsedAs(indirectBufferRef.Get(),
+ kIndirectBufferForBackendResourceTracking);
mUsageTracker.AddReferencedBuffer(indirectBufferRef.Get());
+
+ // Then we can just track indirectBuffer for frontend validation and ignore its
+ // indirect buffer usage in backend resource tracking.
+ scope.BufferUsedAs(indirectBuffer, kIndirectBufferForFrontendValidation);
+ } else {
+ scope.BufferUsedAs(indirectBuffer, wgpu::BufferUsage::Indirect |
+ kIndirectBufferForBackendResourceTracking);
}
AddDispatchSyncScope(std::move(scope));
diff --git a/src/dawn/native/IndirectDrawValidationEncoder.cpp b/src/dawn/native/IndirectDrawValidationEncoder.cpp
index b95cea8..e373d9c 100644
--- a/src/dawn/native/IndirectDrawValidationEncoder.cpp
+++ b/src/dawn/native/IndirectDrawValidationEncoder.cpp
@@ -585,6 +585,9 @@
if ((draw.type == IndirectDrawMetadata::DrawType::NonIndexed ||
!device->IsValidationEnabled()) &&
!draw.duplicateBaseVertexInstance) {
+ // We will use the original indirect buffer directly as the indirect buffer.
+ usageTracker->BufferUsedAs(draw.cmd->indirectBuffer.Get(),
+ kIndirectBufferForBackendResourceTracking);
continue;
}
@@ -625,8 +628,10 @@
DAWN_TRY(outputParamsBuffer.EnsureCapacity(outputParamsSize));
DAWN_TRY(batchDataBuffer.EnsureCapacity(requiredBatchDataBufferSize));
- // We swap the indirect buffer used so we need to explicitly add the usage.
- usageTracker->BufferUsedAs(outputParamsBuffer.GetBuffer(), wgpu::BufferUsage::Indirect);
+ // We swap the indirect buffer used so we need to explicitly add the usage. `outputParamsBuffer`
+ // is an internal buffer so we don't need to validate it against the resource usage scope rules.
+ usageTracker->BufferUsedAs(outputParamsBuffer.GetBuffer(),
+ kIndirectBufferForBackendResourceTracking);
// Now we allocate and populate host-side batch data to be copied to the GPU.
for (Pass& pass : passes) {
diff --git a/src/dawn/native/RenderEncoderBase.cpp b/src/dawn/native/RenderEncoderBase.cpp
index acc8e93..67af801 100644
--- a/src/dawn/native/RenderEncoderBase.cpp
+++ b/src/dawn/native/RenderEncoderBase.cpp
@@ -233,15 +233,19 @@
mIndirectDrawMetadata.AddIndirectDraw(indirectBuffer, indirectOffset,
duplicateBaseVertexInstance, cmd);
+
+ // We only set usage as `kIndirectBufferForFrontendValidation` so that the indirect
+ // usage can be ignored in the backends because `indirectBuffer` is actually not
+ // used as an indirect buffer.
+ mUsageTracker.BufferUsedAs(indirectBuffer, kIndirectBufferForFrontendValidation);
} else {
cmd->indirectBuffer = indirectBuffer;
cmd->indirectOffset = indirectOffset;
- }
- // TODO(crbug.com/dawn/1166): Adding the indirectBuffer is needed for correct usage
- // validation, but it will unnecessarily transition to indirectBuffer usage in the
- // backend.
- mUsageTracker.BufferUsedAs(indirectBuffer, wgpu::BufferUsage::Indirect);
+ mUsageTracker.BufferUsedAs(indirectBuffer,
+ kIndirectBufferForFrontendValidation |
+ kIndirectBufferForBackendResourceTracking);
+ }
mDrawCount++;
@@ -294,15 +298,19 @@
mCommandBufferState.GetIndexFormat(), mCommandBufferState.GetIndexBufferSize(),
mCommandBufferState.GetIndexBufferOffset(), indirectBuffer, indirectOffset,
duplicateBaseVertexInstance, cmd);
+
+ // We only set usage as `kIndirectBufferForFrontendValidation` so that the indirect
+ // usage can be ignored in the backends because `indirectBuffer` is actually not
+ // used as an indirect buffer.
+ mUsageTracker.BufferUsedAs(indirectBuffer, kIndirectBufferForFrontendValidation);
} else {
cmd->indirectBuffer = indirectBuffer;
cmd->indirectOffset = indirectOffset;
- }
- // TODO(crbug.com/dawn/1166): Adding the indirectBuffer is needed for correct usage
- // validation, but it will unecessarily transition to indirectBuffer usage in the
- // backend.
- mUsageTracker.BufferUsedAs(indirectBuffer, wgpu::BufferUsage::Indirect);
+ mUsageTracker.BufferUsedAs(indirectBuffer,
+ kIndirectBufferForFrontendValidation |
+ kIndirectBufferForBackendResourceTracking);
+ }
mDrawCount++;
@@ -387,12 +395,23 @@
mIndirectDrawMetadata.AddMultiDrawIndirect(duplicateBaseVertexInstance, cmd);
- // TODO(crbug.com/dawn/1166): Adding the indirectBuffer is needed for correct usage
- // validation, but it will unecessarily transition to indirectBuffer usage in the
- // backend.
- mUsageTracker.BufferUsedAs(indirectBuffer, wgpu::BufferUsage::Indirect);
+ if (GetDevice()->IsValidationEnabled() ||
+ GetDevice()->MayRequireDuplicationOfIndirectParameters()) {
+ // We only set usage as `kIndirectBufferForFrontendValidation` because
+ // `indirectBuffer` may not be used as an indirect buffer. The usage of
+ // `indirectBuffer` may be updated in `EncodeIndirectDrawValidationCommands()` in
+ // `EncodingContext::ExitRenderPass()`, which is only called when above conditions
+ // are both met.
+ mUsageTracker.BufferUsedAs(indirectBuffer, kIndirectBufferForFrontendValidation);
+ } else {
+ mUsageTracker.BufferUsedAs(indirectBuffer,
+ kIndirectBufferForFrontendValidation |
+ kIndirectBufferForBackendResourceTracking);
+ }
if (drawCountBuffer != nullptr) {
- mUsageTracker.BufferUsedAs(drawCountBuffer, wgpu::BufferUsage::Indirect);
+ mUsageTracker.BufferUsedAs(drawCountBuffer,
+ kIndirectBufferForFrontendValidation |
+ kIndirectBufferForBackendResourceTracking);
}
mDrawCount += maxDrawCount;
@@ -483,12 +502,23 @@
mCommandBufferState.GetIndexBufferSize(),
mCommandBufferState.GetIndexBufferOffset(), duplicateBaseVertexInstance, cmd);
- // TODO(crbug.com/dawn/1166): Adding the indirectBuffer is needed for correct usage
- // validation, but it will unecessarily transition to indirectBuffer usage in the
- // backend.
- mUsageTracker.BufferUsedAs(indirectBuffer, wgpu::BufferUsage::Indirect);
+ if (GetDevice()->IsValidationEnabled() ||
+ GetDevice()->MayRequireDuplicationOfIndirectParameters()) {
+ // We only set usage as `kIndirectBufferForFrontendValidation` because
+ // `indirectBuffer` may not be used as an indirect buffer. The usage of
+ // `indirectBuffer` may be updated in `EncodeIndirectDrawValidationCommands()` in
+ // `EncodingContext::ExitRenderPass()`, which is only called when above conditions
+ // are both met.
+ mUsageTracker.BufferUsedAs(indirectBuffer, kIndirectBufferForFrontendValidation);
+ } else {
+ mUsageTracker.BufferUsedAs(indirectBuffer,
+ kIndirectBufferForFrontendValidation |
+ kIndirectBufferForBackendResourceTracking);
+ }
if (drawCountBuffer != nullptr) {
- mUsageTracker.BufferUsedAs(drawCountBuffer, wgpu::BufferUsage::Indirect);
+ mUsageTracker.BufferUsedAs(drawCountBuffer,
+ kIndirectBufferForFrontendValidation |
+ kIndirectBufferForBackendResourceTracking);
}
mDrawCount += maxDrawCount;
diff --git a/src/dawn/native/d3d12/BufferD3D12.cpp b/src/dawn/native/d3d12/BufferD3D12.cpp
index 2da4fe9..ad91461 100644
--- a/src/dawn/native/d3d12/BufferD3D12.cpp
+++ b/src/dawn/native/d3d12/BufferD3D12.cpp
@@ -84,7 +84,7 @@
resourceState |= (D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE |
D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE);
}
- if (usage & wgpu::BufferUsage::Indirect) {
+ if (usage & kIndirectBufferForBackendResourceTracking) {
resourceState |= D3D12_RESOURCE_STATE_INDIRECT_ARGUMENT;
}
if (usage & wgpu::BufferUsage::QueryResolve) {
diff --git a/src/dawn/native/dawn_platform.h b/src/dawn/native/dawn_platform.h
index 821821d..0f49346 100644
--- a/src/dawn/native/dawn_platform.h
+++ b/src/dawn/native/dawn_platform.h
@@ -55,6 +55,18 @@
static constexpr wgpu::BufferUsage kInternalCopySrcBuffer =
static_cast<wgpu::BufferUsage>(1u << 29);
+// Add an extra buffer usage (indirect-for-backend-resource-tracking) for backend resource
+// tracking. We won't do buffer usage transitions when the new buffer usage only contains
+// wgpu::BufferUsage::Indirect and doesn't contain kInternalIndirectBufferForBackendResourceTracking
+// on the backends.
+static constexpr wgpu::BufferUsage kIndirectBufferForBackendResourceTracking =
+ static_cast<wgpu::BufferUsage>(1u << 28);
+
+// Define `kIndirectBufferForFrontendValidation` as an alias of wgpu::BufferUsage::Indirect just
+// for front-end validation on the indirect buffer usage.
+static constexpr wgpu::BufferUsage kIndirectBufferForFrontendValidation =
+ wgpu::BufferUsage::Indirect;
+
// Extra texture usages
// Usage to denote an extra tag value used in system specific ways.
// - Used to store that attachments are used more than once in PassResourceUsageTracker.
diff --git a/src/dawn/native/vulkan/BufferVk.cpp b/src/dawn/native/vulkan/BufferVk.cpp
index 958cfd1..9f24c5f 100644
--- a/src/dawn/native/vulkan/BufferVk.cpp
+++ b/src/dawn/native/vulkan/BufferVk.cpp
@@ -107,7 +107,7 @@
flags |= VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT;
}
}
- if (usage & wgpu::BufferUsage::Indirect) {
+ if (usage & kIndirectBufferForBackendResourceTracking) {
flags |= VK_PIPELINE_STAGE_DRAW_INDIRECT_BIT;
}
if (usage & wgpu::BufferUsage::QueryResolve) {
@@ -147,7 +147,7 @@
if (usage & kReadOnlyStorageBuffer) {
flags |= VK_ACCESS_SHADER_READ_BIT;
}
- if (usage & wgpu::BufferUsage::Indirect) {
+ if (usage & kIndirectBufferForBackendResourceTracking) {
flags |= VK_ACCESS_INDIRECT_COMMAND_READ_BIT;
}
if (usage & wgpu::BufferUsage::QueryResolve) {
diff --git a/src/dawn/native/vulkan/CommandBufferVk.cpp b/src/dawn/native/vulkan/CommandBufferVk.cpp
index c3eade0..1481329 100644
--- a/src/dawn/native/vulkan/CommandBufferVk.cpp
+++ b/src/dawn/native/vulkan/CommandBufferVk.cpp
@@ -209,10 +209,22 @@
VkPipelineStageFlags srcStages = 0;
VkPipelineStageFlags dstStages = 0;
+ // `kIndirectBufferForFrontendValidation` is only for the front-end validation and should be
+ // totally ignored in the backend resource tracking because:
+ // 1. When old usage contains kIndirectBufferForFrontendValidation while new usage just
+ // removes kIndirectBufferForFrontendValidation, the barrier is actually unnecessary.
+ // 2. When usage == kIndirectBufferForFrontendValidation, dstStages would be NONE, which is
+ // not allowed by Vulkan SPEC unless synchronization2 is enabled.
+ // We remove `kIndirectBufferForFrontendValidation` in this function instead of in the
+ // function `BufferVk::TrackUsageAndGetResourceBarrier()` because on Vulkan backend this
+ // is the only place that we need to handle `kIndirectBufferForFrontendValidation`.
+ wgpu::BufferUsage usage =
+ scope.bufferSyncInfos[i].usage & (~kIndirectBufferForFrontendValidation);
+
VkBufferMemoryBarrier bufferBarrier;
- if (buffer->TrackUsageAndGetResourceBarrier(
- recordingContext, scope.bufferSyncInfos[i].usage,
- scope.bufferSyncInfos[i].shaderStages, &bufferBarrier, &srcStages, &dstStages)) {
+ if (buffer->TrackUsageAndGetResourceBarrier(recordingContext, usage,
+ scope.bufferSyncInfos[i].shaderStages,
+ &bufferBarrier, &srcStages, &dstStages)) {
MergeBufferBarrier((dstStages & vertexStages) ? &vertexBarriers : &nonVertexBarriers,
srcStages, dstStages, bufferBarrier);
}
