mirror
/
electron
mirror of https://github.com/electron/electron
1
0
Fork 0
Code Issues Releases Activity
electron/patches/angle/cherry-pick-e4669a74888d.patch

396 lines
17 KiB
Diff
Raw Permalink Blame History

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Charlie Lao <cclao@google.com>
Date: Tue, 8 Aug 2023 10:14:47 -0700
Subject: M116: Vulkan: Fix data race with DynamicDescriptorPool
Right now DynamicDescriptorPool::destroyCachedDescriptorSet can be
called from garbage clean up thread, while simultaneously accessed from
context main thread, and data race will happen and cause bugs. This can
only happen when the buffer is not being suballocated. In this case,
suballocation owns the bufferBlock and bufferBlock gets destroyed when
suballocation is destroyed from garbage collection thread. If buffer is
suballocated, the shared group owns pool which owns bufferBlocks and
they gets destroyed from shared group with the share group lock. This CL
avoids this race problem by release the shared cacheKey when the buffer
is released, while we still had the shared group lock.
Bug: chromium:1469542
Change-Id: Ie6235fcfb77dee2a12b2ebde44042c3845fc0aca
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4790523
Reviewed-by: Yuly Novikov <ynovikov@chromium.org>
diff --git a/src/libANGLE/renderer/vulkan/BufferVk.cpp b/src/libANGLE/renderer/vulkan/BufferVk.cpp
index 0898e7d67f47b0d87476a70b9bede290d22ca87a..2957ddf5dede7ffcf7a918ba2f6c8235feb7357b 100644
--- a/src/libANGLE/renderer/vulkan/BufferVk.cpp
+++ b/src/libANGLE/renderer/vulkan/BufferVk.cpp
@@ -285,7 +285,7 @@ void BufferVk::release(ContextVk *contextVk)
RendererVk *renderer = contextVk->getRenderer();
if (mBuffer.valid())
{
- mBuffer.releaseBufferAndDescriptorSetCache(contextVk);
+ mBuffer.releaseBufferAndDescriptorSetCache(renderer);
}
if (mStagingBuffer.valid())
{
@@ -628,7 +628,7 @@ angle::Result BufferVk::ghostMappedBuffer(ContextVk *contextVk,
memcpy(dstMapPtr, srcMapPtr, static_cast<size_t>(mState.getSize()));
}
- src.releaseBufferAndDescriptorSetCache(contextVk);
+ src.releaseBufferAndDescriptorSetCache(contextVk->getRenderer());
// Return the already mapped pointer with the offset adjustment to avoid the call to unmap().
*mapPtr = dstMapPtr + offset;
@@ -966,7 +966,7 @@ angle::Result BufferVk::acquireAndUpdate(ContextVk *contextVk,
if (src.valid())
{
- src.releaseBufferAndDescriptorSetCache(contextVk);
+ src.releaseBufferAndDescriptorSetCache(contextVk->getRenderer());
}
return angle::Result::Continue;
@@ -1073,7 +1073,7 @@ angle::Result BufferVk::acquireBufferHelper(ContextVk *contextVk, size_t sizeInB
if (mBuffer.valid())
{
- mBuffer.releaseBufferAndDescriptorSetCache(contextVk);
+ mBuffer.releaseBufferAndDescriptorSetCache(renderer);
}
// Allocate the buffer directly
diff --git a/src/libANGLE/renderer/vulkan/Suballocation.h b/src/libANGLE/renderer/vulkan/Suballocation.h
index d25481bdbbb12d9377eb8ae540a8603a16e453b1..276f6b4c854df2a2a4a353f2893c4bc2df9e8502 100644
--- a/src/libANGLE/renderer/vulkan/Suballocation.h
+++ b/src/libANGLE/renderer/vulkan/Suballocation.h
@@ -86,6 +86,13 @@ class BufferBlock final : angle::NonCopyable
{
mDescriptorSetCacheManager.addKey(sharedCacheKey);
}
+ void releaseAllCachedDescriptorSetCacheKeys(RendererVk *renderer)
+ {
+ if (!mDescriptorSetCacheManager.empty())
+ {
+ mDescriptorSetCacheManager.releaseKeys(renderer);
+ }
+ }
private:
mutable std::mutex mVirtualBlockMutex;
diff --git a/src/libANGLE/renderer/vulkan/TextureVk.cpp b/src/libANGLE/renderer/vulkan/TextureVk.cpp
index 1375e39971faff88db94c09b50b8c0db761eaba2..903def6e88e8d0f3a16f1f891429b9e088191f32 100644
--- a/src/libANGLE/renderer/vulkan/TextureVk.cpp
+++ b/src/libANGLE/renderer/vulkan/TextureVk.cpp
@@ -1617,7 +1617,7 @@ void TextureVk::releaseAndDeleteImageAndViews(ContextVk *contextVk)
mBufferViews.release(contextVk);
mRedefinedLevels.reset();
- mDescriptorSetCacheManager.releaseKeys(contextVk);
+ mDescriptorSetCacheManager.releaseKeys(contextVk->getRenderer());
}
void TextureVk::initImageUsageFlags(ContextVk *contextVk, angle::FormatID actualFormatID)
@@ -2845,7 +2845,7 @@ angle::Result TextureVk::syncState(const gl::Context *context,
mBufferViews.release(contextVk);
mBufferViews.init(renderer, offset, size);
- mDescriptorSetCacheManager.releaseKeys(contextVk);
+ mDescriptorSetCacheManager.releaseKeys(renderer);
return angle::Result::Continue;
}
@@ -3285,7 +3285,7 @@ void TextureVk::releaseImageViews(ContextVk *contextVk)
{
RendererVk *renderer = contextVk->getRenderer();
- mDescriptorSetCacheManager.releaseKeys(contextVk);
+ mDescriptorSetCacheManager.releaseKeys(renderer);
if (mImage == nullptr)
{
diff --git a/src/libANGLE/renderer/vulkan/vk_cache_utils.cpp b/src/libANGLE/renderer/vulkan/vk_cache_utils.cpp
index abab85dcfb5da21630b9e8987543a09e7fb9e6c4..965e6ee4e1f8349c057e7fdbca7ebaa4593823ac 100644
--- a/src/libANGLE/renderer/vulkan/vk_cache_utils.cpp
+++ b/src/libANGLE/renderer/vulkan/vk_cache_utils.cpp
@@ -2588,11 +2588,19 @@ void ReleaseCachedObject(ContextVk *contextVk, const FramebufferDesc &desc)
{
contextVk->getShareGroup()->getFramebufferCache().erase(contextVk, desc);
}
+void ReleaseCachedObject(RendererVk *renderer, const FramebufferDesc &desc)
+{
+ UNREACHABLE();
+}
void ReleaseCachedObject(ContextVk *contextVk, const DescriptorSetDescAndPool &descAndPool)
+{
+ UNREACHABLE();
+}
+void ReleaseCachedObject(RendererVk *renderer, const DescriptorSetDescAndPool &descAndPool)
{
ASSERT(descAndPool.mPool != nullptr);
- descAndPool.mPool->releaseCachedDescriptorSet(contextVk, descAndPool.mDesc);
+ descAndPool.mPool->releaseCachedDescriptorSet(renderer, descAndPool.mDesc);
}
void DestroyCachedObject(RendererVk *renderer, const FramebufferDesc &desc)
@@ -6255,6 +6263,22 @@ void SharedCacheKeyManager<SharedCacheKeyT>::releaseKeys(ContextVk *contextVk)
mSharedCacheKeys.clear();
}
+template <class SharedCacheKeyT>
+void SharedCacheKeyManager<SharedCacheKeyT>::releaseKeys(RendererVk *renderer)
+{
+ for (SharedCacheKeyT &sharedCacheKey : mSharedCacheKeys)
+ {
+ if (*sharedCacheKey.get() != nullptr)
+ {
+ // Immediate destroy the cached object and the key itself when first releaseKeys call is
+ // made
+ ReleaseCachedObject(renderer, *(*sharedCacheKey.get()));
+ *sharedCacheKey.get() = nullptr;
+ }
+ }
+ mSharedCacheKeys.clear();
+}
+
template <class SharedCacheKeyT>
void SharedCacheKeyManager<SharedCacheKeyT>::destroyKeys(RendererVk *renderer)
{
diff --git a/src/libANGLE/renderer/vulkan/vk_cache_utils.h b/src/libANGLE/renderer/vulkan/vk_cache_utils.h
index 2d9784c1e0db828a5aacefa19168795da85fc5a5..c7213a1193fee21595737047e2379231039b66e9 100644
--- a/src/libANGLE/renderer/vulkan/vk_cache_utils.h
+++ b/src/libANGLE/renderer/vulkan/vk_cache_utils.h
@@ -1937,6 +1937,7 @@ class SharedCacheKeyManager
void addKey(const SharedCacheKeyT &key);
// Iterate over the descriptor array and release the descriptor and cache.
void releaseKeys(ContextVk *contextVk);
+ void releaseKeys(RendererVk *rendererVk);
// Iterate over the descriptor array and destroy the descriptor and cache.
void destroyKeys(RendererVk *renderer);
void clear();
diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.cpp b/src/libANGLE/renderer/vulkan/vk_helpers.cpp
index b6efd6855a819d28f60a79848dd811a347704d44..b56e78f8f5335e14826c58bfc521bd7d8e55caeb 100644
--- a/src/libANGLE/renderer/vulkan/vk_helpers.cpp
+++ b/src/libANGLE/renderer/vulkan/vk_helpers.cpp
@@ -3598,7 +3598,7 @@ angle::Result DynamicDescriptorPool::allocateNewPool(Context *context)
return mDescriptorPools[mCurrentPoolIndex]->get().init(context, mPoolSizes, mMaxSetsPerPool);
}
-void DynamicDescriptorPool::releaseCachedDescriptorSet(ContextVk *contextVk,
+void DynamicDescriptorPool::releaseCachedDescriptorSet(RendererVk *renderer,
const DescriptorSetDesc &desc)
{
VkDescriptorSet descriptorSet;
@@ -3612,7 +3612,7 @@ void DynamicDescriptorPool::releaseCachedDescriptorSet(ContextVk *contextVk,
// Wrap it with helper object so that it can be GPU tracked and add it to resource list.
DescriptorSetHelper descriptorSetHelper(poolOut->get().getResourceUse(), descriptorSet);
poolOut->get().addGarbage(std::move(descriptorSetHelper));
- checkAndReleaseUnusedPool(contextVk->getRenderer(), poolOut);
+ checkAndReleaseUnusedPool(renderer, poolOut);
}
}
@@ -4807,6 +4807,12 @@ void BufferHelper::release(RendererVk *renderer)
if (mSuballocation.valid())
{
+ if (!mSuballocation.isSuballocated())
+ {
+ // Destroy cacheKeys now to avoid getting into situation that having to destroy
+ // descriptorSet from garbage collection thread.
+ mSuballocation.getBufferBlock()->releaseAllCachedDescriptorSetCacheKeys(renderer);
+ }
renderer->collectSuballocationGarbage(mUse, std::move(mSuballocation),
std::move(mBufferForVertexArray));
}
@@ -4815,17 +4821,15 @@ void BufferHelper::release(RendererVk *renderer)
ASSERT(!mBufferForVertexArray.valid());
}
-void BufferHelper::releaseBufferAndDescriptorSetCache(ContextVk *contextVk)
+void BufferHelper::releaseBufferAndDescriptorSetCache(RendererVk *renderer)
{
- RendererVk *renderer = contextVk->getRenderer();
-
if (renderer->hasResourceUseFinished(getResourceUse()))
{
mDescriptorSetCacheManager.destroyKeys(renderer);
}
else
{
- mDescriptorSetCacheManager.releaseKeys(contextVk);
+ mDescriptorSetCacheManager.releaseKeys(renderer);
}
release(renderer);
diff --git a/src/libANGLE/renderer/vulkan/vk_helpers.h b/src/libANGLE/renderer/vulkan/vk_helpers.h
index 29f7d2141fab70e0f542b05004b21701883a39c9..d81f9a4b9b07c6e34094d6d275a04c74c2652d3a 100644
--- a/src/libANGLE/renderer/vulkan/vk_helpers.h
+++ b/src/libANGLE/renderer/vulkan/vk_helpers.h
@@ -247,7 +247,7 @@ class DynamicDescriptorPool final : angle::NonCopyable
VkDescriptorSet *descriptorSetOut,
SharedDescriptorSetCacheKey *sharedCacheKeyOut);
- void releaseCachedDescriptorSet(ContextVk *contextVk, const DescriptorSetDesc &desc);
+ void releaseCachedDescriptorSet(RendererVk *renderer, const DescriptorSetDesc &desc);
void destroyCachedDescriptorSet(RendererVk *renderer, const DescriptorSetDesc &desc);
template <typename Accumulator>
@@ -771,7 +771,7 @@ class BufferHelper : public ReadWriteResource
void destroy(RendererVk *renderer);
void release(RendererVk *renderer);
- void releaseBufferAndDescriptorSetCache(ContextVk *contextVk);
+ void releaseBufferAndDescriptorSetCache(RendererVk *renderer);
BufferSerial getBufferSerial() const { return mSerial; }
BufferSerial getBlockSerial() const
diff --git a/src/tests/gl_tests/TransformFeedbackTest.cpp b/src/tests/gl_tests/TransformFeedbackTest.cpp
index ea3eeea4015dcb56f710c873b22c95d168bb287c..226eb1f049bb708e0221c81e9b836a67594eb7b0 100644
--- a/src/tests/gl_tests/TransformFeedbackTest.cpp
+++ b/src/tests/gl_tests/TransformFeedbackTest.cpp
@@ -507,8 +507,8 @@ TEST_P(TransformFeedbackTest, UseAsUBOThenUpdateThenCapture)
const std::array<uint32_t, 12> kInitialData = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11};
const std::array<uint32_t, 12> kUpdateData = {
- 0x12345678u, 0x9ABCDEF0u, 0x13579BDFu, 0x2468ACE0u, 0x23456781u, 0xABCDEF09u,
- 0x3579BDF1u, 0x468ACE02u, 0x34567812u, 0xBCDEF09Au, 0x579BDF13u, 0x68ACE024u,
+ 0x12345678u, 0x9ABCDEF0u, 0x13579BDFu, 0x2468ACE0u, 0x23456781u, 0xABCDEF09u,
+ 0x3579BDF1u, 0x468ACE02u, 0x34567812u, 0xBCDEF09Au, 0x579BDF13u, 0x68ACE024u,
};
GLBuffer buffer;
@@ -3749,9 +3749,9 @@ void main()
constexpr size_t kCapturedVaryingsCount = 3;
constexpr std::array<size_t, kCapturedVaryingsCount> kCaptureSizes = {8, 9, 4};
const std::vector<float> kExpected[kCapturedVaryingsCount] = {
- {0.27, 0.30, 0.33, 0.36, 0.39, 0.42, 0.45, 0.48},
- {0.63, 0.66, 0.69, 0.72, 0.75, 0.78, 0.81, 0.84, 0.87},
- {0.25, 0.5, 0.75, 1.0},
+ {0.27, 0.30, 0.33, 0.36, 0.39, 0.42, 0.45, 0.48},
+ {0.63, 0.66, 0.69, 0.72, 0.75, 0.78, 0.81, 0.84, 0.87},
+ {0.25, 0.5, 0.75, 1.0},
};
ANGLE_GL_PROGRAM_TRANSFORM_FEEDBACK(program, kVS, kFS, tfVaryings, GL_INTERLEAVED_ATTRIBS);
@@ -3848,9 +3848,9 @@ void main()
constexpr size_t kCapturedVaryingsCount = 3;
constexpr std::array<size_t, kCapturedVaryingsCount> kCaptureSizes = {1, 2, 1};
const std::vector<float> kExpected[kCapturedVaryingsCount] = {
- {0.25},
- {0.5, 0.75},
- {1.0},
+ {0.25},
+ {0.5, 0.75},
+ {1.0},
};
ANGLE_GL_PROGRAM_TRANSFORM_FEEDBACK(program, kVS, kFS, tfVaryings, GL_SEPARATE_ATTRIBS);
@@ -4392,6 +4392,51 @@ TEST_P(TransformFeedbackTest, RenderOnceChangeXfbBufferRenderAgain)
glEndTransformFeedback();
}
+// Test bufferData call and transform feedback.
+TEST_P(TransformFeedbackTest, BufferDataAndTransformFeedback)
+{
+ const char kVS[] = R"(#version 300 es
+flat out highp int var;
+void main() {
+var = 1;
+})";
+
+ const char kFS[] = R"(#version 300 es
+flat in highp int var;
+out highp int color;
+void main() {
+color = var;
+})";
+
+ ANGLE_GL_PROGRAM(program, kVS, kFS);
+
+ GLTexture texture;
+ glBindTexture(GL_TEXTURE_2D, texture);
+ glTexStorage2D(GL_TEXTURE_2D, 1, GL_R32I, 1, 1);
+
+ GLFramebuffer fbo;
+ glBindFramebuffer(GL_FRAMEBUFFER, fbo);
+ glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0);
+ EXPECT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+
+ constexpr int kClearColor[] = {123, 0, 0, 0};
+ glClearBufferiv(GL_COLOR, 0, kClearColor);
+ glDrawArrays(GL_POINTS, 0, 1);
+
+ const char *kVarying = "var";
+ glTransformFeedbackVaryings(program, 1, &kVarying, GL_INTERLEAVED_ATTRIBS);
+ glLinkProgram(program);
+ ASSERT_GL_NO_ERROR();
+
+ GLBuffer buffer;
+ glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, buffer);
+ glBufferData(GL_TRANSFORM_FEEDBACK_BUFFER, 0x7ffc * 10000, nullptr, GL_DYNAMIC_READ);
+ glBeginTransformFeedback(GL_POINTS);
+ glDrawArrays(GL_POINTS, 0, 1);
+ glEndTransformFeedback();
+ glFlush();
+}
+
GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(TransformFeedbackTest);
ANGLE_INSTANTIATE_TEST_ES3_AND(TransformFeedbackTest,
ES3_VULKAN()
diff --git a/src/tests/gl_tests/UniformBufferTest.cpp b/src/tests/gl_tests/UniformBufferTest.cpp
index 4d005c0740d5ff918af103236aa18e8249e00acc..71b85043bd9ca49018e9ed79fa36f24f8499757e 100644
--- a/src/tests/gl_tests/UniformBufferTest.cpp
+++ b/src/tests/gl_tests/UniformBufferTest.cpp
@@ -3422,6 +3422,50 @@ void main() {
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
}
+// Calling BufferData and use it in a loop to force descriptorSet creation and destroy.
+TEST_P(UniformBufferTest, BufferDataInLoop)
+{
+ glClear(GL_COLOR_BUFFER_BIT);
+
+ // Use large buffer size to get around suballocation, so that we will gets a new buffer with
+ // bufferData call.
+ static constexpr size_t kBufferSize = 4 * 1024 * 1024;
+ std::vector<float> floatData;
+ floatData.resize(kBufferSize / (sizeof(float)), 0.0f);
+ floatData[0] = 0.5f;
+ floatData[1] = 0.75f;
+ floatData[2] = 0.25f;
+ floatData[3] = 1.0f;
+
+ GLTexture textures[2];
+ GLFramebuffer fbos[2];
+ for (int i = 0; i < 2; i++)
+ {
+ glBindTexture(GL_TEXTURE_2D, textures[i]);
+ glTexStorage2D(GL_TEXTURE_2D, 1, GL_RGBA8, 256, 256);
+
+ glBindFramebuffer(GL_FRAMEBUFFER, fbos[i]);
+ glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, textures[i], 0);
+ EXPECT_GL_FRAMEBUFFER_COMPLETE(GL_FRAMEBUFFER);
+ }
+
+ for (int loop = 0; loop < 10; loop++)
+ {
+ int i = loop & 0x1;
+ // Switch FBO to get around deferred flush
+ glBindFramebuffer(GL_FRAMEBUFFER, fbos[i]);
+ glBindBuffer(GL_UNIFORM_BUFFER, mUniformBuffer);
+ glBufferData(GL_UNIFORM_BUFFER, kBufferSize, floatData.data(), GL_STATIC_DRAW);
+
+ glBindBufferBase(GL_UNIFORM_BUFFER, 0, mUniformBuffer);
+ glUniformBlockBinding(mProgram, mUniformBufferIndex, 0);
+ drawQuad(mProgram, essl3_shaders::PositionAttrib(), 0.5f);
+ glFlush();
+ }
+ ASSERT_GL_NO_ERROR();
+ EXPECT_PIXEL_NEAR(0, 0, 128, 191, 64, 255, 1);
+}
+
class WebGL2UniformBufferTest : public UniformBufferTest
{
protected:
View Git Blame Copy Permalink