Fix mode modeset fallback.

The CL fixes the following issues:
1. When looping over pending modeset requests, do not depend on the
dynamic vector size.
2. If the display modeset has failed, do not always select the next
available mode, check that it's valid first.
3. Retry configuring displays when any of them fail, not just the last
one.
4. Use default values for unittest variables.

BUG=b/168800852
TEST=should configure multiple displays even after the first mode is
rejected. display_unittests

Change-Id: I285d02f4b7f653588b53c48655cc05514264242d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2417008Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Gil Dekel <gildekel@chromium.org>
Commit-Queue: Mark Yacoub <markyacoub@google.com>
Cr-Commit-Position: refs/heads/master@{#808472}

ui/display/manager/configure_displays_task.cc

@@ -118,11 +118,18 @@ void ConfigureDisplaysTask::Run() {
 
   {
     base::AutoReset<bool> recursivity_guard(&is_configuring_, true);
+    // The callback passed to delegate_->Configure() could be run synchronously
+    // or async. If it runs synchronously and any display fails and tries to
+    // reconfigure, new requests will get added to |pending_request_indexes_|,
+    // then another attempt to reconfigure will recursively call Run(). However
+    // |is_configuring_| will block the run due to the reason mentioned above.
+    // Hence, after we configure the first set of pending requests (loop over
+    // |current_pending_requests_count|), we check again if the new requests
+    // were added (!pending_request_indexes_.empty()).
     while (!pending_request_indexes_.empty()) {
-      // Loop over all the current requests, then it will loop again making sure
-      // no new requests were added and are pending.
       std::vector<display::DisplayConfigurationParams> config_requests;
-      for (size_t i = 0; i < pending_request_indexes_.size(); ++i) {
+      size_t current_pending_requests_count = pending_request_indexes_.size();
+      for (size_t i = 0; i < current_pending_requests_count; ++i) {
         size_t index = pending_request_indexes_.front();
         DisplayConfigureRequest* request = &requests_[index];
         pending_request_indexes_.pop();
@@ -215,9 +222,12 @@ void ConfigureDisplaysTask::OnConfigured(
       if (!display_success) {
         const DisplayConfigureRequest* request =
             GetRequestForDisplayId(display_id, requests_).base();
-        const_cast<DisplayConfigureRequest*>(request)->mode =
+        const DisplayMode* next_mode =
             FindNextMode(*request->display, request->mode);
-        should_reconfigure = !!request->mode;
+        if (next_mode) {
+          const_cast<DisplayConfigureRequest*>(request)->mode = next_mode;
+          should_reconfigure = true;
+        }
       }
     }
     // When reconfiguring, reconfigure all displays, not only the failing ones

ui/display/manager/configure_displays_task_unittest.cc

@@ -23,8 +23,6 @@ class ConfigureDisplaysTaskTest : public testing::Test {
  public:
   ConfigureDisplaysTaskTest()
       : delegate_(&log_),
-        callback_called_(false),
-        status_(ConfigureDisplaysTask::ERROR),
         small_mode_(gfx::Size(1366, 768), false, 60.0f),
         big_mode_(gfx::Size(2560, 1600), false, 60.0f) {
     displays_[0] = FakeDisplaySnapshot::Builder()
@@ -52,8 +50,8 @@ class ConfigureDisplaysTaskTest : public testing::Test {
   ActionLogger log_;
   TestNativeDisplayDelegate delegate_;
 
-  bool callback_called_;
-  ConfigureDisplaysTask::Status status_;
+  bool callback_called_ = false;
+  ConfigureDisplaysTask::Status status_ = ConfigureDisplaysTask::ERROR;
 
   const DisplayMode small_mode_;
   const DisplayMode big_mode_;
@@ -188,6 +186,9 @@ TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplayFails) {
                         GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
                                        &big_mode_})
                             .c_str(),
+                        GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
+                                       &small_mode_})
+                            .c_str(),
                         GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
                                        &small_mode_})
                             .c_str(),
@@ -218,6 +219,9 @@ TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplaysPartialSuccess) {
                         GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
                                        &big_mode_})
                             .c_str(),
+                        GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
+                                       &small_mode_})
+                            .c_str(),
                         GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
                                        &small_mode_})
                             .c_str(),
