Fixing scheduler order number DCHECK when called from ShouldYield.

Fixes https://crbug.com/1400202. The unit test's code is the simplest
explanation: ShouldYield is called from a task that hasn't released its
fence, FindNextTaskFromRoot asserts because the sequence's next task
order_num is greater than the wait fence's order number.

This is harmless if there is only scheduler thread (because the
recursion into FindNextFromRoot immediately terminates because the
sequence is already running). Leaving the check until I find the root
cause of https://crbug.com/1400203.

Bug: 1400202
Change-Id: I459a06c0725552df3855dbad6db67833bfeafcc5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4110028
Auto-Submit: Ramy El Garawany <elgarawany@google.com>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1084008}

gpu/command_buffer/service/scheduler_dfs.cc

@@ -576,11 +576,13 @@ SchedulerDfs::Sequence* SchedulerDfs::FindNextTaskFromRoot(
               << " (order_num: " << fence_iter->first.order_num << ").";
     Sequence* release_sequence =
         GetSequence(fence_iter->first.release_sequence_id);
-    // Sanity check to make sure we're not processing a sequence on another
-    // thread that's past
-    DCHECK(!(release_sequence && release_sequence->HasTasks() &&
-             release_sequence->tasks_.front().order_num >=
-                 fence_iter->first.order_num));
+    // ShouldYield might be calling this function, and a dependency might depend
+    // on the calling sequence, which might have not released its fences yet.
+    if (release_sequence && release_sequence->HasTasks() &&
+        release_sequence->tasks_.front().order_num >=
+            fence_iter->first.order_num) {
+      continue;
+    }
     if (Sequence* result = FindNextTaskFromRoot(release_sequence);
         result != nullptr) {
       return result;
@@ -617,7 +619,7 @@ SchedulerDfs::Sequence* SchedulerDfs::FindNextTask() {
   // dependency tied to another thread.
   for (const SchedulingState& state : sorted_sequences) {
     Sequence* root_sequence = GetSequence(state.sequence_id);
-    DVLOG(10) << "RunNextTask: Calling FindNextTask on sequence "
+    DVLOG(10) << "FindNextTask: Calling FindNextTaskFromRoot on sequence "
               << root_sequence->sequence_id().value();
     if (Sequence* sequence = FindNextTaskFromRoot(root_sequence);
         sequence != nullptr) {
@@ -767,9 +769,7 @@ void SchedulerDfs::ExecuteSequence(const SequenceId sequence_id) {
   total_blocked_time_ += blocked_time;
 
   // Reset pointers after reaquiring the lock.
-  thread_state = &per_thread_state_map_[task_runner];
   sequence = GetSequence(sequence_id);
-
   if (sequence) {
     sequence->FinishTask();
   }

gpu/command_buffer/service/scheduler_dfs_unittest.cc

@@ -15,7 +15,6 @@
 #include "base/functional/bind.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
-#include "base/test/test_simple_task_runner.h"
 #include "base/time/time.h"
 #include "gpu/command_buffer/service/scheduler.h"
 #include "gpu/command_buffer/service/sync_point_manager.h"
@@ -171,9 +170,9 @@ class SchedulerDfsTaskRunOrderTest : public SchedulerDfsTest {
         sync_point_manager()->CreateSyncPointClientState(
             kNamespaceId, command_buffer_id, sequence_id);
 
-    sequence_info_.emplace(std::make_pair(
+    sequence_info_.emplace(
         sequence_key,
-        SequenceInfo(sequence_id, command_buffer_id, release_state)));
+        SequenceInfo(sequence_id, command_buffer_id, release_state));
   }
 
   void CreateExternalSequence(int sequence_key) {
@@ -182,9 +181,9 @@ class SchedulerDfsTaskRunOrderTest : public SchedulerDfsTest {
     auto release_state = sync_point_manager()->CreateSyncPointClientState(
         kNamespaceId, command_buffer_id, order_data->sequence_id());
 
-    sequence_info_.emplace(std::make_pair(
+    sequence_info_.emplace(
         sequence_key,
-        SequenceInfo(std::move(order_data), command_buffer_id, release_state)));
+        SequenceInfo(std::move(order_data), command_buffer_id, release_state));
   }
 
   void DestroySequence(int sequence_key) {
@@ -205,9 +204,9 @@ class SchedulerDfsTaskRunOrderTest : public SchedulerDfsTest {
     ASSERT_TRUE(info_it != sequence_info_.end());
 
     uint64_t release = release_sync + 1;
-    sync_tokens_.emplace(std::make_pair(
+    sync_tokens_.emplace(
         release_sync,
-        SyncToken(kNamespaceId, info_it->second.command_buffer_id, release)));
+        SyncToken(kNamespaceId, info_it->second.command_buffer_id, release));
   }
 
   static void RunExternalTask(base::OnceClosure task,
@@ -605,6 +604,56 @@ TEST_F(SchedulerDfsTest, ReleaseSequenceShouldYield) {
   scheduler()->DestroySequence(sequence_id2);
 }
 
+// Tests a situation where a sequence's WaitFence has an order number less than
+// the sequence's first order number, because the sequence is currently running,
+// and called ShouldYield before release the WaitFence.
+TEST_F(SchedulerDfsTest, ShouldYieldIsValidWhenSequenceReleaseIsPending) {
+  SequenceId sequence_id1 =
+      scheduler()->CreateSequenceForTesting(SchedulingPriority::kHigh);
+  CommandBufferNamespace namespace_id = CommandBufferNamespace::GPU_IO;
+  CommandBufferId command_buffer_id1 = CommandBufferId::FromUnsafeValue(1);
+  scoped_refptr<SyncPointClientState> release_state1 =
+      sync_point_manager()->CreateSyncPointClientState(
+          namespace_id, command_buffer_id1, sequence_id1);
+
+  SequenceId sequence_id2 =
+      scheduler()->CreateSequenceForTesting(SchedulingPriority::kNormal);
+  CommandBufferId command_buffer_id2 = CommandBufferId::FromUnsafeValue(2);
+  scoped_refptr<SyncPointClientState> release_state2 =
+      sync_point_manager()->CreateSyncPointClientState(
+          namespace_id, command_buffer_id2, sequence_id2);
+
+  SyncToken sync_token1(namespace_id, command_buffer_id1, 1);
+  SyncToken sync_token2(namespace_id, command_buffer_id2, 2);
+
+  // Job 1.1 doesn't depend on anything.
+  scheduler()->ScheduleTask(
+      Scheduler::Task(sequence_id1, GetClosure([&] {
+                        EXPECT_FALSE(scheduler()->ShouldYield(sequence_id1));
+                        release_state1->ReleaseFenceSync(1);
+                      }),
+                      {}));
+
+  // Job 2.1 depends on Job 1.1.
+  scheduler()->ScheduleTask(Scheduler::Task(sequence_id2, GetClosure([&] {
+                                              release_state2->ReleaseFenceSync(
+                                                  sync_token2.release_count());
+                                            }),
+                                            {sync_token1}));
+
+  // Job 1.2 depends on Job 2.1.
+  scheduler()->ScheduleTask(
+      Scheduler::Task(sequence_id1, GetClosure([&] {}), {sync_token2}));
+
+  RunAllPendingTasks();
+
+  release_state1->Destroy();
+  release_state2->Destroy();
+
+  scheduler()->DestroySequence(sequence_id1);
+  scheduler()->DestroySequence(sequence_id2);
+}
+
 TEST_F(SchedulerDfsTest, ReentrantEnableSequenceShouldNotDeadlock) {
   SequenceId sequence_id1 =
       scheduler()->CreateSequenceForTesting(SchedulingPriority::kHigh);