Vulkan: Make DescriptorPoolHelper a Resource

src/libANGLE/renderer/vulkan/ContextVk.cpp

@@ -3962,14 +3962,22 @@ angle::Result ContextVk::handleDirtyComputeDriverUniforms(const gl::Context *con
                                              &mDriverUniforms[PipelineType::Compute]);
 }
 
-void ContextVk::handleDirtyDriverUniformsBindingImpl(
-    vk::CommandBuffer *commandBuffer,
-    VkPipelineBindPoint bindPoint,
-    const DriverUniformsDescriptorSet &driverUniforms)
+void ContextVk::handleDirtyDriverUniformsBindingImpl(vk::CommandBuffer *commandBuffer,
+                                                     VkPipelineBindPoint bindPoint,
+                                                     DriverUniformsDescriptorSet *driverUniforms)
 {
+    // The descriptor pool that this descriptor set was allocated from needs to be retained when the
+    // descriptor set is used in a new command. Since the descriptor pools are specific to each
+    // ContextVk, we only need to retain them once to ensure the reference count and Serial are
+    // updated correctly.
+    if (!driverUniforms->descriptorPoolBinding.get().usedInRecordedCommands())
+    {
+        driverUniforms->descriptorPoolBinding.get().retain(&mResourceUseList);
+    }
+
     commandBuffer->bindDescriptorSets(
         mExecutable->getPipelineLayout(), bindPoint, DescriptorSetIndex::DriverUniforms, 1,
-        &driverUniforms.descriptorSet, 1, &driverUniforms.dynamicOffset);
+        &driverUniforms->descriptorSet, 1, &driverUniforms->dynamicOffset);
 }
 
 angle::Result ContextVk::handleDirtyGraphicsDriverUniformsBinding(const gl::Context *context,
@@ -3977,7 +3985,7 @@ angle::Result ContextVk::handleDirtyGraphicsDriverUniformsBinding(const gl::Cont
 {
     // Bind the driver descriptor set.
     handleDirtyDriverUniformsBindingImpl(commandBuffer, VK_PIPELINE_BIND_POINT_GRAPHICS,
-                                         mDriverUniforms[PipelineType::Graphics]);
+                                         &mDriverUniforms[PipelineType::Graphics]);
     return angle::Result::Continue;
 }
 
@@ -3986,7 +3994,7 @@ angle::Result ContextVk::handleDirtyComputeDriverUniformsBinding(const gl::Conte
 {
     // Bind the driver descriptor set.
     handleDirtyDriverUniformsBindingImpl(commandBuffer, VK_PIPELINE_BIND_POINT_COMPUTE,
-                                         mDriverUniforms[PipelineType::Compute]);
+                                         &mDriverUniforms[PipelineType::Compute]);
     return angle::Result::Continue;
 }
 
@@ -4026,6 +4034,9 @@ angle::Result ContextVk::updateDriverUniformsDescriptorSet(
     if (driverUniforms->descriptorSetCache.get(bufferSerial.getValue(),
                                                &driverUniforms->descriptorSet))
     {
+        // The descriptor pool that this descriptor set was allocated from needs to be retained each
+        // time the descriptor set is used in a new command.
+        driverUniforms->descriptorPoolBinding.get().retain(&mResourceUseList);
         return angle::Result::Continue;
     }
 

src/libANGLE/renderer/vulkan/ContextVk.h

@@ -903,7 +903,7 @@ class ContextVk : public ContextImpl, public vk::Context
                                                  vk::CommandBufferHelper *commandBufferHelper);
     void handleDirtyDriverUniformsBindingImpl(vk::CommandBuffer *commandBuffer,
                                               VkPipelineBindPoint bindPoint,
-                                              const DriverUniformsDescriptorSet &driverUniforms);
+                                              DriverUniformsDescriptorSet *driverUniforms);
     angle::Result handleDirtyDescriptorSets(const gl::Context *context,
                                             vk::CommandBuffer *commandBuffer);
     angle::Result allocateDriverUniforms(size_t driverUniformsSize,

src/libANGLE/renderer/vulkan/ProgramExecutableVk.cpp

@@ -378,8 +378,12 @@ angle::Result ProgramExecutableVk::allocUniformAndXfbDescriptorSet(
     auto iter = mUniformsAndXfbDescriptorSetCache.find(xfbBufferDesc);
     if (iter != mUniformsAndXfbDescriptorSetCache.end())
     {
-        mDescriptorSets[ToUnderlying(DescriptorSetIndex::UniformsAndXfb)] = iter->second;
         *newDescriptorSetAllocated                                        = false;
+        mDescriptorSets[ToUnderlying(DescriptorSetIndex::UniformsAndXfb)] = iter->second;
+        // The descriptor pool that this descriptor set was allocated from needs to be retained each
+        // time the descriptor set is used in a new command.
+        mDescriptorPoolBindings[ToUnderlying(DescriptorSetIndex::UniformsAndXfb)].get().retain(
+            &contextVk->getResourceUseList());
         return angle::Result::Continue;
     }
 
@@ -1397,6 +1401,10 @@ angle::Result ProgramExecutableVk::updateTexturesDescriptorSet(ContextVk *contex
     if (iter != mTextureDescriptorsCache.end())
     {
         mDescriptorSets[ToUnderlying(DescriptorSetIndex::Texture)] = iter->second;
+        // The descriptor pool that this descriptor set was allocated from needs to be retained each
+        // time the descriptor set is used in a new command.
+        mDescriptorPoolBindings[ToUnderlying(DescriptorSetIndex::Texture)].get().retain(
+            &contextVk->getResourceUseList());
         return angle::Result::Continue;
     }
 

src/libANGLE/renderer/vulkan/ResourceVk.h

@@ -200,6 +200,7 @@ ANGLE_INLINE void Resource::retain(ResourceUseList *resourceUseList)
     // Store reference in resource list.
     resourceUseList->add(mUse);
 }
+
 }  // namespace vk
 }  // namespace rx
 

