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/+/2332677Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793591}

gpu/command_buffer/service/external_semaphore.cc

@@ -165,12 +165,7 @@ void ExternalSemaphore::Reset() {
   }
 
   if (gl_semaphore_ != 0) {
-    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)
+    if (gl::GLApi* api = gl::g_current_gl_context)
       api->glDeleteSemaphoresEXTFn(1, &gl_semaphore_);
   }
 
@@ -188,6 +183,12 @@ 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,8 +39,9 @@ class ExternalSemaphore {
 
   void Reset();
 
-  // Get the GL semaphore. The ownership is not transferred to caller.
-  unsigned int GetGLSemaphore();
+  // Take the GL semaphore. The ownership is transferred to caller. The caller
+  // is responsible for releasing it.
+  unsigned int TakeGLSemaphore();
 
   // Get a VkSemaphore. The ownership is not transferred to caller.
   VkSemaphore GetVkSemaphore();
@@ -49,6 +50,13 @@ 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,11 +6,9 @@
 
 #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 {
@@ -28,8 +26,8 @@ constexpr size_t kMaxSemaphoresInPool = 16;
 }  // namespace
 
 ExternalSemaphorePool::ExternalSemaphorePool(
-    SharedContextState* shared_context_state)
-    : shared_context_state_(shared_context_state) {}
+    viz::VulkanContextProvider* context_provider)
+    : context_provider_(context_provider) {}
 
 ExternalSemaphorePool::~ExternalSemaphorePool() = default;
 
@@ -39,8 +37,7 @@ ExternalSemaphore ExternalSemaphorePool::GetOrCreateSemaphore() {
     semaphores_.pop_front();
     return semaphore;
   }
-  return ExternalSemaphore::Create(
-      shared_context_state_->vk_context_provider());
+  return ExternalSemaphore::Create(context_provider_);
 }
 
 void ExternalSemaphorePool::ReturnSemaphore(ExternalSemaphore semaphore) {
@@ -53,19 +50,16 @@ void ExternalSemaphorePool::ReturnSemaphores(
     std::vector<ExternalSemaphore> semaphores) {
   DCHECK_LE(semaphores_.size(), kMaxSemaphoresInPool);
 
-  while (!semaphores.empty() && semaphores_.size() < kMaxSemaphoresInPool) {
-    auto& semaphore = semaphores.back();
+#if DCHECK_IS_ON()
+  for (auto& semaphore : semaphores)
     DCHECK(semaphore);
-    semaphores_.emplace_back(std::move(semaphore));
-    semaphores.pop_back();
-  }
-
-  if (semaphores.empty())
-    return;
+#endif
 
-  // Need a GL context current for releasing semaphores.
-  if (!gl::GLContext::GetCurrent())
-    shared_context_state_->MakeCurrent(/*surface=*/nullptr, /*needs_gl=*/true);
+  std::move(
+      semaphores.begin(),
+      semaphores.begin() + std::min(kMaxSemaphoresInPool - semaphores_.size(),
+                                    semaphores.size()),
+      std::back_inserter(semaphores_));
 }
 
 void ExternalSemaphorePool::ReturnSemaphoresWithFenceHelper(
@@ -77,10 +71,7 @@ void ExternalSemaphorePool::ReturnSemaphoresWithFenceHelper(
 
   if (semaphores.empty())
     return;
-
-  auto* fence_helper = shared_context_state_->vk_context_provider()
-                           ->GetDeviceQueue()
-                           ->GetFenceHelper();
+  auto* fence_helper = 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,13 +13,15 @@
 #include "base/memory/weak_ptr.h"
 #include "gpu/command_buffer/service/external_semaphore.h"
 
-namespace gpu {
+namespace viz {
+class VulkanContextProvider;
+}
 
-class SharedContextState;
+namespace gpu {
 
 class ExternalSemaphorePool {
  public:
-  explicit ExternalSemaphorePool(SharedContextState* shared_context_state);
+  explicit ExternalSemaphorePool(viz::VulkanContextProvider* context_provider);
   ~ExternalSemaphorePool();
 
   ExternalSemaphorePool(const ExternalSemaphorePool&) = delete;
@@ -41,7 +43,7 @@ class ExternalSemaphorePool {
       std::vector<ExternalSemaphore> semaphores);
 
  private:
-  SharedContextState* const shared_context_state_;
+  viz::VulkanContextProvider* const context_provider_;
   base::circular_deque<ExternalSemaphore> semaphores_;
   base::WeakPtrFactory<ExternalSemaphorePool> weak_ptr_factory_{this};
 };

gpu/command_buffer/service/external_vk_image_backing.cc

@@ -400,8 +400,7 @@ bool ExternalVkImageBacking::BeginAccess(
     GLuint texture_id = texture_passthrough_
                             ? texture_passthrough_->service_id()
                             : texture_->service_id();
-    if (!gl::GLContext::GetCurrent())
-      context_state()->MakeCurrent(/*gl_surface=*/nullptr, /*needs_gl=*/true);
+    context_state()->MakeCurrent(/*gl_surface=*/nullptr, /*needs_gl=*/true);
 
     GrVkImageInfo info;
     auto result = backend_texture_.getVkImageInfo(&info);
@@ -483,8 +482,7 @@ void ExternalVkImageBacking::EndAccess(bool readonly,
     GLuint texture_id = texture_passthrough_
                             ? texture_passthrough_->service_id()
                             : texture_->service_id();
-    if (!gl::GLContext::GetCurrent())
-      context_state()->MakeCurrent(/*gl_surface=*/nullptr, /*needs_gl=*/true);
+    context_state()->MakeCurrent(/*gl_surface=*/nullptr, /*needs_gl=*/true);
     std::vector<ExternalSemaphore> external_semaphores;
     BeginAccessInternal(true, &external_semaphores);
     DCHECK_LE(external_semaphores.size(), 1u);
@@ -540,18 +538,10 @@ 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.
-    // |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)));
+    fence_helper()->EnqueueCleanupTaskForSubmittedWork(
+        base::BindOnce([](std::vector<ExternalSemaphore>,
+                          VulkanDeviceQueue* device_queue, bool device_lost) {},
+                       std::move(semaphores)));
   }
 }
 

gpu/command_buffer/service/external_vk_image_gl_representation.cc

@@ -58,12 +58,13 @@ void ExternalVkImageGLRepresentationShared::AcquireTexture(
     ExternalSemaphore* semaphore,
     GLuint texture_id,
     VkImageLayout src_layout) {
-  GLuint gl_semaphore = semaphore->GetGLSemaphore();
+  GLuint gl_semaphore = semaphore->TakeGLSemaphore();
   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);
   }
 }
 
@@ -82,7 +83,7 @@ ExternalSemaphore ExternalVkImageGLRepresentationShared::ReleaseTexture(
     return {};
   }
 
-  GLuint gl_semaphore = semaphore.GetGLSemaphore();
+  GLuint gl_semaphore = semaphore.TakeGLSemaphore();
   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.
@@ -96,6 +97,7 @@ 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,10 +55,9 @@ gl::GLContextAttribs GenerateGLContextAttribs(
     attribs.webgl_compatibility_context =
         IsWebGLContextType(attribs_helper.context_type);
 
-    // Always use the global texture and semaphore share group for the
-    // passthrough command decoder
+    // Always use the global texture 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>(this);
+            std::make_unique<ExternalSemaphorePool>(vk_context_provider_);
 #endif
         use_virtualized_gl_contexts_ = false;
         DCHECK(gr_context_);

ui/gl/gl_context.h

@@ -82,7 +82,6 @@ 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,11 +35,6 @@
 #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
@@ -188,14 +183,6 @@ 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,7 +188,6 @@ 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;
@@ -944,8 +943,6 @@ 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 =
@@ -1118,10 +1115,6 @@ 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,7 +113,6 @@ 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();