Added a way for sync point clients to issue out of order waits.

This patch fixes a subtle bug that could occur if a sync point client
is waiting without a corresponding order number. This can occur for any
wait that is not done within the usual command buffer processing loop.
An example is the command buffer stub OnSignalSyncToken() function
which issues a wait directly on the IO thread.

Previously, a similar concept was introduced in SyncPointClientWaiter
which was used to wait out of order when a Gpu Memory Buffer was
destroyed. Because of the need for this functionality in the regular
SyncPointClient, I have folded the SyncPointClientWaiter functions into
SyncPointClient under WaitOutOfOrder() & WaitOutOfOrderNonThreadSafe().

Because of how subtle this bug is, further state tracking is now done
so we can test whether or not an order number is processing or not.
Using that test, DCHECKs have been added so Wait()/WaitNonThreadSafe
can only be called when processing an order number, and the the out of
order variants can only be called when not processing an order number.

BUG=514815
TEST=Added unit tests to test SignalSyncToken().

Review URL: https://codereview.chromium.org/1568563002

Cr-Commit-Position: refs/heads/master@{#368441}

7 files changed, 173 insertions, 53 deletions

diff --git a/gpu/command_buffer/service/in_process_command_buffer.cc b/gpu/command_buffer/service/in_process_command_buffer.cc
index 0ef43fb..69e6724 100644
--- a/gpu/command_buffer/service/in_process_command_buffer.cc
+++ b/gpu/command_buffer/service/in_process_command_buffer.cc
@@ -942,8 +942,8 @@ void InProcessCommandBuffer::SignalSyncTokenOnGpuThread(
     return;
   }
 
-  sync_point_client_->Wait(release_state.get(), sync_token.release_count(),
-                           WrapCallback(callback));
+  sync_point_client_->WaitOutOfOrder(
+      release_state.get(), sync_token.release_count(), WrapCallback(callback));
 }
 
 void InProcessCommandBuffer::SignalQuery(unsigned query_id,
diff --git a/gpu/command_buffer/service/sync_point_manager.cc b/gpu/command_buffer/service/sync_point_manager.cc
index 97ba993..fbef08b 100644
--- a/gpu/command_buffer/service/sync_point_manager.cc
+++ b/gpu/command_buffer/service/sync_point_manager.cc
@@ -64,6 +64,7 @@ void SyncPointOrderData::BeginProcessingOrderNumber(uint32_t order_num) {
   DCHECK(processing_thread_checker_.CalledOnValidThread());
   DCHECK_GE(order_num, current_order_num_);
   current_order_num_ = order_num;
+  paused_ = false;
 
   // Catch invalid waits which were waiting on fence syncs that do not exist.
   // When we begin processing an order number, we should release any fence
@@ -88,9 +89,17 @@ void SyncPointOrderData::BeginProcessingOrderNumber(uint32_t order_num) {
   }
 }
 
+void SyncPointOrderData::PauseProcessingOrderNumber(uint32_t order_num) {
+  DCHECK(processing_thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(current_order_num_, order_num);
+  DCHECK(!paused_);
+  paused_ = true;
+}
+
 void SyncPointOrderData::FinishProcessingOrderNumber(uint32_t order_num) {
   DCHECK(processing_thread_checker_.CalledOnValidThread());
   DCHECK_EQ(current_order_num_, order_num);
+  DCHECK(!paused_);
 
   // Catch invalid waits which were waiting on fence syncs that do not exist.
   // When we end processing an order number, we should release any fence syncs
@@ -128,6 +137,7 @@ SyncPointOrderData::OrderFence::~OrderFence() {}
 
 SyncPointOrderData::SyncPointOrderData()
     : current_order_num_(0),
+      paused_(false),
       destroyed_(false),
       processed_order_num_(0),
       unprocessed_order_num_(0) {}
@@ -242,15 +252,21 @@ void SyncPointClientState::ReleaseFenceSyncLocked(
 }
 
 SyncPointClient::~SyncPointClient() {
-  // Release all fences on destruction.
-  ReleaseFenceSync(UINT64_MAX);
+  if (namespace_id_ != gpu::CommandBufferNamespace::INVALID) {
+    // Release all fences on destruction.
+    client_state_->ReleaseFenceSync(UINT64_MAX);
 
-  sync_point_manager_->DestroySyncPointClient(namespace_id_, client_id_);
+    sync_point_manager_->DestroySyncPointClient(namespace_id_, client_id_);
+  }
 }
 
 bool SyncPointClient::Wait(SyncPointClientState* release_state,
                            uint64_t release_count,
                            const base::Closure& wait_complete_callback) {
+  // Validate that this Wait call is between BeginProcessingOrderNumber() and
+  // FinishProcessingOrderNumber(), or else we may deadlock.
+  DCHECK(client_state_->order_data()->IsProcessingOrderNumber());
+
   const uint32_t wait_order_number =
       client_state_->order_data()->current_order_num();
 
@@ -273,22 +289,15 @@ bool SyncPointClient::WaitNonThreadSafe(
               base::Bind(&RunOnThread, runner, wait_complete_callback));
 }
 
-void SyncPointClient::ReleaseFenceSync(uint64_t release) {
-  client_state_->ReleaseFenceSync(release);
-}
-
-SyncPointClient::SyncPointClient(SyncPointManager* sync_point_manager,
-                                 scoped_refptr<SyncPointOrderData> order_data,
-                                 CommandBufferNamespace namespace_id,
-                                 uint64_t client_id)
-    : sync_point_manager_(sync_point_manager),
-      client_state_(new SyncPointClientState(order_data)),
-      namespace_id_(namespace_id),
-      client_id_(client_id) {}
+bool SyncPointClient::WaitOutOfOrder(
+    SyncPointClientState* release_state,
+    uint64_t release_count,
+    const base::Closure& wait_complete_callback) {
+  // Validate that this Wait call is not between BeginProcessingOrderNumber()
+  // and FinishProcessingOrderNumber(), or else we may deadlock.
+  DCHECK(!client_state_ ||
+         !client_state_->order_data()->IsProcessingOrderNumber());
 
-bool SyncPointClientWaiter::Wait(SyncPointClientState* release_state,
-                                 uint64_t release_count,
-                                 const base::Closure& wait_complete_callback) {
   // No order number associated with the current execution context, using
   // UINT32_MAX will just assume the release is in the SyncPointClientState's
   // order numbers to be executed.
@@ -300,15 +309,37 @@ bool SyncPointClientWaiter::Wait(SyncPointClientState* release_state,
   return true;
 }
 
-bool SyncPointClientWaiter::WaitNonThreadSafe(
+bool SyncPointClient::WaitOutOfOrderNonThreadSafe(
     SyncPointClientState* release_state,
     uint64_t release_count,
     scoped_refptr<base::SingleThreadTaskRunner> runner,
     const base::Closure& wait_complete_callback) {
-  return Wait(release_state, release_count,
-              base::Bind(&RunOnThread, runner, wait_complete_callback));
+  return WaitOutOfOrder(
+      release_state, release_count,
+      base::Bind(&RunOnThread, runner, wait_complete_callback));
+}
+
+void SyncPointClient::ReleaseFenceSync(uint64_t release) {
+  // Validate that this Release call is between BeginProcessingOrderNumber() and
+  // FinishProcessingOrderNumber(), or else we may deadlock.
+  DCHECK(client_state_->order_data()->IsProcessingOrderNumber());
+  client_state_->ReleaseFenceSync(release);
 }
 
+SyncPointClient::SyncPointClient()
+    : sync_point_manager_(nullptr),
+      namespace_id_(gpu::CommandBufferNamespace::INVALID),
+      client_id_(0) {}
+
+SyncPointClient::SyncPointClient(SyncPointManager* sync_point_manager,
+                                 scoped_refptr<SyncPointOrderData> order_data,
+                                 CommandBufferNamespace namespace_id,
+                                 uint64_t client_id)
+    : sync_point_manager_(sync_point_manager),
+      client_state_(new SyncPointClientState(order_data)),
+      namespace_id_(namespace_id),
+      client_id_(client_id) {}
+
 SyncPointManager::SyncPointManager(bool allow_threaded_wait)
     : allow_threaded_wait_(allow_threaded_wait),
       // To reduce the risk that a sync point created in a previous GPU process
@@ -342,6 +373,10 @@ scoped_ptr<SyncPointClient> SyncPointManager::CreateSyncPointClient(
   return make_scoped_ptr(result.first->second);
 }
 
+scoped_ptr<SyncPointClient> SyncPointManager::CreateSyncPointClientWaiter() {
+  return make_scoped_ptr(new SyncPointClient);
+}
+
 scoped_refptr<SyncPointClientState> SyncPointManager::GetSyncPointClientState(
     CommandBufferNamespace namespace_id, uint64_t client_id) {
   if (namespace_id >= 0) {
diff --git a/gpu/command_buffer/service/sync_point_manager.h b/gpu/command_buffer/service/sync_point_manager.h
index cecf9fe..97cdc6f 100644
--- a/gpu/command_buffer/service/sync_point_manager.h
+++ b/gpu/command_buffer/service/sync_point_manager.h
@@ -42,6 +42,7 @@ class GPU_EXPORT SyncPointOrderData
 
   uint32_t GenerateUnprocessedOrderNumber(SyncPointManager* sync_point_manager);
   void BeginProcessingOrderNumber(uint32_t order_num);
+  void PauseProcessingOrderNumber(uint32_t order_num);
   void FinishProcessingOrderNumber(uint32_t order_num);
 
   uint32_t processed_order_num() const {
@@ -59,6 +60,11 @@ class GPU_EXPORT SyncPointOrderData
     return current_order_num_;
   }
 
+  bool IsProcessingOrderNumber() {
+    DCHECK(processing_thread_checker_.CalledOnValidThread());
+    return !paused_ && current_order_num_ > processed_order_num();
+  }
+
  private:
   friend class base::RefCountedThreadSafe<SyncPointOrderData>;
   friend class SyncPointClientState;
@@ -97,6 +103,9 @@ class GPU_EXPORT SyncPointOrderData
   // Current IPC order number being processed (only used on processing thread).
   uint32_t current_order_num_;
 
+  // Whether or not the current order number is being processed or paused.
+  bool paused_;
+
   // This lock protects destroyed_, processed_order_num_,
   // unprocessed_order_num_, and order_fence_queue_. All order numbers (n) in
   // order_fence_queue_ must follow the invariant:
@@ -140,7 +149,6 @@ class GPU_EXPORT SyncPointClientState
  private:
   friend class base::RefCountedThreadSafe<SyncPointClientState>;
   friend class SyncPointClient;
-  friend class SyncPointClientWaiter;
   friend class SyncPointOrderData;
 
   struct ReleaseCallback {
@@ -213,11 +221,30 @@ class GPU_EXPORT SyncPointClient {
                          scoped_refptr<base::SingleThreadTaskRunner> runner,
                          const base::Closure& wait_complete_callback);
 
+  // Unordered waits are waits which do not occur within the global order number
+  // processing order (IE. Not between the corresponding
+  // SyncPointOrderData::BeginProcessingOrderNumber() and
+  // SyncPointOrderData::FinishProcessingOrderNumber() calls). Because fence
+  // sync releases must occur within a corresponding order number, these waits
+  // cannot deadlock because they can never depend on any fence sync releases.
+  // This is useful for IPC messages that may be processed out of order with
+  // respect to regular command buffer processing.
+  bool WaitOutOfOrder(SyncPointClientState* release_state,
+                      uint64_t release_count,
+                      const base::Closure& wait_complete_callback);
+
+  bool WaitOutOfOrderNonThreadSafe(
+      SyncPointClientState* release_state,
+      uint64_t release_count,
+      scoped_refptr<base::SingleThreadTaskRunner> runner,
+      const base::Closure& wait_complete_callback);
+
   void ReleaseFenceSync(uint64_t release);
 
  private:
   friend class SyncPointManager;
 
+  SyncPointClient();
   SyncPointClient(SyncPointManager* sync_point_manager,
                   scoped_refptr<SyncPointOrderData> order_data,
                   CommandBufferNamespace namespace_id,
@@ -236,32 +263,6 @@ class GPU_EXPORT SyncPointClient {
   DISALLOW_COPY_AND_ASSIGN(SyncPointClient);
 };
 
-// A SyncPointClientWaiter is a Sync Point Client which can only wait and on
-// fence syncs and not release any fence syncs itself. Because they cannot
-// release any fence syncs they do not need an associated order number since
-// deadlocks cannot happen. Note that it is important that this class does
-// not exist in the same execution context as a SyncPointClient, or else a
-// deadlock could occur. Basically, SyncPointClientWaiter::Wait() should never
-// be called between SyncPointOrderData::BeginProcessingOrderNumber() and
-// SyncPointOrderData::FinishProcessingOrderNumber() on the same thread.
-class GPU_EXPORT SyncPointClientWaiter {
- public:
-  SyncPointClientWaiter() {}
-  ~SyncPointClientWaiter() {}
-
-  bool Wait(SyncPointClientState* release_state,
-            uint64_t release_count,
-            const base::Closure& wait_complete_callback);
-
-  bool WaitNonThreadSafe(SyncPointClientState* release_state,
-                         uint64_t release_count,
-                         scoped_refptr<base::SingleThreadTaskRunner> runner,
-                         const base::Closure& wait_complete_callback);
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(SyncPointClientWaiter);
-};
-
 // This class manages the sync points, which allow cross-channel
 // synchronization.
 class GPU_EXPORT SyncPointManager {
@@ -275,6 +276,10 @@ class GPU_EXPORT SyncPointManager {
       CommandBufferNamespace namespace_id,
       uint64_t client_id);
 
+  // Creates a sync point client which cannot process order numbers but can only
+  // Wait out of order.
+  scoped_ptr<SyncPointClient> CreateSyncPointClientWaiter();
+
   // Finds the state of an already created sync point client.
   scoped_refptr<SyncPointClientState> GetSyncPointClientState(
     CommandBufferNamespace namespace_id, uint64_t client_id);
diff --git a/gpu/command_buffer/service/sync_point_manager_unittest.cc b/gpu/command_buffer/service/sync_point_manager_unittest.cc
index 28321c4..cc67ff9 100644
--- a/gpu/command_buffer/service/sync_point_manager_unittest.cc
+++ b/gpu/command_buffer/service/sync_point_manager_unittest.cc
@@ -85,11 +85,19 @@ TEST_F(SyncPointManagerTest, BasicSyncPointOrderDataTest) {
   EXPECT_EQ(order_num, order_data->current_order_num());
   EXPECT_EQ(0u, order_data->processed_order_num());
   EXPECT_EQ(order_num, order_data->unprocessed_order_num());
+  EXPECT_TRUE(order_data->IsProcessingOrderNumber());
+
+  order_data->PauseProcessingOrderNumber(order_num);
+  EXPECT_FALSE(order_data->IsProcessingOrderNumber());
+
+  order_data->BeginProcessingOrderNumber(order_num);
+  EXPECT_TRUE(order_data->IsProcessingOrderNumber());
 
   order_data->FinishProcessingOrderNumber(order_num);
   EXPECT_EQ(order_num, order_data->current_order_num());
   EXPECT_EQ(order_num, order_data->processed_order_num());
   EXPECT_EQ(order_num, order_data->unprocessed_order_num());
+  EXPECT_FALSE(order_data->IsProcessingOrderNumber());
 }
 
 TEST_F(SyncPointManagerTest, SyncPointClientRegistration) {
@@ -127,7 +135,11 @@ TEST_F(SyncPointManagerTest, BasicFenceSyncRelease) {
   EXPECT_EQ(0u, client_state->fence_sync_release());
   EXPECT_FALSE(client_state->IsFenceSyncReleased(1));
 
+  const uint32_t order_num =
+      order_data->GenerateUnprocessedOrderNumber(sync_point_manager_.get());
+  order_data->BeginProcessingOrderNumber(order_num);
   client->ReleaseFenceSync(1);
+  order_data->FinishProcessingOrderNumber(order_num);
 
   EXPECT_EQ(1u, client_state->fence_sync_release());
   EXPECT_TRUE(client_state->IsFenceSyncReleased(1));
@@ -150,7 +162,11 @@ TEST_F(SyncPointManagerTest, MultipleClientsPerOrderData) {
   scoped_refptr<SyncPointClientState> client_state1 = client1->client_state();
   scoped_refptr<SyncPointClientState> client_state2 = client2->client_state();
 
+  const uint32_t order_num =
+      order_data->GenerateUnprocessedOrderNumber(sync_point_manager_.get());
+  order_data->BeginProcessingOrderNumber(order_num);
   client1->ReleaseFenceSync(1);
+  order_data->FinishProcessingOrderNumber(order_num);
 
   EXPECT_TRUE(client_state1->IsFenceSyncReleased(1));
   EXPECT_FALSE(client_state2->IsFenceSyncReleased(1));
diff --git a/gpu/command_buffer/tests/gl_fence_sync_unittest.cc b/gpu/command_buffer/tests/gl_fence_sync_unittest.cc
index 2d76476..50de9c1 100644
--- a/gpu/command_buffer/tests/gl_fence_sync_unittest.cc
+++ b/gpu/command_buffer/tests/gl_fence_sync_unittest.cc
@@ -67,4 +67,34 @@ TEST_F(GLFenceSyncTest, SimpleReleaseWait) {
   EXPECT_EQ(0u, gl2_client_state->fence_sync_release());
 }
 
+static void TestCallback(int* storage, int assign) {
+  *storage = assign;
+}
+
+TEST_F(GLFenceSyncTest, SimpleReleaseSignal) {
+  gl1_.MakeCurrent();
+
+  // Pause the command buffer so the fence sync does not immediately trigger.
+  gl1_.SetCommandsPaused(true);
+
+  const GLuint64 fence_sync = glInsertFenceSyncCHROMIUM();
+  SyncToken sync_token;
+  glGenUnverifiedSyncTokenCHROMIUM(fence_sync, sync_token.GetData());
+  glFlush();
+  ASSERT_TRUE(sync_token.HasData());
+
+  gl2_.MakeCurrent();
+  int callback_called = 0;
+  gl2_.SignalSyncToken(sync_token,
+                       base::Bind(TestCallback, &callback_called, 1));
+
+  gl1_.MakeCurrent();
+  EXPECT_EQ(0, callback_called);
+
+  gl1_.SetCommandsPaused(false);
+  glFinish();
+
+  EXPECT_EQ(1, callback_called);
+}
+
 }  // namespace gpu
diff --git a/gpu/command_buffer/tests/gl_manager.cc b/gpu/command_buffer/tests/gl_manager.cc
index 8632914..812d8fd 100644
--- a/gpu/command_buffer/tests/gl_manager.cc
+++ b/gpu/command_buffer/tests/gl_manager.cc
@@ -21,6 +21,7 @@
 #include "gpu/command_buffer/client/transfer_buffer.h"
 #include "gpu/command_buffer/common/constants.h"
 #include "gpu/command_buffer/common/gles2_cmd_utils.h"
+#include "gpu/command_buffer/common/sync_token.h"
 #include "gpu/command_buffer/common/value_state.h"
 #include "gpu/command_buffer/service/command_buffer_service.h"
 #include "gpu/command_buffer/service/context_group.h"
@@ -122,6 +123,8 @@ GLManager::Options::Options()
 GLManager::GLManager()
     : sync_point_manager_(nullptr),
       context_lost_allowed_(false),
+      pause_commands_(false),
+      paused_order_num_(0),
       command_buffer_id_(g_next_command_buffer_id++),
       next_fence_sync_release_(1) {
   SetupBaseContext();
@@ -402,11 +405,25 @@ void GLManager::PumpCommands() {
   uint32_t order_num = 0;
   if (sync_point_manager_) {
     // If sync point manager is supported, assign order numbers to commands.
-    order_num = sync_point_order_data_->GenerateUnprocessedOrderNumber(
-        sync_point_manager_);
+    if (paused_order_num_) {
+      // Was previous paused, continue to process the order number.
+      order_num = paused_order_num_;
+      paused_order_num_ = 0;
+    } else {
+      order_num = sync_point_order_data_->GenerateUnprocessedOrderNumber(
+          sync_point_manager_);
+    }
     sync_point_order_data_->BeginProcessingOrderNumber(order_num);
   }
 
+  if (pause_commands_) {
+    // Do not process commands, simply store the current order number.
+    paused_order_num_ = order_num;
+
+    sync_point_order_data_->PauseProcessingOrderNumber(order_num);
+    return;
+  }
+
   gpu_scheduler_->PutChanged();
   ::gpu::CommandBuffer::State state = command_buffer_->GetLastState();
   if (!context_lost_allowed_) {
@@ -533,7 +550,20 @@ bool GLManager::IsFenceSyncFlushReceived(uint64_t release) {
 
 void GLManager::SignalSyncToken(const gpu::SyncToken& sync_token,
                                 const base::Closure& callback) {
-  NOTIMPLEMENTED();
+  if (sync_point_manager_) {
+    scoped_refptr<gpu::SyncPointClientState> release_state =
+        sync_point_manager_->GetSyncPointClientState(
+            sync_token.namespace_id(), sync_token.command_buffer_id());
+
+    if (release_state) {
+      sync_point_client_->WaitOutOfOrder(release_state.get(),
+                                         sync_token.release_count(), callback);
+      return;
+    }
+  }
+
+  // Something went wrong, just run the callback now.
+  callback.Run();
 }
 
 bool GLManager::CanWaitUnverifiedSyncToken(const gpu::SyncToken* sync_token) {
diff --git a/gpu/command_buffer/tests/gl_manager.h b/gpu/command_buffer/tests/gl_manager.h
index 2b42c3f..5ff83f0 100644
--- a/gpu/command_buffer/tests/gl_manager.h
+++ b/gpu/command_buffer/tests/gl_manager.h
@@ -92,6 +92,8 @@ class GLManager : private GpuControl {
 
   void SetSurface(gfx::GLSurface* surface);
 
+  void SetCommandsPaused(bool paused) { pause_commands_ = paused; }
+
   gles2::GLES2Decoder* decoder() const {
     return decoder_.get();
   }
@@ -169,6 +171,8 @@ class GLManager : private GpuControl {
   scoped_ptr<TransferBuffer> transfer_buffer_;
   scoped_ptr<gles2::GLES2Implementation> gles2_implementation_;
   bool context_lost_allowed_;
+  bool pause_commands_;
+  uint32_t paused_order_num_;
 
   const uint64_t command_buffer_id_;
   uint64_t next_fence_sync_release_;