Vulkan: Fix synchronization bug
When binding a texture using a bind group layout entry with an empty
visibility mask we will skip shader usages of the texture when
setting the pipeline stages of the barrier. However, the subresource
state will still be updated as if we didn't skip shader usages. This
leads to incorrect synchronization and triggers an assertion in some
cases.
For example, the following case will lead to an incorrect pipeline
barrier:
1. Create a texture and use it in some way (zero initialization is
enough).
2. A render pass binds that texture as a TextureBinding with no
shader stages (pointless but legal). The render pass contains no
other resources (other than the color attachment it is rendering
to).
Before the render pass we will insert an image memory barrier with a
dstAccessMask of VK_ACCESS_SHADER_READ_BIT. However, the dstStages of
the pipeline barrier does not contain a corresponding pipeline stage
(VK_PIPELINE_STAGE_VERTEX_SHADER_BIT or
VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT).
To solve this, remove shader usages when no shader stages are present
before updating each subresource's state, instead of after.
Change-Id: I969ebe30dc4f4c613bdc2b0261c3b1197c0cda71
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/171441
Commit-Queue: Albin Bernhardsson <albin.bernhardsson@arm.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/native/vulkan/TextureVk.cpp b/src/dawn/native/vulkan/TextureVk.cpp
index fb8ab56..dc90fc8 100644
--- a/src/dawn/native/vulkan/TextureVk.cpp
+++ b/src/dawn/native/vulkan/TextureVk.cpp
@@ -1310,47 +1310,47 @@
wgpu::ShaderStage allNewShaderStages = wgpu::ShaderStage::None;
wgpu::ShaderStage allLastShaderStages = wgpu::ShaderStage::None;
- mSubresourceLastSyncInfos.Merge(
- subresourceSyncInfos, [&](const SubresourceRange& range, TextureSyncInfo* lastSyncInfo,
- const TextureSyncInfo& newSyncInfo) {
- if (newSyncInfo.usage == wgpu::TextureUsage::None ||
- CanReuseWithoutBarrier(lastSyncInfo->usage, newSyncInfo.usage,
- lastSyncInfo->shaderStages, newSyncInfo.shaderStages)) {
- return;
- }
+ mSubresourceLastSyncInfos.Merge(subresourceSyncInfos, [&](const SubresourceRange& range,
+ TextureSyncInfo* lastSyncInfo,
+ const TextureSyncInfo& newSyncInfo) {
+ wgpu::TextureUsage newUsage = newSyncInfo.usage;
+ if (newSyncInfo.shaderStages == wgpu::ShaderStage::None) {
+ // If the image isn't used in any shader stages, ignore shader usages. Eg. ignore a
+ // texture binding that isn't actually sampled in any shader.
+ newUsage &= ~kShaderTextureUsages;
+ }
- imageBarriers->push_back(
- BuildMemoryBarrier(this, lastSyncInfo->usage, newSyncInfo.usage, range));
+ if (newUsage == wgpu::TextureUsage::None ||
+ CanReuseWithoutBarrier(lastSyncInfo->usage, newUsage, lastSyncInfo->shaderStages,
+ newSyncInfo.shaderStages)) {
+ return;
+ }
- allLastUsages |= lastSyncInfo->usage;
- allNewUsages |= newSyncInfo.usage;
+ imageBarriers->push_back(BuildMemoryBarrier(this, lastSyncInfo->usage, newUsage, range));
- allLastShaderStages |= lastSyncInfo->shaderStages;
- allNewShaderStages |= newSyncInfo.shaderStages;
+ allLastUsages |= lastSyncInfo->usage;
+ allNewUsages |= newUsage;
- if (lastSyncInfo->usage == newSyncInfo.usage &&
- IsSubset(lastSyncInfo->usage, kReadOnlyTextureUsages)) {
- // Read only usage and no layout transition. We can keep previous shader stages so
- // future uses in those stages don't insert barriers.
- lastSyncInfo->shaderStages |= newSyncInfo.shaderStages;
- } else {
- // Image was altered by write or layout transition. We need to clear previous shader
- // stages so future uses in those stages will insert barriers.
- lastSyncInfo->shaderStages = newSyncInfo.shaderStages;
- }
- lastSyncInfo->usage = newSyncInfo.usage;
- });
+ allLastShaderStages |= lastSyncInfo->shaderStages;
+ allNewShaderStages |= newSyncInfo.shaderStages;
+
+ if (lastSyncInfo->usage == newUsage &&
+ IsSubset(lastSyncInfo->usage, kReadOnlyTextureUsages)) {
+ // Read only usage and no layout transition. We can keep previous shader stages so
+ // future uses in those stages don't insert barriers.
+ lastSyncInfo->shaderStages |= newSyncInfo.shaderStages;
+ } else {
+ // Image was altered by write or layout transition. We need to clear previous shader
+ // stages so future uses in those stages will insert barriers.
+ lastSyncInfo->shaderStages = newSyncInfo.shaderStages;
+ }
+ lastSyncInfo->usage = newUsage;
+ });
if (mExternalState != ExternalState::InternalOnly) {
TweakTransitionForExternalUsage(recordingContext, imageBarriers, transitionBarrierStart);
}
- if (allNewShaderStages == wgpu::ShaderStage::None) {
- // If the image isn't used in any shader stages, ignore shader usages. Eg. ignore a texture
- // binding that isn't actually sampled in any shader.
- allNewUsages &= ~kShaderTextureUsages;
- }
-
// Skip adding pipeline stages if no barrier was needed to avoid putting TOP_OF_PIPE in the
// destination stages.
if (allNewUsages != wgpu::TextureUsage::None) {
diff --git a/src/dawn/tests/end2end/BindGroupTests.cpp b/src/dawn/tests/end2end/BindGroupTests.cpp
index 731fd8f..b88e750 100644
--- a/src/dawn/tests/end2end/BindGroupTests.cpp
+++ b/src/dawn/tests/end2end/BindGroupTests.cpp
@@ -1252,13 +1252,17 @@
TEST_P(BindGroupTests, BindGroupLayoutVisibilityCanBeNone) {
utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, kRTSize, kRTSize);
- wgpu::BindGroupLayoutEntry entry;
- entry.binding = 0;
- entry.visibility = wgpu::ShaderStage::None;
- entry.buffer.type = wgpu::BufferBindingType::Uniform;
+ wgpu::BindGroupLayoutEntry entries[2];
+ entries[0].binding = 0;
+ entries[0].visibility = wgpu::ShaderStage::None;
+ entries[0].buffer.type = wgpu::BufferBindingType::Uniform;
+ entries[1].binding = 1;
+ entries[1].visibility = wgpu::ShaderStage::None;
+ entries[1].texture.sampleType = wgpu::TextureSampleType::Float;
+
wgpu::BindGroupLayoutDescriptor descriptor;
- descriptor.entryCount = 1;
- descriptor.entries = &entry;
+ descriptor.entryCount = 2;
+ descriptor.entries = entries;
wgpu::BindGroupLayout layout = device.CreateBindGroupLayout(&descriptor);
wgpu::RenderPipeline pipeline = MakeTestPipeline(renderPass, {}, {layout});
@@ -1266,8 +1270,16 @@
std::array<float, 4> color = {1, 0, 0, 1};
wgpu::Buffer uniformBuffer =
utils::CreateBufferFromData(device, &color, sizeof(color), wgpu::BufferUsage::Uniform);
- wgpu::BindGroup bindGroup =
- utils::MakeBindGroup(device, layout, {{0, uniformBuffer, 0, sizeof(color)}});
+
+ wgpu::TextureDescriptor textureDescriptor;
+ textureDescriptor.size = {kRTSize, kRTSize};
+ textureDescriptor.format = wgpu::TextureFormat::RGBA8Unorm;
+ textureDescriptor.usage = wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::TextureBinding;
+ wgpu::Texture texture = device.CreateTexture(&textureDescriptor);
+ wgpu::TextureView textureView = texture.CreateView();
+
+ wgpu::BindGroup bindGroup = utils::MakeBindGroup(
+ device, layout, {{0, uniformBuffer, 0, sizeof(color)}, {1, textureView}});
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo);
