Revert "Vulkan: Make DescriptorPoolHelper a Resource"

src/libANGLE/renderer/vulkan/ContextVk.cpp

@@ -4003,22 +4003,14 @@ angle::Result ContextVk::handleDirtyComputeDriverUniforms(const gl::Context *con
                                              &mDriverUniforms[PipelineType::Compute]);
 }
 
-void ContextVk::handleDirtyDriverUniformsBindingImpl(vk::CommandBuffer *commandBuffer,
-                                                     VkPipelineBindPoint bindPoint,
-                                                     DriverUniformsDescriptorSet *driverUniforms)
+void ContextVk::handleDirtyDriverUniformsBindingImpl(
+    vk::CommandBuffer *commandBuffer,
+    VkPipelineBindPoint bindPoint,
+    const 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,
@@ -4026,7 +4018,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;
 }
 
@@ -4035,7 +4027,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;
 }
 
@@ -4075,9 +4067,6 @@ 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

@@ -908,7 +908,7 @@ class ContextVk : public ContextImpl, public vk::Context
                                                  vk::CommandBufferHelper *commandBufferHelper);
     void handleDirtyDriverUniformsBindingImpl(vk::CommandBuffer *commandBuffer,
                                               VkPipelineBindPoint bindPoint,
-                                              DriverUniformsDescriptorSet *driverUniforms);
+                                              const 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,12 +378,8 @@ angle::Result ProgramExecutableVk::allocUniformAndXfbDescriptorSet(
     auto iter = mUniformsAndXfbDescriptorSetCache.find(xfbBufferDesc);
     if (iter != mUniformsAndXfbDescriptorSetCache.end())
     {
-        *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());
+        *newDescriptorSetAllocated                                        = false;
         return angle::Result::Continue;
     }
 
@@ -1401,10 +1397,6 @@ 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,7 +200,6 @@ 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

@@ -2631,6 +2631,7 @@ 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())
     {
-        ASSERT(!isCurrentlyInUse(contextVk->getLastCompletedQueueSerial()));
+        // This could be improved by recycling the descriptor pool.
         mDescriptorPool.destroy(contextVk->getDevice());
     }
 
@@ -1927,9 +1927,6 @@ 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;
 }
 
@@ -1971,7 +1968,6 @@ void DynamicDescriptorPool::destroy(VkDevice device)
     }
 
     mDescriptorPools.clear();
-    mCurrentPoolIndex          = 0;
     mCachedDescriptorSetLayout = VK_NULL_HANDLE;
 }
 
@@ -1985,7 +1981,6 @@ void DynamicDescriptorPool::release(ContextVk *contextVk)
     }
 
     mDescriptorPools.clear();
-    mCurrentPoolIndex          = 0;
     mCachedDescriptorSetLayout = VK_NULL_HANDLE;
 }
 
@@ -2010,6 +2005,15 @@ 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]);
     }
 
@@ -2021,11 +2025,10 @@ 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() &&
-            !mDescriptorPools[poolIndex]->get().isCurrentlyInUse(lastCompletedSerial))
+            !contextVk->isSerialInUse(mDescriptorPools[poolIndex]->get().getSerial()))
         {
             mCurrentPoolIndex = poolIndex;
             found             = true;

src/libANGLE/renderer/vulkan/vk_helpers.h

@@ -237,15 +237,18 @@ 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.
+// 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.
 
 // 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 : public Resource
+class DescriptorPoolHelper
 {
   public:
     DescriptorPoolHelper();
-    ~DescriptorPoolHelper() override;
+    ~DescriptorPoolHelper();
 
     bool valid() { return mDescriptorPool.valid(); }
 
@@ -261,9 +264,14 @@ class DescriptorPoolHelper : public Resource
                                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,6 +89,7 @@ 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,7 +5,6 @@
 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,51 +14,22 @@
 #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(kSize);
-        setWindowHeight(kSize);
+        setWindowWidth(128);
+        setWindowHeight(128);
         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)
@@ -69,7 +40,7 @@ class MultithreadingTest : public ANGLETest
         EGLDisplay dpy    = window->getDisplay();
         EGLConfig config  = window->getConfig();
 
-        constexpr EGLint kPBufferSize = kSize;
+        constexpr EGLint kPBufferSize = 256;
 
         std::vector<std::thread> threads(threadCount);
         for (size_t threadIdx = 0; threadIdx < threadCount; threadIdx++)
@@ -116,10 +87,6 @@ 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
@@ -235,88 +202,7 @@ TEST_P(MultithreadingTest, MultiContextDraw)
     runMultithreadedGLTest(testBody, 4);
 }
 
-// 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.
+// Test that multiple threads can draw and read back pixels correctly.
 // Using eglSwapBuffers stresses race conditions around use of QueueSerials.
 TEST_P(MultithreadingTest, MultiContextDrawWithSwapBuffers)
 {
@@ -480,8 +366,6 @@ ANGLE_INSTANTIATE_TEST(MultithreadingTest,
                        WithNoVirtualContexts(ES2_OPENGL()),
                        WithNoVirtualContexts(ES3_OPENGL()),
                        WithNoVirtualContexts(ES2_OPENGLES()),
-                       WithNoVirtualContexts(ES3_OPENGLES()),
-                       WithNoVirtualContexts(ES2_VULKAN()),
-                       WithNoVirtualContexts(ES3_VULKAN()));
+                       WithNoVirtualContexts(ES3_OPENGLES()));
 
 }  // namespace angle