Move LayoutCaches to ShareGroup

Testing with TSN found a race condition with RefCounted objects
(DescriptorSetLayout and PipelineLayout). Rather than add more lock
calls to protect accesses to mRefCount and mObject recommendation was to
put these caches in the ShareGroup (basically part of the context).
Locking at the GL level will ensure that two threads that share the same
context will not access the ShareGroup at the same time.

The ShareGroup also works because these layouts are not destroyed until
the context is destroyed so don't have to worry about other threads
(e.g. command processor thread) accessing them.

Bug: b/168744561
Change-Id: Icc0aa07bf4787a69572d6ec62da2f21d286232c3
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2437509
Commit-Queue: Courtney Goeltzenleuchter <[email protected]>
Reviewed-by: Jamie Madill <[email protected]>
Reviewed-by: Charlie Lao <[email protected]>

Author: courtney-g

src/libANGLE/Context.cpp

@@ -619,12 +619,14 @@ egl::Error Context::onDestroy(const egl::Display *display)
     mState.mFramebufferManager->release(this);
     mState.mMemoryObjectManager->release(this);
     mState.mSemaphoreManager->release(this);
-    mState.mShareGroup->release(this);
 
     mThreadPool.reset();
 
     mImplementation->onDestroy(this);
 
+    // Backend requires implementation to be destroyed first to close down all the objects
+    mState.mShareGroup->release(display);
+
     mOverlay.destroy(this);
 
     return egl::NoError();

src/libANGLE/Display.cpp

@@ -471,10 +471,14 @@ void ShareGroup::addRef()
     mRefCount++;
 }
 
-void ShareGroup::release(const gl::Context *context)
+void ShareGroup::release(const Display *display)
 {
     if (--mRefCount == 0)
     {
+        if (mImplementation)
+        {
+            mImplementation->onDestroy(display);
+        }
         delete this;
     }
 }

src/libANGLE/Display.h

@@ -73,7 +73,7 @@ class ShareGroup final : angle::NonCopyable
 
     void addRef();
 
-    void release(const gl::Context *context);
+    void release(const egl::Display *display);
 
     rx::ShareGroupImpl *getImplementation() const { return mImplementation; }
 

src/libANGLE/renderer/DisplayImpl.h

@@ -57,6 +57,7 @@ class ShareGroupImpl : angle::NonCopyable
   public:
     ShareGroupImpl() {}
     virtual ~ShareGroupImpl() {}
+    virtual void onDestroy(const egl::Display *display) {}
 };
 
 class DisplayImpl : public EGLImplFactory, public angle::Subject

src/libANGLE/renderer/vulkan/ContextVk.cpp

@@ -852,7 +852,7 @@ angle::Result ContextVk::initialize()
 
         vk::DescriptorSetLayoutDesc desc =
             getDriverUniformsDescriptorSetDesc(kPipelineStages[pipeline]);
-        ANGLE_TRY(mRenderer->getDescriptorSetLayout(
+        ANGLE_TRY(getDescriptorSetLayoutCache().getDescriptorSetLayout(
             this, desc, &mDriverUniforms[pipeline].descriptorSetLayout));
 
         vk::DescriptorSetLayoutBindingVector bindingVector;

src/libANGLE/renderer/vulkan/ContextVk.h

@@ -16,6 +16,7 @@
 #include "common/vulkan/vk_headers.h"
 #include "libANGLE/renderer/ContextImpl.h"
 #include "libANGLE/renderer/renderer_utils.h"
+#include "libANGLE/renderer/vulkan/DisplayVk.h"
 #include "libANGLE/renderer/vulkan/OverlayVk.h"
 #include "libANGLE/renderer/vulkan/PersistentCommandPool.h"
 #include "libANGLE/renderer/vulkan/RendererVk.h"
@@ -230,6 +231,17 @@ class ContextVk : public ContextImpl, public vk::Context
                                                                    const GLuint *baseInstances,
                                                                    GLsizei drawcount) override;
 
+    // ShareGroup
+    ShareGroupVk *getShareGroupVk() { return mShareGroupVk; }
+    PipelineLayoutCache &getPipelineLayoutCache()
+    {
+        return mShareGroupVk->getPipelineLayoutCache();
+    }
+    DescriptorSetLayoutCache &getDescriptorSetLayoutCache()
+    {
+        return mShareGroupVk->getDescriptorSetLayoutCache();
+    }
+
     // Device loss
     gl::GraphicsResetStatus getResetStatus() override;
 

src/libANGLE/renderer/vulkan/DisplayVk.cpp

@@ -289,4 +289,12 @@ bool DisplayVk::isRobustResourceInitEnabled() const
     // We return true if any surface was created with robust resource init enabled.
     return mHasSurfaceWithRobustInit;
 }
