Perform Modeset of all displays in 1 commit.

Perform 1 drm commit consisting of all displays being modeset/disabled
together.
This guarantees that the kernel is aware of the states of all displays
at the same time, especially in case of inter-dependency.

ScreenManagerTest.{CheckForSecondValidController,CheckMultipleControllersAfterBeingDisabled}

BUG: b/172069469, b/168753412
TEST: modesetting kohaku with external display,
Change-Id: I4bb5b253eda793cc5807ab0000cc5e14854dc3d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2551490Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Commit-Queue: Mark Yacoub <markyacoub@google.com>
Cr-Commit-Position: refs/heads/master@{#843704}

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

@@ -9,6 +9,7 @@ namespace ui {
 CrtcCommitRequest::CrtcCommitRequest(uint32_t crtc_id,
                                      uint32_t connector_id,
                                      drmModeModeInfo mode,
+                                     gfx::Point origin,
                                      HardwareDisplayPlaneList* plane_list,
                                      DrmOverlayPlaneList overlays,
                                      bool should_enable)
@@ -16,6 +17,7 @@ CrtcCommitRequest::CrtcCommitRequest(uint32_t crtc_id,
       crtc_id_(crtc_id),
       connector_id_(connector_id),
       mode_(mode),
+      origin_(origin),
       plane_list_(plane_list),
       overlays_(std::move(overlays)) {
   DCHECK(!should_enable || DrmOverlayPlane::GetPrimaryPlane(overlays_));
@@ -28,6 +30,7 @@ CrtcCommitRequest::CrtcCommitRequest(const CrtcCommitRequest& other)
       crtc_id_(other.crtc_id_),
       connector_id_(other.connector_id_),
       mode_(other.mode_),
+      origin_(other.origin_),
       plane_list_(other.plane_list_),
       overlays_(DrmOverlayPlane::Clone(other.overlays_)) {}
 
@@ -36,11 +39,12 @@ CrtcCommitRequest CrtcCommitRequest::EnableCrtcRequest(
     uint32_t crtc_id,
     uint32_t connector_id,
     drmModeModeInfo mode,
+    gfx::Point origin,
     HardwareDisplayPlaneList* plane_list,
     DrmOverlayPlaneList overlays) {
   DCHECK(plane_list && !overlays.empty());
 
-  return CrtcCommitRequest(crtc_id, connector_id, mode, plane_list,
+  return CrtcCommitRequest(crtc_id, connector_id, mode, origin, plane_list,
                            std::move(overlays), /*should_enable=*/true);
 }
 
@@ -49,7 +53,7 @@ CrtcCommitRequest CrtcCommitRequest::DisableCrtcRequest(
     uint32_t crtc_id,
     uint32_t connector_id,
     HardwareDisplayPlaneList* plane_list) {
-  return CrtcCommitRequest(crtc_id, connector_id, {}, plane_list,
+  return CrtcCommitRequest(crtc_id, connector_id, {}, gfx::Point(), plane_list,
                            DrmOverlayPlaneList(), /*should_enable=*/false);
 }
 

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

@@ -9,6 +9,7 @@
 #include <stdint.h>
 #include <xf86drmMode.h>
 
+#include "ui/gfx/geometry/point.h"
 #include "ui/ozone/platform/drm/gpu/drm_device.h"
 #include "ui/ozone/platform/drm/gpu/drm_overlay_plane.h"
 
@@ -31,6 +32,7 @@ class CrtcCommitRequest {
       uint32_t crtc_id,
       uint32_t connector_id,
       drmModeModeInfo mode,
+      gfx::Point origin,
       HardwareDisplayPlaneList* plane_list,
       DrmOverlayPlaneList overlays);
 
@@ -43,6 +45,7 @@ class CrtcCommitRequest {
   uint32_t crtc_id() const { return crtc_id_; }
   uint32_t connector_id() const { return connector_id_; }
   const drmModeModeInfo& mode() const { return mode_; }
+  const gfx::Point& origin() const { return origin_; }
   HardwareDisplayPlaneList* plane_list() const { return plane_list_; }
   const DrmOverlayPlaneList& overlays() const { return overlays_; }
 
@@ -50,6 +53,7 @@ class CrtcCommitRequest {
   CrtcCommitRequest(uint32_t crtc_id,
                     uint32_t connector_id,
                     drmModeModeInfo mode,
+                    gfx::Point origin,
                     HardwareDisplayPlaneList* plane_list,
                     DrmOverlayPlaneList overlays,
                     bool should_enable);
@@ -58,6 +62,7 @@ class CrtcCommitRequest {
   const uint32_t crtc_id_ = 0;
   const uint32_t connector_id_ = 0;
   const drmModeModeInfo mode_ = {};
+  const gfx::Point origin_;
   HardwareDisplayPlaneList* plane_list_ = nullptr;
   const DrmOverlayPlaneList overlays_;
 };

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

@@ -168,7 +168,6 @@ void DrmGpuDisplayManager::RelinquishDisplayControl() {
 
 bool DrmGpuDisplayManager::ConfigureDisplays(
     const std::vector<display::DisplayConfigurationParams>& config_requests) {
-  bool config_success = true;
   ScreenManager::ControllerConfigsList controllers_to_configure;
 
   for (const auto& config : config_requests) {
@@ -176,8 +175,7 @@ bool DrmGpuDisplayManager::ConfigureDisplays(
     DrmDisplay* display = FindDisplay(display_id);
     if (!display) {
       LOG(ERROR) << "There is no display with ID " << display_id;
-      config_success = false;
-      continue;
+      return false;
     }
 
     std::unique_ptr<drmModeModeInfo> mode_ptr =
@@ -185,8 +183,7 @@ bool DrmGpuDisplayManager::ConfigureDisplays(
     if (config.mode) {
       if (!FindModeForDisplay(mode_ptr.get(), *config.mode.value(),
                               display->modes(), displays_)) {
-        config_success = false;
-        continue;
+        return false;
       }
     }
 
@@ -205,33 +202,25 @@ bool DrmGpuDisplayManager::ConfigureDisplays(
     controllers_to_configure.push_back(std::move(params));
   }
 
-  if (controllers_to_configure.empty())
-    return config_success;
-
   if (clear_overlay_cache_callback_)
     clear_overlay_cache_callback_.Run();
 
-  config_success &=
+  bool config_success =
       screen_manager_->ConfigureDisplayControllers(controllers_to_configure);
-  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; });
 
+  for (const auto& controller : controllers_to_configure) {
     if (config_success) {
-      display->SetOrigin(config->origin);
+      FindDisplay(controller.display_id)->SetOrigin(controller.origin);
     } else {
-      if (config->mode) {
+      if (controller.mode) {
         VLOG(1) << "Failed to enable device="
-                << display->drm()->device_path().value()
-                << " crtc=" << display->crtc()
-                << " connector=" << display->connector();
+                << controller.drm->device_path().value()
+                << " crtc=" << controller.crtc
+                << " connector=" << controller.connector;
       } else {
         VLOG(1) << "Failed to disable device="
-                << display->drm()->device_path().value()
-                << " crtc=" << display->crtc();
+                << controller.drm->device_path().value()
+                << " crtc=" << controller.crtc;
       }
     }
   }

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

@@ -105,7 +105,7 @@ void HardwareDisplayController::GetModesetPropsForCrtcs(
     overlays.push_back(primary.Clone());
 
     CrtcCommitRequest request = CrtcCommitRequest::EnableCrtcRequest(
-        controller->crtc(), controller->connector(), modeset_mode,
+        controller->crtc(), controller->connector(), modeset_mode, origin_,
         &owned_hardware_planes_, std::move(overlays));
     commit_request->push_back(std::move(request));
   }

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

@@ -288,7 +288,7 @@ TEST_P(HardwareDisplayPlaneManagerTest, ResettingConnectorCache) {
 
       ui::CrtcCommitRequest request = ui::CrtcCommitRequest::EnableCrtcRequest(
           crtc_properties_[i].id, connector_properties[i].id, kDefaultMode,
-          &state, std::move(overlays));
+          gfx::Point(), &state, std::move(overlays));
       commit_request.push_back(std::move(request));
     }
 
@@ -309,22 +309,22 @@ TEST_P(HardwareDisplayPlaneManagerTest, ResettingConnectorCache) {
       ui::DrmOverlayPlaneList overlays;
       overlays.push_back(ui::DrmOverlayPlane(fake_buffer_, nullptr));
       commit_request.push_back(ui::CrtcCommitRequest::EnableCrtcRequest(
-          crtc_properties_[0].id, kConnectorIdBase, kDefaultMode, &state,
-          std::move(overlays)));
+          crtc_properties_[0].id, kConnectorIdBase, kDefaultMode, gfx::Point(),
+          &state, std::move(overlays)));
     }
     {
       ui::DrmOverlayPlaneList overlays;
       overlays.push_back(ui::DrmOverlayPlane(fake_buffer_, nullptr));
       commit_request.push_back(ui::CrtcCommitRequest::EnableCrtcRequest(
-          crtc_properties_[1].id, kConnectorIdBase + 1, kDefaultMode, &state,
-          std::move(overlays)));
+          crtc_properties_[1].id, kConnectorIdBase + 1, kDefaultMode,
+          gfx::Point(), &state, std::move(overlays)));
     }
     {
       ui::DrmOverlayPlaneList overlays;
       overlays.push_back(ui::DrmOverlayPlane(fake_buffer_, nullptr));
       commit_request.push_back(ui::CrtcCommitRequest::EnableCrtcRequest(
-          crtc_properties_[2].id, kConnectorIdBase + 3, kDefaultMode, &state,
-          std::move(overlays)));
+          crtc_properties_[2].id, kConnectorIdBase + 3, kDefaultMode,
+          gfx::Point(), &state, std::move(overlays)));
     }
 
     EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
@@ -346,8 +346,8 @@ TEST_P(HardwareDisplayPlaneManagerLegacyTest, Modeset) {
   ui::DrmOverlayPlaneList overlays;
   overlays.push_back(plane.Clone());
   commit_request.push_back(ui::CrtcCommitRequest::EnableCrtcRequest(
-      crtc_properties_[0].id, connector_properties_[0].id, kDefaultMode, &state,
-      std::move(overlays)));
+      crtc_properties_[0].id, connector_properties_[0].id, kDefaultMode,
+      gfx::Point(), &state, std::move(overlays)));
   EXPECT_FALSE(fake_drm_->plane_manager()->Commit(
       std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
 
@@ -495,8 +495,8 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, Modeset) {
   overlays.push_back(ui::DrmOverlayPlane(fake_buffer_, nullptr));
 
   commit_request.push_back(ui::CrtcCommitRequest::EnableCrtcRequest(
-      crtc_properties_[0].id, connector_properties_[0].id, kDefaultMode, &state,
-      std::move(overlays)));
+      crtc_properties_[0].id, connector_properties_[0].id, kDefaultMode,
+      gfx::Point(), &state, std::move(overlays)));
   EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
       std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
 
@@ -530,8 +530,8 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, CheckPropsAfterModeset) {
   ui::DrmOverlayPlaneList overlays;
   overlays.push_back(ui::DrmOverlayPlane(fake_buffer_, nullptr));
   commit_request.push_back(ui::CrtcCommitRequest::EnableCrtcRequest(
-      crtc_properties_[0].id, connector_properties_[0].id, kDefaultMode, &state,
-      std::move(overlays)));
+      crtc_properties_[0].id, connector_properties_[0].id, kDefaultMode,
+      gfx::Point(), &state, std::move(overlays)));
   EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
       std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
 
@@ -570,7 +570,7 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, CheckPropsAfterDisable) {
     overlays.push_back(ui::DrmOverlayPlane(fake_buffer_, nullptr));
     commit_request.push_back(ui::CrtcCommitRequest::EnableCrtcRequest(
         crtc_properties_[0].id, connector_properties_[0].id, kDefaultMode,
-        &state, std::move(overlays)));
+        gfx::Point(), &state, std::move(overlays)));
     EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
         std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
   }

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

