Make SetGetBuffer async

This removes one sync IPC at initialization time, and after we release the command buffer.
We make sure we handle the possibility of having more than one SetGetBuffer in flight by
counting them on client side and service side, making sure we only ever compare |get| values
for the same buffer.

Bug: 484375
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Ife91dd5846e9d82f03e5bfe5d022a06284389756
Reviewed-on: https://chromium-review.googlesource.com/502016
Commit-Queue: Antoine Labour <piman@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Victor Miura <vmiura@chromium.org>
Cr-Commit-Position: refs/heads/master@{#472312}

content/renderer/pepper/ppb_graphics_3d_impl.cc

@@ -113,9 +113,11 @@ gpu::CommandBuffer::State PPB_Graphics3D_Impl::WaitForTokenInRange(
 }
 
 gpu::CommandBuffer::State PPB_Graphics3D_Impl::WaitForGetOffsetInRange(
+    uint32_t set_get_buffer_count,
     int32_t start,
     int32_t end) {
-  return GetCommandBuffer()->WaitForGetOffsetInRange(start, end);
+  return GetCommandBuffer()->WaitForGetOffsetInRange(set_get_buffer_count,
+                                                     start, end);
 }
 
 void PPB_Graphics3D_Impl::EnsureWorkVisible() {

content/renderer/pepper/ppb_graphics_3d_impl.h

@@ -48,8 +48,10 @@ class PPB_Graphics3D_Impl : public ppapi::PPB_Graphics3D_Shared,
   PP_Bool Flush(int32_t put_offset) override;
   gpu::CommandBuffer::State WaitForTokenInRange(int32_t start,
                                                 int32_t end) override;
-  gpu::CommandBuffer::State WaitForGetOffsetInRange(int32_t start,
-                                                    int32_t end) override;
+  gpu::CommandBuffer::State WaitForGetOffsetInRange(
+      uint32_t set_get_buffer_count,
+      int32_t start,
+      int32_t end) override;
   void EnsureWorkVisible() override;
   void TakeFrontBuffer() override;
   void ReturnFrontBuffer(const gpu::Mailbox& mailbox,

gpu/command_buffer/client/client_test_helper.cc

@@ -42,14 +42,19 @@ CommandBuffer::State MockCommandBufferBase::WaitForTokenInRange(int32_t start,
 }
 
 CommandBuffer::State MockCommandBufferBase::WaitForGetOffsetInRange(
+    uint32_t set_get_buffer_count,
     int32_t start,
     int32_t end) {
-  state_.get_offset = put_offset_;
-  OnFlush();
+  EXPECT_EQ(set_get_buffer_count, state_.set_get_buffer_count);
+  if (state_.get_offset != put_offset_) {
+    state_.get_offset = put_offset_;
+    OnFlush();
+  }
   return state_;
 }
 
 void MockCommandBufferBase::SetGetBuffer(int transfer_buffer_id) {
+  ++state_.set_get_buffer_count;
   ring_buffer_buffer_ = GetTransferBuffer(transfer_buffer_id);
   ring_buffer_ =
       static_cast<CommandBufferEntry*>(ring_buffer_buffer_->memory());

gpu/command_buffer/client/client_test_helper.h

@@ -35,7 +35,9 @@ class MockCommandBufferBase : public CommandBufferServiceBase {
 
   State GetLastState() override;
   State WaitForTokenInRange(int32_t start, int32_t end) override;
-  State WaitForGetOffsetInRange(int32_t start, int32_t end) override;
+  State WaitForGetOffsetInRange(uint32_t set_get_buffer_count,
+                                int32_t start,
+                                int32_t end) override;
   void SetGetBuffer(int transfer_buffer_id) override;
   void SetGetOffset(int32_t get_offset) override;
   void SetReleaseCount(uint64_t release_count) override;

gpu/command_buffer/client/cmd_buffer_helper.cc

@@ -35,6 +35,8 @@ CommandBufferHelper::CommandBufferHelper(CommandBuffer* command_buffer)
       last_put_sent_(0),
       cached_last_token_read_(0),
       cached_get_offset_(0),
+      set_get_buffer_count_(0),
+      service_on_old_buffer_(false),
 #if defined(CMD_HELPER_PERIODIC_FLUSH_CHECK)
       commands_issued_(0),
 #endif
@@ -117,12 +119,15 @@ bool CommandBufferHelper::AllocateRingBuffer() {
   ring_buffer_ = buffer;
   ring_buffer_id_ = id;
   command_buffer_->SetGetBuffer(id);
+  ++set_get_buffer_count_;
   entries_ = static_cast<CommandBufferEntry*>(ring_buffer_->memory());
   total_entry_count_ = ring_buffer_size_ / sizeof(CommandBufferEntry);
   // Call to SetGetBuffer(id) above resets get and put offsets to 0.
   // No need to query it through IPC.
   put_ = 0;
+  last_put_sent_ = 0;
   cached_get_offset_ = 0;
+  service_on_old_buffer_ = true;
   CalcImmediateEntries(0);
   return true;
 }
@@ -153,7 +158,12 @@ CommandBufferHelper::~CommandBufferHelper() {
 }
 
 void CommandBufferHelper::UpdateCachedState(const CommandBuffer::State& state) {
-  cached_get_offset_ = state.get_offset;
+  // If the service hasn't seen the current get buffer yet (i.e. hasn't
+  // processed the latest SetGetBuffer), it's as if it hadn't processed anything
+  // in it, i.e. get == 0.
+  service_on_old_buffer_ =
+      (state.set_get_buffer_count != set_get_buffer_count_);
+  cached_get_offset_ = service_on_old_buffer_ ? 0 : state.get_offset;
   cached_last_token_read_ = state.token;
   context_lost_ = error::IsError(state.error);
 }
@@ -164,8 +174,8 @@ bool CommandBufferHelper::WaitForGetOffsetInRange(int32_t start, int32_t end) {
   if (!usable()) {
     return false;
   }
-  CommandBuffer::State last_state =
-      command_buffer_->WaitForGetOffsetInRange(start, end);
+  CommandBuffer::State last_state = command_buffer_->WaitForGetOffsetInRange(
+      set_get_buffer_count_, start, end);
   UpdateCachedState(last_state);
   return !context_lost_;
 }
@@ -214,12 +224,13 @@ bool CommandBufferHelper::Finish() {
     return false;
   }
   // If there is no work just exit.
-  if (put_ == cached_get_offset_) {
+  if (put_ == cached_get_offset_ && !service_on_old_buffer_) {
     return true;
   }
   DCHECK(HaveRingBuffer() ||
          error::IsError(command_buffer_->GetLastState().error));
-  Flush();
+  if (last_put_sent_ != put_)
+    Flush();
   if (!WaitForGetOffsetInRange(put_, put_))
     return false;
   DCHECK_EQ(cached_get_offset_, put_);

gpu/command_buffer/client/cmd_buffer_helper.h

@@ -301,6 +301,8 @@ class GPU_EXPORT CommandBufferHelper
   int32_t last_put_sent_;
   int32_t cached_last_token_read_;
   int32_t cached_get_offset_;
+  uint32_t set_get_buffer_count_;
+  bool service_on_old_buffer_;
 
 #if defined(CMD_HELPER_PERIODIC_FLUSH_CHECK)
   int commands_issued_;

gpu/command_buffer/client/cmd_buffer_helper_test.cc

@@ -69,15 +69,18 @@ class CommandBufferServiceLocked : public CommandBufferService {
 
   int FlushCount() { return flush_count_; }
 
-  State WaitForGetOffsetInRange(int32_t start, int32_t end) override {
+  State WaitForGetOffsetInRange(uint32_t set_get_buffer_count,
+                                int32_t start,
+                                int32_t end) override {
     // Flush only if it's required to unblock this Wait.
     if (last_flush_ != -1 &&
         !CommandBuffer::InRange(start, end, previous_put_offset_)) {
       previous_put_offset_ = last_flush_;
       CommandBufferService::Flush(last_flush_);
       last_flush_ = -1;
     }
-    return CommandBufferService::WaitForGetOffsetInRange(start, end);
+    return CommandBufferService::WaitForGetOffsetInRange(set_get_buffer_count,
+                                                         start, end);
   }
 
  private:

gpu/command_buffer/common/command_buffer.h

@@ -25,8 +25,8 @@ class GPU_EXPORT CommandBuffer {
           release_count(0),
           error(error::kNoError),
           context_lost_reason(error::kUnknown),
-          generation(0) {
-    }
+          generation(0),
+          set_get_buffer_count(0) {}
 
     // The offset (in entries) from which the reader is reading.
     int32_t get_offset;
@@ -52,6 +52,10 @@ class GPU_EXPORT CommandBuffer {
     // time a new state is retrieved from the command processor, so that
     // consistency can be kept even if IPC messages are processed out-of-order.
     uint32_t generation;
+
+    // Number of times SetGetBuffer was called. This allows the client to verify
+    // that |get| corresponds (or not) to the last buffer it set.
+    uint32_t set_get_buffer_count;
   };
 
   struct ConsoleMessage {
@@ -95,11 +99,15 @@ class GPU_EXPORT CommandBuffer {
   virtual State WaitForTokenInRange(int32_t start, int32_t end) = 0;
 
   // The writer calls this to wait until the current get offset is within a
-  // specific range, inclusive. Can return early if an error is generated.
-  virtual State WaitForGetOffsetInRange(int32_t start, int32_t end) = 0;
+  // specific range, inclusive, after SetGetBuffer was called exactly
+  // set_get_buffer_count times. Can return early if an error is generated.
+  virtual State WaitForGetOffsetInRange(uint32_t set_get_buffer_count,
+                                        int32_t start,
+                                        int32_t end) = 0;
 
   // Sets the buffer commands are read from.
-  // Also resets the get and put offsets to 0.
+  // Also resets the get and put offsets to 0, and increments
+  // set_get_buffer_count.
   virtual void SetGetBuffer(int32_t transfer_buffer_id) = 0;
 
   // Create a transfer buffer of the given size. Returns its ID or -1 on

gpu/command_buffer/common/command_buffer_mock.h

@@ -26,7 +26,10 @@ class MockCommandBuffer : public CommandBufferServiceBase {
   MOCK_METHOD1(Flush, void(int32_t put_offset));
   MOCK_METHOD1(OrderingBarrier, void(int32_t put_offset));
   MOCK_METHOD2(WaitForTokenInRange, State(int32_t start, int32_t end));
-  MOCK_METHOD2(WaitForGetOffsetInRange, State(int32_t start, int32_t end));
+  MOCK_METHOD3(WaitForGetOffsetInRange,
+               State(uint32_t set_get_buffer_count,
+                     int32_t start,
+                     int32_t end));
   MOCK_METHOD1(SetGetBuffer, void(int32_t transfer_buffer_id));
   MOCK_METHOD1(SetGetOffset, void(int32_t get_offset));
   MOCK_METHOD1(SetReleaseCount, void(uint64_t release_count));

gpu/command_buffer/service/command_buffer_service.cc

@@ -50,6 +50,7 @@ CommandBufferService::CommandBufferService(
       token_(0),
       release_count_(0),
       generation_(0),
+      set_get_buffer_count_(0),
       error_(error::kNoError),
       context_lost_reason_(error::kUnknown) {}
 
@@ -63,6 +64,7 @@ CommandBufferService::State CommandBufferService::GetLastState() {
   state.error = error_;
   state.context_lost_reason = context_lost_reason_;
   state.generation = ++generation_;
+  state.set_get_buffer_count = set_get_buffer_count_;
 
   return state;
 }
@@ -81,9 +83,12 @@ CommandBuffer::State CommandBufferService::WaitForTokenInRange(int32_t start,
 }
 
 CommandBuffer::State CommandBufferService::WaitForGetOffsetInRange(
+    uint32_t set_get_buffer_count,
     int32_t start,
     int32_t end) {
-  DCHECK(error_ != error::kNoError || InRange(start, end, get_offset_));
+  DCHECK(error_ != error::kNoError ||
+         (InRange(start, end, get_offset_) &&
+          (set_get_buffer_count == set_get_buffer_count_)));
   return GetLastState();
 }
 
@@ -114,6 +119,7 @@ void CommandBufferService::SetGetBuffer(int32_t transfer_buffer_id) {
   num_entries_ = size / sizeof(CommandBufferEntry);
   put_offset_ = 0;
   SetGetOffset(0);
+  ++set_get_buffer_count_;
   if (!get_buffer_change_callback_.is_null()) {
     get_buffer_change_callback_.Run(ring_buffer_id_);
   }

gpu/command_buffer/service/command_buffer_service.h

@@ -59,7 +59,9 @@ class GPU_EXPORT CommandBufferService : public CommandBufferServiceBase {
   void Flush(int32_t put_offset) override;
   void OrderingBarrier(int32_t put_offset) override;
   State WaitForTokenInRange(int32_t start, int32_t end) override;
-  State WaitForGetOffsetInRange(int32_t start, int32_t end) override;
+  State WaitForGetOffsetInRange(uint32_t set_get_buffer_count,
+                                int32_t start,
+                                int32_t end) override;
   void SetGetBuffer(int32_t transfer_buffer_id) override;
   scoped_refptr<Buffer> CreateTransferBuffer(size_t size, int32_t* id) override;
   void DestroyTransferBuffer(int32_t id) override;
@@ -114,6 +116,7 @@ class GPU_EXPORT CommandBufferService : public CommandBufferServiceBase {
   int32_t token_;
   uint64_t release_count_;
   uint32_t generation_;
+  uint64_t set_get_buffer_count_;
   error::Error error_;
   error::ContextLostReason context_lost_reason_;
 

gpu/command_buffer/service/command_buffer_service_unittest.cc

@@ -126,8 +126,12 @@ TEST_F(CommandBufferServiceTest, SetGetBuffer) {
   EXPECT_CALL(*change_callback, GetBufferChanged(ring_buffer_id))
       .WillOnce(Return(true));
 
+  uint32_t set_get_buffer_count =
+      command_buffer_->GetLastState().set_get_buffer_count;
   command_buffer_->SetGetBuffer(ring_buffer_id);
   EXPECT_EQ(0, GetGetOffset());
+  EXPECT_EQ(set_get_buffer_count + 1,
+            command_buffer_->GetLastState().set_get_buffer_count);
 }
 
 TEST_F(CommandBufferServiceTest, DefaultTokenIsZero) {

gpu/ipc/client/command_buffer_proxy_impl.cc

@@ -371,6 +371,7 @@ gpu::CommandBuffer::State CommandBufferProxyImpl::WaitForTokenInRange(
 }
 
 gpu::CommandBuffer::State CommandBufferProxyImpl::WaitForGetOffsetInRange(
+    uint32_t set_get_buffer_count,
     int32_t start,
     int32_t end) {
   CheckLock();
@@ -386,14 +387,16 @@ gpu::CommandBuffer::State CommandBufferProxyImpl::WaitForGetOffsetInRange(
     return last_state_;
   }
   TryUpdateState();
-  if (!InRange(start, end, last_state_.get_offset) &&
+  if (((set_get_buffer_count != last_state_.set_get_buffer_count) ||
+       !InRange(start, end, last_state_.get_offset)) &&
       last_state_.error == gpu::error::kNoError) {
     gpu::CommandBuffer::State state;
-    if (Send(new GpuCommandBufferMsg_WaitForGetOffsetInRange(route_id_, start,
-                                                             end, &state)))
+    if (Send(new GpuCommandBufferMsg_WaitForGetOffsetInRange(
+            route_id_, set_get_buffer_count, start, end, &state)))
       SetStateFromSyncReply(state);
   }
-  if (!InRange(start, end, last_state_.get_offset) &&
+  if (((set_get_buffer_count != last_state_.set_get_buffer_count) ||
+       !InRange(start, end, last_state_.get_offset)) &&
       last_state_.error == gpu::error::kNoError) {
     LOG(ERROR) << "GPU state invalid after WaitForGetOffsetInRange.";
     OnGpuSyncReplyError();

gpu/ipc/client/command_buffer_proxy_impl.h

@@ -98,7 +98,9 @@ class GPU_EXPORT CommandBufferProxyImpl
   void Flush(int32_t put_offset) override;
   void OrderingBarrier(int32_t put_offset) override;
   State WaitForTokenInRange(int32_t start, int32_t end) override;
-  State WaitForGetOffsetInRange(int32_t start, int32_t end) override;
+  State WaitForGetOffsetInRange(uint32_t set_get_buffer_count,
+                                int32_t start,
+                                int32_t end) override;
   void SetGetBuffer(int32_t shm_id) override;
   scoped_refptr<gpu::Buffer> CreateTransferBuffer(size_t size,
                                                   int32_t* id) override;

gpu/ipc/common/gpu_command_buffer_traits.cc

@@ -38,40 +38,6 @@ namespace IPC {
 
 namespace IPC {
 
-void ParamTraits<gpu::CommandBuffer::State>::GetSize(base::PickleSizer* s,
-                                                     const param_type& p) {
-  GetParamSize(s, p.get_offset);
-  GetParamSize(s, p.token);
-  GetParamSize(s, p.error);
-  GetParamSize(s, p.generation);
-}
-
-void ParamTraits<gpu::CommandBuffer::State>::Write(base::Pickle* m,
-                                                   const param_type& p) {
-  WriteParam(m, p.get_offset);
-  WriteParam(m, p.token);
-  WriteParam(m, p.error);
-  WriteParam(m, p.generation);
-}
-
-bool ParamTraits<gpu::CommandBuffer::State>::Read(const base::Pickle* m,
-                                                  base::PickleIterator* iter,
-                                                  param_type* p) {
-  if (ReadParam(m, iter, &p->get_offset) &&
-      ReadParam(m, iter, &p->token) &&
-      ReadParam(m, iter, &p->error) &&
-      ReadParam(m, iter, &p->generation)) {
-    return true;
-  } else {
-    return false;
-  }
-}
-
-void ParamTraits<gpu::CommandBuffer::State> ::Log(const param_type& p,
-                                                  std::string* l) {
-  l->append("<CommandBuffer::State>");
-}
-
 void ParamTraits<gpu::SyncToken>::GetSize(base::PickleSizer* s,
                                           const param_type& p) {
   DCHECK(!p.HasData() || p.verified_flush());

gpu/ipc/common/gpu_command_buffer_traits.h

@@ -5,7 +5,6 @@
 #ifndef GPU_IPC_COMMON_GPU_COMMAND_BUFFER_TRAITS_H_
 #define GPU_IPC_COMMON_GPU_COMMAND_BUFFER_TRAITS_H_
 
-#include "gpu/command_buffer/common/command_buffer.h"
 #include "gpu/command_buffer/common/id_type.h"
 #include "gpu/gpu_export.h"
 #include "gpu/ipc/common/gpu_command_buffer_traits_multi.h"
@@ -21,17 +20,6 @@ struct TextureInUseResponse;
 
 namespace IPC {
 
-template <>
-struct GPU_EXPORT ParamTraits<gpu::CommandBuffer::State> {
-  using param_type = gpu::CommandBuffer::State;
-  static void GetSize(base::PickleSizer* s, const param_type& p);
-  static void Write(base::Pickle* m, const param_type& p);
-  static bool Read(const base::Pickle* m,
-                   base::PickleIterator* iter,
-                   param_type* p);
-  static void Log(const param_type& p, std::string* l);
-};
-
 template <>
 struct GPU_EXPORT ParamTraits<gpu::SyncToken> {
   using param_type = gpu::SyncToken;