ui/display: Update display configuration API for unified modeset testing

This CL updates the display configurator API and tests to reflect
unified modeset testing.

Bug: b:176819129
Test: display_unittests && ozone_unittests
Change-Id: Ib87bd884457320b0fafd764d818bb3b2b56d7364
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2613152
Commit-Queue: Gil Dekel <gildekel@chromium.org>
Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841578}

chromecast/browser/cast_display_configurator.cc

@@ -234,17 +234,15 @@ void CastDisplayConfigurator::OnDisplayConfigured(
     display::DisplaySnapshot* display,
     const display::DisplayMode* mode,
     const gfx::Point& origin,
-    const base::flat_map<int64_t, bool>& statuses) {
+    bool config_success) {
   DCHECK(display);
   DCHECK(mode);
   DCHECK_EQ(display, display_);
-  DCHECK_EQ(statuses.size(), 1UL);
 
   const gfx::Rect bounds(origin, mode->size());
-  bool success = statuses.at(display_->display_id());
-  DVLOG(1) << __func__ << " success=" << success
+  DVLOG(1) << __func__ << " success=" << config_success
            << " bounds=" << bounds.ToString();
-  if (success) {
+  if (config_success) {
     // Need to update the display state otherwise it becomes stale.
     display_->set_current_mode(mode);
     display_->set_origin(origin);

chromecast/browser/cast_display_configurator.h

@@ -64,7 +64,7 @@ class CastDisplayConfigurator : public display::NativeDisplayObserver {
   void OnDisplayConfigured(display::DisplaySnapshot* display,
                            const display::DisplayMode* mode,
                            const gfx::Point& origin,
-                           const base::flat_map<int64_t, bool>& statuses);
+                           bool statuses);
   void UpdateScreen(int64_t display_id,
                     const gfx::Rect& bounds,
                     float device_scale_factor,

ui/display/fake/fake_display_delegate.cc

@@ -132,9 +132,9 @@ void FakeDisplayDelegate::GetDisplays(GetDisplaysCallback callback) {
 void FakeDisplayDelegate::Configure(
     const std::vector<display::DisplayConfigurationParams>& config_requests,
     ConfigureCallback callback) {
-  base::flat_map<int64_t, bool> statuses;
+  bool config_success = true;
   for (const auto& config : config_requests) {
-    bool configure_success = false;
+    bool request_success = false;
 
     if (config.mode.has_value()) {
       // Find display snapshot of display ID.
@@ -147,19 +147,20 @@ void FakeDisplayDelegate::Configure(
         // Check that config mode is appropriate for the display snapshot.
         for (const auto& existing_mode : snapshot->get()->modes()) {
           if (AreModesEqual(*existing_mode.get(), *config.mode.value().get())) {
-            configure_success = true;
+            request_success = true;
             break;
           }
         }
       }
     } else {
       // This is a request to turn off the display.
-      configure_success = true;
+      request_success = true;
     }
-    statuses.insert(std::make_pair(config.id, configure_success));
+    config_success &= request_success;
   }
 
-  configure_callbacks_.push(base::BindOnce(std::move(callback), statuses));
+  configure_callbacks_.push(
+      base::BindOnce(std::move(callback), config_success));
 
   // Start the timer if it's not already running. If there are multiple queued
   // configuration requests then ConfigureDone() will handle starting the

ui/display/manager/configure_displays_task.cc

@@ -113,15 +113,6 @@ int ComputeDisplayResolutionEnum(const DisplayMode* mode) {
   return width_idx * base::size(kDisplayResolutionSamples) + height_idx + 1;
 }
 
-std::__wrap_iter<const DisplayConfigureRequest*> GetRequestForDisplayId(
-    int64_t display_id,
-    const std::vector<DisplayConfigureRequest>& requests) {
-  return find_if(requests.begin(), requests.end(),
-                 [display_id](const DisplayConfigureRequest& request) {
-                   return request.display->display_id() == display_id;
-                 });
-}
-
 void UpdateResolutionAndRefreshRateUma(const DisplayConfigureRequest& request) {
   const bool internal =
       request.display->type() == DISPLAY_CONNECTION_TYPE_INTERNAL;
@@ -236,55 +227,37 @@ void ConfigureDisplaysTask::OnDisplaySnapshotsInvalidated() {
   std::move(callback_).Run(task_status_);
 }
 
-void ConfigureDisplaysTask::OnConfigured(
-    const base::flat_map<int64_t, bool>& statuses) {
-  bool config_success = true;
-  // Check if all displays are successfully configured.
-  for (const auto& status : statuses) {
-    int64_t display_id = status.first;
-    bool display_success = status.second;
-    config_success &= display_success;
-
-    auto request = GetRequestForDisplayId(display_id, requests_);
-    DCHECK(request != requests_.end());
-
-    VLOG(2) << "Configured status=" << display_success
-            << " display=" << request->display->display_id()
-            << " origin=" << request->origin.ToString()
-            << " mode=" << (request->mode ? request->mode->ToString() : "null");
+void ConfigureDisplaysTask::OnConfigured(bool config_success) {
+  bool should_reconfigure = false;
 
-    UpdateAttemptSucceededUma(request->display, display_success);
-  }
-
-  // Update displays upon success or prep |requests_| for reconfiguration.
-  if (config_success) {
-    for (auto& request : requests_) {
+  for (auto& request : requests_) {
+    // Update displays upon success or prep |requests_| for reconfiguration.
+    if (config_success) {
       request.display->set_current_mode(request.mode);
       request.display->set_origin(request.origin);
-    }
-  } else {
-    bool should_reconfigure = false;
-    // For the failing config, check if there is another mode to be requested.
-    // If there is one, attempt to reconfigure everything again.
-    for (const auto& status : statuses) {
-      int64_t display_id = status.first;
-      bool display_success = status.second;
-      if (!display_success) {
-        const DisplayConfigureRequest* request =
-            GetRequestForDisplayId(display_id, requests_).base();
-        const DisplayMode* next_mode =
-            FindNextMode(*request->display, request->mode);
-        if (next_mode) {
-          const_cast<DisplayConfigureRequest*>(request)->mode = next_mode;
-          should_reconfigure = true;
-        }
+    } 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;
       }
     }
-    if (should_reconfigure) {
-      task_status_ = PARTIAL_SUCCESS;
-      Run();
-      return;
-    }
+
+    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;
+    Run();
+    return;
   }
 
   // Update the final state.

ui/display/manager/configure_displays_task.h

@@ -66,7 +66,7 @@ class DISPLAY_MANAGER_EXPORT ConfigureDisplaysTask
   void OnDisplaySnapshotsInvalidated() override;
 
  private:
-  void OnConfigured(const base::flat_map<int64_t, bool>& statuses);
+  void OnConfigured(bool config_status);
 
   NativeDisplayDelegate* delegate_;  // Not owned.
 

ui/display/manager/display_configurator_unittest.cc

@@ -1033,7 +1033,7 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
   // This test should attempt to configure a mirror mode that will not succeed
   // and should end up in extended mode.
   native_display_delegate_->set_max_configurable_pixels(
-      modes[3]->size().GetArea());
+      modes[1]->size().GetArea());
   state_controller_.set_state(MULTIPLE_DISPLAY_STATE_MULTI_MIRROR);
   UpdateOutputs(2, true);
 
@@ -1046,17 +1046,32 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
           GetCrtcAction(
               {outputs_[1]->display_id(), gfx::Point(0, 0), modes[0].get()})
               .c_str(),
-          // First mode tried is expected to fail and it will
-          // retry with the 4th mode in the list (for non-internal displays).
+          // First try is expected to fail and it will retry with the next
+          // largest mode in the list (for non-internal displays).since the
+          // internal display will always fail, the display configurator will
+          // attempt all of the external display's available modes before it
+          // gives up.
           GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
                          outputs_[0]->native_mode()})
               .c_str(),
           GetCrtcAction(
               {outputs_[1]->display_id(), gfx::Point(0, 0), modes[3].get()})
               .c_str(),
+          GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
+                         outputs_[0]->native_mode()})
+              .c_str(),
+          GetCrtcAction(
+              {outputs_[1]->display_id(), gfx::Point(0, 0), modes[2].get()})
+              .c_str(),
+          GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
+                         outputs_[0]->native_mode()})
+              .c_str(),
+          GetCrtcAction(
+              {outputs_[1]->display_id(), gfx::Point(0, 0), modes[1].get()})
+              .c_str(),
           // Since it was requested to go into mirror mode and the configured
           // modes were different, it should now try and setup a valid
-          // configurable extended mode.
+          // configurable extended mode in the same order described above.
           GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
                          outputs_[0]->native_mode()})
               .c_str(),
@@ -1073,6 +1088,22 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
                                            DisplayConfigurator::kVerticalGap),
                          modes[3].get()})
               .c_str(),