@@ -241,8 +241,9 @@ bool ScreenManager::ConfigureDisplayControllers(
   bool config_success = true;
   // Perform display configurations together for the same DRM only.
   for (const auto& configs_on_drm : displays_for_drm_devices) {
+    const ControllerConfigsList& controllers_params = configs_on_drm.second;
     config_success &=
-        TestModeset(configs_on_drm.second) && Modeset(configs_on_drm.second);
+        TestModeset(controllers_params) && Modeset(controllers_params);
   }
 
   if (config_success)
@@ -284,14 +285,13 @@ bool ScreenManager::TestModeset(
 }
 
 bool ScreenManager::Modeset(const ControllerConfigsList& controllers_params) {
-  TRACE_EVENT0("drm", "ScreenManager::Modeset");
+  TRACE_EVENT1("drm", "ScreenManager::Modeset", "display_count",
+               controllers_params.size());
 
-  bool modeset_success = true;
+  CommitRequest commit_request;
+  auto drm = controllers_params[0].drm;
 
   for (const auto& params : controllers_params) {
-    // Commit one controller at a time.
-    CommitRequest commit_request;
-    bool status = true;
     if (params.mode) {
       auto it = FindDisplayController(params.drm, params.crtc);
       DCHECK(controllers_.end() != it);
@@ -300,30 +300,27 @@ bool ScreenManager::Modeset(const ControllerConfigsList& controllers_params) {
       DrmOverlayPlane primary_plane = GetModesetBuffer(
           controller, gfx::Rect(params.origin, ModeSize(*params.mode)),
           GetModifiersForPrimaryFormat(controller), /*is_testing=*/false);
-      if (primary_plane.buffer) {
-        SetDisplayControllerForEnableAndGetProps(
-            &commit_request, params.drm, params.crtc, params.connector,
-            params.origin, *params.mode, primary_plane);
-      } else {
-        status = false;
-      }
+      if (!primary_plane.buffer)
+        return false;
+
+      SetDisplayControllerForEnableAndGetProps(
+          &commit_request, params.drm, params.crtc, params.connector,
+          params.origin, *params.mode, primary_plane);
 
     } else {
-      status = SetDisableDisplayControllerForDisableAndGetProps(
+      bool disable_set = SetDisableDisplayControllerForDisableAndGetProps(
           &commit_request, params.drm, params.crtc);
+      if (!disable_set)
+        return false;
     }
+  }
 
-    CommitRequest request_for_update = commit_request;
-    if (status) {
-      status &= params.drm->plane_manager()->Commit(
-          std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET);
-      UpdateControllerStateAfterModeset(params, request_for_update, status);
-    }
+  bool commit_status = drm->plane_manager()->Commit(
+      commit_request, DRM_MODE_ATOMIC_ALLOW_MODESET);
 
-    modeset_success &= status;
-  }
+  UpdateControllerStateAfterModeset(drm, commit_request, commit_status);
 
-  return modeset_success;
+  return commit_status;
 }
 
 void ScreenManager::SetDisplayControllerForEnableAndGetProps(
@@ -394,31 +391,33 @@ bool ScreenManager::SetDisableDisplayControllerForDisableAndGetProps(
 }
 
 void ScreenManager::UpdateControllerStateAfterModeset(
-    const ControllerConfigParams& config,
+    const scoped_refptr<DrmDevice>& drm,
     const CommitRequest& commit_request,
     bool did_succeed) {
-  for (auto& crtc_request : commit_request) {
+  for (const CrtcCommitRequest& crtc_request : commit_request) {
     bool was_enabled = (crtc_request.should_enable());
 
     HardwareDisplayControllers::iterator it =
-        FindDisplayController(config.drm, crtc_request.crtc_id());
+        FindDisplayController(drm, crtc_request.crtc_id());
     if (it != controllers_.end()) {
       it->get()->UpdateState(was_enabled, DrmOverlayPlane::GetPrimaryPlane(
                                               crtc_request.overlays()));
 
       // If the CRTC is mirrored, move it to the mirror controller.
       if (did_succeed && was_enabled)
-        HandleMirrorIfExists(config, it);
+        HandleMirrorIfExists(drm, crtc_request, it);
     }
   }
 }
 
 void ScreenManager::HandleMirrorIfExists(
-    const ControllerConfigParams& config,
+    const scoped_refptr<DrmDevice>& drm,
+    const CrtcCommitRequest& crtc_request,
     const HardwareDisplayControllers::iterator& controller) {
-  gfx::Rect modeset_bounds(config.origin, ModeSize(*config.mode));
+  gfx::Rect modeset_bounds(crtc_request.origin(),
+                           ModeSize(crtc_request.mode()));
   HardwareDisplayControllers::iterator mirror =
-      FindActiveDisplayControllerByLocation(config.drm, modeset_bounds);
+      FindActiveDisplayControllerByLocation(drm, modeset_bounds);
   // TODO(dnicoara): This is hacky, instead the DrmDisplay and
   // CrtcController should be merged and picking the mode should be done
   // properly within HardwareDisplayController.
@@ -426,7 +425,7 @@ void ScreenManager::HandleMirrorIfExists(
     // TODO(markyacoub): RemoveCrtc makes a blocking commit to
     // DisableOverlayPlanes. This should be redesigned and included as part of
     // the Modeset commit.
-    (*mirror)->AddCrtc((*controller)->RemoveCrtc(config.drm, config.crtc));
+    (*mirror)->AddCrtc((*controller)->RemoveCrtc(drm, crtc_request.crtc_id()));
     controllers_.erase(controller);
   }
 }

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

@@ -106,7 +106,6 @@ class ScreenManager {
       uint32_t crtc);
 
   bool TestModeset(const ControllerConfigsList& controllers_params);
-
   bool Modeset(const ControllerConfigsList& controllers_params);
 
   // Configures a display controller to be enabled. The display controller is
@@ -131,12 +130,13 @@ class ScreenManager {
       const scoped_refptr<DrmDevice>& drm,
       uint32_t crtc);
 
-  void UpdateControllerStateAfterModeset(const ControllerConfigParams& config,
+  void UpdateControllerStateAfterModeset(const scoped_refptr<DrmDevice>& drm,
                                          const CommitRequest& commit_request,
                                          bool did_succeed);
 
   void HandleMirrorIfExists(
-      const ControllerConfigParams& config,
+      const scoped_refptr<DrmDevice>& drm,
+      const CrtcCommitRequest& crtc_request,
       const HardwareDisplayControllers::iterator& controller);
 
   // Returns an iterator into |controllers_| for the controller located at

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

@@ -287,7 +287,7 @@ TEST_F(ScreenManagerTest, CheckForSecondValidController) {
   screen_manager_->ConfigureDisplayControllers(controllers_to_enable);
 
   EXPECT_EQ(drm_->get_test_modeset_count(), 1);
-  EXPECT_EQ(drm_->get_commit_modeset_count(), 2);
+  EXPECT_EQ(drm_->get_commit_modeset_count(), 1);
 
   EXPECT_TRUE(screen_manager_->GetDisplayController(GetPrimaryBounds()));
   EXPECT_TRUE(screen_manager_->GetDisplayController(GetSecondaryBounds()));
@@ -407,7 +407,7 @@ TEST_F(ScreenManagerTest, CheckMultipleControllersAfterBeingDisabled) {
   EXPECT_EQ(drm_->get_test_modeset_count(),
             test_modeset_count_before_disable + 1);
   EXPECT_EQ(drm_->get_commit_modeset_count(),
-            commit_modeset_count_before_disable + 2);
+            commit_modeset_count_before_disable + 1);
 
   EXPECT_FALSE(screen_manager_->GetDisplayController(GetPrimaryBounds()));
   EXPECT_FALSE(screen_manager_->GetDisplayController(GetSecondaryBounds()));
@@ -647,17 +647,23 @@ TEST_F(ScreenManagerTest, MonitorDisabledInMirrorMode) {
   screen_manager_->AddDisplayController(drm_, kSecondaryCrtc,
                                         kSecondaryConnector);
 
-  ScreenManager::ControllerConfigsList controllers_to_enable;
-  controllers_to_enable.emplace_back(
-      kPrimaryDisplayId, drm_, kPrimaryCrtc, kPrimaryConnector,
-      GetPrimaryBounds().origin(),
-      std::make_unique<drmModeModeInfo>(kDefaultMode));
-  drmModeModeInfo secondary_mode = kDefaultMode;
-  controllers_to_enable.emplace_back(
-      kSecondaryDisplayId, drm_, kSecondaryCrtc, kSecondaryConnector,
-      GetPrimaryBounds().origin(),
-      std::make_unique<drmModeModeInfo>(secondary_mode));
+  // Enable in Mirror Mode.
+  {
+    ScreenManager::ControllerConfigsList controllers_to_enable;
+    controllers_to_enable.emplace_back(
+        kPrimaryDisplayId, drm_, kPrimaryCrtc, kPrimaryConnector,
+        GetPrimaryBounds().origin(),
+        std::make_unique<drmModeModeInfo>(kDefaultMode));
+    drmModeModeInfo secondary_mode = kDefaultMode;
+    controllers_to_enable.emplace_back(
+        kSecondaryDisplayId, drm_, kSecondaryCrtc, kSecondaryConnector,
+        GetPrimaryBounds().origin(),
+        std::make_unique<drmModeModeInfo>(secondary_mode));
+    screen_manager_->ConfigureDisplayControllers(controllers_to_enable);
+  }
+
   // Disable display Controller.
+  ScreenManager::ControllerConfigsList controllers_to_enable;
   controllers_to_enable.emplace_back(0, drm_, kSecondaryCrtc, 0, gfx::Point(),
                                      nullptr);
   screen_manager_->ConfigureDisplayControllers(controllers_to_enable);