Commit 0e65454d by Jamie Madill Committed by Commit Bot

Vulkan: Fix circular dependency with resource updates.

The old implementation would try to keep recording draw commands to the same framebuffer write operation even if the vertex array buffer data changed. This would lead to a broken dependency graph. Fix this by forcing any current render operations to create a new node in this case, giving a correct command graph. Old design: - render (creates a CommandBufferNode A) - update buffer (creates a CommandBufferNode B which happens after A) - render (to CommandBuffer A, and gives a circular dependency with B) New design - render (CommandBufferNode A) - update buffer (CommandBufferNode B, happens after A) - render (CommandBufferNode C, happens after B) This also renames some methods to try to clarify them. Bug: angleproject:2350 Change-Id: I6559bed4ed3f58f68771662422c5bef6a505282b Reviewed-on: https://chromium-review.googlesource.com/907416Reviewed-by: 's avatarCorentin Wallez <cwallez@chromium.org> Reviewed-by: 's avatarFrank Henigman <fjhenigman@chromium.org> Commit-Queue: Jamie Madill <jmadill@chromium.org>
parent 469708e7
......@@ -195,8 +195,10 @@ vk::Error BufferVk::setDataImpl(ContextVk *contextVk,
stagingBuffer.getDeviceMemory().unmap(device);
// Enqueue a copy command on the GPU.
// 'beginWriteOperation' will stop any subsequent rendering from using the old buffer data,
// by marking any current read operations / command buffers as 'finished'.
vk::CommandBuffer *commandBuffer = nullptr;
ANGLE_TRY(recordWriteCommands(renderer, &commandBuffer));
ANGLE_TRY(beginWriteOperation(renderer, &commandBuffer));
// Insert a barrier to ensure reads from the buffer are complete.
// TODO(jmadill): Insert minimal barriers.
......
......@@ -54,7 +54,9 @@ Error InitAndBeginCommandBuffer(VkDevice device,
// CommandBufferNode implementation.
CommandBufferNode::CommandBufferNode()
: mIsDependency(false), mVisitedState(VisitedState::Unvisited)
: mHasHappensAfterDependencies(false),
mVisitedState(VisitedState::Unvisited),
mIsFinishedRecording(false)
{
}
......@@ -69,11 +71,13 @@ CommandBufferNode::~CommandBufferNode()
CommandBuffer *CommandBufferNode::getOutsideRenderPassCommands()
{
ASSERT(!mIsFinishedRecording);
return &mOutsideRenderPassCommands;
}
CommandBuffer *CommandBufferNode::getInsideRenderPassCommands()
{
ASSERT(!mIsFinishedRecording);
return &mInsideRenderPassCommands;
}
......@@ -81,6 +85,8 @@ Error CommandBufferNode::startRecording(VkDevice device,
const CommandPool &commandPool,
CommandBuffer **commandsOut)
{
ASSERT(!mIsFinishedRecording);
VkCommandBufferInheritanceInfo inheritanceInfo;
inheritanceInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_INHERITANCE_INFO;
inheritanceInfo.pNext = nullptr;
......@@ -100,6 +106,8 @@ Error CommandBufferNode::startRecording(VkDevice device,
Error CommandBufferNode::startRenderPassRecording(RendererVk *renderer, CommandBuffer **commandsOut)
{
ASSERT(!mIsFinishedRecording);
// Get a compatible RenderPass from the cache so we can initialize the inheritance info.
// TODO(jmadill): Use different query method for compatible vs conformant render pass.
vk::RenderPass *compatibleRenderPass;
......@@ -123,6 +131,16 @@ Error CommandBufferNode::startRenderPassRecording(RendererVk *renderer, CommandB
return NoError();
}
bool CommandBufferNode::isFinishedRecording() const
{
return mIsFinishedRecording;
}
void CommandBufferNode::finishRecording()
{
mIsFinishedRecording = true;
}
void CommandBufferNode::storeRenderPassInfo(const Framebuffer &framebuffer,
const gl::Rectangle renderArea,
const std::vector<VkClearValue> &clearValues)
......@@ -136,7 +154,7 @@ void CommandBufferNode::appendColorRenderTarget(Serial serial, RenderTargetVk *c
{
// TODO(jmadill): Layout transition?
mRenderPassDesc.packColorAttachment(*colorRenderTarget->format, colorRenderTarget->samples);
colorRenderTarget->resource->setWriteNode(this, serial);
colorRenderTarget->resource->onWriteResource(this, serial);
}
void CommandBufferNode::appendDepthStencilRenderTarget(Serial serial,
......@@ -145,7 +163,7 @@ void CommandBufferNode::appendDepthStencilRenderTarget(Serial serial,
// TODO(jmadill): Layout transition?
mRenderPassDesc.packDepthStencilAttachment(*depthStencilRenderTarget->format,
depthStencilRenderTarget->samples);
depthStencilRenderTarget->resource->setWriteNode(this, serial);
depthStencilRenderTarget->resource->onWriteResource(this, serial);
}
void CommandBufferNode::initAttachmentDesc(VkAttachmentDescription *desc)
......@@ -161,58 +179,69 @@ void CommandBufferNode::initAttachmentDesc(VkAttachmentDescription *desc)
desc->finalLayout = VK_IMAGE_LAYOUT_UNDEFINED;
}
void CommandBufferNode::addDependency(CommandBufferNode *node)
// static
void CommandBufferNode::SetHappensBeforeDependency(CommandBufferNode *beforeNode,
CommandBufferNode *afterNode)
{
mDependencies.emplace_back(node);
node->markAsDependency();
ASSERT(node != this && !node->hasDependency(this));
afterNode->mHappensBeforeDependencies.emplace_back(beforeNode);
beforeNode->setHasHappensAfterDependencies();
beforeNode->finishRecording();
ASSERT(beforeNode != afterNode && !beforeNode->happensAfter(afterNode));
}
void CommandBufferNode::addDependencies(const std::vector<CommandBufferNode *> &nodes)
// static
void CommandBufferNode::SetHappensBeforeDependencies(
const std::vector<CommandBufferNode *> &beforeNodes,
CommandBufferNode *afterNode)
{
mDependencies.insert(mDependencies.end(), nodes.begin(), nodes.end());
afterNode->mHappensBeforeDependencies.insert(afterNode->mHappensBeforeDependencies.end(),
beforeNodes.begin(), beforeNodes.end());
// TODO(jmadill): is there a faster way to do this?
for (CommandBufferNode *node : nodes)
for (CommandBufferNode *beforeNode : beforeNodes)
{
node->markAsDependency();
ASSERT(node != this && !node->hasDependency(this));
beforeNode->setHasHappensAfterDependencies();
beforeNode->finishRecording();
ASSERT(beforeNode != afterNode && !beforeNode->happensAfter(afterNode));
}
}
bool CommandBufferNode::hasDependencies() const
bool CommandBufferNode::hasHappensBeforeDependencies() const
{
return !mDependencies.empty();
return !mHappensBeforeDependencies.empty();
}
void CommandBufferNode::markAsDependency()
void CommandBufferNode::setHasHappensAfterDependencies()
{
mIsDependency = true;
mHasHappensAfterDependencies = true;
}
bool CommandBufferNode::isDependency() const
bool CommandBufferNode::hasHappensAfterDependencies() const
{
return mIsDependency;
return mHasHappensAfterDependencies;
}
// Do not call this in anything but testing code, since it's slow.
bool CommandBufferNode::hasDependency(CommandBufferNode *searchNode)
bool CommandBufferNode::happensAfter(CommandBufferNode *beforeNode)
{
std::set<CommandBufferNode *> visitedList;
std::vector<CommandBufferNode *> openList;
openList.insert(openList.begin(), mDependencies.begin(), mDependencies.end());
openList.insert(openList.begin(), mHappensBeforeDependencies.begin(),
mHappensBeforeDependencies.end());
while (!openList.empty())
{
CommandBufferNode *node = openList.back();
CommandBufferNode *checkNode = openList.back();
openList.pop_back();
if (visitedList.count(node) == 0)
if (visitedList.count(checkNode) == 0)
{
if (node == searchNode)
if (checkNode == beforeNode)
{
return true;
}
visitedList.insert(node);
openList.insert(openList.end(), node->mDependencies.begin(), node->mDependencies.end());
visitedList.insert(checkNode);
openList.insert(openList.end(), checkNode->mHappensBeforeDependencies.begin(),
checkNode->mHappensBeforeDependencies.end());
}
}
......@@ -227,7 +256,8 @@ VisitedState CommandBufferNode::visitedState() const
void CommandBufferNode::visitDependencies(std::vector<CommandBufferNode *> *stack)
{
ASSERT(mVisitedState == VisitedState::Unvisited);
stack->insert(stack->end(), mDependencies.begin(), mDependencies.end());
stack->insert(stack->end(), mHappensBeforeDependencies.begin(),
mHappensBeforeDependencies.end());
mVisitedState = VisitedState::Ready;
}
......
......@@ -43,6 +43,9 @@ class CommandBufferNode final : angle::NonCopyable
// For rendering commands (draws).
Error startRenderPassRecording(RendererVk *renderer, CommandBuffer **commandsOut);
bool isFinishedRecording() const;
void finishRecording();
// Commands for storing info relevant to the RenderPass.
// RenderTargets must be added in order, with the depth/stencil being added last.
void storeRenderPassInfo(const Framebuffer &framebuffer,
......@@ -52,13 +55,15 @@ class CommandBufferNode final : angle::NonCopyable
void appendDepthStencilRenderTarget(Serial serial, RenderTargetVk *depthStencilRenderTarget);
// Commands for linking nodes in the dependency graph.
void addDependency(CommandBufferNode *node);
void addDependencies(const std::vector<CommandBufferNode *> &nodes);
bool hasDependencies() const;
bool isDependency() const;
static void SetHappensBeforeDependency(CommandBufferNode *beforeNode,
CommandBufferNode *afterNode);
static void SetHappensBeforeDependencies(const std::vector<CommandBufferNode *> &beforeNodes,
CommandBufferNode *afterNode);
bool hasHappensBeforeDependencies() const;
bool hasHappensAfterDependencies() const;
// Used for testing only.
bool hasDependency(CommandBufferNode *searchNode);
bool happensAfter(CommandBufferNode *beforeNode);
// Commands for traversing the node on a flush operation.
VisitedState visitedState() const;
......@@ -67,7 +72,7 @@ class CommandBufferNode final : angle::NonCopyable
private:
void initAttachmentDesc(VkAttachmentDescription *desc);
void markAsDependency();
void setHasHappensAfterDependencies();
// Only used if we need a RenderPass for these commands.
RenderPassDesc mRenderPassDesc;
......@@ -80,12 +85,17 @@ class CommandBufferNode final : angle::NonCopyable
CommandBuffer mOutsideRenderPassCommands;
CommandBuffer mInsideRenderPassCommands;
// Dependency commands must finish before these command can execute.
std::vector<CommandBufferNode *> mDependencies;
bool mIsDependency;
// These commands must be submitted before 'this' command can be submitted correctly.
std::vector<CommandBufferNode *> mHappensBeforeDependencies;
// If this is true, other commands exist that must be submitted after 'this' command.
bool mHasHappensAfterDependencies;
// Used when traversing the dependency graph.
VisitedState mVisitedState;
// Is recording currently enabled?
bool mIsFinishedRecording;
};
} // namespace vk
} // namespace rx
......
......@@ -226,7 +226,7 @@ gl::Error ContextVk::setupDraw(const gl::Context *context,
ASSERT(texture);
TextureVk *textureVk = vk::GetImpl(texture);
textureVk->setReadNode(renderNode, mRenderer->getCurrentQueueSerial());
textureVk->onReadResource(renderNode, mRenderer->getCurrentQueueSerial());
}
}
......
......@@ -153,7 +153,7 @@ gl::Error FramebufferVk::clear(const gl::Context *context, GLbitfield mask)
RendererVk *renderer = vk::GetImpl(context)->getRenderer();
vk::CommandBuffer *commandBuffer = nullptr;
ANGLE_TRY(recordWriteCommands(renderer, &commandBuffer));
ANGLE_TRY(beginWriteOperation(renderer, &commandBuffer));
Serial currentSerial = renderer->getCurrentQueueSerial();
......@@ -164,7 +164,8 @@ gl::Error FramebufferVk::clear(const gl::Context *context, GLbitfield mask)
RenderTargetVk *renderTarget = nullptr;
ANGLE_TRY(colorAttachment.getRenderTarget(context, &renderTarget));
renderTarget->resource->setWriteNode(getCurrentWriteNode(currentSerial), currentSerial);
renderTarget->resource->onWriteResource(getCurrentWriteOperation(currentSerial),
currentSerial);
renderTarget->image->changeLayoutWithStages(
VK_IMAGE_ASPECT_COLOR_BIT, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL,
......@@ -267,7 +268,7 @@ gl::Error FramebufferVk::readPixels(const gl::Context *context,
renderTarget->extents, vk::StagingUsage::Read));
vk::CommandBuffer *commandBuffer = nullptr;
ANGLE_TRY(recordWriteCommands(renderer, &commandBuffer));
ANGLE_TRY(beginWriteOperation(renderer, &commandBuffer));
stagingImage.getImage().changeLayoutTop(VK_IMAGE_ASPECT_COLOR_BIT, VK_IMAGE_LAYOUT_GENERAL,
commandBuffer);
......@@ -478,9 +479,9 @@ gl::Error FramebufferVk::getRenderNode(const gl::Context *context, vk::CommandBu
RendererVk *renderer = contextVk->getRenderer();
Serial currentSerial = renderer->getCurrentQueueSerial();
if (isCurrentlyRecording(currentSerial) && mLastRenderNodeSerial == currentSerial)
if (hasCurrentWriteOperation(currentSerial) && mLastRenderNodeSerial == currentSerial)
{
*nodeOut = getCurrentWriteNode(currentSerial);
*nodeOut = getCurrentWriteOperation(currentSerial);
ASSERT((*nodeOut)->getInsideRenderPassCommands()->valid());
return gl::NoError();
}
......
......@@ -793,7 +793,7 @@ vk::Error RendererVk::flushCommandGraph(const gl::Context *context, vk::CommandB
{
// Only process commands that don't have child commands. The others will be pulled in
// automatically. Also skip commands that have already been visited.
if (topLevelNode->isDependency() ||
if (topLevelNode->hasHappensAfterDependencies() ||
topLevelNode->visitedState() != vk::VisitedState::Unvisited)
continue;
......
......@@ -364,7 +364,7 @@ vk::Error WindowSurfaceVk::initializeImpl(RendererVk *renderer)
// Allocate a command buffer for clearing our images to black.
vk::CommandBuffer *commandBuffer = nullptr;
ANGLE_TRY(recordWriteCommands(renderer, &commandBuffer));
ANGLE_TRY(beginWriteOperation(renderer, &commandBuffer));
VkClearColorValue transparentBlack;
transparentBlack.float32[0] = 0.0f;
......@@ -429,7 +429,7 @@ egl::Error WindowSurfaceVk::swap(const gl::Context *context)
RendererVk *renderer = displayVk->getRenderer();
vk::CommandBuffer *swapCommands = nullptr;
ANGLE_TRY(recordWriteCommands(renderer, &swapCommands));
ANGLE_TRY(beginWriteOperation(renderer, &swapCommands));
auto &image = mSwapchainImages[mCurrentSwapchainImageIndex];
......
......@@ -254,7 +254,7 @@ gl::Error TextureVk::setSubImageImpl(ContextVk *contextVk,
stagingImage.getDeviceMemory().unmap(device);
vk::CommandBuffer *commandBuffer = nullptr;
ANGLE_TRY(recordWriteCommands(renderer, &commandBuffer));
ANGLE_TRY(beginWriteOperation(renderer, &commandBuffer));
stagingImage.getImage().changeLayoutWithStages(
VK_IMAGE_ASPECT_COLOR_BIT, VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL,
......
......@@ -116,14 +116,14 @@ void VertexArrayVk::updateDrawDependencies(vk::CommandBufferNode *readNode,
for (auto attribIndex : activeAttribsMask)
{
ASSERT(mCurrentArrayBufferResources[attribIndex]);
mCurrentArrayBufferResources[attribIndex]->setReadNode(readNode, serial);
mCurrentArrayBufferResources[attribIndex]->onReadResource(readNode, serial);
}
// Handle the bound element array buffer.
if (drawType == DrawType::Elements)
{
ASSERT(mCurrentElementArrayBufferResource);
mCurrentElementArrayBufferResource->setReadNode(readNode, serial);
mCurrentElementArrayBufferResource->onReadResource(readNode, serial);
}
}
......
......@@ -1346,7 +1346,7 @@ VkFrontFace GetFrontFace(GLenum frontFace)
} // namespace gl_vk
ResourceVk::ResourceVk() : mCurrentWriteNode(nullptr)
ResourceVk::ResourceVk() : mCurrentWriteOperation(nullptr)
{
}
......@@ -1360,8 +1360,8 @@ void ResourceVk::updateQueueSerial(Serial queueSerial)
if (queueSerial > mStoredQueueSerial)
{
mCurrentWriteNode = nullptr;
mCurrentReadNodes.clear();
mCurrentWriteOperation = nullptr;
mCurrentReadOperations.clear();
mStoredQueueSerial = queueSerial;
}
}
......@@ -1371,25 +1371,26 @@ Serial ResourceVk::getQueueSerial() const
return mStoredQueueSerial;
}
bool ResourceVk::isCurrentlyRecording(Serial currentSerial) const
bool ResourceVk::hasCurrentWriteOperation(Serial currentSerial) const
{
return (mStoredQueueSerial == currentSerial && mCurrentWriteNode != nullptr);
return (mStoredQueueSerial == currentSerial && mCurrentWriteOperation != nullptr &&
!mCurrentWriteOperation->isFinishedRecording());
}
vk::CommandBufferNode *ResourceVk::getCurrentWriteNode(Serial currentSerial)
vk::CommandBufferNode *ResourceVk::getCurrentWriteOperation(Serial currentSerial)
{
ASSERT(currentSerial == mStoredQueueSerial);
return mCurrentWriteNode;
return mCurrentWriteOperation;
}
vk::CommandBufferNode *ResourceVk::getNewWriteNode(RendererVk *renderer)
{
vk::CommandBufferNode *newCommands = renderer->allocateCommandNode();
setWriteNode(newCommands, renderer->getCurrentQueueSerial());
onWriteResource(newCommands, renderer->getCurrentQueueSerial());
return newCommands;
}
vk::Error ResourceVk::recordWriteCommands(RendererVk *renderer,
vk::Error ResourceVk::beginWriteOperation(RendererVk *renderer,
vk::CommandBuffer **commandBufferOut)
{
vk::CommandBufferNode *commands = getNewWriteNode(renderer);
......@@ -1399,31 +1400,32 @@ vk::Error ResourceVk::recordWriteCommands(RendererVk *renderer,
return vk::NoError();
}
void ResourceVk::setWriteNode(vk::CommandBufferNode *writeNode, Serial serial)
void ResourceVk::onWriteResource(vk::CommandBufferNode *writeOperation, Serial serial)
{
updateQueueSerial(serial);
// Make sure any open reads and writes finish before we execute |newCommands|.
if (!mCurrentReadNodes.empty())
// Make sure any open reads and writes finish before we execute 'newCommands'.
if (!mCurrentReadOperations.empty())
{
writeNode->addDependencies(mCurrentReadNodes);
mCurrentReadNodes.clear();
vk::CommandBufferNode::SetHappensBeforeDependencies(mCurrentReadOperations, writeOperation);
mCurrentReadOperations.clear();
}
if (mCurrentWriteNode)
if (mCurrentWriteOperation)
{
writeNode->addDependency(mCurrentWriteNode);
vk::CommandBufferNode::SetHappensBeforeDependency(mCurrentWriteOperation, writeOperation);
}
mCurrentWriteNode = writeNode;
mCurrentWriteOperation = writeOperation;
}
void ResourceVk::setReadNode(vk::CommandBufferNode *readNode, Serial serial)
void ResourceVk::onReadResource(vk::CommandBufferNode *readOperation, Serial serial)
{
if (isCurrentlyRecording(serial))
if (hasCurrentWriteOperation(serial))
{
// Link the current write node to "readNode".
readNode->addDependency(getCurrentWriteNode(serial));
// Ensure 'readOperation' happens after the current write commands.
vk::CommandBufferNode::SetHappensBeforeDependency(getCurrentWriteOperation(serial),
readOperation);
ASSERT(mStoredQueueSerial == serial);
}
else
......@@ -1431,8 +1433,8 @@ void ResourceVk::setReadNode(vk::CommandBufferNode *readNode, Serial serial)
updateQueueSerial(serial);
}
// Track "readNode" in this resource.
mCurrentReadNodes.push_back(readNode);
// Add the read operation to the list of nodes currently reading this resource.
mCurrentReadOperations.push_back(readOperation);
}
} // namespace rx
......
......@@ -684,29 +684,29 @@ class ResourceVk
void updateQueueSerial(Serial queueSerial);
Serial getQueueSerial() const;
// Returns true if any tracked read or write nodes match |currentSerial|.
bool isCurrentlyRecording(Serial currentSerial) const;
// Returns true if any tracked read or write nodes match 'currentSerial'.
bool hasCurrentWriteOperation(Serial currentSerial) const;
// Returns the active write node, and asserts |currentSerial| matches the stored serial.
vk::CommandBufferNode *getCurrentWriteNode(Serial currentSerial);
// Returns the active write node, and asserts 'currentSerial' matches the stored serial.
vk::CommandBufferNode *getCurrentWriteOperation(Serial currentSerial);
// Allocates a new write node and calls setWriteNode internally.
// Allocates a new write node and calls onWriteResource internally.
vk::CommandBufferNode *getNewWriteNode(RendererVk *renderer);
// Allocates a write node via getNewWriteNode and returns a started command buffer.
// The started command buffer will render outside of a RenderPass.
vk::Error recordWriteCommands(RendererVk *renderer, vk::CommandBuffer **commandBufferOut);
vk::Error beginWriteOperation(RendererVk *renderer, vk::CommandBuffer **commandBufferOut);
// Called on an operation that will modify this ResourceVk.
void setWriteNode(vk::CommandBufferNode *newCommands, Serial serial);
void onWriteResource(vk::CommandBufferNode *writeOperation, Serial serial);
// Sets up the dependency relations. |readNode| has the commands that read from this object.
void setReadNode(vk::CommandBufferNode *readNode, Serial serial);
// Sets up dependency relations. 'readOperation' has the commands that read from this object.
void onReadResource(vk::CommandBufferNode *readOperation, Serial serial);
private:
Serial mStoredQueueSerial;
std::vector<vk::CommandBufferNode *> mCurrentReadNodes;
vk::CommandBufferNode *mCurrentWriteNode;
std::vector<vk::CommandBufferNode *> mCurrentReadOperations;
vk::CommandBufferNode *mCurrentWriteOperation;
};
} // namespace rx
......
......@@ -252,6 +252,19 @@ TEST_P(SimpleOperationTest, DrawQuad)
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
}
// Simple double quad test.
TEST_P(SimpleOperationTest, DrawQuadTwice)
{
ANGLE_GL_PROGRAM(program, kBasicVertexShader, kGreenFragmentShader);
drawQuad(program.get(), "position", 0.5f, 1.0f, true);
drawQuad(program.get(), "position", 0.5f, 1.0f, true);
ASSERT_GL_NO_ERROR();
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
}
// Simple line test.
TEST_P(SimpleOperationTest, DrawLine)
{
......
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