Skip to content

A
angle
Commit 7870cf3f by Shahbaz Youssefi Committed by Commit Bot
Options

Vulkan: Fix RAW hazard with storage images

Semantic revert of 78304b47 and 7bce5194. 7bce5194 assumed that a read transition between same layouts is a noop, but that's not true if said layout is GENERAL. This is only possible if an image is simultaneously bound as storage and sampled image. This bug was discovered by the new syncval VVL warning. Bug: b/156661359 Change-Id: I05f94160ca1b05b715701564e27fccee31a8aa45 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2404742 Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org> Reviewed-by: 's avatarJamie Madill <jmadill@chromium.org> Reviewed-by: 's avatarCharlie Lao <cclao@google.com>
parent 2999ad4e
Showing with 43 additions and 10 deletions
src/libANGLE/renderer/vulkan/vk_helpers.cpp
...@@ -82,6 +82,8 @@ struct ImageMemoryBarrierData ...@@ -82,6 +82,8 @@ struct ImageMemoryBarrierData
// Access mask when transitioning out from this layout. Note that source access mask never // Access mask when transitioning out from this layout. Note that source access mask never
// needs a READ bit, as WAR hazards don't need memory barriers (just execution barriers). // needs a READ bit, as WAR hazards don't need memory barriers (just execution barriers).
VkAccessFlags srcAccessMask; VkAccessFlags srcAccessMask;
// Read or write.
ResourceAccess type;
// CommandBufferHelper tracks an array of PipelineBarriers. This indicates which array element // CommandBufferHelper tracks an array of PipelineBarriers. This indicates which array element
// this should be merged into. Right now we track individual barrier for every PipelineStage. If // this should be merged into. Right now we track individual barrier for every PipelineStage. If
// layout has a single stage mask bit, we use that stage as index. If layout has multiple stage // layout has a single stage mask bit, we use that stage as index. If layout has multiple stage
...@@ -106,6 +108,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -106,6 +108,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
0, 0,
// Transition from: there's no data in the image to care about. // Transition from: there's no data in the image to care about.
0, 0,
ResourceAccess::ReadOnly,
PipelineStage::InvalidEnum, PipelineStage::InvalidEnum,
}, },
}, },
...@@ -120,6 +123,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -120,6 +123,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
0, 0,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_MEMORY_WRITE_BIT, VK_ACCESS_MEMORY_WRITE_BIT,
ResourceAccess::ReadOnly,
PipelineStage::InvalidEnum, PipelineStage::InvalidEnum,
}, },
}, },
...@@ -134,6 +138,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -134,6 +138,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT, VK_ACCESS_SHADER_READ_BIT,
// Transition from: RAR and WAR don't need memory barrier. // Transition from: RAR and WAR don't need memory barrier.
0, 0,
ResourceAccess::ReadOnly,
// In case of multiple destination stages, We barrier the earliest stage // In case of multiple destination stages, We barrier the earliest stage
PipelineStage::TopOfPipe, PipelineStage::TopOfPipe,
}, },
...@@ -149,6 +154,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -149,6 +154,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_WRITE_BIT,
ResourceAccess::Write,
// In case of multiple destination stages, We barrier the earliest stage // In case of multiple destination stages, We barrier the earliest stage
PipelineStage::TopOfPipe, PipelineStage::TopOfPipe,
}, },
...@@ -164,6 +170,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -164,6 +170,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_TRANSFER_READ_BIT, VK_ACCESS_TRANSFER_READ_BIT,
// Transition from: RAR and WAR don't need memory barrier. // Transition from: RAR and WAR don't need memory barrier.
0, 0,
ResourceAccess::ReadOnly,
PipelineStage::Transfer, PipelineStage::Transfer,
}, },
}, },
...@@ -178,6 +185,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -178,6 +185,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_TRANSFER_WRITE_BIT, VK_ACCESS_TRANSFER_WRITE_BIT,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_TRANSFER_WRITE_BIT, VK_ACCESS_TRANSFER_WRITE_BIT,
ResourceAccess::Write,
PipelineStage::Transfer, PipelineStage::Transfer,
}, },
}, },
...@@ -192,6 +200,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -192,6 +200,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT, VK_ACCESS_SHADER_READ_BIT,
// Transition from: RAR and WAR don't need memory barrier. // Transition from: RAR and WAR don't need memory barrier.
0, 0,
ResourceAccess::ReadOnly,
PipelineStage::VertexShader, PipelineStage::VertexShader,
}, },
}, },
...@@ -206,6 +215,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -206,6 +215,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_WRITE_BIT,
ResourceAccess::Write,
PipelineStage::VertexShader, PipelineStage::VertexShader,
}, },
}, },
...@@ -220,6 +230,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -220,6 +230,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT, VK_ACCESS_SHADER_READ_BIT,
// Transition from: RAR and WAR don't need memory barrier. // Transition from: RAR and WAR don't need memory barrier.
0, 0,
ResourceAccess::ReadOnly,
PipelineStage::GeometryShader, PipelineStage::GeometryShader,
}, },
}, },
...@@ -234,6 +245,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -234,6 +245,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_WRITE_BIT,
ResourceAccess::Write,
PipelineStage::GeometryShader, PipelineStage::GeometryShader,
}, },
}, },
...@@ -248,6 +260,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -248,6 +260,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT, VK_ACCESS_SHADER_READ_BIT,
// Transition from: RAR and WAR don't need memory barrier. // Transition from: RAR and WAR don't need memory barrier.
0, 0,
ResourceAccess::ReadOnly,
PipelineStage::FragmentShader, PipelineStage::FragmentShader,
}, },
}, },
...@@ -262,6 +275,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -262,6 +275,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_WRITE_BIT,
ResourceAccess::Write,
PipelineStage::FragmentShader, PipelineStage::FragmentShader,
}, },
}, },
...@@ -276,6 +290,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -276,6 +290,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT, VK_ACCESS_SHADER_READ_BIT,
// Transition from: RAR and WAR don't need memory barrier. // Transition from: RAR and WAR don't need memory barrier.
0, 0,
ResourceAccess::ReadOnly,
PipelineStage::ComputeShader, PipelineStage::ComputeShader,
}, },
}, },
...@@ -290,6 +305,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -290,6 +305,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_WRITE_BIT,
ResourceAccess::Write,
PipelineStage::ComputeShader, PipelineStage::ComputeShader,
}, },
}, },
...@@ -304,6 +320,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -304,6 +320,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT, VK_ACCESS_SHADER_READ_BIT,
// Transition from: RAR and WAR don't need memory barrier. // Transition from: RAR and WAR don't need memory barrier.
0, 0,
ResourceAccess::ReadOnly,
// In case of multiple destination stages, We barrier the earliest stage // In case of multiple destination stages, We barrier the earliest stage
PipelineStage::VertexShader, PipelineStage::VertexShader,
}, },
...@@ -319,6 +336,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -319,6 +336,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_SHADER_WRITE_BIT, VK_ACCESS_SHADER_WRITE_BIT,
ResourceAccess::Write,
// In case of multiple destination stages, We barrier the earliest stage // In case of multiple destination stages, We barrier the earliest stage
PipelineStage::VertexShader, PipelineStage::VertexShader,
}, },
...@@ -334,6 +352,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -334,6 +352,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_COLOR_ATTACHMENT_READ_BIT | VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT, VK_ACCESS_COLOR_ATTACHMENT_READ_BIT | VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT, VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT,
ResourceAccess::Write,
PipelineStage::ColorAttachmentOutput, PipelineStage::ColorAttachmentOutput,
}, },
}, },
...@@ -348,6 +367,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -348,6 +367,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_READ_BIT, VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_READ_BIT,
// Transition from: RAR and WAR don't need memory barrier. // Transition from: RAR and WAR don't need memory barrier.
0, 0,
ResourceAccess::ReadOnly,
PipelineStage::EarlyFragmentTest, PipelineStage::EarlyFragmentTest,
}, },
}, },
...@@ -362,6 +382,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -362,6 +382,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_READ_BIT | VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BIT, VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_READ_BIT | VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BIT,
// Transition from: all writes must finish before barrier. // Transition from: all writes must finish before barrier.
VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BIT, VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BIT,
ResourceAccess::Write,
PipelineStage::EarlyFragmentTest, PipelineStage::EarlyFragmentTest,
}, },
}, },
...@@ -381,6 +402,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory ...@@ -381,6 +402,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
0, 0,
// Transition from: RAR and WAR don't need memory barrier. // Transition from: RAR and WAR don't need memory barrier.
0, 0,
ResourceAccess::ReadOnly,
PipelineStage::BottomOfPipe, PipelineStage::BottomOfPipe,
}, },
}, },
...@@ -3340,6 +3362,23 @@ bool ImageHelper::isDepthOrStencil() const ...@@ -3340,6 +3362,23 @@ bool ImageHelper::isDepthOrStencil() const
return mFormat->actualImageFormat().hasDepthOrStencilBits(); return mFormat->actualImageFormat().hasDepthOrStencilBits();
} }
bool ImageHelper::isReadBarrierNecessary(ImageLayout newLayout) const
{
// If transitioning to a different layout, we need always need a barrier.
if (mCurrentLayout != newLayout)
{
return true;
}
// RAR (read-after-read) is not a hazard and doesn't require a barrier.
//
// RAW (read-after-write) hazards always require a memory barrier. This can only happen if the
// layout (same as new layout) is writable which in turn is only possible if the image is
// simultaneously bound for shader write (i.e. the layout is GENERAL).
const ImageMemoryBarrierData &layoutData = kImageMemoryBarrierData[mCurrentLayout];
return layoutData.type == ResourceAccess::Write;
}
void ImageHelper::changeLayoutAndQueue(VkImageAspectFlags aspectMask, void ImageHelper::changeLayoutAndQueue(VkImageAspectFlags aspectMask,
ImageLayout newLayout, ImageLayout newLayout,
uint32_t newQueueFamilyIndex, uint32_t newQueueFamilyIndex,
...@@ -3465,10 +3504,9 @@ bool ImageHelper::updateLayoutAndBarrier(VkImageAspectFlags aspectMask, ...@@ -3465,10 +3504,9 @@ bool ImageHelper::updateLayoutAndBarrier(VkImageAspectFlags aspectMask,
if (newLayout == mCurrentLayout) if (newLayout == mCurrentLayout)
{ {
const ImageMemoryBarrierData &layoutData = kImageMemoryBarrierData[mCurrentLayout]; const ImageMemoryBarrierData &layoutData = kImageMemoryBarrierData[mCurrentLayout];
// srcAccessMask == 0 is only possible for read-only layouts. RAR is not a hazard and // RAR is not a hazard and doesn't require a barrier, especially as the image layout hasn't
// doesn't require a barrier, especially as the image layout hasn't changed. The following // changed. The following asserts that such a barrier is not attempted.
// asserts that such a barrier is not attempted. ASSERT(layoutData.type == ResourceAccess::Write);
ASSERT(layoutData.srcAccessMask != 0);
// No layout change, only memory barrier is required // No layout change, only memory barrier is required
barrier->mergeMemoryBarrier(layoutData.srcStageMask, layoutData.dstStageMask, barrier->mergeMemoryBarrier(layoutData.srcStageMask, layoutData.dstStageMask,
layoutData.srcAccessMask, layoutData.dstAccessMask); layoutData.srcAccessMask, layoutData.dstAccessMask);
......
src/libANGLE/renderer/vulkan/vk_helpers.h
...@@ -1447,12 +1447,7 @@ class ImageHelper final : public Resource, public angle::Subject ...@@ -1447,12 +1447,7 @@ class ImageHelper final : public Resource, public angle::Subject
} }
// This function can be used to prevent issuing redundant layout transition commands. // This function can be used to prevent issuing redundant layout transition commands.
bool isReadBarrierNecessary(ImageLayout newLayout) const bool isReadBarrierNecessary(ImageLayout newLayout) const;
{
// If transitioning to a different layout, we always need a barrier. Otherwise RAR
// (read-after-read) is not a hazard and doesn't require a barrier.
return mCurrentLayout != newLayout;
}
void recordReadBarrier(VkImageAspectFlags aspectMask, void recordReadBarrier(VkImageAspectFlags aspectMask,
ImageLayout newLayout, ImageLayout newLayout,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment