Reland "vulkan: reuse gl semaphores"

This reverts commit 3bd0e5ef.

Reason for revert: I incorrectly identified this as the cause of crbug.com/1111764

Original change's description:
> Revert "vulkan: reuse gl semaphores"
> 
> This reverts commit 5c8c9a69.
> 
> Reason for revert: Two Vulkan angle_perftests have begun crashing consistently with this change in the blame range. Reverting to confirm if this is the cause.
> 
> Otherwise there is also an Angle roll in the range.
> 
> crbug.com/1111764
> 
> 
> Original change's description:
> > vulkan: reuse gl semaphores
> > 
> > For passthrough, with extension EGL_ANGLE_display_semaphore_share_group,
> > semaphores are shared globally.
> > For GLES2Decoder, all GL contexts are in the same shared group, so
> > semaphores are also shared globally.
> > So we can reused GL semaphores cross GL contexts.
> > 
> > Bug: 1004772
> > Change-Id: I1d23cdc991b2e99d6fe70fdd0560c97dafe73e7c
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2319368
> > Commit-Queue: Peng Huang <penghuang@chromium.org>
> > Reviewed-by: Jonathan Backer <backer@chromium.org>
> > Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#793398}
> 
> TBR=penghuang@chromium.org,backer@chromium.org,vasilyt@chromium.org
> 
> Change-Id: I63953b02f7f06a6e77a3266e3fc4eadee2f882f3
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1004772
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2332677
> Reviewed-by: Jonathan Ross <jonross@chromium.org>
> Commit-Queue: Jonathan Ross <jonross@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#793591}

TBR=penghuang@chromium.org,backer@chromium.org,jonross@chromium.org,vasilyt@chromium.org

# Not skipping CQ checks because this is a reland.

Bug: 1004772
Change-Id: I06ccd88922ba2f7163fe553e86529fc550689a87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2332837Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793657}

gpu/command_buffer/service/external_semaphore.cc

@@ -165,7 +165,12 @@ void ExternalSemaphore::Reset() {
   }
 
   if (gl_semaphore_ != 0) {
-    if (gl::GLApi* api = gl::g_current_gl_context)
+    auto* current_gl = gl::g_current_gl_context_tls->Get();
+    auto* api = current_gl->Driver ? current_gl->Api : nullptr;
+    // We assume there is always one GL context current. If there isn't a
+    // GL context current, we assume the last GL context is destroyed, in that
+    // case, we will skip glDeleteSemaphoresEXT().
+    if (api)
       api->glDeleteSemaphoresEXTFn(1, &gl_semaphore_);
   }
 
@@ -183,12 +188,6 @@ unsigned int ExternalSemaphore::GetGLSemaphore() {
   return gl_semaphore_;
 }
 