+
+void ShareGroupVk::onDestroy(const egl::Display *display)
+{
+    DisplayVk *displayVk = vk::GetImpl(display);
+
+    mPipelineLayoutCache.destroy(displayVk->getDevice());
+    mDescriptorSetLayoutCache.destroy(displayVk->getDevice());
+}
 }  // namespace rx

src/libANGLE/renderer/vulkan/DisplayVk.h

@@ -12,6 +12,7 @@
 
 #include "common/MemoryBuffer.h"
 #include "libANGLE/renderer/DisplayImpl.h"
+#include "libANGLE/renderer/vulkan/vk_cache_utils.h"
 #include "libANGLE/renderer/vulkan/vk_utils.h"
 
 namespace rx
@@ -22,6 +23,20 @@ class ShareGroupVk : public ShareGroupImpl
 {
   public:
     ShareGroupVk() {}
+    void onDestroy(const egl::Display *display) override;
+
+    // PipelineLayoutCache and DescriptorSetLayoutCache can be shared between multiple threads
+    // accessing them via shared contexts. The ShareGroup locks around gl entrypoints ensuring
+    // synchronous update to the caches.
+    PipelineLayoutCache &getPipelineLayoutCache() { return mPipelineLayoutCache; }
+    DescriptorSetLayoutCache &getDescriptorSetLayoutCache() { return mDescriptorSetLayoutCache; }
+
+  private:
+    // ANGLE uses a PipelineLayout cache to store compatible pipeline layouts.
+    PipelineLayoutCache mPipelineLayoutCache;
+
+    // DescriptorSetLayouts are also managed in a cache.
+    DescriptorSetLayoutCache mDescriptorSetLayoutCache;
 };
 
 class DisplayVk : public DisplayImpl, public vk::Context

src/libANGLE/renderer/vulkan/ProgramExecutableVk.cpp

@@ -10,6 +10,7 @@
 
 #include "libANGLE/renderer/glslang_wrapper_utils.h"
 #include "libANGLE/renderer/vulkan/BufferVk.h"
+#include "libANGLE/renderer/vulkan/DisplayVk.h"
 #include "libANGLE/renderer/vulkan/GlslangWrapperVk.h"
 #include "libANGLE/renderer/vulkan/ProgramPipelineVk.h"
 #include "libANGLE/renderer/vulkan/ProgramVk.h"
