Commit 61c6aecc by Jamie Madill Committed by Commit Bot

Vulkan: Fold read access into write flags on buffer writes

This could lead to a subtle bug: Say we use a buffer as a storage buffer and write to it. We then must issue a memory barrier before using it as a uniform buffer. However we would set the SHADER_READ bit as a read mask on a storage buffer access. This seems like it could lead to a missing barrier. Bug: angleproject:4178 Change-Id: I486002739b7fb000ffacc0a1e996784b7875e7ba Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1943034 Commit-Queue: Jamie Madill <jmadill@chromium.org> Reviewed-by: 's avatarShahbaz Youssefi <syoussefi@chromium.org> Reviewed-by: 's avatarJonah Ryan-Davis <jonahr@google.com>
parent a654d351
...@@ -162,8 +162,9 @@ angle::Result BufferVk::copySubData(const gl::Context *context, ...@@ -162,8 +162,9 @@ angle::Result BufferVk::copySubData(const gl::Context *context,
// Handle self-dependency especially. // Handle self-dependency especially.
if (sourceBuffer->mBuffer.getBuffer().getHandle() == mBuffer.getBuffer().getHandle()) if (sourceBuffer->mBuffer.getBuffer().getHandle() == mBuffer.getBuffer().getHandle())
{ {
mBuffer.onSelfReadWrite(contextVk, VK_ACCESS_TRANSFER_READ_BIT, // We set the TRANSFER_READ_BIT to be conservative.
VK_ACCESS_TRANSFER_WRITE_BIT); mBuffer.onSelfReadWrite(contextVk,
VK_ACCESS_TRANSFER_READ_BIT | VK_ACCESS_TRANSFER_WRITE_BIT);
ANGLE_TRY(mBuffer.recordCommands(contextVk, &commandBuffer)); ANGLE_TRY(mBuffer.recordCommands(contextVk, &commandBuffer));
} }
......
...@@ -1338,8 +1338,9 @@ void ProgramVk::updateBuffersDescriptorSet(ContextVk *contextVk, ...@@ -1338,8 +1338,9 @@ void ProgramVk::updateBuffersDescriptorSet(ContextVk *contextVk,
if (isStorageBuffer) if (isStorageBuffer)
{ {
bufferHelper.onWrite(contextVk, recorder, VK_ACCESS_SHADER_READ_BIT, // We set the SHADER_READ_BIT to be conservative.
VK_ACCESS_SHADER_WRITE_BIT); bufferHelper.onWrite(contextVk, recorder,
VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT);
} }
else else
{ {
...@@ -1401,8 +1402,9 @@ void ProgramVk::updateAtomicCounterBuffersDescriptorSet(ContextVk *contextVk, ...@@ -1401,8 +1402,9 @@ void ProgramVk::updateAtomicCounterBuffersDescriptorSet(ContextVk *contextVk,
BufferVk *bufferVk = vk::GetImpl(bufferBinding.get()); BufferVk *bufferVk = vk::GetImpl(bufferBinding.get());
vk::BufferHelper &bufferHelper = bufferVk->getBuffer(); vk::BufferHelper &bufferHelper = bufferVk->getBuffer();
bufferHelper.onWrite(contextVk, recorder, VK_ACCESS_SHADER_READ_BIT, // We set SHADER_READ_BIT to be conservative.
VK_ACCESS_SHADER_WRITE_BIT); bufferHelper.onWrite(contextVk, recorder,
VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT);
writtenBindings.set(binding); writtenBindings.set(binding);
} }
......
...@@ -129,7 +129,7 @@ void TransformFeedbackVk::addFramebufferDependency(ContextVk *contextVk, ...@@ -129,7 +129,7 @@ void TransformFeedbackVk::addFramebufferDependency(ContextVk *contextVk,
ASSERT(buffer != nullptr); ASSERT(buffer != nullptr);
vk::BufferHelper &bufferHelper = vk::GetImpl(buffer)->getBuffer(); vk::BufferHelper &bufferHelper = vk::GetImpl(buffer)->getBuffer();
bufferHelper.onWrite(contextVk, framebuffer, 0, VK_ACCESS_SHADER_WRITE_BIT); bufferHelper.onWrite(contextVk, framebuffer, VK_ACCESS_SHADER_WRITE_BIT);
} }
} }
......
...@@ -774,7 +774,7 @@ angle::Result UtilsVk::clearBuffer(ContextVk *contextVk, ...@@ -774,7 +774,7 @@ angle::Result UtilsVk::clearBuffer(ContextVk *contextVk,
ANGLE_TRY(dest->recordCommands(contextVk, &commandBuffer)); ANGLE_TRY(dest->recordCommands(contextVk, &commandBuffer));
// Tell dest it's being written to. // Tell dest it's being written to.
dest->onSelfReadWrite(contextVk, 0, VK_ACCESS_SHADER_WRITE_BIT); dest->onSelfReadWrite(contextVk, VK_ACCESS_SHADER_WRITE_BIT);
const vk::Format &destFormat = dest->getViewFormat(); const vk::Format &destFormat = dest->getViewFormat();
......
...@@ -1473,8 +1473,7 @@ void BufferHelper::release(RendererVk *renderer) ...@@ -1473,8 +1473,7 @@ void BufferHelper::release(RendererVk *renderer)
renderer->collectGarbageAndReinit(&mUse, &mBuffer, &mBufferView, &mDeviceMemory); renderer->collectGarbageAndReinit(&mUse, &mBuffer, &mBufferView, &mDeviceMemory);
} }
bool BufferHelper::needsOnWriteBarrier(VkAccessFlags readAccessType, bool BufferHelper::needsOnWriteBarrier(VkAccessFlags writeAccessType,
VkAccessFlags writeAccessType,
VkAccessFlags *barrierSrcOut, VkAccessFlags *barrierSrcOut,
VkAccessFlags *barrierDstOut) VkAccessFlags *barrierDstOut)
{ {
...@@ -1483,20 +1482,18 @@ bool BufferHelper::needsOnWriteBarrier(VkAccessFlags readAccessType, ...@@ -1483,20 +1482,18 @@ bool BufferHelper::needsOnWriteBarrier(VkAccessFlags readAccessType,
// Note: mCurrentReadAccess is not part of barrier src flags as "anything-after-read" is // Note: mCurrentReadAccess is not part of barrier src flags as "anything-after-read" is
// satisified by execution barriers alone. // satisified by execution barriers alone.
*barrierSrcOut = mCurrentWriteAccess; *barrierSrcOut = mCurrentWriteAccess;
*barrierDstOut = readAccessType | writeAccessType; *barrierDstOut = writeAccessType;
mCurrentWriteAccess = writeAccessType; mCurrentWriteAccess = writeAccessType;
mCurrentReadAccess = readAccessType; mCurrentReadAccess = 0;
return needsBarrier; return needsBarrier;
} }
void BufferHelper::onWriteAccess(ContextVk *contextVk, void BufferHelper::onWriteAccess(ContextVk *contextVk, VkAccessFlags writeAccessType)
VkAccessFlags readAccessType,
VkAccessFlags writeAccessType)
{ {
VkAccessFlags barrierSrc, barrierDst; VkAccessFlags barrierSrc, barrierDst;
if (needsOnWriteBarrier(readAccessType, writeAccessType, &barrierSrc, &barrierDst)) if (needsOnWriteBarrier(writeAccessType, &barrierSrc, &barrierDst))
{ {
addGlobalMemoryBarrier(barrierSrc, barrierDst, VK_PIPELINE_STAGE_ALL_COMMANDS_BIT); addGlobalMemoryBarrier(barrierSrc, barrierDst, VK_PIPELINE_STAGE_ALL_COMMANDS_BIT);
} }
......
...@@ -485,13 +485,10 @@ class BufferHelper final : public CommandGraphResource ...@@ -485,13 +485,10 @@ class BufferHelper final : public CommandGraphResource
addReadDependency(contextVk, reader); addReadDependency(contextVk, reader);
onReadAccess(reader, readAccessType); onReadAccess(reader, readAccessType);
} }
void onWrite(ContextVk *contextVk, void onWrite(ContextVk *contextVk, CommandGraphResource *writer, VkAccessFlags writeAccessType)
CommandGraphResource *writer,
VkAccessFlags readAccessType,
VkAccessFlags writeAccessType)
{ {
addWriteDependency(contextVk, writer); addWriteDependency(contextVk, writer);
onWriteAccess(contextVk, readAccessType, writeAccessType); onWriteAccess(contextVk, writeAccessType);
} }
// Helper for setting a graph dependency between two buffers. This is a specialized function as // Helper for setting a graph dependency between two buffers. This is a specialized function as
// both buffers may incur a memory barrier. Using |onRead| followed by |onWrite| between the // both buffers may incur a memory barrier. Using |onRead| followed by |onWrite| between the
...@@ -503,19 +500,17 @@ class BufferHelper final : public CommandGraphResource ...@@ -503,19 +500,17 @@ class BufferHelper final : public CommandGraphResource
{ {
addReadDependency(contextVk, reader); addReadDependency(contextVk, reader);
onReadAccess(reader, readAccessType); onReadAccess(reader, readAccessType);
reader->onWriteAccess(contextVk, 0, writeAccessType); reader->onWriteAccess(contextVk, writeAccessType);
} }
// Helper for setting a barrier when different parts of the same buffer is being read from and // Helper for setting a barrier when different parts of the same buffer is being read from and
// written to in the same command. // written to in the same command.
void onSelfReadWrite(ContextVk *contextVk, void onSelfReadWrite(ContextVk *contextVk, VkAccessFlags writeAccessType)
VkAccessFlags readAccessType,
VkAccessFlags writeAccessType)
{ {
if (mCurrentReadAccess || mCurrentWriteAccess) if (mCurrentReadAccess || mCurrentWriteAccess)
{ {
finishCurrentCommands(contextVk); finishCurrentCommands(contextVk);
} }
onWriteAccess(contextVk, readAccessType, writeAccessType); onWriteAccess(contextVk, writeAccessType);
} }
// Set write access mask when the buffer is modified externally, e.g. by host. There is no // Set write access mask when the buffer is modified externally, e.g. by host. There is no
// graph resource to create a dependency to. // graph resource to create a dependency to.
...@@ -586,13 +581,10 @@ class BufferHelper final : public CommandGraphResource ...@@ -586,13 +581,10 @@ class BufferHelper final : public CommandGraphResource
VK_PIPELINE_STAGE_ALL_COMMANDS_BIT); VK_PIPELINE_STAGE_ALL_COMMANDS_BIT);
} }
} }
bool needsOnWriteBarrier(VkAccessFlags readAccessType, bool needsOnWriteBarrier(VkAccessFlags writeAccessType,
VkAccessFlags writeAccessType,
VkAccessFlags *barrierSrcOut, VkAccessFlags *barrierSrcOut,
VkAccessFlags *barrierDstOut); VkAccessFlags *barrierDstOut);
void onWriteAccess(ContextVk *contextVk, void onWriteAccess(ContextVk *contextVk, VkAccessFlags writeAccessType);
VkAccessFlags readAccessType,
VkAccessFlags writeAccessType);
// Vulkan objects. // Vulkan objects.
Buffer mBuffer; Buffer mBuffer;
......
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