-unsigned int ExternalSemaphore::TakeGLSemaphore() {
-  auto gl_semaphore = GetGLSemaphore();
-  gl_semaphore_ = 0;
-  return gl_semaphore;
-}
-
 VkSemaphore ExternalSemaphore::GetVkSemaphore() {
   DCHECK(handle_.is_valid());
   if (semaphore_ == VK_NULL_HANDLE) {

gpu/command_buffer/service/external_semaphore.h

@@ -39,9 +39,8 @@ class ExternalSemaphore {
 
   void Reset();
 
-  // Take the GL semaphore. The ownership is transferred to caller. The caller
-  // is responsible for releasing it.
-  unsigned int TakeGLSemaphore();
+  // Get the GL semaphore. The ownership is not transferred to caller.
+  unsigned int GetGLSemaphore();
 
   // Get a VkSemaphore. The ownership is not transferred to caller.
   VkSemaphore GetVkSemaphore();
@@ -50,13 +49,6 @@ class ExternalSemaphore {
   SemaphoreHandle handle() { return handle_.Duplicate(); }
 
  private:
-  // GL semaphore cannot be shared between passthrough GL contexts,
-  // since gl contexts are not created with the same global shared group with
-  // passthrough. So we cannot reuse GL semaphores safely.
-  // TODO(penghuang): share GL semaphore across GL contexts and reuse
-  // GL semaphores.
-  unsigned int GetGLSemaphore();
-
   viz::VulkanContextProvider* context_provider_ = nullptr;
   VkSemaphore semaphore_ = VK_NULL_HANDLE;
   SemaphoreHandle handle_;

gpu/command_buffer/service/external_semaphore_pool.cc

@@ -6,9 +6,11 @@
 
 #include "build/build_config.h"
 #include "components/viz/common/gpu/vulkan_context_provider.h"
+#include "gpu/command_buffer/service/shared_context_state.h"
 #include "gpu/vulkan/vulkan_device_queue.h"
 #include "gpu/vulkan/vulkan_fence_helper.h"
 #include "gpu/vulkan/vulkan_implementation.h"
+#include "ui/gl/gl_context.h"
 
 namespace gpu {
 namespace {
@@ -26,8 +28,8 @@ constexpr size_t kMaxSemaphoresInPool = 16;
 }  // namespace
 
 ExternalSemaphorePool::ExternalSemaphorePool(
-    viz::VulkanContextProvider* context_provider)
-    : context_provider_(context_provider) {}
+    SharedContextState* shared_context_state)
+    : shared_context_state_(shared_context_state) {}
 
 ExternalSemaphorePool::~ExternalSemaphorePool() = default;
 
@@ -37,7 +39,8 @@ ExternalSemaphore ExternalSemaphorePool::GetOrCreateSemaphore() {
     semaphores_.pop_front();
     return semaphore;
   }
-  return ExternalSemaphore::Create(context_provider_);
+  return ExternalSemaphore::Create(
+      shared_context_state_->vk_context_provider());
 }
 
 void ExternalSemaphorePool::ReturnSemaphore(ExternalSemaphore semaphore) {
@@ -50,16 +53,19 @@ void ExternalSemaphorePool::ReturnSemaphores(
     std::vector<ExternalSemaphore> semaphores) {
   DCHECK_LE(semaphores_.size(), kMaxSemaphoresInPool);
 
-#if DCHECK_IS_ON()
-  for (auto& semaphore : semaphores)
+  while (!semaphores.empty() && semaphores_.size() < kMaxSemaphoresInPool) {
+    auto& semaphore = semaphores.back();
     DCHECK(semaphore);
-#endif
+    semaphores_.emplace_back(std::move(semaphore));
+    semaphores.pop_back();
+  }
 
-  std::move(
-      semaphores.begin(),
-      semaphores.begin() + std::min(kMaxSemaphoresInPool - semaphores_.size(),
-                                    semaphores.size()),
-      std::back_inserter(semaphores_));
+  if (semaphores.empty())
+    return;
+
+  // Need a GL context current for releasing semaphores.
+  if (!gl::GLContext::GetCurrent())
+    shared_context_state_->MakeCurrent(/*surface=*/nullptr, /*needs_gl=*/true);
 }
 
 void ExternalSemaphorePool::ReturnSemaphoresWithFenceHelper(
@@ -71,7 +77,10 @@ void ExternalSemaphorePool::ReturnSemaphoresWithFenceHelper(
 
   if (semaphores.empty())
     return;
-  auto* fence_helper = context_provider_->GetDeviceQueue()->GetFenceHelper();
+
+  auto* fence_helper = shared_context_state_->vk_context_provider()
+                           ->GetDeviceQueue()
+                           ->GetFenceHelper();
   fence_helper->EnqueueCleanupTaskForSubmittedWork(base::BindOnce(
       [](base::WeakPtr<ExternalSemaphorePool> pool,
          std::vector<ExternalSemaphore> semaphores,

gpu/command_buffer/service/external_semaphore_pool.h

@@ -13,15 +13,13 @@
 #include "base/memory/weak_ptr.h"
 #include "gpu/command_buffer/service/external_semaphore.h"
 
-namespace viz {
-class VulkanContextProvider;
-}
-
 namespace gpu {
 
+class SharedContextState;
+
 class ExternalSemaphorePool {
  public:
-  explicit ExternalSemaphorePool(viz::VulkanContextProvider* context_provider);
+  explicit ExternalSemaphorePool(SharedContextState* shared_context_state);
   ~ExternalSemaphorePool();
 
   ExternalSemaphorePool(const ExternalSemaphorePool&) = delete;
@@ -43,7 +41,7 @@ class ExternalSemaphorePool {
       std::vector<ExternalSemaphore> semaphores);
 
  private:
-  viz::VulkanContextProvider* const context_provider_;
+  SharedContextState* const shared_context_state_;
   base::circular_deque<ExternalSemaphore> semaphores_;
   base::WeakPtrFactory<ExternalSemaphorePool> weak_ptr_factory_{this};
 };

gpu/command_buffer/service/external_vk_image_backing.cc

@@ -400,7 +400,8 @@ bool ExternalVkImageBacking::BeginAccess(
     GLuint texture_id = texture_passthrough_
                             ? texture_passthrough_->service_id()
                             : texture_->service_id();
-    context_state()->MakeCurrent(/*gl_surface=*/nullptr, /*needs_gl=*/true);
+    if (!gl::GLContext::GetCurrent())
+      context_state()->MakeCurrent(/*gl_surface=*/nullptr, /*needs_gl=*/true);
 
     GrVkImageInfo info;
     auto result = backend_texture_.getVkImageInfo(&info);
@@ -482,7 +483,8 @@ void ExternalVkImageBacking::EndAccess(bool readonly,
     GLuint texture_id = texture_passthrough_
                             ? texture_passthrough_->service_id()
                             : texture_->service_id();
-    context_state()->MakeCurrent(/*gl_surface=*/nullptr, /*needs_gl=*/true);
+    if (!gl::GLContext::GetCurrent())
+      context_state()->MakeCurrent(/*gl_surface=*/nullptr, /*needs_gl=*/true);
     std::vector<ExternalSemaphore> external_semaphores;
     BeginAccessInternal(true, &external_semaphores);
     DCHECK_LE(external_semaphores.size(), 1u);
@@ -538,10 +540,18 @@ void ExternalVkImageBacking::AddSemaphoresToPendingListOrRelease(
     // signalling but have not been signalled. In that case, we have to release
     // them via fence helper to make sure all submitted GPU works is finished
     // before releasing them.
-    fence_helper()->EnqueueCleanupTaskForSubmittedWork(
-        base::BindOnce([](std::vector<ExternalSemaphore>,
-                          VulkanDeviceQueue* device_queue, bool device_lost) {},
-                       std::move(semaphores)));
+    // |context_state_| is out live fence_helper, so it is safe to use
+    // base::Unretained(context_state_).
+    fence_helper()->EnqueueCleanupTaskForSubmittedWork(base::BindOnce(
+        [](SharedContextState* shared_context_state,
+           std::vector<ExternalSemaphore>, VulkanDeviceQueue* device_queue,
+           bool device_lost) {
+          if (!gl::GLContext::GetCurrent()) {
+            shared_context_state->MakeCurrent(/*surface=*/nullptr,
+                                              /*needs_gl=*/true);
+          }
+        },
+        base::Unretained(context_state_), std::move(semaphores)));
   }
 }
 

gpu/command_buffer/service/external_vk_image_gl_representation.cc

@@ -58,13 +58,12 @@ void ExternalVkImageGLRepresentationShared::AcquireTexture(
     ExternalSemaphore* semaphore,
     GLuint texture_id,
     VkImageLayout src_layout) {
-  GLuint gl_semaphore = semaphore->TakeGLSemaphore();
+  GLuint gl_semaphore = semaphore->GetGLSemaphore();
   if (gl_semaphore) {
     GLenum gl_layout = ToGLImageLayout(src_layout);
     auto* api = gl::g_current_gl_context;
     api->glWaitSemaphoreEXTFn(gl_semaphore, 0, nullptr, 1, &texture_id,
                               &gl_layout);
-    api->glDeleteSemaphoresEXTFn(1, &gl_semaphore);
   }
 }
 
@@ -83,7 +82,7 @@ ExternalSemaphore ExternalVkImageGLRepresentationShared::ReleaseTexture(
     return {};
   }
 
-  GLuint gl_semaphore = semaphore.TakeGLSemaphore();
+  GLuint gl_semaphore = semaphore.GetGLSemaphore();
   if (!gl_semaphore) {
     // TODO(crbug.com/933452): We should be able to semaphore_handle this
     // failure more gracefully rather than shutting down the whole process.
@@ -97,7 +96,6 @@ ExternalSemaphore ExternalVkImageGLRepresentationShared::ReleaseTexture(
   auto* api = gl::g_current_gl_context;
   api->glSignalSemaphoreEXTFn(gl_semaphore, 0, nullptr, 1, &texture_id,
                               &gl_layout);
-  api->glDeleteSemaphoresEXTFn(1, &gl_semaphore);
   // Base on the spec, the glSignalSemaphoreEXT() call just inserts signal
   // semaphore command in the gl context. It may or may not flush the context
   // which depends on the implementation. So to make it safe, we always call

gpu/command_buffer/service/service_utils.cc

@@ -55,9 +55,10 @@ gl::GLContextAttribs GenerateGLContextAttribs(
     attribs.webgl_compatibility_context =
         IsWebGLContextType(attribs_helper.context_type);
 
-    // Always use the global texture share group for the passthrough command
-    // decoder
+    // Always use the global texture and semaphore share group for the
+    // passthrough command decoder
     attribs.global_texture_share_group = true;
+    attribs.global_semaphore_share_group = true;
 
     attribs.robust_resource_initialization = true;
     attribs.robust_buffer_access = true;

gpu/command_buffer/service/shared_context_state.cc

@@ -139,7 +139,7 @@ SharedContextState::SharedContextState(
 #if BUILDFLAG(ENABLE_VULKAN)
         gr_context_ = vk_context_provider_->GetGrContext();
         external_semaphore_pool_ =
-            std::make_unique<ExternalSemaphorePool>(vk_context_provider_);
+            std::make_unique<ExternalSemaphorePool>(this);
 #endif
         use_virtualized_gl_contexts_ = false;
         DCHECK(gr_context_);

ui/gl/gl_context.h

@@ -82,6 +82,7 @@ struct GL_EXPORT GLContextAttribs {
   bool bind_generates_resource = true;
   bool webgl_compatibility_context = false;
   bool global_texture_share_group = false;
+  bool global_semaphore_share_group = false;
   bool robust_resource_initialization = false;
   bool robust_buffer_access = false;
   int client_major_es_version = 3;

ui/gl/gl_context_egl.cc

@@ -35,6 +35,11 @@
 #define EGL_DISPLAY_TEXTURE_SHARE_GROUP_ANGLE 0x33AF
 #endif /* EGL_ANGLE_display_texture_share_group */
 
+#ifndef EGL_ANGLE_display_semaphore_share_group
+#define EGL_ANGLE_display_semaphore_share_group 1
+#define EGL_DISPLAY_SEMAPHORE_SHARE_GROUP_ANGLE 0x348D
+#endif /* EGL_ANGLE_display_semaphore_share_group */
+
 #ifndef EGL_ANGLE_create_context_client_arrays
 #define EGL_ANGLE_create_context_client_arrays 1
 #define EGL_CONTEXT_CLIENT_ARRAYS_ENABLED_ANGLE 0x3452
@@ -183,6 +188,14 @@ bool GLContextEGL::Initialize(GLSurface* compatible_surface,
     DCHECK(!attribs.global_texture_share_group);
   }
 
+  if (GLSurfaceEGL::IsDisplaySemaphoreShareGroupSupported()) {
+    context_attributes.push_back(EGL_DISPLAY_SEMAPHORE_SHARE_GROUP_ANGLE);
+    context_attributes.push_back(
+        attribs.global_semaphore_share_group ? EGL_TRUE : EGL_FALSE);
+  } else {
+    DCHECK(!attribs.global_semaphore_share_group);
+  }
+
   if (GLSurfaceEGL::IsCreateContextClientArraysSupported()) {
     // Disable client arrays if the context supports it
     context_attributes.push_back(EGL_CONTEXT_CLIENT_ARRAYS_ENABLED_ANGLE);

ui/gl/gl_surface_egl.cc

@@ -188,6 +188,7 @@ bool g_egl_ext_colorspace_display_p3_passthrough = false;
 bool g_egl_flexible_surface_compatibility_supported = false;
 bool g_egl_robust_resource_init_supported = false;
 bool g_egl_display_texture_share_group_supported = false;
+bool g_egl_display_semaphore_share_group_supported = false;
 bool g_egl_create_context_client_arrays_supported = false;
 bool g_egl_android_native_fence_sync_supported = false;
 bool g_egl_ext_pixel_format_float_supported = false;
@@ -943,6 +944,8 @@ bool GLSurfaceEGL::InitializeOneOffCommon() {
 
   g_egl_display_texture_share_group_supported =
       HasEGLExtension("EGL_ANGLE_display_texture_share_group");
+  g_egl_display_semaphore_share_group_supported =
+      HasEGLExtension("EGL_ANGLE_display_semaphore_share_group");
   g_egl_create_context_client_arrays_supported =
       HasEGLExtension("EGL_ANGLE_create_context_client_arrays");
   g_egl_robust_resource_init_supported =
@@ -1115,6 +1118,10 @@ bool GLSurfaceEGL::IsDisplayTextureShareGroupSupported() {
   return g_egl_display_texture_share_group_supported;
 }
 
+bool GLSurfaceEGL::IsDisplaySemaphoreShareGroupSupported() {
+  return g_egl_display_semaphore_share_group_supported;
+}
+
 bool GLSurfaceEGL::IsCreateContextClientArraysSupported() {
   return g_egl_create_context_client_arrays_supported;
 }

ui/gl/gl_surface_egl.h

@@ -113,6 +113,7 @@ class GL_EXPORT GLSurfaceEGL : public GLSurface {
   static bool IsEGLFlexibleSurfaceCompatibilitySupported();
   static bool IsRobustResourceInitSupported();
   static bool IsDisplayTextureShareGroupSupported();
+  static bool IsDisplaySemaphoreShareGroupSupported();
   static bool IsCreateContextClientArraysSupported();
   static bool IsAndroidNativeFenceSyncSupported();
   static bool IsPixelFormatFloatSupported();