Report progress to watchdog for allocation/upload.

This CL makes the SharedImageBackingFactoryGLTexture notify the GPU
watchdog that progress is being made around the code that requests
memory allocation or texture upload. This is an attempt to improve the
crash rate due to allocation taking a long time (as opposed to it being
a GPU hang).

Bug: 1082197
Test: added unit test coverage.
Change-Id: Ic9530fd53df95d5ce0b675231c0ac52e682c69e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2215256
Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
Reviewed-by: Maggie Chen <magchen@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813055}

gpu/command_buffer/service/shared_image_backing_factory_gl_texture.h

@@ -22,6 +22,10 @@ class Size;
 class ColorSpace;
 }  // namespace gfx
 
+namespace gl {
+class ProgressReporter;
+}  // namespace gl
+
 namespace gpu {
 class SharedImageBacking;
 class SharedImageBatchAccessManager;
@@ -43,7 +47,8 @@ class GPU_GLES2_EXPORT SharedImageBackingFactoryGLTexture
       const GpuDriverBugWorkarounds& workarounds,
       const GpuFeatureInfo& gpu_feature_info,
       ImageFactory* image_factory,
-      SharedImageBatchAccessManager* batch_access_manager);
+      SharedImageBatchAccessManager* batch_access_manager,
+      gl::ProgressReporter* progress_reporter);
   ~SharedImageBackingFactoryGLTexture() override;
 
   // SharedImageBackingFactory implementation.
@@ -170,6 +175,10 @@ class GPU_GLES2_EXPORT SharedImageBackingFactoryGLTexture
   SharedImageBackingGLCommon::UnpackStateAttribs attribs;
   GpuDriverBugWorkarounds workarounds_;
 
+  // Used to notify the watchdog before a buffer allocation in case it takes
+  // long.
+  gl::ProgressReporter* const progress_reporter_ = nullptr;
+
 #if defined(OS_ANDROID)
   SharedImageBatchAccessManager* batch_access_manager_ = nullptr;
 #endif

gpu/command_buffer/service/shared_image_backing_factory_gl_texture_unittest.cc

@@ -29,6 +29,7 @@
 #include "gpu/config/gpu_feature_info.h"
 #include "gpu/config/gpu_preferences.h"
 #include "gpu/config/gpu_test_config.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkPromiseImageTexture.h"
 #include "third_party/skia/include/core/SkSurface.h"
@@ -44,6 +45,9 @@
 #include "ui/gl/gl_image_stub.h"
 #include "ui/gl/gl_surface.h"
 #include "ui/gl/init/gl_factory.h"
+#include "ui/gl/progress_reporter.h"
+
+using testing::AtLeast;
 
 namespace gpu {
 namespace {
@@ -79,6 +83,15 @@ bool IsAndroid() {
 #endif
 }
 
+class MockProgressReporter : public gl::ProgressReporter {
+ public:
+  MockProgressReporter() = default;
+  ~MockProgressReporter() override = default;
+
+  // gl::ProgressReporter implementation.
+  MOCK_METHOD0(ReportProgress, void());
+};
+
 class SharedImageBackingFactoryGLTextureTestBase
     : public testing::TestWithParam<std::tuple<bool, viz::ResourceFormat>> {
  public:
@@ -105,7 +118,7 @@ class SharedImageBackingFactoryGLTextureTestBase
     preferences.use_passthrough_cmd_decoder = use_passthrough();
     backing_factory_ = std::make_unique<SharedImageBackingFactoryGLTexture>(
         preferences, workarounds, GpuFeatureInfo(), factory,
-        shared_image_manager_->batch_access_manager());
+        shared_image_manager_->batch_access_manager(), &progress_reporter_);
 
     memory_type_tracker_ = std::make_unique<MemoryTypeTracker>(nullptr);
     shared_image_representation_factory_ =
@@ -118,11 +131,31 @@ class SharedImageBackingFactoryGLTextureTestBase
            gles2::PassthroughCommandDecoderSupported();
   }
 
+  bool can_create_non_scanout_shared_image(viz::ResourceFormat format) const {
+    if (format == viz::ResourceFormat::BGRA_1010102 ||
+        format == viz::ResourceFormat::RGBA_1010102) {
+      return supports_ar30_ || supports_ab30_;
+    } else if (format == viz::ResourceFormat::ETC1) {
+      return supports_etc1_;
+    }
+    return true;
+  }
+
+  bool can_create_scanout_or_gmb_shared_image(
+      viz::ResourceFormat format) const {
+    if (format == viz::ResourceFormat::BGRA_1010102)
+      return supports_ar30_;
+    else if (format == viz::ResourceFormat::RGBA_1010102)
+      return supports_ab30_;
+    return true;
+  }
+
   viz::ResourceFormat get_format() { return std::get<1>(GetParam()); }
 
   GrDirectContext* gr_context() { return context_state_->gr_context(); }
 
  protected:
+  ::testing::NiceMock<MockProgressReporter> progress_reporter_;
   scoped_refptr<gl::GLSurface> surface_;
   scoped_refptr<gl::GLContext> context_;
   scoped_refptr<SharedContextState> context_state_;
@@ -213,6 +246,9 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, Basic) {
     return;
   }
 
