Refactor CommandBufferHelper to use Jump to loop

instead of Noop. This makes it actually work if you
tell the command buffer helper to use less than
the entire command buffer.

TEST=modified unit tests and ran GLES2 conformance tests.
BUG=NONE

Review URL: http://codereview.chromium.org/2509001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@48835 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 75 insertions, 40 deletions

diff --git a/gpu/command_buffer/client/cmd_buffer_helper.cc b/gpu/command_buffer/client/cmd_buffer_helper.cc
index 72724d7..ceec30e 100644
--- a/gpu/command_buffer/client/cmd_buffer_helper.cc
+++ b/gpu/command_buffer/client/cmd_buffer_helper.cc
@@ -12,7 +12,8 @@ namespace gpu {
 CommandBufferHelper::CommandBufferHelper(CommandBuffer* command_buffer)
     : command_buffer_(command_buffer),
       entries_(NULL),
-      entry_count_(0),
+      total_entry_count_(0),
+      usable_entry_count_(0),
       token_(0),
       last_token_read_(-1),
       get_(0),
@@ -30,7 +31,12 @@ bool CommandBufferHelper::Initialize(int32 ring_buffer_size) {
   if (num_ring_buffer_entries > state.num_entries) {
     return false;
   }
-  entry_count_ = num_ring_buffer_entries;
+
+  const int32 kJumpEntries =
+      sizeof(cmd::Jump) / sizeof(*entries_);  // NOLINT
+
+  total_entry_count_ = num_ring_buffer_entries;
+  usable_entry_count_ = total_entry_count_ - kJumpEntries;
   put_ = state.put_offset;
   SynchronizeState(state);
   return true;
@@ -99,16 +105,16 @@ void CommandBufferHelper::WaitForToken(int32 token) {
 
 // Waits for available entries, basically waiting until get >= put + count + 1.
 // It actually waits for contiguous entries, so it may need to wrap the buffer
-// around, adding noops. Thus this function may change the value of put_.
-// The function will return early if an error occurs, in which case the
-// available space may not be available.
+// around, adding a jump. Thus this function may change the value of put_. The
+// function will return early if an error occurs, in which case the available
+// space may not be available.
 void CommandBufferHelper::WaitForAvailableEntries(int32 count) {
-  CHECK(count < entry_count_);
-  if (put_ + count > entry_count_) {
+  CHECK(count < usable_entry_count_);
+  if (put_ + count > usable_entry_count_) {
     // There's not enough room between the current put and the end of the
-    // buffer, so we need to wrap. We will add noops all the way to the end,
-    // but we need to make sure get wraps first, actually that get is 1 or
-    // more (since put will wrap to 0 after we add the noops).
+    // buffer, so we need to wrap. We will add a jump back to the start, but we
+    // need to make sure get wraps first, actually that get is 1 or more (since
+    // put will wrap to 0 after we add the jump).
     DCHECK_LE(1, put_);
     Flush();
     while (get_ > put_ || get_ == 0) {
@@ -117,14 +123,8 @@ void CommandBufferHelper::WaitForAvailableEntries(int32 count) {
       if (!Flush())
         return;
     }
-    // Insert Noops to fill out buffer.
-    int32 num_entries = entry_count_ - put_;
-    while (num_entries > 0) {
-      int32 num_to_skip = std::min(CommandHeader::kMaxSize, num_entries);
-      cmd::Noop::Set(&entries_[put_], num_to_skip);
-      put_ += num_to_skip;
-      num_entries -= num_to_skip;
-    }
+    // Insert a jump back to the beginning.
+    cmd::Jump::Set(&entries_[put_], 0);
     put_ = 0;
   }
   // If we have enough room, return immediatly.
@@ -143,8 +143,9 @@ CommandBufferEntry* CommandBufferHelper::GetSpace(uint32 entries) {
   WaitForAvailableEntries(entries);
   CommandBufferEntry* space = &entries_[put_];
   put_ += entries;
-  DCHECK_LE(put_, entry_count_);
-  if (put_ == entry_count_) {
+  DCHECK_LE(put_, usable_entry_count_);
+  if (put_ == usable_entry_count_) {
+    cmd::Jump::Set(&entries_[put_], 0);
     put_ = 0;
   }
   return space;
diff --git a/gpu/command_buffer/client/cmd_buffer_helper.h b/gpu/command_buffer/client/cmd_buffer_helper.h
index cc93057..49ff97e 100644
--- a/gpu/command_buffer/client/cmd_buffer_helper.h
+++ b/gpu/command_buffer/client/cmd_buffer_helper.h
@@ -202,7 +202,7 @@ class CommandBufferHelper {
 
   // Returns the number of available entries (they may not be contiguous).
   int32 AvailableEntries() {
-    return (get_ - put_ - 1 + entry_count_) % entry_count_;
+    return (get_ - put_ - 1 + usable_entry_count_) % usable_entry_count_;
   }
 
   // Synchronize with current service state.
@@ -211,7 +211,8 @@ class CommandBufferHelper {
   CommandBuffer* command_buffer_;
   Buffer ring_buffer_;
   CommandBufferEntry *entries_;
-  int32 entry_count_;
+  int32 total_entry_count_;  // the total number of entries
+  int32 usable_entry_count_;  // the usable number (ie, minus space for jump)
   int32 token_;
   int32 last_token_read_;
   int32 get_;
diff --git a/gpu/command_buffer/client/cmd_buffer_helper_test.cc b/gpu/command_buffer/client/cmd_buffer_helper_test.cc
index a61d83b..1f3b53e 100644
--- a/gpu/command_buffer/client/cmd_buffer_helper_test.cc
+++ b/gpu/command_buffer/client/cmd_buffer_helper_test.cc
@@ -24,20 +24,42 @@ using testing::DoAll;
 using testing::Invoke;
 using testing::_;
 
-const int32 kNumCommandEntries = 10;
+const int32 kTotalNumCommandEntries = 12;
+const int32 kUsableNumCommandEntries = 10;
 const int32 kCommandBufferSizeBytes =
-    kNumCommandEntries * sizeof(CommandBufferEntry);
+    kTotalNumCommandEntries * sizeof(CommandBufferEntry);
+const int32 kUnusedCommandId = 5;  // we use 0 and 2 currently.
 
 // Test fixture for CommandBufferHelper test - Creates a CommandBufferHelper,
 // using a CommandBufferEngine with a mock AsyncAPIInterface for its interface
 // (calling it directly, not through the RPC mechanism).
 class CommandBufferHelperTest : public testing::Test {
  protected:
+  // Helper so mock can handle the Jump command.
+  class DoJumpCommand {
+   public:
+    explicit DoJumpCommand(CommandParser* parser)
+        : parser_(parser) {
+    }
+
+    error::Error DoCommand(
+        unsigned int command,
+        unsigned int arg_count,
+        const void* cmd_data) {
+      const cmd::Jump* jump_cmd = static_cast<const cmd::Jump*>(cmd_data);
+      parser_->set_get(jump_cmd->offset);
+      return error::kNoError;
+    };
+
+   private:
+    CommandParser* parser_;
+  };
+
   virtual void SetUp() {
     api_mock_.reset(new AsyncAPIMock);
     // ignore noops in the mock - we don't want to inspect the internals of the
     // helper.
-    EXPECT_CALL(*api_mock_, DoCommand(0, _, _))
+    EXPECT_CALL(*api_mock_, DoCommand(cmd::kNoop, _, _))
         .WillRepeatedly(Return(error::kNoError));
 
     command_buffer_.reset(new CommandBufferService);
@@ -51,6 +73,11 @@ class CommandBufferHelperTest : public testing::Test {
                                 0,
                                 api_mock_.get());
 
+    do_jump_command_.reset(new DoJumpCommand(parser_));
+    EXPECT_CALL(*api_mock_, DoCommand(cmd::kJump, _, _))
+        .WillRepeatedly(
+            Invoke(do_jump_command_.get(), &DoJumpCommand::DoCommand));
+
     gpu_processor_.reset(new GPUProcessor(
         command_buffer_.get(), NULL, parser_, 1));
     command_buffer_->SetPutOffsetChangeCallback(NewCallback(
@@ -104,7 +131,7 @@ class CommandBufferHelperTest : public testing::Test {
       // put to the limit) and the bottom side (from 0 to get).
       if (put >= parser_put) {
         // we're on the top side, check we are below the limit.
-        EXPECT_GE(kNumCommandEntries, limit);
+        EXPECT_GE(kTotalNumCommandEntries, limit);
       } else {
         // we're on the bottom side, check we are below get.
         EXPECT_GT(parser_get, limit);
@@ -135,6 +162,7 @@ class CommandBufferHelperTest : public testing::Test {
   CommandParser* parser_;
   scoped_ptr<CommandBufferHelper> helper_;
   Sequence sequence_;
+  scoped_ptr<DoJumpCommand> do_jump_command_;
 };
 
 // Checks that commands in the buffer are properly executed, and that the
@@ -147,17 +175,17 @@ TEST_F(CommandBufferHelperTest, TestCommandProcessing) {
   EXPECT_EQ(0, GetGetOffset());
 
   // Add 3 commands through the helper
-  AddCommandWithExpect(error::kNoError, 1, 0, NULL);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId, 0, NULL);
 
   CommandBufferEntry args1[2];
   args1[0].value_uint32 = 3;
   args1[1].value_float = 4.f;
-  AddCommandWithExpect(error::kNoError, 2, 2, args1);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId, 2, args1);
 
   CommandBufferEntry args2[2];
   args2[0].value_uint32 = 5;
   args2[1].value_float = 6.f;
-  AddCommandWithExpect(error::kNoError, 3, 2, args2);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId, 2, args2);
 
   helper_->Flush();
   // Check that the engine has work to do now.
@@ -184,7 +212,7 @@ TEST_F(CommandBufferHelperTest, TestCommandWrapping) {
   args1[1].value_float = 4.f;
 
   for (unsigned int i = 0; i < 5; ++i) {
-    AddCommandWithExpect(error::kNoError, i + 1, 2, args1);
+    AddCommandWithExpect(error::kNoError, kUnusedCommandId + i, 2, args1);
   }
 
   helper_->Finish();
@@ -200,7 +228,7 @@ TEST_F(CommandBufferHelperTest, TestCommandWrapping) {
 TEST_F(CommandBufferHelperTest, TestCommandWrappingExactMultiple) {
   const int32 kCommandSize = 5;
   const size_t kNumArgs = kCommandSize - 1;
-  COMPILE_ASSERT(kNumCommandEntries % kCommandSize == 0,
+  COMPILE_ASSERT(kUsableNumCommandEntries % kCommandSize == 0,
                  Not_multiple_of_num_command_entries);
   CommandBufferEntry args1[kNumArgs];
   for (size_t ii = 0; ii < kNumArgs; ++ii) {
@@ -208,7 +236,8 @@ TEST_F(CommandBufferHelperTest, TestCommandWrappingExactMultiple) {
   }
 
   for (unsigned int i = 0; i < 5; ++i) {
-    AddCommandWithExpect(error::kNoError, i + 1, kNumArgs, args1);
+    AddCommandWithExpect(
+        error::kNoError, i + kUnusedCommandId, kNumArgs, args1);
   }
 
   helper_->Finish();
@@ -227,10 +256,10 @@ TEST_F(CommandBufferHelperTest, TestAvailableEntries) {
   args[1].value_float = 4.f;
 
   // Add 2 commands through the helper - 8 entries
-  AddCommandWithExpect(error::kNoError, 1, 0, NULL);
-  AddCommandWithExpect(error::kNoError, 2, 0, NULL);
-  AddCommandWithExpect(error::kNoError, 3, 2, args);
-  AddCommandWithExpect(error::kNoError, 4, 2, args);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId + 1, 0, NULL);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId + 2, 0, NULL);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId + 3, 2, args);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId + 4, 2, args);
 
   // Ask for 5 entries.
   helper_->WaitForAvailableEntries(5);
@@ -239,7 +268,7 @@ TEST_F(CommandBufferHelperTest, TestAvailableEntries) {
   CheckFreeSpace(put, 5);
 
   // Add more commands.
-  AddCommandWithExpect(error::kNoError, 5, 2, args);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId + 5, 2, args);
 
   // Wait until everything is done done.
   helper_->Finish();
@@ -258,7 +287,7 @@ TEST_F(CommandBufferHelperTest, TestToken) {
   args[1].value_float = 4.f;
 
   // Add a first command.
-  AddCommandWithExpect(error::kNoError, 3, 2, args);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId + 3, 2, args);
   // keep track of the buffer position.
   CommandBufferOffset command1_put = get_helper_put();
   int32 token = helper_->InsertToken();
@@ -267,7 +296,7 @@ TEST_F(CommandBufferHelperTest, TestToken) {
       .WillOnce(DoAll(Invoke(api_mock_.get(), &AsyncAPIMock::SetToken),
                       Return(error::kNoError)));
   // Add another command.
-  AddCommandWithExpect(error::kNoError, 4, 2, args);
+  AddCommandWithExpect(error::kNoError, kUnusedCommandId + 4, 2, args);
   helper_->WaitForToken(token);
   // check that the get pointer is beyond the first command.
   EXPECT_LE(command1_put, GetGetOffset());
diff --git a/gpu/command_buffer/service/cmd_parser.cc b/gpu/command_buffer/service/cmd_parser.cc
index 191ddfa..a4b4755 100644
--- a/gpu/command_buffer/service/cmd_parser.cc
+++ b/gpu/command_buffer/service/cmd_parser.cc
@@ -58,7 +58,11 @@ error::Error CommandParser::ProcessCommand() {
   if (result != error::kNoError) {
     ReportError(header.command, result);
   }
-  get_ = (get + header.size) % entry_count_;
+
+  // If get was not set somewhere else advance it.
+  if (get == get_) {
+    get_ = (get + header.size) % entry_count_;
+  }
   return result;
 }