src/libANGLE/renderer/vulkan/UtilsVk.cpp

@@ -2626,7 +2626,6 @@ angle::Result UtilsVk::allocateDescriptorSet(ContextVk *contextVk,
             .get()
             .ptr(),
         1, bindingOut, descriptorSetOut));
-    bindingOut->get().updateSerial(contextVk->getCurrentQueueSerial());
     return angle::Result::Continue;
 }
 

src/libANGLE/renderer/vulkan/vk_helpers.cpp

@@ -1874,7 +1874,7 @@ angle::Result DescriptorPoolHelper::init(ContextVk *contextVk,
 {
     if (mDescriptorPool.valid())
     {
-        // This could be improved by recycling the descriptor pool.
+        ASSERT(!isCurrentlyInUse(contextVk->getLastCompletedQueueSerial()));
         mDescriptorPool.destroy(contextVk->getDevice());
     }
 
@@ -1927,6 +1927,9 @@ angle::Result DescriptorPoolHelper::allocateSets(ContextVk *contextVk,
     ANGLE_VK_TRY(contextVk, mDescriptorPool.allocateDescriptorSets(contextVk->getDevice(),
                                                                    allocInfo, descriptorSetsOut));
 
+    // The pool is still in use every time a new descriptor set is allocated from it.
+    retain(&contextVk->getResourceUseList());
+
     return angle::Result::Continue;
 }
 
@@ -1968,6 +1971,7 @@ void DynamicDescriptorPool::destroy(VkDevice device)
     }
 
     mDescriptorPools.clear();
+    mCurrentPoolIndex          = 0;
     mCachedDescriptorSetLayout = VK_NULL_HANDLE;
 }
 
@@ -1981,6 +1985,7 @@ void DynamicDescriptorPool::release(ContextVk *contextVk)
     }
 
     mDescriptorPools.clear();
+    mCurrentPoolIndex          = 0;
     mCachedDescriptorSetLayout = VK_NULL_HANDLE;
 }
 
