Skip to content

A
angle
Commit 2f3d18f2 by Shahbaz Youssefi Committed by Commit Bot
Options

Vulkan: Fix unresolve disagreement between FB and RP

FramebufferVk::updateRenderPass reset the render pass description, but not the framebuffer description. This caused a disagreement between the two regarding which attachments need to be unresolved. Later, FramebufferVk::startNewRenderPass could miscalculate whether a new framebuffer needs to be generated based on changes in unresolve attachments. For example: - say in the first render pass color needs to be unresolved. Both RP and FB desc would remember this (each being keys for their respective caches). - A following operation triggers syncState such that FB desc changes (for example rebind of attachment), which cleared RP desc but not FB desc's unresolve attachment state. - If the next render pass does not require an unresolve (for example due to a clear or invalidate), then the framebuffer is not recreated because according to RP desc at the start and end of FramebufferVk::startNewRenderPass there has been no change in unresolve mask. * At start there's no unresolve because of syncState clearing it. * At end there's no unresolve because there's no need for unresolve. - In the end, the framebuffer used for the first render pass would be used for the second render pass as well. Note that: * The first render pass included an unresolve, i.e. two subpasses. * The second render pass requires one subpass according to its RP desc. It's quite easy to accidentally have the framebuffer correctly recreated (based on the reset RP desc) before FramebufferVk::startNewRenderPass. Note that since syncState has called updateRenderPass, FB desc has necessarily changed, and mFramebuffer is nullptr, so any call to FramebufferVk::getFramebuffer would recreate the framebuffer. Both clear and invalidate call FramebufferVk::getFramebuffer. The issue is reproducible in situations where clear/invalidate has been called before the framebuffer is modified, and then draw is issued after the modification. An ASSERT is added to catch discrepencies between RP desc and FB desc to catch bugs even when the issue doesn't manifest itself as a VVL error. Bug: angleproject:4836 Change-Id: I8a0d116402a6c298377d03e0908baa942019ccd5 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2442379Reviewed-by: 's avatarJamie Madill <jmadill@chromium.org> Reviewed-by: 's avatarTim Van Patten <timvp@google.com> Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
parent 527064f1
Showing with 30 additions and 12 deletions
src/libANGLE/renderer/vulkan/FramebufferVk.cpp
...@@ -271,6 +271,21 @@ bool HasResolveAttachment(const gl::AttachmentArray<RenderTargetVk *> &colorRend ...@@ -271,6 +271,21 @@ bool HasResolveAttachment(const gl::AttachmentArray<RenderTargetVk *> &colorRend
} }
return false; return false;
} }
vk::FramebufferNonResolveAttachmentMask MakeUnresolveAttachmentMask(const vk::RenderPassDesc &desc)
{
vk::FramebufferNonResolveAttachmentMask unresolveMask(
desc.getColorUnresolveAttachmentMask().bits());
if (desc.hasDepthUnresolveAttachment())
{
unresolveMask.set(vk::kUnpackedDepthIndex);
}
if (desc.hasStencilUnresolveAttachment())
{
unresolveMask.set(vk::kUnpackedStencilIndex);
}
return unresolveMask;
}
} // anonymous namespace } // anonymous namespace
// static // static
...@@ -1821,6 +1836,8 @@ void FramebufferVk::updateRenderPassDesc() ...@@ -1821,6 +1836,8 @@ void FramebufferVk::updateRenderPassDesc()
mRenderPassDesc.packDepthStencilResolveAttachment(hasDepth, hasStencil); mRenderPassDesc.packDepthStencilResolveAttachment(hasDepth, hasStencil);
} }
} }
mCurrentFramebufferDesc.updateUnresolveMask({});
} }
angle::Result FramebufferVk::getFramebuffer(ContextVk *contextVk, angle::Result FramebufferVk::getFramebuffer(ContextVk *contextVk,
...@@ -2248,6 +2265,10 @@ angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk, ...@@ -2248,6 +2265,10 @@ angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk,
bool previousUnresolveDepth = mRenderPassDesc.hasDepthUnresolveAttachment(); bool previousUnresolveDepth = mRenderPassDesc.hasDepthUnresolveAttachment();
bool previousUnresolveStencil = mRenderPassDesc.hasStencilUnresolveAttachment(); bool previousUnresolveStencil = mRenderPassDesc.hasStencilUnresolveAttachment();
// Make sure render pass and framebuffer are in agreement w.r.t unresolve attachments.
ASSERT(mCurrentFramebufferDesc.getUnresolveAttachmentMask() ==
MakeUnresolveAttachmentMask(mRenderPassDesc));
// Color attachments. // Color attachments.
const auto &colorRenderTargets = mRenderTargetCache.getColors(); const auto &colorRenderTargets = mRenderTargetCache.getColors();
vk::PackedAttachmentIndex colorIndexVk(0); vk::PackedAttachmentIndex colorIndexVk(0);
...@@ -2471,17 +2492,7 @@ angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk, ...@@ -2471,17 +2492,7 @@ angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk,
// Make sure framebuffer is recreated. // Make sure framebuffer is recreated.
mFramebuffer = nullptr; mFramebuffer = nullptr;
vk::FramebufferNonResolveAttachmentMask unresolveMask(unresolveColorMask.bits()); mCurrentFramebufferDesc.updateUnresolveMask(MakeUnresolveAttachmentMask(mRenderPassDesc));
if (unresolveDepth)
{
unresolveMask.set(vk::kUnpackedDepthIndex);
}
if (unresolveStencil)
{
unresolveMask.set(vk::kUnpackedStencilIndex);
}
mCurrentFramebufferDesc.updateUnresolveMask(unresolveMask);
} }
vk::Framebuffer *framebuffer = nullptr; vk::Framebuffer *framebuffer = nullptr;
......
src/libANGLE/renderer/vulkan/vk_cache_utils.cpp
...@@ -2649,6 +2649,11 @@ uint32_t FramebufferDesc::attachmentCount() const ...@@ -2649,6 +2649,11 @@ uint32_t FramebufferDesc::attachmentCount() const
return count; return count;
} }
FramebufferNonResolveAttachmentMask FramebufferDesc::getUnresolveAttachmentMask() const
{
return mUnresolveAttachmentMask;
}
// SamplerDesc implementation. // SamplerDesc implementation.
SamplerDesc::SamplerDesc() SamplerDesc::SamplerDesc()
{ {
......
src/libANGLE/renderer/vulkan/vk_cache_utils.h
...@@ -1086,7 +1086,6 @@ class FramebufferDesc ...@@ -1086,7 +1086,6 @@ class FramebufferDesc
void updateDepthStencil(ImageViewSubresourceSerial serial); void updateDepthStencil(ImageViewSubresourceSerial serial);
void updateDepthStencilResolve(ImageViewSubresourceSerial serial); void updateDepthStencilResolve(ImageViewSubresourceSerial serial);
size_t hash() const; size_t hash() const;
void reset();
bool operator==(const FramebufferDesc &other) const; bool operator==(const FramebufferDesc &other) const;
...@@ -1098,7 +1097,10 @@ class FramebufferDesc ...@@ -1098,7 +1097,10 @@ class FramebufferDesc
return mSerials[kFramebufferDescColorIndexOffset + index]; return mSerials[kFramebufferDescColorIndexOffset + index];
} }
FramebufferNonResolveAttachmentMask getUnresolveAttachmentMask() const;
private: private:
void reset();
void update(uint32_t index, ImageViewSubresourceSerial serial); void update(uint32_t index, ImageViewSubresourceSerial serial);
// Note: this is an exclusive index. If there is one index it will be "1". // Note: this is an exclusive index. If there is one index it will be "1".
......
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