ui/display: Submit modeset requests individually during fallback

This CL changes the fallback strategy in ConfigureDisplaysTask to submit
requests individually when an atomic modeset test fails. More
particularly, it groups displays by the physical connectors they are
attached to and submit these request groups for atomic modesetting.

For example, if the request {internal display, MST1-1, MST1-2, HDMI}
fails as a whole, the next configuration attempt would repeatedly try
each of the following groups until a modeset succeeds: {internal
display}, {MST1-1, MST1-2} and {HDMI}. When a request group fails
modeset, the request with the highest bandwidth requirement is
downgraded first.

Finally, if any of the request groups exhausts all its alternative
modes, the entire configuration attempt terminates and signals back an
ERROR status.

In addition, this CL adds extensive testing for multiple display
configurations, including multiple MST hubs, nested MST structures,
MST and non-MST (i.e. independent) external displays, bad MST hubs, and
configurations with no internal displays, such as a Chromebox.

Bug: b:176819129, b:177356832
Test: display_unittests && ozone_unittests
Change-Id: Iac2b7f0330a74396c72f8d3867552dde593800e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2621674
Commit-Queue: Gil Dekel <gildekel@chromium.org>
Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Reviewed-by: Gil Dekel <gildekel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846418}

ui/display/manager/configure_displays_task.cc

@@ -8,6 +8,7 @@
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
+#include "base/containers/flat_set.h"
 #include "base/containers/queue.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
@@ -17,6 +18,7 @@
 #include "ui/display/manager/display_util.h"
 #include "ui/display/types/display_configuration_params.h"
 #include "ui/display/types/display_constants.h"
+#include "ui/display/types/display_mode.h"
 #include "ui/display/types/display_snapshot.h"
 #include "ui/display/types/native_display_delegate.h"
 
@@ -132,19 +134,31 @@ void UpdateResolutionAndRefreshRateUma(const DisplayConfigureRequest& request) {
   histogram->Add(request.mode ? std::round(request.mode->refresh_rate()) : 0);
 }
 
