Vulkan: Simplify present history logic
Instead of marking `ImagePresentOperation` as corresponding to an older
swapchain, accumulate all present history in the `mOldSwapchains`.
Keeping old Swapchain's `ImagePresentOperation` with Fences can lead to
multiple "old swapchains" lists. This may cause situations exceeding
the `kMaxOldSwapchains` limit.
Keeping items with Fences may help cleanup resources faster, but I think
that it is not worth the code complexity. It also should be safe to
recycle Fences at the same time as the Semaphores (when the first image
is recycled).
Also this CL may fix bug:
if (mPresentHistory.empty() || mPresentHistory.back().imageIndex == kInvalidImageIndex)
The above condition will always pass when using actual present Fences
(VK_STRUCTURE_TYPE_SWAPCHAIN_PRESENT_FENCE_INFO_EXT).
Bug: angleproject:7847
Change-Id: I39d854b03733dcb976c6bd7eb5f848d0cbece5d6
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4481251
Reviewed-by: Charlie Lao <cclao@google.com>
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Commit-Queue: Igor Nazarov <i.nazarov@samsung.com>
diff --git a/src/libANGLE/renderer/vulkan/SurfaceVk.cpp b/src/libANGLE/renderer/vulkan/SurfaceVk.cpp
index 866869d..cddf1f2 100644
--- a/src/libANGLE/renderer/vulkan/SurfaceVk.cpp
+++ b/src/libANGLE/renderer/vulkan/SurfaceVk.cpp
@@ -34,8 +34,8 @@
// the VkSurfaceCapabilitiesKHR spec for more details.
constexpr uint32_t kSurfaceSizedBySwapchain = 0xFFFFFFFFu;
-// Special value for ImagePresentOperation::imageIndex meaning that it corresponds to an older
-// swapchain present operation and so the index is no longer relevant.
+// Special value for ImagePresentOperation::imageIndex meaning that using actual Present Fence
+// (not inferred from Acquire - VK_STRUCTURE_TYPE_SWAPCHAIN_PRESENT_FENCE_INFO_EXT).
constexpr uint32_t kInvalidImageIndex = std::numeric_limits<uint32_t>::max();
GLint GetSampleCount(const egl::Config *config)
@@ -394,21 +394,14 @@
vk::Fence &&presentFence,
std::deque<impl::ImagePresentOperation> *presentHistory)
{
- // The history looks like this:
- //
- // <entries for old swapchains, imageIndex == UINT32_MAX> <entries for this swapchain>
- //
// Walk the list backwards and find the entry for the given image index. That's the last
// present with that image. Associate the fence with that present operation.
for (size_t historyIndex = 0; historyIndex < presentHistory->size(); ++historyIndex)
{
impl::ImagePresentOperation &presentOperation =
(*presentHistory)[presentHistory->size() - historyIndex - 1];
- if (presentOperation.imageIndex == kInvalidImageIndex)
- {
- // No previous presentation with this index.
- break;
- }
+ // Must not use this function if using actual Present Fence (not inferred from Acquire).
+ ASSERT(presentOperation.imageIndex != kInvalidImageIndex);
if (presentOperation.imageIndex == imageIndex)
{
@@ -767,28 +760,44 @@
SwapchainCleanupData::~SwapchainCleanupData()
{
ASSERT(swapchain == VK_NULL_HANDLE);
+ ASSERT(fences.empty());
ASSERT(semaphores.empty());
}
SwapchainCleanupData::SwapchainCleanupData(SwapchainCleanupData &&other)
- : swapchain(other.swapchain), semaphores(std::move(other.semaphores))
+ : swapchain(other.swapchain),
+ fences(std::move(other.fences)),
+ semaphores(std::move(other.semaphores))
{
other.swapchain = VK_NULL_HANDLE;
}
-void SwapchainCleanupData::destroy(VkDevice device, vk::Recycler<vk::Semaphore> *semaphoreRecycler)
+void SwapchainCleanupData::destroy(VkDevice device,
+ vk::Recycler<vk::Fence> *fenceRecycler,
+ vk::Recycler<vk::Semaphore> *semaphoreRecycler)
{
- if (swapchain)
+ // Check status and recycle fences before destroying the Swapchain.
+ // There is a bug on some Intel drivers, that causes crash if check Fence status after Swapchain
+ // destruction. Resetting Fence after Swapchain destruction works as expected.
+ for (vk::Fence &fence : fences)
{
- vkDestroySwapchainKHR(device, swapchain, nullptr);
- swapchain = VK_NULL_HANDLE;
+ ASSERT(fence.getStatus(device) == VK_SUCCESS);
+ fenceRecycler->recycle(std::move(fence));
}
+ fences.clear();
for (vk::Semaphore &semaphore : semaphores)
{
semaphoreRecycler->recycle(std::move(semaphore));
}
semaphores.clear();
+
+ // Destroy Swapchain last to mitigate the Intel driver bug.
+ if (swapchain)
+ {
+ vkDestroySwapchainKHR(device, swapchain, nullptr);
+ swapchain = VK_NULL_HANDLE;
+ }
}
ImagePresentOperation::ImagePresentOperation() : imageIndex(kInvalidImageIndex) {}
@@ -831,7 +840,7 @@
// Destroy old swapchains
for (SwapchainCleanupData &oldSwapchain : oldSwapchains)
{
- oldSwapchain.destroy(device, semaphoreRecycler);
+ oldSwapchain.destroy(device, fenceRecycler, semaphoreRecycler);
}
oldSwapchains.clear();
}
@@ -927,7 +936,7 @@
}
for (SwapchainCleanupData &oldSwapchain : mOldSwapchains)
{
- oldSwapchain.destroy(device, &mPresentSemaphoreRecycler);
+ oldSwapchain.destroy(device, &mPresentFenceRecycler, &mPresentSemaphoreRecycler);
}
mOldSwapchains.clear();
@@ -1249,15 +1258,14 @@
// The old(er) swapchains still need to be kept to be scheduled for destruction.
VkSwapchainKHR swapchainToDestroy = VK_NULL_HANDLE;
- if (mPresentHistory.empty() || mPresentHistory.back().imageIndex == kInvalidImageIndex)
+ if (mPresentHistory.empty())
{
// Destroy the current (never-used) swapchain.
swapchainToDestroy = mSwapchain;
}
- // Place any present operation that's not associated with a fence into mOldSwapchains. That
- // gets scheduled for destruction when the semaphore of the first image of the next swapchain
- // can be recycled.
+ // Place all present operation into mOldSwapchains. That gets scheduled for destruction when the
+ // semaphore of the first image of the next swapchain can be recycled.
SwapchainCleanupData cleanupData;
// If the swapchain is not being immediately destroyed, schedule it for destruction.
@@ -1266,45 +1274,26 @@
cleanupData.swapchain = mSwapchain;
}
- std::vector<impl::ImagePresentOperation> historyToKeep;
- while (!mPresentHistory.empty())
+ for (impl::ImagePresentOperation &presentOperation : mPresentHistory)
{
- impl::ImagePresentOperation &presentOperation = mPresentHistory.back();
-
- // If this is about an older swapchain, let it be.
- if (presentOperation.imageIndex == kInvalidImageIndex)
- {
- ASSERT(presentOperation.fence.valid());
- break;
- }
-
- // Reset the index, so it's not processed in the future.
- presentOperation.imageIndex = kInvalidImageIndex;
-
if (presentOperation.fence.valid())
{
- // If there is already a fence associated with it, let it be cleaned up once the fence
- // is signaled.
- historyToKeep.push_back(std::move(presentOperation));
+ cleanupData.fences.emplace_back(std::move(presentOperation.fence));
}
- else
+
+ if (presentOperation.semaphore.valid())
{
- ASSERT(presentOperation.semaphore.valid());
-
- // Otherwise accumulate it in mOldSwapchains.
cleanupData.semaphores.emplace_back(std::move(presentOperation.semaphore));
-
- // Accumulate any previous swapchains that are pending destruction too.
- for (SwapchainCleanupData &oldSwapchain : presentOperation.oldSwapchains)
- {
- mOldSwapchains.emplace_back(std::move(oldSwapchain));
- }
- presentOperation.oldSwapchains.clear();
}
- mPresentHistory.pop_back();
+ // Accumulate any previous swapchains that are pending destruction too.
+ for (SwapchainCleanupData &oldSwapchain : presentOperation.oldSwapchains)
+ {
+ mOldSwapchains.emplace_back(std::move(oldSwapchain));
+ }
+ presentOperation.oldSwapchains.clear();
}
- std::move(historyToKeep.begin(), historyToKeep.end(), std::back_inserter(mPresentHistory));
+ mPresentHistory.clear();
// If too many old swapchains have accumulated, wait idle and destroy them. This is to prevent
// failures due to too many swapchains allocated.
@@ -1318,12 +1307,14 @@
ANGLE_TRY(finish(contextVk));
for (SwapchainCleanupData &oldSwapchain : mOldSwapchains)
{
- oldSwapchain.destroy(contextVk->getDevice(), &mPresentSemaphoreRecycler);
+ oldSwapchain.destroy(contextVk->getDevice(), &mPresentFenceRecycler,
+ &mPresentSemaphoreRecycler);
}
mOldSwapchains.clear();
}
- if (cleanupData.swapchain != VK_NULL_HANDLE || !cleanupData.semaphores.empty())
+ if (cleanupData.swapchain != VK_NULL_HANDLE || !cleanupData.fences.empty() ||
+ !cleanupData.semaphores.empty())
{
mOldSwapchains.emplace_back(std::move(cleanupData));
}
@@ -2146,7 +2137,8 @@
// If there is no fence associated with the history, it can't be cleaned up yet.
if (!presentOperation.fence.valid())
{
- // Can't have an old present operations without a fence.
+ // |kInvalidImageIndex| is only possible when |VkSwapchainPresentFenceInfoEXT| is used,
+ // in which case |fence| is always valid.
ASSERT(presentOperation.imageIndex != kInvalidImageIndex);
break;
}
@@ -2175,7 +2167,8 @@
impl::ImagePresentOperation presentOperation = std::move(mPresentHistory.front());
mPresentHistory.pop_front();
- // We can't be stuck on a presentation to an old swapchain without a fence.
+ // |kInvalidImageIndex| is only possible when |VkSwapchainPresentFenceInfoEXT| is used, in
+ // which case |fence| is always valid.
ASSERT(presentOperation.imageIndex != kInvalidImageIndex);
// Move clean up data to the next (now first) present operation, if any. Note that there
diff --git a/src/libANGLE/renderer/vulkan/SurfaceVk.h b/src/libANGLE/renderer/vulkan/SurfaceVk.h
index d54682e..1a21811 100644
--- a/src/libANGLE/renderer/vulkan/SurfaceVk.h
+++ b/src/libANGLE/renderer/vulkan/SurfaceVk.h
@@ -133,7 +133,7 @@
{
static constexpr size_t kSwapHistorySize = 2;
-// Old swapchain and associated present semaphores that need to be scheduled for
+// Old swapchain and associated present fences/semaphores that need to be scheduled for
// recycling/destruction when appropriate.
struct SwapchainCleanupData : angle::NonCopyable
{
@@ -141,12 +141,15 @@
SwapchainCleanupData(SwapchainCleanupData &&other);
~SwapchainCleanupData();
- void destroy(VkDevice device, vk::Recycler<vk::Semaphore> *semaphoreRecycler);
+ void destroy(VkDevice device,
+ vk::Recycler<vk::Fence> *fenceRecycler,
+ vk::Recycler<vk::Semaphore> *semaphoreRecycler);
// The swapchain to be destroyed.
VkSwapchainKHR swapchain = VK_NULL_HANDLE;
- // Any present semaphores that were pending recycle at the time the swapchain was recreated will
- // be scheduled for recycling at the same time as the swapchain's destruction.
+ // Any present fences/semaphores that were pending recycle at the time the swapchain was
+ // recreated will be scheduled for recycling at the same time as the swapchain's destruction.
+ std::vector<vk::Fence> fences;
std::vector<vk::Semaphore> semaphores;
};