@@ -2005,15 +2010,6 @@ angle::Result DynamicDescriptorPool::allocateSetsAndGetInfo(
             *newPoolAllocatedOut = true;
         }
 
-        // Make sure the old binding knows the descriptor sets can still be in-use. We only need
-        // to update the serial when we move to a new pool. This is because we only check serials
-        // when we move to a new pool.
-        if (bindingOut->valid())
-        {
-            Serial currentSerial = contextVk->getCurrentQueueSerial();
-            bindingOut->get().updateSerial(currentSerial);
-        }
-
         bindingOut->set(mDescriptorPools[mCurrentPoolIndex]);
     }
 
@@ -2025,10 +2021,11 @@ angle::Result DynamicDescriptorPool::allocateNewPool(ContextVk *contextVk)
 {
     bool found = false;
 
+    Serial lastCompletedSerial = contextVk->getLastCompletedQueueSerial();
     for (size_t poolIndex = 0; poolIndex < mDescriptorPools.size(); ++poolIndex)
     {
         if (!mDescriptorPools[poolIndex]->isReferenced() &&
-            !contextVk->isSerialInUse(mDescriptorPools[poolIndex]->get().getSerial()))
+            !mDescriptorPools[poolIndex]->get().isCurrentlyInUse(lastCompletedSerial))
         {
             mCurrentPoolIndex = poolIndex;
             found             = true;

src/libANGLE/renderer/vulkan/vk_helpers.h

@@ -237,18 +237,15 @@ class DynamicShadowBuffer : public angle::NonCopyable
 
 // Uses DescriptorPool to allocate descriptor sets as needed. If a descriptor pool becomes full, we
 // allocate new pools internally as needed. RendererVk takes care of the lifetime of the discarded
-// pools. Note that we used a fixed layout for descriptor pools in ANGLE. Uniform buffers must
-// use set zero and combined Image Samplers must use set 1. We conservatively count each new set
-// using the maximum number of descriptor sets and buffers with each allocation. Currently: 2
-// (Vertex/Fragment) uniform buffers and 64 (MAX_ACTIVE_TEXTURES) image/samplers.
+// pools. Note that we used a fixed layout for descriptor pools in ANGLE.
 
 // Shared handle to a descriptor pool. Each helper is allocated from the dynamic descriptor pool.
 // Can be used to share descriptor pools between multiple ProgramVks and the ContextVk.
-class DescriptorPoolHelper
+class DescriptorPoolHelper : public Resource
 {
   public:
     DescriptorPoolHelper();
-    ~DescriptorPoolHelper();
+    ~DescriptorPoolHelper() override;
 
     bool valid() { return mDescriptorPool.valid(); }
 
@@ -264,14 +261,9 @@ class DescriptorPoolHelper
                                uint32_t descriptorSetCount,
                                VkDescriptorSet *descriptorSetsOut);
 
-    void updateSerial(Serial serial) { mMostRecentSerial = serial; }
-
-    Serial getSerial() const { return mMostRecentSerial; }
-
   private:
     uint32_t mFreeDescriptorSets;
     DescriptorPool mDescriptorPool;
-    Serial mMostRecentSerial;
 };
 
 using RefCountedDescriptorPoolHelper  = RefCounted<DescriptorPoolHelper>;

src/tests/angle_end2end_tests.gni

@@ -89,7 +89,6 @@ angle_end2end_tests_sources = [
   "gl_tests/MultisampleCompatibilityTest.cpp",
   "gl_tests/MultisampleTest.cpp",
   "gl_tests/MultisampledRenderToTextureTest.cpp",
-  "gl_tests/MultithreadingTest.cpp",
   "gl_tests/MultiviewDrawTest.cpp",
   "gl_tests/ObjectAllocationTest.cpp",
   "gl_tests/OcclusionQueriesTest.cpp",

src/tests/angle_white_box_tests.gni

@@ -5,6 +5,7 @@
 angle_white_box_tests_sources = [
   "egl_tests/EGLFeatureControlTest.cpp",
   "gl_tests/FormatPrintTest.cpp",
+  "gl_tests/MultithreadingTest.cpp",
   "test_utils/ANGLETest.cpp",
   "test_utils/ANGLETest.h",
   "util_tests/PrintSystemInfoTest.cpp",

src/tests/gl_tests/MultithreadingTest.cpp

@@ -14,22 +14,51 @@
 #include <mutex>
 #include <thread>
 
+#include "libANGLE/renderer/vulkan/vk_helpers.h"
+
 namespace angle
 {
 
+constexpr char kExtensionName[] = "GL_ANGLE_get_image";
+static constexpr int kSize      = 256;
+
 class MultithreadingTest : public ANGLETest
 {
   protected:
     MultithreadingTest()
     {
-        setWindowWidth(128);
-        setWindowHeight(128);
+        setWindowWidth(kSize);
+        setWindowHeight(kSize);
         setConfigRedBits(8);
         setConfigGreenBits(8);
         setConfigBlueBits(8);
         setConfigAlphaBits(8);
     }
 
+    void testSetUp() override
+    {
+        mMaxSetsPerPool = rx::vk::DynamicDescriptorPool::GetMaxSetsPerPoolForTesting();
+        mMaxSetsPerPoolMultiplier =
+            rx::vk::DynamicDescriptorPool::GetMaxSetsPerPoolMultiplierForTesting();
+    }
+
+    void testTearDown() override
+    {
+        rx::vk::DynamicDescriptorPool::SetMaxSetsPerPoolForTesting(mMaxSetsPerPool);
+        rx::vk::DynamicDescriptorPool::SetMaxSetsPerPoolMultiplierForTesting(
+            mMaxSetsPerPoolMultiplier);
+    }
+
+    static constexpr uint32_t kMaxSetsForTesting           = 1;
+    static constexpr uint32_t kMaxSetsMultiplierForTesting = 1;
+
+    void limitMaxSets()
+    {
+        rx::vk::DynamicDescriptorPool::SetMaxSetsPerPoolForTesting(kMaxSetsForTesting);
+        rx::vk::DynamicDescriptorPool::SetMaxSetsPerPoolMultiplierForTesting(
+            kMaxSetsMultiplierForTesting);
+    }
+
     void runMultithreadedGLTest(
         std::function<void(EGLSurface surface, size_t threadIndex)> testBody,
         size_t threadCount)
@@ -40,7 +69,7 @@ class MultithreadingTest : public ANGLETest
         EGLDisplay dpy    = window->getDisplay();
         EGLConfig config  = window->getConfig();
 
-        constexpr EGLint kPBufferSize = 256;
+        constexpr EGLint kPBufferSize = kSize;
 
         std::vector<std::thread> threads(threadCount);
         for (size_t threadIdx = 0; threadIdx < threadCount; threadIdx++)
@@ -87,6 +116,10 @@ class MultithreadingTest : public ANGLETest
             thread.join();
         }
     }
+
+  private:
+    uint32_t mMaxSetsPerPool;
+    uint32_t mMaxSetsPerPoolMultiplier;
 };
 
 // Test that it's possible to make one context current on different threads
@@ -202,7 +235,88 @@ TEST_P(MultithreadingTest, MultiContextDraw)
     runMultithreadedGLTest(testBody, 4);
 }
 
-// Test that multiple threads can draw and read back pixels correctly.
+// Test that multiple threads can draw and readback pixels successfully at the same time with small
+// descriptor pools.
+TEST_P(MultithreadingTest, MultiContextDrawSmallDescriptorPools)
+{
+    ANGLE_SKIP_TEST_IF(!platformSupportsMultithreading());
+    // Verify the extension is enabled.
+    ASSERT_TRUE(IsGLExtensionEnabled(kExtensionName));
+
+    // Must be before program creation to limit the descriptor pool sizes when creating the pipeline
+    // layout.
+    limitMaxSets();
+
+    auto testBody = [](EGLSurface surface, size_t thread) {
+        constexpr size_t kIterationsPerThread = 16;
+        constexpr size_t kDrawsPerIteration   = 16;
+
+        ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), essl1_shaders::fs::UniformColor());
+        glUseProgram(program);
+
+        GLTexture copyTexture;
+        glBindTexture(GL_TEXTURE_2D, copyTexture);
+        glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, kSize, kSize, 0, GL_RGBA, GL_UNSIGNED_BYTE,
+                     nullptr);
+        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
+        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
+        ASSERT_GL_NO_ERROR();
+
+        GLint colorLocation = glGetUniformLocation(program, essl1_shaders::ColorUniform());
+
+        auto quadVertices = GetQuadVertices();
+
+        GLBuffer vertexBuffer;
+        glBindBuffer(GL_ARRAY_BUFFER, vertexBuffer);
+        glBufferData(GL_ARRAY_BUFFER, sizeof(GLfloat) * 3 * 6, quadVertices.data(), GL_STATIC_DRAW);
+
+        GLint positionLocation = glGetAttribLocation(program, essl1_shaders::PositionAttrib());
+        glEnableVertexAttribArray(positionLocation);
+        glVertexAttribPointer(positionLocation, 3, GL_FLOAT, GL_FALSE, 0, 0);
+
+        // Pack pixels tightly.
+        glPixelStorei(GL_PACK_ALIGNMENT, 1);
+
+        std::vector<GLColor> actualData(kSize * kSize);
+
+        for (size_t iteration = 0; iteration < kIterationsPerThread; iteration++)
+        {
+            // Base the clear color on the thread and iteration indexes so every clear color is
+            // unique
+            const GLColor color(static_cast<GLubyte>(thread % 255),
+                                static_cast<GLubyte>(iteration % 255), 0, 255);
+            const angle::Vector4 floatColor = color.toNormalizedVector();
+            glUniform4fv(colorLocation, 1, floatColor.data());
+
+            for (size_t draw = 0; draw < kDrawsPerIteration; draw++)
+            {
+                glDrawArrays(GL_TRIANGLES, 0, 6);
+                ASSERT_GL_NO_ERROR();
+
+                // Perform CopyTexImage2D
+                glBindTexture(GL_TEXTURE_2D, copyTexture);
+                glCopyTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 0, 0, kSize, kSize, 0);
+                ASSERT_GL_NO_ERROR();
+            }
+
+            // There's a good chance this test will crash before failing, but if not we'll try and
+            // verify the contents of the copied texture.
+            // TODO(http://anglebug.com/5204): Need to re-enable for Linux/Windows.
+            if (IsGLExtensionEnabled(kExtensionName) && !(IsLinux() || IsWindows()))
+            {
+                // Verify glCopyTexImage2D() was successful.
+                glGetTexImageANGLE(GL_TEXTURE_2D, 0, GL_RGBA, GL_UNSIGNED_BYTE, actualData.data());
+                EXPECT_GL_NO_ERROR();
+                EXPECT_EQ(color, actualData[0]);
+            }
+        }
+    };
+    runMultithreadedGLTest(testBody, 4);
+}
+
+// Test that multiple threads can draw and read back pixels correctly with small descriptor pools.
 // Using eglSwapBuffers stresses race conditions around use of QueueSerials.
 TEST_P(MultithreadingTest, MultiContextDrawWithSwapBuffers)
 {
@@ -366,6 +480,8 @@ ANGLE_INSTANTIATE_TEST(MultithreadingTest,
                        WithNoVirtualContexts(ES2_OPENGL()),
                        WithNoVirtualContexts(ES3_OPENGL()),
                        WithNoVirtualContexts(ES2_OPENGLES()),
-                       WithNoVirtualContexts(ES3_OPENGLES()));
+                       WithNoVirtualContexts(ES3_OPENGLES()),
+                       WithNoVirtualContexts(ES2_VULKAN()),
+                       WithNoVirtualContexts(ES3_VULKAN()));
 
 }  // namespace angle