+          GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
+                         outputs_[0]->native_mode()})
+              .c_str(),
+          GetCrtcAction({outputs_[1]->display_id(),
+                         gfx::Point(0, modes[0]->size().height() +
+                                           DisplayConfigurator::kVerticalGap),
+                         modes[2].get()})
+              .c_str(),
+          GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
+                         outputs_[0]->native_mode()})
+              .c_str(),
+          GetCrtcAction({outputs_[1]->display_id(),
+                         gfx::Point(0, modes[0]->size().height() +
+                                           DisplayConfigurator::kVerticalGap),
+                         modes[1].get()})
+              .c_str(),
           nullptr),
       log_->GetActionsAndClear());
 }

ui/display/manager/test/test_native_display_delegate.cc

@@ -73,15 +73,15 @@ bool TestNativeDisplayDelegate::Configure(
 void TestNativeDisplayDelegate::Configure(
     const std::vector<display::DisplayConfigurationParams>& config_requests,
     ConfigureCallback callback) {
-  base::flat_map<int64_t, bool> statuses;
+  bool config_success = true;
   for (const auto& config : config_requests)
-    statuses.insert(std::make_pair(config.id, Configure(config)));
+    config_success &= Configure(config);
 
   if (run_async_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback), statuses));
+        FROM_HERE, base::BindOnce(std::move(callback), config_success));
   } else {
-    std::move(callback).Run(statuses);
+    std::move(callback).Run(config_success);
   }
 }
 

ui/display/types/native_display_delegate.h

@@ -25,8 +25,7 @@ struct DisplayConfigurationParams;
 
 using GetDisplaysCallback =
     base::OnceCallback<void(const std::vector<DisplaySnapshot*>&)>;
-using ConfigureCallback =
-    base::OnceCallback<void(const base::flat_map<int64_t, bool>&)>;
+using ConfigureCallback = base::OnceCallback<void(bool)>;
 using GetHDCPStateCallback =
     base::OnceCallback<void(bool, HDCPState, ContentProtectionMethod)>;
 using SetHDCPStateCallback = base::OnceCallback<void(bool)>;

ui/ozone/demo/window_manager.cc

@@ -108,13 +108,10 @@ void WindowManager::OnDisplaysAcquired(
   }
 }
 
-void WindowManager::OnDisplayConfigured(
-    const int64_t display_id,
-    const gfx::Rect& bounds,
-    const base::flat_map<int64_t, bool>& statuses) {
-  DCHECK_EQ(statuses.size(), 1UL);
-
-  if (statuses.at(display_id)) {
+void WindowManager::OnDisplayConfigured(const int64_t display_id,
+                                        const gfx::Rect& bounds,
+                                        bool config_success) {
+  if (config_success) {
     std::unique_ptr<DemoWindow> window(
         new DemoWindow(this, renderer_factory_.get(), bounds));
     window->Start();

ui/ozone/demo/window_manager.h

@@ -41,7 +41,7 @@ class WindowManager : public display::NativeDisplayObserver {
       const std::vector<display::DisplaySnapshot*>& displays);
   void OnDisplayConfigured(const int64_t display_id,
                            const gfx::Rect& bounds,
-                           const base::flat_map<int64_t, bool>& statuses);
+                           bool config_success);
 
   // display::NativeDisplayDelegate:
   void OnConfigurationChanged() override;

ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc

@@ -166,9 +166,9 @@ void DrmGpuDisplayManager::RelinquishDisplayControl() {
     drm->DropMaster();
 }
 
-base::flat_map<int64_t, bool> DrmGpuDisplayManager::ConfigureDisplays(
+bool DrmGpuDisplayManager::ConfigureDisplays(
     const std::vector<display::DisplayConfigurationParams>& config_requests) {
-  base::flat_map<int64_t, bool> statuses;
+  bool config_success = true;
   ScreenManager::ControllerConfigsList controllers_to_configure;
 
   for (const auto& config : config_requests) {
@@ -176,7 +176,7 @@ base::flat_map<int64_t, bool> DrmGpuDisplayManager::ConfigureDisplays(
     DrmDisplay* display = FindDisplay(display_id);
     if (!display) {
       LOG(ERROR) << "There is no display with ID " << display_id;
-      statuses.insert(std::make_pair(display_id, false));
+      config_success = false;
       continue;
     }
 
@@ -185,7 +185,7 @@ base::flat_map<int64_t, bool> DrmGpuDisplayManager::ConfigureDisplays(
     if (config.mode) {
       if (!FindModeForDisplay(mode_ptr.get(), *config.mode.value(),
                               display->modes(), displays_)) {
-        statuses.insert(std::make_pair(display_id, false));
+        config_success = false;
         continue;
       }
     }
@@ -206,22 +206,21 @@ base::flat_map<int64_t, bool> DrmGpuDisplayManager::ConfigureDisplays(
   }
 
   if (controllers_to_configure.empty())
-    return statuses;
+    return config_success;
 
   if (clear_overlay_cache_callback_)
     clear_overlay_cache_callback_.Run();
 
-  auto config_statuses =
+  config_success &=
       screen_manager_->ConfigureDisplayControllers(controllers_to_configure);
-  for (const auto& status : config_statuses) {
-    int64_t display_id = status.first;
-    bool success = status.second;
+  for (const auto& controller : controllers_to_configure) {
+    int64_t display_id = controller.display_id;
     DrmDisplay* display = FindDisplay(display_id);
     auto config = std::find_if(
         config_requests.begin(), config_requests.end(),
         [display_id](const auto& request) { return request.id == display_id; });
 
-    if (success) {
+    if (config_success) {
       display->SetOrigin(config->origin);
     } else {
       if (config->mode) {
@@ -235,11 +234,9 @@ base::flat_map<int64_t, bool> DrmGpuDisplayManager::ConfigureDisplays(
                 << " crtc=" << display->crtc();
       }
     }
-
-    statuses.insert(std::make_pair(display_id, success));
   }
 
-  return statuses;
+  return config_success;
 }
 
 bool DrmGpuDisplayManager::GetHDCPState(

ui/ozone/platform/drm/gpu/drm_gpu_display_manager.h

@@ -52,7 +52,7 @@ class DrmGpuDisplayManager {
   bool TakeDisplayControl();
   void RelinquishDisplayControl();
 
-  base::flat_map<int64_t, bool> ConfigureDisplays(
+  bool ConfigureDisplays(
       const std::vector<display::DisplayConfigurationParams>& config_requests);
   bool GetHDCPState(int64_t display_id,
                     display::HDCPState* state,

ui/ozone/platform/drm/gpu/drm_thread.cc

@@ -328,13 +328,11 @@ void DrmThread::RefreshNativeDisplays(
 
 void DrmThread::ConfigureNativeDisplays(
     const std::vector<display::DisplayConfigurationParams>& config_requests,
-    base::OnceCallback<void(const base::flat_map<int64_t, bool>&)> callback) {
+    base::OnceCallback<void(bool)> callback) {
   TRACE_EVENT0("drm", "DrmThread::ConfigureNativeDisplays");
 
-  base::flat_map<int64_t, bool> statuses =
-      display_manager_->ConfigureDisplays(config_requests);
-
-  std::move(callback).Run(statuses);
+  bool config_success = display_manager_->ConfigureDisplays(config_requests);
+  std::move(callback).Run(config_success);
 }
 
 void DrmThread::TakeDisplayControl(base::OnceCallback<void(bool)> callback) {

ui/ozone/platform/drm/gpu/screen_manager.cc

@@ -101,12 +101,6 @@ std::vector<uint64_t> GetModifiersForPrimaryFormat(
   return controller->GetFormatModifiersForModesetting(fourcc_format);
 }
 
-bool AreAllStatusesTrue(base::flat_map<int64_t, bool>& display_statuses) {
-  auto it = find_if(display_statuses.begin(), display_statuses.end(),
-                    [](const auto status) { return status.second == false; });
-  return (it == display_statuses.end());
-}
-
 }  // namespace
 
 ScreenManager::ScreenManager() = default;
@@ -227,7 +221,7 @@ void ScreenManager::RemoveDisplayControllers(
     UpdateControllerToWindowMapping();
 }
 
-base::flat_map<int64_t, bool> ScreenManager::ConfigureDisplayControllers(
+bool ScreenManager::ConfigureDisplayControllers(
     const ControllerConfigsList& controllers_params) {
   TRACE_EVENT0("drm", "ScreenManager::ConfigureDisplayControllers");
 
@@ -244,29 +238,17 @@ base::flat_map<int64_t, bool> ScreenManager::ConfigureDisplayControllers(
     displays_for_drm_devices[params.drm].emplace_back(params);
   }
 
-  base::flat_map<int64_t, bool> statuses;
+  bool config_success = true;
   // Perform display configurations together for the same DRM only.
   for (const auto& configs_on_drm : displays_for_drm_devices) {
-    auto display_statuses = TestAndModeset(configs_on_drm.second);
-    statuses.insert(display_statuses.begin(), display_statuses.end());
+    config_success &=
+        TestModeset(configs_on_drm.second) && Modeset(configs_on_drm.second);
   }
 
-  if (AreAllStatusesTrue(statuses))
+  if (config_success)
     UpdateControllerToWindowMapping();
 
-  return statuses;
-}
-
-base::flat_map<int64_t, bool> ScreenManager::TestAndModeset(
-    const ControllerConfigsList& controllers_params) {
-  if (!TestModeset(controllers_params)) {
-    base::flat_map<int64_t, bool> statuses;
-    for (const auto& params : controllers_params)
-      statuses.insert(std::make_pair(params.display_id, false));
-    return statuses;
-  }
-
-  return Modeset(controllers_params);
+  return config_success;
 }
 
 bool ScreenManager::TestModeset(
@@ -301,11 +283,10 @@ bool ScreenManager::TestModeset(
       DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET);
 }
 
-base::flat_map<int64_t, bool> ScreenManager::Modeset(
-    const ControllerConfigsList& controllers_params) {
+bool ScreenManager::Modeset(const ControllerConfigsList& controllers_params) {
   TRACE_EVENT0("drm", "ScreenManager::Modeset");
 
-  base::flat_map<int64_t, bool> statuses;
+  bool modeset_success = true;
 
   for (const auto& params : controllers_params) {
     // Commit one controller at a time.
@@ -339,10 +320,10 @@ base::flat_map<int64_t, bool> ScreenManager::Modeset(
       UpdateControllerStateAfterModeset(params, request_for_update, status);
     }
 
-    statuses.insert(std::make_pair(params.display_id, status));
+    modeset_success &= status;
   }
 
-  return statuses;
+  return modeset_success;
 }
 
 void ScreenManager::SetDisplayControllerForEnableAndGetProps(

ui/ozone/platform/drm/gpu/screen_manager.h

@@ -68,7 +68,7 @@ class ScreenManager {
   void RemoveDisplayControllers(const CrtcsWithDrmList& controllers_to_remove);
 
   // Enables/Disables the display controller based on if a mode exists.
-  base::flat_map<int64_t, bool> ConfigureDisplayControllers(
+  bool ConfigureDisplayControllers(
       const ControllerConfigsList& controllers_params);
 
   // Returns a reference to the display controller configured to display within
@@ -105,13 +105,9 @@ class ScreenManager {
       const scoped_refptr<DrmDevice>& drm,
       uint32_t crtc);
 
-  base::flat_map<int64_t, bool> TestAndModeset(
-      const ControllerConfigsList& controllers_params);
-
   bool TestModeset(const ControllerConfigsList& controllers_params);
 
-  base::flat_map<int64_t, bool> Modeset(
-      const ControllerConfigsList& controllers_params);
+  bool Modeset(const ControllerConfigsList& controllers_params);
 
   // Configures a display controller to be enabled. The display controller is
   // identified by (|crtc|, |connector|) and the controller is to be modeset

ui/ozone/platform/drm/host/drm_display_host_manager.cc

@@ -237,15 +237,11 @@ void DrmDisplayHostManager::UpdateDisplays(
 void DrmDisplayHostManager::ConfigureDisplays(
     const std::vector<display::DisplayConfigurationParams>& config_requests,
     display::ConfigureCallback callback) {
-  base::flat_map<int64_t, bool> dummy_statuses;
-  bool is_any_dummy = false;
   for (auto& config : config_requests) {
-    is_any_dummy |= GetDisplay(config.id)->is_dummy();
-    dummy_statuses.insert(std::make_pair(config.id, true));
-  }
-  if (is_any_dummy) {
-    std::move(callback).Run(dummy_statuses);
-    return;
+    if (GetDisplay(config.id)->is_dummy()) {
+      std::move(callback).Run(true);
+      return;
+    }
   }
 
   proxy_->GpuConfigureNativeDisplays(config_requests, std::move(callback));

ui/ozone/platform/drm/host/host_drm_device.cc

@@ -136,16 +136,11 @@ void HostDrmDevice::GpuConfigureNativeDisplays(
   if (IsConnected()) {
     drm_device_->ConfigureNativeDisplays(config_requests, std::move(callback));
   } else {
-    // If not connected, report failure to config.
-    base::flat_map<int64_t, bool> dummy_statuses;
-    for (const auto& config : config_requests)
-      dummy_statuses.insert(std::make_pair(config.id, false));
-
     // Post this task to protect the callstack from accumulating too many
     // recursive calls to ConfigureDisplaysTask::Run() in cases in which the GPU
     // process crashes repeatedly.
     base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback), dummy_statuses));
+        FROM_HERE, base::BindOnce(std::move(callback), false));
   }
 }
 

ui/ozone/public/mojom/drm_device.mojom

@@ -51,11 +51,11 @@ interface DrmDevice {
   // Instructs the GPU to abandon a DRM device.
   RemoveGraphicsDevice(mojo_base.mojom.FilePath path);
 
-  // Configures (Enables/Disables) DRM displays, returns a map: each configured
-  // display ID to its status, true on success.
+  // Configures (Enables/Disables) DRM displays, returns whether or not the
+  // configuration was successful.
   ConfigureNativeDisplays(
       array<display.mojom.DisplayConfigurationParams> config_requests) =>
-    (map<int64, bool> statuses);
+    (bool config_success);
 
   // Gets high-definition content protection (HDCP) (DRM as in
   // digital rights management) state.