@@ -736,7 +737,6 @@ angle::Result ProgramExecutableVk::createPipelineLayout(
 {
     const gl::State &glState                   = glContext->getState();
     ContextVk *contextVk                       = vk::GetImpl(glContext);
-    RendererVk *renderer                       = contextVk->getRenderer();
     gl::TransformFeedback *transformFeedback   = glState.getCurrentTransformFeedback();
     const gl::ProgramExecutable &glExecutable  = getGlExecutable();
     const gl::ShaderBitSet &linkedShaderStages = glExecutable.getLinkedShaderStages();
@@ -786,7 +786,7 @@ angle::Result ProgramExecutableVk::createPipelineLayout(
                                                        xfbBufferCount, &uniformsAndXfbSetDesc);
     }
 
-    ANGLE_TRY(renderer->getDescriptorSetLayout(
+    ANGLE_TRY(contextVk->getDescriptorSetLayoutCache().getDescriptorSetLayout(
         contextVk, uniformsAndXfbSetDesc,
         &mDescriptorSetLayouts[ToUnderlying(DescriptorSetIndex::UniformsAndXfb)]));
 
@@ -814,7 +814,7 @@ angle::Result ProgramExecutableVk::createPipelineLayout(
                                   contextVk->useOldRewriteStructSamplers(), &resourcesSetDesc);
     }
 
-    ANGLE_TRY(renderer->getDescriptorSetLayout(
+    ANGLE_TRY(contextVk->getDescriptorSetLayoutCache().getDescriptorSetLayout(
         contextVk, resourcesSetDesc,
         &mDescriptorSetLayouts[ToUnderlying(DescriptorSetIndex::ShaderResource)]));
 
@@ -829,7 +829,7 @@ angle::Result ProgramExecutableVk::createPipelineLayout(
                                     activeTextures, &texturesSetDesc);
     }
 
-    ANGLE_TRY(renderer->getDescriptorSetLayout(
+    ANGLE_TRY(contextVk->getDescriptorSetLayoutCache().getDescriptorSetLayout(
         contextVk, texturesSetDesc,
         &mDescriptorSetLayouts[ToUnderlying(DescriptorSetIndex::Texture)]));
 
@@ -838,7 +838,7 @@ angle::Result ProgramExecutableVk::createPipelineLayout(
         glExecutable.isCompute() ? VK_SHADER_STAGE_COMPUTE_BIT : VK_SHADER_STAGE_ALL_GRAPHICS;
     vk::DescriptorSetLayoutDesc driverUniformsSetDesc =
         contextVk->getDriverUniformsDescriptorSetDesc(driverUniformsStages);
-    ANGLE_TRY(renderer->getDescriptorSetLayout(
+    ANGLE_TRY(contextVk->getDescriptorSetLayoutCache().getDescriptorSetLayout(
         contextVk, driverUniformsSetDesc,
         &mDescriptorSetLayouts[ToUnderlying(DescriptorSetIndex::DriverUniforms)]));
 
@@ -852,8 +852,8 @@ angle::Result ProgramExecutableVk::createPipelineLayout(
     pipelineLayoutDesc.updateDescriptorSetLayout(DescriptorSetIndex::DriverUniforms,
                                                  driverUniformsSetDesc);
 
-    ANGLE_TRY(renderer->getPipelineLayout(contextVk, pipelineLayoutDesc, mDescriptorSetLayouts,
-                                          &mPipelineLayout));
+    ANGLE_TRY(contextVk->getPipelineLayoutCache().getPipelineLayout(
+        contextVk, pipelineLayoutDesc, mDescriptorSetLayouts, &mPipelineLayout));
 
     // Initialize descriptor pools.
     ANGLE_TRY(initDynamicDescriptorPools(

src/libANGLE/renderer/vulkan/RendererVk.cpp

@@ -535,9 +535,6 @@ void RendererVk::onDestroy()
         mFenceRecycler.destroy(mDevice);
     }
 
-    mPipelineLayoutCache.destroy(mDevice);
-    mDescriptorSetLayoutCache.destroy(mDevice);
-
     mPipelineCache.destroy(mDevice);
     mSamplerCache.destroy(this);
     mYuvConversionCache.destroy(this);
@@ -2045,26 +2042,6 @@ const gl::Limitations &RendererVk::getNativeLimitations() const
     return mNativeLimitations;
 }
 
-angle::Result RendererVk::getDescriptorSetLayout(
-    ContextVk *context,
-    const vk::DescriptorSetLayoutDesc &desc,
-    vk::BindingPointer<vk::DescriptorSetLayout> *descriptorSetLayoutOut)
-{
-    std::lock_guard<decltype(mDescriptorSetLayoutCacheMutex)> lock(mDescriptorSetLayoutCacheMutex);
-    return mDescriptorSetLayoutCache.getDescriptorSetLayout(context, desc, descriptorSetLayoutOut);
-}
-
-angle::Result RendererVk::getPipelineLayout(
-    vk::Context *context,
-    const vk::PipelineLayoutDesc &desc,
-    const vk::DescriptorSetLayoutPointerArray &descriptorSetLayouts,
-    vk::BindingPointer<vk::PipelineLayout> *pipelineLayoutOut)
-{
-    std::lock_guard<decltype(mPipelineLayoutCacheMutex)> lock(mPipelineLayoutCacheMutex);
-    return mPipelineLayoutCache.getPipelineLayout(context, desc, descriptorSetLayouts,
-                                                  pipelineLayoutOut);
-}
-
 angle::Result RendererVk::getPipelineCacheSize(DisplayVk *displayVk, size_t *pipelineCacheSizeOut)
 {
     VkResult result = mPipelineCache.getCacheData(mDevice, pipelineCacheSizeOut, nullptr);