+  const bool should_succeed = can_create_non_scanout_shared_image(get_format());
+  if (should_succeed)
+    EXPECT_CALL(progress_reporter_, ReportProgress).Times(AtLeast(1));
   auto mailbox = Mailbox::GenerateForSharedImage();
   auto format = get_format();
   gfx::Size size(256, 256);
@@ -225,12 +261,7 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, Basic) {
       mailbox, format, surface_handle, size, color_space, surface_origin,
       alpha_type, usage, false /* is_thread_safe */);
 
-  // As long as either |chromium_image_ar30| or |chromium_image_ab30| is
-  // enabled, we can create a non-scanout SharedImage with format
-  // viz::ResourceFormat::{BGRA,RGBA}_1010102.
-  if ((format == viz::ResourceFormat::BGRA_1010102 ||
-       format == viz::ResourceFormat::RGBA_1010102) &&
-      !supports_ar30_ && !supports_ab30_) {
+  if (!should_succeed) {
     EXPECT_FALSE(backing);
     return;
   }
@@ -347,6 +378,11 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, Image) {
       bot_config.Matches("mac passthrough")) {
     return;
   }
+
+  const bool should_succeed =
+      can_create_scanout_or_gmb_shared_image(get_format());
+  if (should_succeed)
+    EXPECT_CALL(progress_reporter_, ReportProgress).Times(AtLeast(1));
   auto mailbox = Mailbox::GenerateForSharedImage();
   auto format = get_format();
   gfx::Size size(256, 256);
@@ -359,15 +395,12 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, Image) {
       mailbox, format, surface_handle, size, color_space, surface_origin,
       alpha_type, usage, false /* is_thread_safe */);
 
