Vulkan: Make DescriptorPoolHelper a Resource

src/libANGLE/renderer/vulkan/ContextVk.cpp

@@ -4044,14 +4044,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,
@@ -4059,7 +4067,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;
 }
 
@@ -4068,7 +4076,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;
 }
 
@@ -4108,6 +4116,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

@@ -916,7 +916,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

@@ -382,8 +382,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;
     }
 
@@ -1398,6 +1402,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

@@ -2634,7 +2634,6 @@ angle::Result UtilsVk::allocateDescriptorSet(ContextVk *contextVk,
             .get()
             .ptr(),
         1, bindingOut, descriptorSetOut));
-    bindingOut->get().updateSerial(contextVk->getCurrentQueueSerial());
 
     mObjectPerfCounters.descriptorSetsAllocated++;
 

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_white_box_tests.gni

@@ -21,6 +21,7 @@ angle_white_box_tests_vulkan_sources = [
   "gl_tests/VulkanDescriptorSetTest.cpp",
   "gl_tests/VulkanFormatTablesTest.cpp",
   "gl_tests/VulkanFramebufferTest.cpp",
+  "gl_tests/VulkanMultithreadingTest.cpp",
   "gl_tests/VulkanPerformanceCounterTest.cpp",
   "gl_tests/VulkanUniformUpdatesTest.cpp",
 ]

src/tests/gl_tests/VulkanMultithreadingTest.cpp

+//
+// Copyright 2020 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// VulkanMultithreadingTest.cpp : Tests of multithreaded rendering specific to the Vulkan back end.
+
+#include "platform/FeaturesVk.h"
+#include "test_utils/ANGLETest.h"
+#include "test_utils/gl_raii.h"
+#include "util/EGLWindow.h"
+
+#include <atomic>
+#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 VulkanMultithreadingTest : public ANGLETest
+{
+  protected:
+    VulkanMultithreadingTest()
+    {
+        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)
+    {
+        std::mutex mutex;
+
+        EGLWindow *window = getEGLWindow();
+        EGLDisplay dpy    = window->getDisplay();
+        EGLConfig config  = window->getConfig();
+
+        constexpr EGLint kPBufferSize = kSize;
+
+        std::vector<std::thread> threads(threadCount);
+        for (size_t threadIdx = 0; threadIdx < threadCount; threadIdx++)
+        {
+            threads[threadIdx] = std::thread([&, threadIdx]() {
+                EGLSurface surface = EGL_NO_SURFACE;
+                EGLContext ctx     = EGL_NO_CONTEXT;
+
+                {
+                    std::lock_guard<decltype(mutex)> lock(mutex);
+
+                    // Initialize the pbuffer and context
+                    EGLint pbufferAttributes[] = {
+                        EGL_WIDTH, kPBufferSize, EGL_HEIGHT, kPBufferSize, EGL_NONE, EGL_NONE,
+                    };
+                    surface = eglCreatePbufferSurface(dpy, config, pbufferAttributes);
+                    EXPECT_EGL_SUCCESS();
+
+                    ctx = window->createContext(EGL_NO_CONTEXT);
+                    EXPECT_NE(EGL_NO_CONTEXT, ctx);
+
+                    EXPECT_EGL_TRUE(eglMakeCurrent(dpy, surface, surface, ctx));
+                    EXPECT_EGL_SUCCESS();
+                }
+
+                testBody(surface, threadIdx);
+
+                {
+                    std::lock_guard<decltype(mutex)> lock(mutex);
+
+                    // Clean up
+                    EXPECT_EGL_TRUE(
+                        eglMakeCurrent(dpy, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT));
+                    EXPECT_EGL_SUCCESS();
+
+                    eglDestroySurface(dpy, surface);
+                    eglDestroyContext(dpy, ctx);
+                }
+            });
+        }
+
+        for (std::thread &thread : threads)
+        {
+            thread.join();
+        }
+    }
+
+  private:
+    uint32_t mMaxSetsPerPool;
+    uint32_t mMaxSetsPerPoolMultiplier;
+};
+
+// Test that multiple threads can draw and readback pixels successfully at the same time with small
+// descriptor pools.
+TEST_P(VulkanMultithreadingTest, 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);
+}
+
+ANGLE_INSTANTIATE_TEST(VulkanMultithreadingTest, ES2_VULKAN(), ES3_VULKAN());
+
+}  // namespace angle