@@ -252,6 +256,9 @@ TEST_F(ConfigureDisplaysTaskTest, AsyncConfigureWithTwoDisplaysPartialSuccess) {
                         GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
                                        &big_mode_})
                             .c_str(),
+                        GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
+                                       &small_mode_})
+                            .c_str(),
                         GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
                                        &small_mode_})
                             .c_str(),

ui/display/manager/display_configurator_unittest.cc

@@ -1232,6 +1232,7 @@ TEST_F(DisplayConfiguratorTest,
   EXPECT_EQ(
       JoinActions(
           GetCrtcActions(&small_mode_, &big_mode_).c_str(),
+          GetCrtcActions(&small_mode_).c_str(),
           GetCrtcAction({outputs_[1]->display_id(),
                          gfx::Point(0, small_mode_.size().height() +
                                            DisplayConfigurator::kVerticalGap),
@@ -1555,7 +1556,7 @@ TEST_F(DisplayConfiguratorMultiMirroringTest,
   // Initialize with 3 external displays.
   outputs_[0] =
       FakeDisplaySnapshot::Builder()
-          .SetId(kDisplayIds[1])
+          .SetId(kDisplayIds[0])
           .SetType(DISPLAY_CONNECTION_TYPE_HDMI)
           .SetNativeMode(MakeDisplayMode(1920, 1200, true, 60.0))
           .AddMode(MakeDisplayMode(1920, 1200, true, 60.0))  // same AR
@@ -1564,7 +1565,7 @@ TEST_F(DisplayConfiguratorMultiMirroringTest,
           .Build();
   outputs_[1] =
       FakeDisplaySnapshot::Builder()
-          .SetId(kDisplayIds[2])
+          .SetId(kDisplayIds[1])
           .SetType(DISPLAY_CONNECTION_TYPE_HDMI)
           .SetNativeMode(MakeDisplayMode(1920, 1200, false, 60.0))
           .AddMode(MakeDisplayMode(1920, 1200, false, 60.0))  // same AR
@@ -1587,7 +1588,7 @@ TEST_F(DisplayConfiguratorMultiMirroringTest,
   // Find an exactly matching mirror mode while not preserving aspect.
   outputs_[2] =
       FakeDisplaySnapshot::Builder()
-          .SetId(kDisplayIds[0])
+          .SetId(kDisplayIds[2])
           .SetType(DISPLAY_CONNECTION_TYPE_HDMI)
           .SetNativeMode(MakeDisplayMode(1920, 1600, false, 60.0))
           .AddMode(MakeDisplayMode(1920, 1600, false, 60.0))  // same AR
@@ -1599,7 +1600,7 @@ TEST_F(DisplayConfiguratorMultiMirroringTest,
   // Cannot find a matching mirror mode, so enable software mirroring.
   outputs_[2] =
       FakeDisplaySnapshot::Builder()
-          .SetId(kDisplayIds[0])
+          .SetId(kDisplayIds[2])
           .SetType(DISPLAY_CONNECTION_TYPE_HDMI)
           .SetNativeMode(MakeDisplayMode(1920, 1600, false, 60.0))
           .AddMode(MakeDisplayMode(1920, 1600, false, 60.0))  // same AR

ui/display/manager/update_display_configuration_task_unittest.cc

@@ -347,6 +347,9 @@ TEST_F(UpdateDisplayConfigurationTaskTest, FailExtendedConfiguration) {
                                  gfx::Point(0, small_mode_.size().height()),
                                  &big_mode_})
                       .c_str(),
+                  GetCrtcAction(
+                      {displays_[0]->display_id(), gfx::Point(), &small_mode_})
+                      .c_str(),
                   GetCrtcAction({displays_[1]->display_id(),
                                  gfx::Point(0, small_mode_.size().height()),
                                  &small_mode_})