-void UpdateAttemptSucceededUma(DisplaySnapshot* display, bool display_success) {
-  const bool internal = display->type() == DISPLAY_CONNECTION_TYPE_INTERNAL;
-  base::UmaHistogramBoolean(
-      internal ? "ConfigureDisplays.Internal.Modeset.AttemptSucceeded"
-               : "ConfigureDisplays.External.Modeset.AttemptSucceeded",
-      display_success);
+void UpdateAttemptSucceededUma(
+    const std::vector<DisplayConfigureRequest>& requests,
+    bool display_success) {
+  for (const auto& request : requests) {
+    const bool internal =
+        request.display->type() == DISPLAY_CONNECTION_TYPE_INTERNAL;
+    base::UmaHistogramBoolean(
+        internal ? "ConfigureDisplays.Internal.Modeset.AttemptSucceeded"
+                 : "ConfigureDisplays.External.Modeset.AttemptSucceeded",
+        display_success);
+
+    VLOG(2) << "Configured status=" << display_success
+            << " display=" << request.display->display_id()
+            << " origin=" << request.origin.ToString()
+            << " mode=" << (request.mode ? request.mode->ToString() : "null");
+  }
 }
 
-void UpdateFinalStatusUma(const std::vector<DisplayConfigureRequest>& requests,
-                          bool config_success) {
+void UpdateFinalStatusUma(
+    const std::vector<RequestAndStatusList>& requests_and_statuses) {
   int mst_external_displays = 0;
-  size_t total_external_displays = requests.size();
-  for (auto& request : requests) {
+  size_t total_external_displays = requests_and_statuses.size();
+  for (auto& request_and_status : requests_and_statuses) {
+    const DisplayConfigureRequest& request = request_and_status.first;
+
     // Is this display SST (single-stream vs. MST multi-stream).
     bool sst_display = request.display->base_connector_id() &&
                        request.display->path_topology().empty();
@@ -158,7 +172,7 @@ void UpdateFinalStatusUma(const std::vector<DisplayConfigureRequest>& requests,
     base::UmaHistogramBoolean(
         internal ? "ConfigureDisplays.Internal.Modeset.FinalStatus"
                  : "ConfigureDisplays.External.Modeset.FinalStatus",
-        config_success);
+        request_and_status.second);
   }
 
   base::UmaHistogramExactLinear(
@@ -213,9 +227,14 @@ void ConfigureDisplaysTask::Run() {
     UpdateResolutionAndRefreshRateUma(request);
   }
 
-  delegate_->Configure(config_requests,
-                       base::BindOnce(&ConfigureDisplaysTask::OnConfigured,
-                                      weak_ptr_factory_.GetWeakPtr()));
+  const auto& on_configured =
+      pending_display_group_requests_.empty()
+          ? &ConfigureDisplaysTask::OnFirstAttemptConfigured
+          : &ConfigureDisplaysTask::OnRetryConfigured;
+
+  delegate_->Configure(
+      config_requests,
+      base::BindOnce(on_configured, weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ConfigureDisplaysTask::OnConfigurationChanged() {}
@@ -227,45 +246,128 @@ void ConfigureDisplaysTask::OnDisplaySnapshotsInvalidated() {
   std::move(callback_).Run(task_status_);
 }
 
-void ConfigureDisplaysTask::OnConfigured(bool config_success) {
-  bool should_reconfigure = false;
+void ConfigureDisplaysTask::OnFirstAttemptConfigured(bool config_success) {
+  UpdateAttemptSucceededUma(requests_, config_success);
 
-  for (auto& request : requests_) {
-    // Update displays upon success or prep |requests_| for reconfiguration.
+  if (!config_success) {
+    // Partition |requests_| into smaller groups, update the task's state, and
+    // initiate the retry logic. The next time |delegate_|->Configure()
+    // terminates OnRetryConfigured() will be executed instead.
+    PartitionRequests();
+    DCHECK(!pending_display_group_requests_.empty());
+    requests_ = pending_display_group_requests_.front();
+    task_status_ = PARTIAL_SUCCESS;
+    Run();
+    return;
+  }
+
+  // This code execute only when the first modeset attempt fully succeeds.
+  // Update the displays' status and report success.
+  for (const auto& request : requests_) {
+    request.display->set_current_mode(request.mode);
+    request.display->set_origin(request.origin);
+    final_requests_status_.emplace_back(std::make_pair(request, true));
+  }
+
+  UpdateFinalStatusUma(final_requests_status_);
+  std::move(callback_).Run(task_status_);
+}
+
+void ConfigureDisplaysTask::OnRetryConfigured(bool config_success) {
+  UpdateAttemptSucceededUma(requests_, config_success);
+
+  if (!config_success) {
+    // If one of the largest display request can be downgraded, try again.
+    // Otherwise this configuration task is a failure.
+    if (DowngradeLargestRequestWithAlternativeModes()) {
+      Run();
+      return;
+    } else {
+      task_status_ = ERROR;
+    }
+  }
+
+  // This code executes only when this display group request fully succeeds or
+  // fails to modeset. Update the final status of this group.
+  for (const auto& request : requests_) {
+    final_requests_status_.emplace_back(
+        std::make_pair(request, config_success));
     if (config_success) {
       request.display->set_current_mode(request.mode);
       request.display->set_origin(request.origin);
-    } else {
-      // For the failing config, check if there is another mode to be requested.
-      // If there is one, attempt to reconfigure everything again.
-      const DisplayMode* next_mode =
-          FindNextMode(*request.display, request.mode);
-      if (next_mode) {
-        request.mode = next_mode;
-        should_reconfigure = true;
-      }
     }
-
-    VLOG(2) << "Configured status=" << config_success
-            << " display=" << request.display->display_id()
-            << " origin=" << request.origin.ToString()
-            << " mode=" << (request.mode ? request.mode->ToString() : "null");
-
-    UpdateAttemptSucceededUma(request.display, config_success);
   }
 
-  if (should_reconfigure) {
-    task_status_ = PARTIAL_SUCCESS;
+  // Subsequent modeset attempts will be done on the next pending display group,
+  // if one exists.
+  pending_display_group_requests_.pop();
+  requests_.clear();
+  if (!pending_display_group_requests_.empty()) {
+    requests_ = pending_display_group_requests_.front();
     Run();
     return;
   }
 
-  // Update the final state.
-  UpdateFinalStatusUma(requests_, config_success);
-
-  if (!config_success)
-    task_status_ = ERROR;
+  // No more display groups to retry.
+  UpdateFinalStatusUma(final_requests_status_);
   std::move(callback_).Run(task_status_);
 }
 
+void ConfigureDisplaysTask::PartitionRequests() {
+  pending_display_group_requests_ = PartitionedRequestsQueue();
+  base::flat_set<uint64_t> handled_connectors;
+
+  for (size_t i = 0; i < requests_.size(); ++i) {
+    uint64_t connector_id = requests_[i].display->base_connector_id();
+    if (handled_connectors.find(connector_id) != handled_connectors.end())
+      continue;
+
+    std::vector<DisplayConfigureRequest> request_group;
+    for (size_t j = i; j < requests_.size(); ++j) {
+      if (connector_id == requests_[j].display->base_connector_id())
+        request_group.push_back(requests_[j]);
+    }
+
+    pending_display_group_requests_.push(request_group);
+    handled_connectors.insert(connector_id);
+  }
+}
+
+bool ConfigureDisplaysTask::DowngradeLargestRequestWithAlternativeModes() {
+  auto cmp = [](DisplayConfigureRequest* lhs, DisplayConfigureRequest* rhs) {
+    return *lhs->mode < *rhs->mode;
+  };
+  std::priority_queue<DisplayConfigureRequest*,
+                      std::vector<DisplayConfigureRequest*>, decltype(cmp)>
+      sorted_requests(cmp);
+
+  for (auto& request : requests_) {
+    if (request.display->type() == DISPLAY_CONNECTION_TYPE_INTERNAL)
+      continue;
+
+    if (!request.mode)
+      continue;
+
+    sorted_requests.push(&request);
+  }
+
+  // Fail if there are no viable candidates to downgrade
+  if (sorted_requests.empty())
+    return false;
+
+  while (!sorted_requests.empty()) {
+    DisplayConfigureRequest* next_request = sorted_requests.top();
+    sorted_requests.pop();
+
+    const DisplayMode* next_mode =
+        FindNextMode(*next_request->display, next_request->mode);
+    if (next_mode) {
+      next_request->mode = next_mode;
+      return true;
+    }
+  }
+
+  return false;
+}
+
 }  // namespace display

ui/display/manager/configure_displays_task.h

@@ -11,7 +11,6 @@
 #include <vector>
 
 #include "base/callback.h"
-#include "base/containers/flat_map.h"
 #include "base/containers/queue.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -35,7 +34,25 @@ struct DISPLAY_MANAGER_EXPORT DisplayConfigureRequest {
   gfx::Point origin;
 };
 
-// Applies the display configuration asynchronously.
+using RequestAndStatusList = std::pair<DisplayConfigureRequest, bool>;
+
+// ConfigureDisplaysTask is in charge of applying the display configuration as
+// requested by Ash. If the original request fails, the task will attempt to
+// modify the request by downgrading the resolution of one or more of the
+// displays and try again until it either succeeds a modeset or exhaust all
+// available options.
+//
+// Displays are bandwidth constrained in 2 ways: (1) system memory bandwidth
+// (ie: scanning pixels from memory), and (2) link bandwidth (ie: scanning
+// pixels from the SoC to the display). Naturally all displays share (1),
+// however with DisplayPort Multi-stream Transport (DP MST), displays may also
+// share (2). The introduction of MST support drastically increases the
+// likelihood of modeset failures due to (2) since multiple displays will all be
+// sharing the same physical connection.
+//
+// If we're constrained by (1), reducing the resolution of any display will
+// relieve pressure. However if we're constrained by (2), only those displays on
+// the saturated link can relieve pressure.
 class DISPLAY_MANAGER_EXPORT ConfigureDisplaysTask
     : public NativeDisplayObserver {
  public:
@@ -52,6 +69,8 @@ class DISPLAY_MANAGER_EXPORT ConfigureDisplaysTask
   };
 
   using ResponseCallback = base::OnceCallback<void(Status)>;
+  using PartitionedRequestsQueue =
+      std::queue<std::vector<DisplayConfigureRequest>>;
 
   ConfigureDisplaysTask(NativeDisplayDelegate* delegate,
                         const std::vector<DisplayConfigureRequest>& requests,
@@ -66,12 +85,49 @@ class DISPLAY_MANAGER_EXPORT ConfigureDisplaysTask
   void OnDisplaySnapshotsInvalidated() override;
 
  private:
-  void OnConfigured(bool config_status);
+  // Deals with the aftermath of the initial configuration, which attempts to
+  // configure all displays together.
+  // Upon failure, partitions the original request from Ash into smaller
+  // requests where the displays are grouped by the physical connector they
+  // connect to and initiates the retry sequence.
+  void OnFirstAttemptConfigured(bool config_status);
+
+  // Deals with the aftermath of a configuration retry, which attempts to
+  // configure a subset of the displays grouped together by the physical
+  // connector they connect to.
+  // Upon success, initiates the retry sequence on the next group of displays.
+  // Otherwise, downgrades the display with the largest bandwidth requirement
+  // and tries again.
+  // If any of the display groups entirely fail to modeset (i.e. exhaust all
+  // available modes during retry), the configuration will fail as a whole, but
+  // will continue to try to modeset the remaining display groups.
+  void OnRetryConfigured(bool config_status);
+
+  // Partition |requests_| by their base connector id (i.e. the physical
+  // connector the displays are connected to) and populate the result in
+  // |pending_display_group_requests_|. We assume the order of requests
+  // submitted by Ash is important, so the partitioning is done in order.
+  void PartitionRequests();
+
+  // Downgrade the request with the highest bandwidth requirement AND
+  // alternative modes in |requests_| (excluding internal displays and disable
+  // requests). Return false if no request was downgraded.
+  bool DowngradeLargestRequestWithAlternativeModes();
 
   NativeDisplayDelegate* delegate_;  // Not owned.
 
+  // Initially, |requests_| holds the configuration request submitted by Ash.
+  // During retry, |requests_| will represent a group of displays that are
+  // currently attempting configuration.
   std::vector<DisplayConfigureRequest> requests_;
 
+  // A queue of display requests grouped by their
+  // |requests_[index]->display->base_connector_id()|.
+  PartitionedRequestsQueue pending_display_group_requests_;
+
+  // The final requests and their configuration status for UMA.
+  std::vector<RequestAndStatusList> final_requests_status_;
+
   // When the task finishes executing it runs the callback to notify that the
   // task is done and the task status.
   ResponseCallback callback_;

ui/display/manager/update_display_configuration_task_unittest.cc

@@ -16,12 +16,17 @@
 #include "ui/display/manager/display_layout_manager.h"
 #include "ui/display/manager/test/action_logger_util.h"
 #include "ui/display/manager/test/test_native_display_delegate.h"
+#include "ui/display/types/display_constants.h"
 
 namespace display {
 namespace test {
 
 namespace {
 
+// Non-zero generic connector IDs.
+constexpr uint64_t kEdpConnectorId = 71u;
+constexpr uint64_t kSecondConnectorId = kEdpConnectorId + 10u;
+
 class TestSoftwareMirroringController
     : public DisplayConfigurator::SoftwareMirroringController {
  public:
@@ -153,13 +158,17 @@ class UpdateDisplayConfigurationTaskTest : public testing::Test {
                        .SetId(123)
                        .SetNativeMode(small_mode_.Clone())
                        .SetCurrentMode(small_mode_.Clone())
+                       .SetType(DISPLAY_CONNECTION_TYPE_INTERNAL)
+                       .SetBaseConnectorId(kEdpConnectorId)
                        .Build();
 
     displays_[1] = FakeDisplaySnapshot::Builder()
                        .SetId(456)
                        .SetNativeMode(big_mode_.Clone())
                        .SetCurrentMode(big_mode_.Clone())
+                       .SetType(DISPLAY_CONNECTION_TYPE_DISPLAYPORT)
                        .AddMode(small_mode_.Clone())
+                       .SetBaseConnectorId(kSecondConnectorId)
                        .Build();
   }
   ~UpdateDisplayConfigurationTaskTest() override = default;
@@ -340,21 +349,33 @@ TEST_F(UpdateDisplayConfigurationTaskTest, FailExtendedConfiguration) {
   EXPECT_TRUE(configured_);
   EXPECT_FALSE(configuration_status_);
   EXPECT_EQ(
-      JoinActions(GetCrtcAction(
-                      {displays_[0]->display_id(), gfx::Point(), &small_mode_})
-                      .c_str(),
-                  GetCrtcAction({displays_[1]->display_id(),
-                                 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_})
-                      .c_str(),
-                  nullptr),
+      JoinActions(
+          // All displays will fail to modeset together. Initiate retry logic.
+          GetCrtcAction(
+              {displays_[0]->display_id(), gfx::Point(), &small_mode_})
+              .c_str(),
+          GetCrtcAction({displays_[1]->display_id(),
+                         gfx::Point(0, small_mode_.size().height()),
+                         &big_mode_})
+              .c_str(),
+          // Retry logic fails to modeset internal display. Since internal
+          // displays are restricted to their preferred mode, there are no other
+          // modes to try. The configuration will fail, but the external display
+          // will still try to modeset.
+          GetCrtcAction(
+              {displays_[0]->display_id(), gfx::Point(), &small_mode_})
+              .c_str(),
+          // External display fail modeset, downgrade once, and then fail
+          // completely.
+          GetCrtcAction({displays_[1]->display_id(),
+                         gfx::Point(0, small_mode_.size().height()),
+                         &big_mode_})
+              .c_str(),
+          GetCrtcAction({displays_[1]->display_id(),
+                         gfx::Point(0, small_mode_.size().height()),
+                         &small_mode_})
+              .c_str(),
+          nullptr),
       log_.GetActionsAndClear());
 }
 

ui/display/types/display_mode.cc

@@ -21,6 +21,18 @@ std::unique_ptr<DisplayMode> DisplayMode::Clone() const {
       new DisplayMode(size_, is_interlaced_, refresh_rate_));
 }
 
+bool DisplayMode::operator<(const DisplayMode& other) const {
+  if (size_.GetArea() < other.size_.GetArea())
+    return true;
+  if (size_.GetArea() > other.size_.GetArea())
+    return false;
+  if (size_.width() < other.size_.width())
+    return true;
+  if (size_.width() > other.size_.width())
+    return false;
+  return refresh_rate_ < other.refresh_rate_;
+}
+
 std::string DisplayMode::ToString() const {
   return base::StringPrintf("[%s %srate=%f]", size_.ToString().c_str(),
                             is_interlaced_ ? "interlaced " : "", refresh_rate_);

ui/display/types/display_mode.h

@@ -27,6 +27,8 @@ class DISPLAY_TYPES_EXPORT DisplayMode {
   bool is_interlaced() const { return is_interlaced_; }
   float refresh_rate() const { return refresh_rate_; }
 
+  bool operator<(const DisplayMode& other) const;
+
   std::string ToString() const;
 
  private: