Limit access to gpu thread in DisplayResourceProvider

This CL makes sure that DisplayResourceProvider return resources only
when GPU Thread is available to make sure ReleaseImageContexts will
complete in finite time.

This doesn't change anything for Chrome where GPU Thread is always
available, but on webview RenderThread that used for gpu access during
composition is available only during specific times (init, dtor,
DrawAndSwap)

Bug: 1068716

Change-Id: I8fa482ab7399d6ce7639c1d76187be86ce152059
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2138551
Commit-Queue: Vasiliy Telezhnikov <vasilyt@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757967}

android_webview/browser/gfx/hardware_renderer_viz.cc

@@ -124,6 +124,7 @@ HardwareRendererViz::OnViz::OnViz(
                        output_surface_provider->enable_shared_image());
 
   display_->SetVisible(true);
+  display_->DisableGPUAccessByDefault();
 }
 
 HardwareRendererViz::OnViz::~OnViz() {

components/viz/service/display/display.cc

@@ -273,6 +273,9 @@ Display::~Display() {
   allow_schedule_gpu_task_during_destruction_.reset(
       new gpu::ScopedAllowScheduleGpuTask);
 #endif
+  if (resource_provider_) {
+    resource_provider_->SetAllowAccessToGPUThread(true);
+  }
 #if defined(OS_ANDROID)
   // In certain cases, drivers hang when tearing down the display. Finishing
   // before teardown appears to address this. As we're during display teardown,
@@ -585,6 +588,13 @@ bool Display::DrawAndSwap(base::TimeTicks expected_display_time) {
   // switching because the scheduler knows sync token dependencies at that time.
   DisplayResourceProvider::ScopedBatchReturnResources returner(
       resource_provider_.get());
+
+  // Allow access to GPU. If there was no access before DisplayResourceProvider
+  // will return unused resources. So we allow access to GPU after setting up
+  // ScopedBatchReturnResources to delay this after the draw.
+  DisplayResourceProvider::ScopedAllowGPUThreadAccess allow_gpu_access(
+      resource_provider_.get());
+
   base::ElapsedTimer aggregate_timer;
   aggregate_timer.Begin();
   CompositorFrame frame;
@@ -1139,4 +1149,9 @@ base::ScopedClosureRunner Display::GetCacheBackBufferCb() {
   return output_surface_->GetCacheBackBufferCb();
 }
 
+void Display::DisableGPUAccessByDefault() {
+  DCHECK(resource_provider_);
+  resource_provider_->SetAllowAccessToGPUThread(false);
+}
+
 }  // namespace viz

components/viz/service/display/display.h

@@ -115,6 +115,10 @@ class VIZ_SERVICE_EXPORT Display : public DisplaySchedulerClient,
   void SetVisible(bool visible);
   void Resize(const gfx::Size& new_size);
 
+  // This disallows resource provider to access GPU thread to unlock resources
+  // outside of Initialize, DrawAndSwap and dtor.
+  void DisableGPUAccessByDefault();
+
   // Stop drawing until Resize() is called with a new size. If the display
   // hasn't drawn a frame at the current size *and* it's possible to immediately
   // draw then this will run DrawAndSwap() first.

components/viz/service/display/display_resource_provider.cc

@@ -22,6 +22,7 @@
 #include "gpu/command_buffer/client/context_support.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "gpu/command_buffer/common/shared_image_trace_utils.h"
+#include "gpu/ipc/scheduler_sequence.h"
 #include "third_party/skia/include/gpu/GrBackendSurface.h"
 #include "ui/gl/trace_util.h"
 
@@ -36,6 +37,19 @@ base::AtomicSequenceNumber g_next_display_resource_provider_tracing_id;
 
 }  // namespace
 
+class ScopedAllowGpuAccessForDisplayResourceProvider {
+ public:
+  ~ScopedAllowGpuAccessForDisplayResourceProvider() = default;
+
+  explicit ScopedAllowGpuAccessForDisplayResourceProvider(
+      DisplayResourceProvider* provider) {
+    DCHECK(provider->can_access_gpu_thread_);
+  }
+
+ private:
+  gpu::ScopedAllowScheduleGpuTask allow_gpu_;
+};
+
 class ScopedSetActiveTexture {
  public:
   ScopedSetActiveTexture(GLES2Interface* gl, GLenum unit)
@@ -622,8 +636,11 @@ void DisplayResourceProvider::DeleteAndReturnUnusedResourcesToChild(
   if (unused.empty() && !child_info->marked_for_deletion)
     return;
 
-  // Store unused resources while batching is enabled.
-  if (batch_return_resources_lock_count_ > 0) {
+  // Store unused resources while batching is enabled or we can't access gpu
+  // thread right now.
+  // TODO(vasilyt): Technically we need to delay only resources with
+  // |image_context|.
+  if (batch_return_resources_lock_count_ > 0 || !can_access_gpu_thread_) {
     int child_id = child_it->first;
     // Ensure that we have an entry in |batched_returning_resources_| for child
     // even if |unused| is empty, in case child is marked for deletion.
@@ -729,6 +746,7 @@ void DisplayResourceProvider::DeleteAndReturnUnusedResourcesToChild(
 
   if (external_use_client_) {
     if (!image_contexts_to_return.empty()) {
+      ScopedAllowGpuAccessForDisplayResourceProvider allow_gpu(this);
       gpu::SyncToken sync_token = external_use_client_->ReleaseImageContexts(
           std::move(image_contexts_to_return));
       for (auto* resource : external_used_resources) {
@@ -782,6 +800,27 @@ void DisplayResourceProvider::DestroyChildInternal(ChildMap::iterator it,
   DeleteAndReturnUnusedResourcesToChild(it, style, resources_for_child);
 }
 
+void DisplayResourceProvider::TryFlushBatchedResources() {
+  if (batch_return_resources_lock_count_ == 0 && can_access_gpu_thread_) {
+    for (auto& child_resources_kv : batched_returning_resources_) {
+      auto child_it = children_.find(child_resources_kv.first);
+
+      // Remove duplicates from child's unused resources.  Duplicates are
+      // possible when batching is enabled because resources are saved in
+      // |batched_returning_resources_| for removal, and not removed from the
+      // child's |child_to_parent_map|, so the same set of resources can be
+      // saved again using DeclareUsedResourcesForChild() or DestroyChild().
+      auto& unused_resources = child_resources_kv.second;
+      std::sort(unused_resources.begin(), unused_resources.end());
+      auto last = std::unique(unused_resources.begin(), unused_resources.end());
+      unused_resources.erase(last, unused_resources.end());
+
+      DeleteAndReturnUnusedResourcesToChild(child_it, NORMAL, unused_resources);
+    }
+    batched_returning_resources_.clear();
+  }
+}
+
 void DisplayResourceProvider::SetBatchReturnResources(bool batch) {
   if (batch) {
     DCHECK_GE(batch_return_resources_lock_count_, 0);
@@ -796,28 +835,18 @@ void DisplayResourceProvider::SetBatchReturnResources(bool batch) {
     if (batch_return_resources_lock_count_ == 0) {
       DCHECK(scoped_batch_read_access_);
       scoped_batch_read_access_.reset();
-      for (auto& child_resources_kv : batched_returning_resources_) {
-        auto child_it = children_.find(child_resources_kv.first);
-
-        // Remove duplicates from child's unused resources.  Duplicates are
-        // possible when batching is enabled because resources are saved in
-        // |batched_returning_resources_| for removal, and not removed from the
-        // child's |child_to_parent_map|, so the same set of resources can be
-        // saved again using DeclareUsedResourcesForChild() or DestroyChild().
-        auto& unused_resources = child_resources_kv.second;
-        std::sort(unused_resources.begin(), unused_resources.end());
-        auto last =
-            std::unique(unused_resources.begin(), unused_resources.end());
-        unused_resources.erase(last, unused_resources.end());
-
-        DeleteAndReturnUnusedResourcesToChild(child_it, NORMAL,
-                                              unused_resources);
-      }
-      batched_returning_resources_.clear();
+      TryFlushBatchedResources();
     }
   }
 }
 
+void DisplayResourceProvider::SetAllowAccessToGPUThread(bool allow) {
+  can_access_gpu_thread_ = allow;
+  if (allow) {
+    TryFlushBatchedResources();
+  }
+}
+
 DisplayResourceProvider::ScopedReadLockGL::ScopedReadLockGL(
     DisplayResourceProvider* resource_provider,
     ResourceId resource_id)
@@ -1115,4 +1144,16 @@ DisplayResourceProvider::ScopedBatchReadAccess::~ScopedBatchReadAccess() {
     gl_->EndBatchReadAccessSharedImageCHROMIUM();
 }
 
+DisplayResourceProvider::ScopedAllowGPUThreadAccess::ScopedAllowGPUThreadAccess(
+    DisplayResourceProvider* resource_provider)
+    : resource_provider_(resource_provider),
+      was_allowed_(resource_provider->can_access_gpu_thread_) {
+  resource_provider_->SetAllowAccessToGPUThread(true);
+}
+
+DisplayResourceProvider::ScopedAllowGPUThreadAccess::
+    ~ScopedAllowGPUThreadAccess() {
+  resource_provider_->SetAllowAccessToGPUThread(was_allowed_);
+}
+
 }  // namespace viz

components/viz/service/display/display_resource_provider.h

@@ -45,6 +45,7 @@ class GLES2Interface;
 namespace viz {
 
 class ContextProvider;
+class ScopedAllowGpuAccessForDisplayResourceProvider;
 class SharedBitmapManager;
 
 // This class provides abstractions for receiving and using resources from other
@@ -279,6 +280,17 @@ class VIZ_SERVICE_EXPORT DisplayResourceProvider
     DisplayResourceProvider* const resource_provider_;
   };
 
+  class VIZ_SERVICE_EXPORT ScopedAllowGPUThreadAccess {
+   public:
+    explicit ScopedAllowGPUThreadAccess(
+        DisplayResourceProvider* resource_provider);
+    ~ScopedAllowGPUThreadAccess();
+
+   private:
+    DisplayResourceProvider* const resource_provider_;
+    const bool was_allowed_;
+  };
+
   class VIZ_SERVICE_EXPORT SynchronousFence : public ResourceFence {
    public:
     explicit SynchronousFence(gpu::gles2::GLES2Interface* gl);
@@ -341,7 +353,12 @@ class VIZ_SERVICE_EXPORT DisplayResourceProvider
   // Returns the mailbox corresponding to a resource id.
   gpu::Mailbox GetMailbox(int resource_id);
 
+  // Sets if the GPU thread is available (it always is for Chrome, but for
+  // WebView it happens only when Android calls us on RenderThread.
+  void SetAllowAccessToGPUThread(bool allow);
+
  private:
+  friend class ScopedAllowGpuAccessForDisplayResourceProvider;
   enum DeleteStyle {
     NORMAL,
     FOR_SHUTDOWN,
@@ -533,6 +550,7 @@ class VIZ_SERVICE_EXPORT DisplayResourceProvider
   void DestroyChildInternal(ChildMap::iterator it, DeleteStyle style);
 
   void SetBatchReturnResources(bool aggregate);
+  void TryFlushBatchedResources();
 
   THREAD_CHECKER(thread_checker_);
   const Mode mode_;
@@ -569,6 +587,11 @@ class VIZ_SERVICE_EXPORT DisplayResourceProvider
 
   bool enable_shared_images_;
   std::unique_ptr<ScopedBatchReadAccess> scoped_batch_read_access_;
+
+  // Indicates that gpu thread is available and calls like
+  // ReleaseImageContexts() are expected to finish in finite time. It's always
+  // true for Chrome, but on WebView we need to have access to RenderThread.
+  bool can_access_gpu_thread_ = true;
 };
 
 }  // namespace viz

components/viz/service/display/display_resource_provider_unittest.cc

@@ -326,6 +326,104 @@ TEST_P(DisplayResourceProviderTest, LockForExternalUse) {
   child_resource_provider_->RemoveImportedResource(id1);
 }
 
+TEST_P(DisplayResourceProviderTest, LockForExternalUseWebView) {
+  // TODO(penghuang): consider supporting SW mode.
+  if (!use_gpu())
+    return;
+
+  gpu::SyncToken sync_token1(gpu::CommandBufferNamespace::GPU_IO,
+                             gpu::CommandBufferId::FromUnsafeValue(0x123),
+                             0x42);
+  auto mailbox = gpu::Mailbox::Generate();
+  constexpr gfx::Size size(64, 64);
+  TransferableResource gl_resource = TransferableResource::MakeGL(
+      mailbox, GL_LINEAR, GL_TEXTURE_2D, sync_token1, size,
+      false /* is_overlay_candidate */);
+  ResourceId id1 = child_resource_provider_->ImportResource(
+      gl_resource, SingleReleaseCallback::Create(base::DoNothing()));
+  std::vector<ReturnedResource> returned_to_child;
+  int child_id =
+      resource_provider_->CreateChild(GetReturnCallback(&returned_to_child));
+
+  // Transfer some resources to the parent.
+  std::vector<TransferableResource> list;
+  child_resource_provider_->PrepareSendToParent(
+      {id1}, &list,
+      static_cast<RasterContextProvider*>(child_context_provider_.get()));
+  ASSERT_EQ(1u, list.size());
+  EXPECT_TRUE(child_resource_provider_->InUseByConsumer(id1));
+
+  resource_provider_->ReceiveFromChild(child_id, list);
+
+  // In DisplayResourceProvider's namespace, use the mapped resource id.
+  std::unordered_map<ResourceId, ResourceId> resource_map =
+      resource_provider_->GetChildToParentMap(child_id);
+
+  unsigned parent_id = resource_map[list.front().id];
+
+  auto owned_image_context = std::make_unique<ExternalUseClient::ImageContext>(
+      gpu::MailboxHolder(mailbox, sync_token1, GL_TEXTURE_2D), size, RGBA_8888,
+      /*ycbcr_info=*/base::nullopt, /*color_space=*/nullptr);
+  auto* image_context = owned_image_context.get();
+
+  testing::StrictMock<MockExternalUseClient> client;
+  DisplayResourceProvider::LockSetForExternalUse lock_set(
+      resource_provider_.get(), &client);
+  gpu::MailboxHolder holder;
+  EXPECT_CALL(client, CreateImageContext(_, _, _, _, _))
+      .WillOnce(DoAll(SaveArg<0>(&holder),
+                      Return(ByMove(std::move(owned_image_context)))));
+
+  ExternalUseClient::ImageContext* locked_image_context =
+      lock_set.LockResource(parent_id, /*is_video_plane=*/false);
+  EXPECT_EQ(image_context, locked_image_context);
+  ASSERT_EQ(holder.mailbox, mailbox);
+  ASSERT_TRUE(holder.sync_token.HasData());
+
+  // Don't release while locked.
+  EXPECT_CALL(client, ReleaseImageContexts(_)).Times(0);
+  // Return the resources back to the child. Nothing should happen because
+  // of the resource lock.
+  resource_provider_->DeclareUsedResourcesFromChild(child_id, ResourceIdSet());
+  // The resource should not be returned due to the external use lock.
+  EXPECT_EQ(0u, returned_to_child.size());
+
+  // Disable access to gpu thread.
+  resource_provider_->SetAllowAccessToGPUThread(false);
+
+  gpu::SyncToken sync_token2(gpu::CommandBufferNamespace::GPU_IO,
+                             gpu::CommandBufferId::FromUnsafeValue(0x234),
+                             0x456);
+  sync_token2.SetVerifyFlush();
+
+  gpu::SyncToken sync_token3(gpu::CommandBufferNamespace::GPU_IO,
+                             gpu::CommandBufferId::FromUnsafeValue(0x234),
+                             0x567);
+  sync_token3.SetVerifyFlush();
+
+  // Without GPU thread access no ReleaseImageContexts() should happen
+  EXPECT_CALL(client, ReleaseImageContexts(_)).Times(0);
+  // Unlock resources
+  lock_set.UnlockResources(sync_token2);
+  // Resources should not be returned because we can't unlock them on GPU
+  // thread.
+  EXPECT_EQ(0u, returned_to_child.size());
+
+  // We will get a second release of |parent_id| now that we've released our
+  // external lock and have access to GPU thread.
+  EXPECT_CALL(client, ReleaseImageContexts(
+                          testing::ElementsAre(SamePtr(locked_image_context))))
+      .WillOnce(Return(sync_token3));
+  // Enable access to GPU Thread
+  resource_provider_->SetAllowAccessToGPUThread(true);
+
+  // The resource should be returned after the lock is released.
+  EXPECT_EQ(1u, returned_to_child.size());
+  EXPECT_EQ(sync_token3, returned_to_child[0].sync_token);
+  child_resource_provider_->ReceiveReturnsFromParent(returned_to_child);
+  child_resource_provider_->RemoveImportedResource(id1);
+}
+
 TEST_P(DisplayResourceProviderTest, ReadLockCountStopsReturnToChildOrDelete) {
   if (!use_gpu())
     return;

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -372,7 +372,7 @@ gpu::SyncToken SkiaOutputSurfaceImpl::ReleaseImageContexts(
       base::BindOnce(&SkiaOutputSurfaceImplOnGpu::ReleaseImageContexts,
                      base::Unretained(impl_on_gpu_.get()),
                      std::move(image_contexts), sync_fence_release_);
-  gpu_task_scheduler_->ScheduleOrRetainGpuTask(std::move(callback), {});
+  gpu_task_scheduler_->ScheduleGpuTask(std::move(callback), {});
   return sync_token;
 }
 

gpu/ipc/scheduler_sequence.h

@@ -18,6 +18,7 @@
 
 namespace viz {
 class Display;
+class ScopedAllowGpuAccessForDisplayResourceProvider;
 class OutputSurfaceProviderImpl;
 class OverlayProcessorAndroid;
 }  // namespace viz
@@ -35,6 +36,7 @@ class GL_IN_PROCESS_CONTEXT_EXPORT ScopedAllowScheduleGpuTask {
   // Only add more friend declarations for classes that Android WebView is
   // guaranteed to be able to support. Talk to boliu@ if in doubt.
   friend class viz::Display;
+  friend class viz::ScopedAllowGpuAccessForDisplayResourceProvider;
   friend class viz::OutputSurfaceProviderImpl;
   // Overlay is not supported for WebView. However the initialization and
   // destruction of OverlayProcessor requires posting task to gpu thread, which