-  // We can only create a scanout SharedImage with format
-  // viz::ResourceFormat::{BGRA,RGBA}_1010102 if the corresponding
-  // |chromium_image_ar30| or |chromium_image_ab30| is enabled.
-  if ((format == viz::ResourceFormat::BGRA_1010102 && !supports_ar30_) ||
-      (format == viz::ResourceFormat::RGBA_1010102 && !supports_ab30_)) {
+  if (!should_succeed) {
     EXPECT_FALSE(backing);
     return;
   }
   ASSERT_TRUE(backing);
+  ::testing::Mock::VerifyAndClearExpectations(&progress_reporter_);
 
   // Check clearing.
   if (!backing->IsCleared()) {
@@ -472,6 +505,7 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, Image) {
 
   if (!use_passthrough() &&
       context_state_->feature_info()->feature_flags().ext_texture_rg) {
+    EXPECT_CALL(progress_reporter_, ReportProgress).Times(AtLeast(1));
     // Create a R-8 image texture, and check that the internal_format is that
     // of the image (GL_RGBA for TextureImageFactory). This only matters for
     // the validating decoder.
@@ -503,6 +537,9 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, InitialData) {
   for (auto format :
        {viz::ResourceFormat::RGBA_8888, viz::ResourceFormat::ETC1,
         viz::ResourceFormat::BGRA_1010102, viz::ResourceFormat::RGBA_1010102}) {
+    const bool should_succeed = can_create_non_scanout_shared_image(format);
+    if (should_succeed)
+      EXPECT_CALL(progress_reporter_, ReportProgress).Times(AtLeast(1));
     auto mailbox = Mailbox::GenerateForSharedImage();
     gfx::Size size(256, 256);
     auto color_space = gfx::ColorSpace::CreateSRGB();
@@ -514,22 +551,11 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, InitialData) {
     auto backing = backing_factory_->CreateSharedImage(
         mailbox, format, size, color_space, surface_origin, alpha_type, usage,
         initial_data);
-
-    if (format == viz::ResourceFormat::ETC1 && !supports_etc1_) {
-      EXPECT_FALSE(backing);
-      continue;
-    }
-
-    // As long as either |chromium_image_ar30| or |chromium_image_ab30| is
-    // enabled, we can create a non-scanout SharedImage with format
-    // viz::ResourceFormat::{BGRA,RGBA}_1010102.
-    if ((format == viz::ResourceFormat::BGRA_1010102 ||
-         format == viz::ResourceFormat::RGBA_1010102) &&
-        !supports_ar30_ && !supports_ab30_) {
+    ::testing::Mock::VerifyAndClearExpectations(&progress_reporter_);
+    if (!should_succeed) {
       EXPECT_FALSE(backing);
       continue;
     }
-
     ASSERT_TRUE(backing);
     EXPECT_TRUE(backing->IsCleared());
 
@@ -571,6 +597,10 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, InitialData) {
 }
 
 TEST_P(SharedImageBackingFactoryGLTextureTest, InitialDataImage) {
+  const bool should_succeed =
+      can_create_scanout_or_gmb_shared_image(get_format());
+  if (should_succeed)
+    EXPECT_CALL(progress_reporter_, ReportProgress).Times(AtLeast(1));
   auto mailbox = Mailbox::GenerateForSharedImage();
   auto format = get_format();
   gfx::Size size(256, 256);
@@ -582,12 +612,7 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, InitialDataImage) {
   auto backing = backing_factory_->CreateSharedImage(
       mailbox, format, size, color_space, surface_origin, alpha_type, usage,
       initial_data);
-
-  // We can only create a scanout SharedImage with format
-  // viz::ResourceFormat::{BGRA,RGBA}_1010102 if the corresponding
-  // |chromium_image_ar30| or |chromium_image_ab30| is enabled.
-  if ((format == viz::ResourceFormat::BGRA_1010102 && !supports_ar30_) ||
-      (format == viz::ResourceFormat::RGBA_1010102 && !supports_ab30_)) {
+  if (!should_succeed) {
     EXPECT_FALSE(backing);
     return;
   }
@@ -679,6 +704,9 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, InvalidSize) {
 }
 
 TEST_P(SharedImageBackingFactoryGLTextureTest, EstimatedSize) {
+  const bool should_succeed = can_create_non_scanout_shared_image(get_format());
+  if (should_succeed)
+    EXPECT_CALL(progress_reporter_, ReportProgress).Times(AtLeast(1));
   auto mailbox = Mailbox::GenerateForSharedImage();
   auto format = get_format();
   gfx::Size size(256, 256);
@@ -691,12 +719,7 @@ TEST_P(SharedImageBackingFactoryGLTextureTest, EstimatedSize) {
       mailbox, format, surface_handle, size, color_space, surface_origin,
       alpha_type, usage, false /* is_thread_safe */);
 
-  // As long as either |chromium_image_ar30| or |chromium_image_ab30| is
-  // enabled, we can create a non-scanout SharedImage with format
-  // viz::ResourceFormat::{BGRA,RGBA}_1010102.
-  if ((format == viz::ResourceFormat::BGRA_1010102 ||
-       format == viz::ResourceFormat::RGBA_1010102) &&
-      !supports_ar30_ && !supports_ab30_) {
+  if (!should_succeed) {
     EXPECT_FALSE(backing);
     return;
   }
@@ -891,12 +914,7 @@ TEST_P(SharedImageBackingFactoryGLTextureWithGMBTest,
   auto backing = backing_factory_->CreateSharedImage(
       mailbox, kClientId, std::move(handle), format, kNullSurfaceHandle, size,
       color_space, surface_origin, alpha_type, usage);
-
-  // We can only create a GMB SharedImage with format
-  // viz::ResourceFormat::{BGRA,RGBA}_1010102 if the corresponding
-  // |chromium_image_ar30| or |chromium_image_ab30| is enabled.
-  if ((get_format() == viz::ResourceFormat::BGRA_1010102 && !supports_ar30_) ||
-      (get_format() == viz::ResourceFormat::RGBA_1010102 && !supports_ab30_)) {
+  if (!can_create_scanout_or_gmb_shared_image(get_format())) {
     EXPECT_FALSE(backing);
     return;
   }
@@ -952,12 +970,7 @@ TEST_P(SharedImageBackingFactoryGLTextureWithGMBTest,
   auto backing = backing_factory_->CreateSharedImage(
       mailbox, kClientId, std::move(handle), format, kNullSurfaceHandle, size,
       color_space, surface_origin, alpha_type, usage);
-
-  // We can only create a GMB SharedImage with format
-  // viz::ResourceFormat::{BGRA,RGBA}_1010102 if the corresponding
-  // |chromium_image_ar30| or |chromium_image_ab30| is enabled.
-  if ((get_format() == viz::ResourceFormat::BGRA_1010102 && !supports_ar30_) ||
-      (get_format() == viz::ResourceFormat::RGBA_1010102 && !supports_ab30_)) {
+  if (!can_create_scanout_or_gmb_shared_image(get_format())) {
     EXPECT_FALSE(backing);
     return;
   }
@@ -990,12 +1003,7 @@ TEST_P(SharedImageBackingFactoryGLTextureWithGMBTest,
   auto backing = backing_factory_->CreateSharedImage(
       mailbox, kClientId, std::move(handle), format, kNullSurfaceHandle, size,
       color_space, surface_origin, alpha_type, usage);
-
-  // We can only create a GMB SharedImage with format
-  // viz::ResourceFormat::{BGRA,RGBA}_1010102 if the corresponding
-  // |chromium_image_ar30| or |chromium_image_ab30| is enabled.
-  if ((get_format() == viz::ResourceFormat::BGRA_1010102 && !supports_ar30_) ||
-      (get_format() == viz::ResourceFormat::RGBA_1010102 && !supports_ab30_)) {
+  if (!can_create_scanout_or_gmb_shared_image(get_format())) {
     EXPECT_FALSE(backing);
     return;
   }

gpu/command_buffer/service/shared_image_backing_factory_iosurface_unittest.cc

@@ -67,7 +67,8 @@ class SharedImageBackingFactoryIOSurfaceTest : public testing::Test {
 
     backing_factory_ = std::make_unique<SharedImageBackingFactoryGLTexture>(
         preferences, workarounds, GpuFeatureInfo(), &image_factory_,
-        shared_image_manager_.batch_access_manager());
+        shared_image_manager_.batch_access_manager(),
+        /*progress_reporter=*/nullptr);
 
     memory_type_tracker_ = std::make_unique<MemoryTypeTracker>(nullptr);
     shared_image_representation_factory_ =

gpu/command_buffer/service/shared_image_backing_gl_texture.cc

@@ -44,6 +44,7 @@
 #include "ui/gl/gl_image_shared_memory.h"
 #include "ui/gl/gl_implementation.h"
 #include "ui/gl/gl_version_info.h"
+#include "ui/gl/progress_reporter.h"
 #include "ui/gl/scoped_binders.h"
 #include "ui/gl/shared_gl_fence_egl.h"
 #include "ui/gl/trace_util.h"
@@ -75,11 +76,13 @@ SharedImageBackingFactoryGLTexture::SharedImageBackingFactoryGLTexture(
     const GpuDriverBugWorkarounds& workarounds,
     const GpuFeatureInfo& gpu_feature_info,
     ImageFactory* image_factory,
-    SharedImageBatchAccessManager* batch_access_manager)
+    SharedImageBatchAccessManager* batch_access_manager,
+    gl::ProgressReporter* progress_reporter)
     : use_passthrough_(gpu_preferences.use_passthrough_cmd_decoder &&
                        gles2::PassthroughCommandDecoderSupported()),
       image_factory_(image_factory),
-      workarounds_(workarounds) {
+      workarounds_(workarounds),
+      progress_reporter_(progress_reporter) {
 #if defined(OS_ANDROID)
   batch_access_manager_ = batch_access_manager;
 #endif
@@ -505,13 +508,22 @@ SharedImageBackingFactoryGLTexture::CreateSharedImageInternal(
   // the internal format in the LevelInfo. https://crbug.com/628064
   GLuint level_info_internal_format = format_info.gl_format;
   bool is_cleared = false;
+
+  // |scoped_progress_reporter| will notify |progress_reporter_| upon
+  // construction and destruction. We limit the scope so that progress is
+  // reported immediately after allocation/upload and before other GL
+  // operations.
   if (use_buffer) {
-    image = image_factory_->CreateAnonymousImage(
-        size, format_info.buffer_format, gfx::BufferUsage::SCANOUT,
-        surface_handle, &is_cleared);
+    {
+      gl::ScopedProgressReporter scoped_progress_reporter(progress_reporter_);
+      image = image_factory_->CreateAnonymousImage(
+          size, format_info.buffer_format, gfx::BufferUsage::SCANOUT,
+          surface_handle, &is_cleared);
+    }
     // Scanout images have different constraints than GL images and might fail
     // to allocate even if GL images can be created.
     if (!image) {
+      gl::ScopedProgressReporter scoped_progress_reporter(progress_reporter_);
       // TODO(dcastagna): Use BufferUsage::GPU_READ_WRITE instead
       // BufferUsage::GPU_READ once we add it.
       image = image_factory_->CreateAnonymousImage(
@@ -546,6 +558,7 @@ SharedImageBackingFactoryGLTexture::CreateSharedImageInternal(
         image, mailbox, format, size, color_space, surface_origin, alpha_type,
         usage, params, attribs, use_passthrough_);
     if (!pixel_data.empty()) {
+      gl::ScopedProgressReporter scoped_progress_reporter(progress_reporter_);
       result->InitializePixels(format_info.adjusted_format, format_info.gl_type,
                                pixel_data.data());
     }
@@ -561,12 +574,16 @@ SharedImageBackingFactoryGLTexture::CreateSharedImageInternal(
     api->glBindTextureFn(target, result->GetGLServiceId());
 
     if (format_info.supports_storage) {
-      api->glTexStorage2DEXTFn(target, 1, format_info.storage_internal_format,
-                               size.width(), size.height());
+      {
+        gl::ScopedProgressReporter scoped_progress_reporter(progress_reporter_);
+        api->glTexStorage2DEXTFn(target, 1, format_info.storage_internal_format,
+                                 size.width(), size.height());
+      }
 
       if (!pixel_data.empty()) {
         ScopedResetAndRestoreUnpackState scoped_unpack_state(
             api, attribs, true /* uploading_data */);
+        gl::ScopedProgressReporter scoped_progress_reporter(progress_reporter_);
         api->glTexSubImage2DFn(target, 0, 0, 0, size.width(), size.height(),
                                format_info.adjusted_format, format_info.gl_type,
                                pixel_data.data());
@@ -574,12 +591,14 @@ SharedImageBackingFactoryGLTexture::CreateSharedImageInternal(
     } else if (format_info.is_compressed) {
       ScopedResetAndRestoreUnpackState scoped_unpack_state(api, attribs,
                                                            !pixel_data.empty());
+      gl::ScopedProgressReporter scoped_progress_reporter(progress_reporter_);
       api->glCompressedTexImage2DFn(
           target, 0, format_info.image_internal_format, size.width(),
           size.height(), 0, pixel_data.size(), pixel_data.data());
     } else {
       ScopedResetAndRestoreUnpackState scoped_unpack_state(api, attribs,
                                                            !pixel_data.empty());
+      gl::ScopedProgressReporter scoped_progress_reporter(progress_reporter_);
       api->glTexImage2DFn(target, 0, format_info.image_internal_format,
                           size.width(), size.height(), 0,
                           format_info.adjusted_format, format_info.gl_type,

gpu/command_buffer/service/shared_image_factory.cc

@@ -100,7 +100,9 @@ SharedImageFactory::SharedImageFactory(
   if (use_gl) {
     gl_backing_factory_ = std::make_unique<SharedImageBackingFactoryGLTexture>(
         gpu_preferences, workarounds, gpu_feature_info, image_factory,
-        shared_image_manager->batch_access_manager());
+        shared_image_manager->batch_access_manager(),
+        shared_context_state_ ? shared_context_state_->progress_reporter()
+                              : nullptr);
   }
 
   // TODO(ccameron): This block of code should be changed to a switch on