Move the entire modeset operation to Plane Manager Commit

1. Set all props for CRTC, Connector and Plane to Plane Manager to Commit.
2. Maintain a consolidated path to modeset both Atomic and Legacy.
3. Simplify CRTC Controller by removing all modeset operations and maintain its state from PlaneManager::CrtcState.
4. This makes all CRTCs on the same hardware display controller to be modeset at the same time.

BUG=1082882,1015104
TEST=hardware_display_plane_manager_unittest

Change-Id: I2401a1b40ce74158416484e6418239d5fb8c1c25
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429586
Commit-Queue: Mark Yacoub <markyacoub@google.com>
Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814933}

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

@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "base/time/time.h"
 #include "ui/gfx/presentation_feedback.h"
+#include "ui/ozone/platform/drm/common/drm_util.h"
 #include "ui/ozone/platform/drm/gpu/drm_device.h"
 #include "ui/ozone/platform/drm/gpu/drm_dumb_buffer.h"
 #include "ui/ozone/platform/drm/gpu/drm_framebuffer.h"
@@ -22,10 +23,11 @@ CrtcController::CrtcController(const scoped_refptr<DrmDevice>& drm,
                                uint32_t connector)
     : drm_(drm),
       crtc_(crtc),
-      connector_(connector) {}
+      connector_(connector),
+      state_(drm->plane_manager()->GetCrtcStateForCrtcId(crtc)) {}
 
 CrtcController::~CrtcController() {
-  if (!is_disabled_) {
+  if (!is_disabled()) {
     const std::vector<std::unique_ptr<HardwareDisplayPlane>>& all_planes =
         drm_->plane_manager()->planes();
     for (const auto& plane : all_planes) {
@@ -35,57 +37,26 @@ CrtcController::~CrtcController() {
       }
     }
 
-    DisableCursor();
-    drm_->plane_manager()->DisableModeset(crtc_, connector_);
+    CommitRequest commit_request;
+    CrtcRequest crtc_request{crtc_, connector_, nullptr, {}};
+    commit_request.commit_state.push_back(std::move(crtc_request));
+    drm_->plane_manager()->Commit(std::move(commit_request),
+                                  DRM_MODE_ATOMIC_ALLOW_MODESET);
   }
 }
 
-bool CrtcController::Modeset(const DrmOverlayPlane& plane,
-                             const drmModeModeInfo& mode,
-                             const ui::HardwareDisplayPlaneList& plane_list) {
-  if (!drm_->plane_manager()->Modeset(crtc_,
-                                      plane.buffer->opaque_framebuffer_id(),
-                                      connector_, mode, plane_list)) {
-    PLOG(ERROR) << "Failed to modeset: crtc=" << crtc_
-                << " connector=" << connector_
-                << " framebuffer_id=" << plane.buffer->opaque_framebuffer_id()
-                << " mode=" << mode.hdisplay << "x" << mode.vdisplay << "@"
-                << mode.vrefresh;
-    return false;
-  }
-
-  mode_ = mode;
-  is_disabled_ = false;
-
-  // Hold modeset buffer until page flip. This fixes a crash on entering
-  // hardware mirror mode in some circumstances (bug 888553).
-  // TODO(spang): Fix this better by changing how mirrors are set up (bug
-  // 899352).
-  modeset_framebuffer_ = plane.buffer;
-
-  return true;
-}
-
-bool CrtcController::Disable() {
-  if (is_disabled_)
-    return true;
-
-  is_disabled_ = true;
-  DisableCursor();
-  return drm_->plane_manager()->DisableModeset(crtc_, connector_);
-}
-
 bool CrtcController::AssignOverlayPlanes(HardwareDisplayPlaneList* plane_list,
                                          const DrmOverlayPlaneList& overlays,
                                          bool is_modesetting) {
   // If we're in the process of modesetting, the CRTC is still disabled.
   // Once the modeset is done, we expect it to be enabled.
-  DCHECK(is_modesetting || !is_disabled_);
+  DCHECK(is_modesetting || !is_disabled());
 
   const DrmOverlayPlane* primary = DrmOverlayPlane::GetPrimaryPlane(overlays);
-  if (primary && !drm_->plane_manager()->ValidatePrimarySize(*primary, mode_)) {
+  if (primary &&
+      !drm_->plane_manager()->ValidatePrimarySize(*primary, state_.mode)) {
     VLOG(2) << "Trying to pageflip a buffer with the wrong size. Expected "
-            << mode_.hdisplay << "x" << mode_.vdisplay << " got "
+            << ModeSize(state_.mode).ToString() << " got "
             << primary->buffer->size().ToString() << " for"
             << " crtc=" << crtc_ << " connector=" << connector_;
     return true;
@@ -104,7 +75,7 @@ std::vector<uint64_t> CrtcController::GetFormatModifiers(uint32_t format) {
 }
 
 void CrtcController::SetCursor(uint32_t handle, const gfx::Size& size) {
-  if (is_disabled_)
+  if (is_disabled())
     return;
   if (!drm_->SetCursor(crtc_, handle, size)) {
     PLOG(ERROR) << "drmModeSetCursor: device " << drm_->device_path().value()
@@ -114,20 +85,9 @@ void CrtcController::SetCursor(uint32_t handle, const gfx::Size& size) {
 }
 
 void CrtcController::MoveCursor(const gfx::Point& location) {
-  if (is_disabled_)
+  if (is_disabled())
     return;
   drm_->MoveCursor(crtc_, location);
 }
 
-void CrtcController::OnPageFlipComplete() {
-  modeset_framebuffer_ = nullptr;
-}
-
-void CrtcController::DisableCursor() {
-  if (!drm_->SetCursor(crtc_, 0, gfx::Size())) {
-    PLOG(ERROR) << "drmModeSetCursor: device " << drm_->device_path().value()
-                << " crtc " << crtc_ << " disable";
-  }
-}
-
 }  // namespace ui

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

@@ -34,21 +34,11 @@ class CrtcController {
                  uint32_t connector);
   ~CrtcController();
 
-  drmModeModeInfo mode() const { return mode_; }
+  drmModeModeInfo mode() const { return state_.mode; }
   uint32_t crtc() const { return crtc_; }
   uint32_t connector() const { return connector_; }
   const scoped_refptr<DrmDevice>& drm() const { return drm_; }
-  bool is_disabled() const { return is_disabled_; }
-
-  // Calls the appropriate Plane Manager to perform the initial modesetting
-  // operation using |plane| as the buffer for the primary plane. The CRTC
-  // configuration is specified by |mode|.
-  bool Modeset(const DrmOverlayPlane& plane,
-               const drmModeModeInfo& mode,
-               const ui::HardwareDisplayPlaneList& plane_list);
-
-  // Disables the controller.
-  bool Disable();
+  bool is_disabled() const { return !state_.properties.active.value; }
 
   bool AssignOverlayPlanes(HardwareDisplayPlaneList* plane_list,
                            const DrmOverlayPlaneList& planes,
@@ -67,11 +57,7 @@ class CrtcController {
   void SetCursor(uint32_t handle, const gfx::Size& size);
   void MoveCursor(const gfx::Point& location);
 
-  void OnPageFlipComplete();
-
  private:
-  void DisableCursor();
-
   const scoped_refptr<DrmDevice> drm_;
 
   const uint32_t crtc_;
@@ -79,13 +65,7 @@ class CrtcController {
   // TODO(dnicoara) Add support for hardware mirroring (multiple connectors).
   const uint32_t connector_;
 
-  drmModeModeInfo mode_ = {};
-
-  scoped_refptr<DrmFramebuffer> modeset_framebuffer_;
-
-  // Keeps track of the CRTC state. If a surface has been bound, then the value
-  // is set to false. Otherwise it is true.
-  bool is_disabled_ = true;
+  const HardwareDisplayPlaneManager::CrtcState& state_;
 
   DISALLOW_COPY_AND_ASSIGN(CrtcController);
 };

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

@@ -52,14 +52,7 @@ class MockHardwareDisplayPlaneManager : public HardwareDisplayPlaneManager {
                     const std::vector<display::GammaRampRGBEntry>& degamma_lut,
                     const std::vector<display::GammaRampRGBEntry>& gamma_lut));
 
-  bool Modeset(uint32_t crtc_id,
-               uint32_t framebuffer_id,
-               uint32_t connector_id,
-               const drmModeModeInfo& mode,
-               const HardwareDisplayPlaneList& plane_list) override {
-    return false;
-  }
-  bool DisableModeset(uint32_t crtc_id, uint32_t connector) override {
+  bool Commit(CommitRequest commit_request, uint32_t flags) override {
     return false;
   }
   bool Commit(HardwareDisplayPlaneList* plane_list,

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

@@ -85,6 +85,8 @@ class DrmWindowTest : public testing::Test {
   }
 
  protected:
+  void InitializeDrmState(ui::MockDrmDevice* drm, bool is_atomic = true);
+
   base::test::SingleThreadTaskEnvironment task_environment_{
       base::test::SingleThreadTaskEnvironment::MainThreadType::UI};
   scoped_refptr<ui::MockDrmDevice> drm_;
@@ -96,6 +98,14 @@ class DrmWindowTest : public testing::Test {
   gfx::PresentationFeedback last_presentation_feedback_;
 
  private:
+  struct PlaneState {
+    std::vector<uint32_t> formats;
+  };
+
+  struct CrtcState {
+    std::vector<PlaneState> planes;
+  };
+
   DISALLOW_COPY_AND_ASSIGN(DrmWindowTest);
 };
 
@@ -107,11 +117,13 @@ void DrmWindowTest::SetUp() {
   drm_ = new ui::MockDrmDevice(std::move(gbm_device));
   screen_manager_ = std::make_unique<ui::ScreenManager>();
 
+  InitializeDrmState(drm_.get());
+
   screen_manager_->AddDisplayController(drm_, kDefaultCrtc, kDefaultConnector);
   std::vector<ui::ScreenManager::ControllerConfigParams> controllers_to_enable;
-  controllers_to_enable.push_back(
-      {1 /*display_id*/, drm_, kDefaultCrtc, kDefaultConnector, gfx::Point(),
-       std::make_unique<drmModeModeInfo>(kDefaultMode)});
+  controllers_to_enable.emplace_back(
+      1 /*display_id*/, drm_, kDefaultCrtc, kDefaultConnector, gfx::Point(),
+      std::make_unique<drmModeModeInfo>(kDefaultMode));
   screen_manager_->ConfigureDisplayControllers(controllers_to_enable);
 
   drm_device_manager_ = std::make_unique<ui::DrmDeviceManager>(nullptr);
@@ -130,6 +142,110 @@ void DrmWindowTest::TearDown() {
   window->Shutdown();
 }
 
+void DrmWindowTest::InitializeDrmState(ui::MockDrmDevice* drm, bool is_atomic) {
+  // A Sample of CRTC states.
+  std::vector<CrtcState> crtc_states = {
+      {
+          /* .planes = */
+          {{/* .formats = */ {DRM_FORMAT_XRGB8888}}},
+      },
+      {
+          /* .planes = */
+          {{/* .formats = */ {DRM_FORMAT_XRGB8888}}},
+      },
+  };
+
+  constexpr uint32_t kPlaneIdBase = 300;
+  constexpr uint32_t kInFormatsBlobPropIdBase = 400;
+
+  std::vector<ui::MockDrmDevice::CrtcProperties> crtc_properties(
+      crtc_states.size());
+  std::map<uint32_t, std::string> crtc_property_names = {
+      {1000, "ACTIVE"},
+      {1001, "MODE_ID"},
+  };
+
+  std::vector<ui::MockDrmDevice::ConnectorProperties> connector_properties(3);
+  std::map<uint32_t, std::string> connector_property_names = {
+      {2000, "CRTC_ID"},
+  };
+  for (size_t i = 0; i < connector_properties.size(); ++i) {
+    connector_properties[i].id = kDefaultConnector + i;
+    for (const auto& pair : connector_property_names) {
+      connector_properties[i].properties.push_back(
+          {/* .id = */ pair.first, /* .value = */ 0});
+    }
+  }
+
+  std::vector<ui::MockDrmDevice::PlaneProperties> plane_properties;
+  std::map<uint32_t, std::string> plane_property_names = {
+      // Add all required properties.
+      {3000, "CRTC_ID"},
+      {3001, "CRTC_X"},
+      {3002, "CRTC_Y"},
+      {3003, "CRTC_W"},
+      {3004, "CRTC_H"},
+      {3005, "FB_ID"},
+      {3006, "SRC_X"},
+      {3007, "SRC_Y"},
+      {3008, "SRC_W"},
+      {3009, "SRC_H"},
+      // Defines some optional properties we use for convenience.
+      {3010, "type"},
+      {3011, "IN_FORMATS"},
+  };
+
+  uint32_t plane_id = kPlaneIdBase;
+  uint32_t property_id = kInFormatsBlobPropIdBase;
+
+  for (size_t crtc_idx = 0; crtc_idx < crtc_states.size(); ++crtc_idx) {
+    crtc_properties[crtc_idx].id = kDefaultCrtc + crtc_idx;
+    for (const auto& pair : crtc_property_names) {
+      crtc_properties[crtc_idx].properties.push_back(
+          {/* .id = */ pair.first, /* .value = */ 0});
+    }
+
+    std::vector<ui::MockDrmDevice::PlaneProperties> crtc_plane_properties(
+        crtc_states[crtc_idx].planes.size());
+    for (size_t plane_idx = 0; plane_idx < crtc_states[crtc_idx].planes.size();
+         ++plane_idx) {
+      crtc_plane_properties[plane_idx].id = plane_id++;
+      crtc_plane_properties[plane_idx].crtc_mask = 1 << crtc_idx;
+
+      for (const auto& pair : plane_property_names) {
+        uint64_t value = 0;
+        if (pair.first == 3010) {
+          value =
+              plane_idx == 0 ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
+        } else if (pair.first == 3011) {
+          value = property_id++;
+          drm->SetPropertyBlob(ui::MockDrmDevice::AllocateInFormatsBlob(
+              value, crtc_states[crtc_idx].planes[plane_idx].formats,
+              std::vector<drm_format_modifier>()));
+        } else if (pair.first >= 3001 && pair.first <= 3009) {
+          value = 27;
+        }
+
+        crtc_plane_properties[plane_idx].properties.push_back(
+            {/* .id = */ pair.first, /* .value = */ value});
+      }
+    }
+
+    plane_properties.insert(plane_properties.end(),
+                            crtc_plane_properties.begin(),
+                            crtc_plane_properties.end());
+  }
+
+  std::map<uint32_t, std::string> property_names;
+  property_names.insert(crtc_property_names.begin(), crtc_property_names.end());
+  property_names.insert(connector_property_names.begin(),
+                        connector_property_names.end());
+  property_names.insert(plane_property_names.begin(),
+                        plane_property_names.end());
+  drm->InitializeState(crtc_properties, connector_properties, plane_properties,
+                       property_names, /*is_atomic=*/false);
+}
+
 TEST_F(DrmWindowTest, SetCursorImage) {
   const gfx::Size cursor_size(6, 4);
   screen_manager_->GetWindow(kDefaultWidgetHandle)
@@ -166,14 +282,15 @@ TEST_F(DrmWindowTest, CheckCursorSurfaceAfterChangingDevice) {
   auto gbm_device = std::make_unique<ui::MockGbmDevice>();
   scoped_refptr<ui::MockDrmDevice> drm =
       new ui::MockDrmDevice(std::move(gbm_device));
+  InitializeDrmState(drm.get());
 
   screen_manager_->AddDisplayController(drm, kDefaultCrtc, kDefaultConnector);
 
   std::vector<ui::ScreenManager::ControllerConfigParams> controllers_to_enable;
-  controllers_to_enable.push_back(
-      {2 /*display_id*/, drm, kDefaultCrtc, kDefaultConnector,
-       gfx::Point(0, kDefaultMode.vdisplay),
-       std::make_unique<drmModeModeInfo>(kDefaultMode)});
+  controllers_to_enable.emplace_back(
+      2 /*display_id*/, drm, kDefaultCrtc, kDefaultConnector,
+      gfx::Point(0, kDefaultMode.vdisplay),
+      std::make_unique<drmModeModeInfo>(kDefaultMode));
   screen_manager_->ConfigureDisplayControllers(controllers_to_enable);
 
   // Move window to the display on the new device.

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

@@ -89,22 +89,22 @@ bool HardwareDisplayController::ModesetCrtc(const DrmOverlayPlane& primary,
                                             bool use_current_crtc_mode,
                                             const drmModeModeInfo& mode) {
   DCHECK(primary.buffer.get());
-  bool status = true;
+  CommitRequest commit_request;
+  commit_request.plane_list = &owned_hardware_planes_;
 
   GetDrmDevice()->plane_manager()->BeginFrame(&owned_hardware_planes_);
-  DrmOverlayPlaneList plane_list;
-  plane_list.push_back(primary.Clone());
 
   for (const auto& controller : crtc_controllers_) {
-    status &=
-        controller->AssignOverlayPlanes(&owned_hardware_planes_, plane_list,
-                                        /*is_modesetting=*/true);
-
-    status &= controller->Modeset(
-        primary, use_current_crtc_mode ? controller->mode() : mode,
-        owned_hardware_planes_);
+    drmModeModeInfo modeset_mode =
+        use_current_crtc_mode ? controller->mode() : mode;
+    CrtcRequest crtc_request{controller->crtc(), controller->connector(),
+                             &primary, modeset_mode};
+    commit_request.commit_state.push_back(std::move(crtc_request));
   }
 
+  bool status = GetDrmDevice()->plane_manager()->Commit(
+      std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET);
+
   is_disabled_ = false;
   ResetCursor();
   OnModesetComplete(primary);
@@ -114,16 +114,17 @@ bool HardwareDisplayController::ModesetCrtc(const DrmOverlayPlane& primary,
 void HardwareDisplayController::Disable() {
   TRACE_EVENT0("drm", "HDC::Disable");
 
-  for (const auto& controller : crtc_controllers_)
-    // TODO(crbug.com/1015104): Modeset and Disable operations should go
-    // together. The current split is due to how the legacy/atomic split
-    // evolved. It should be cleaned up under the more generic
-    // HardwareDisplayPlaneManager{Legacy,Atomic} calls.
-    controller->Disable();
+  CommitRequest commit_request;
+  commit_request.plane_list = &owned_hardware_planes_;
+
+  for (const auto& controller : crtc_controllers_) {
+    CrtcRequest crtc_request{
+        controller->crtc(), controller->connector(), nullptr, {}};
+    commit_request.commit_state.push_back(std::move(crtc_request));
+  }
 
-  bool ret = GetDrmDevice()->plane_manager()->DisableOverlayPlanes(
-      &owned_hardware_planes_);
-  LOG_IF(ERROR, !ret) << "Can't disable overlays when disabling HDC.";
+  GetDrmDevice()->plane_manager()->Commit(std::move(commit_request),
+                                          DRM_MODE_ATOMIC_ALLOW_MODESET);
 
   is_disabled_ = true;
 }
@@ -356,8 +357,10 @@ void HardwareDisplayController::OnPageFlipComplete(
     return;  // Modeset occured during this page flip.
   time_of_last_flip_ = presentation_feedback.timestamp;
   current_planes_ = std::move(pending_planes);
-  for (const auto& controller : crtc_controllers_)
-    controller->OnPageFlipComplete();
+  for (const auto& controller : crtc_controllers_) {
+    GetDrmDevice()->plane_manager()->ResetModesetBufferOfCrtc(
+        controller->crtc());
+  }
   page_flip_request_ = nullptr;
 }
 

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

@@ -407,13 +407,12 @@ TEST_F(HardwareDisplayControllerTest, AcceptUnderlays) {
 }
 
 TEST_F(HardwareDisplayControllerTest, PageflipMirroredControllers) {
-  controller_->AddCrtc(
-      std::unique_ptr<ui::CrtcController>(new ui::CrtcController(
-          drm_.get(), kSecondaryCrtc, kConnectorIdBase + 1)));
+  controller_->AddCrtc(std::make_unique<ui::CrtcController>(
+      drm_.get(), kSecondaryCrtc, kConnectorIdBase + 1));
 
   ui::DrmOverlayPlane plane1(CreateBuffer(), nullptr);
   EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));
-  EXPECT_EQ(2, drm_->get_commit_count());
+  EXPECT_EQ(1, drm_->get_commit_count());
 
   ui::DrmOverlayPlane plane2(CreateBuffer(), nullptr);
   std::vector<ui::DrmOverlayPlane> planes;
@@ -422,7 +421,7 @@ TEST_F(HardwareDisplayControllerTest, PageflipMirroredControllers) {
   drm_->RunCallbacks();
   EXPECT_EQ(gfx::SwapResult::SWAP_ACK, last_swap_result_);
   EXPECT_EQ(1, page_flips_);
-  EXPECT_EQ(3, drm_->get_commit_count());
+  EXPECT_EQ(2, drm_->get_commit_count());
   // Verify only the displays have a valid framebuffer on the primary plane.
   // First display:
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset, "FB_ID"));
@@ -433,9 +432,8 @@ TEST_F(HardwareDisplayControllerTest, PageflipMirroredControllers) {
 }
 
 TEST_F(HardwareDisplayControllerTest, PlaneStateAfterRemoveCrtc) {
-  controller_->AddCrtc(
-      std::unique_ptr<ui::CrtcController>(new ui::CrtcController(
-          drm_.get(), kSecondaryCrtc, kConnectorIdBase + 1)));
+  controller_->AddCrtc(std::make_unique<ui::CrtcController>(
+      drm_.get(), kSecondaryCrtc, kConnectorIdBase + 1));
 
   ui::DrmOverlayPlane plane1(CreateBuffer(), nullptr);
   EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));

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

@@ -26,6 +26,13 @@ HardwareDisplayPlaneList::HardwareDisplayPlaneList() {
 
 HardwareDisplayPlaneList::~HardwareDisplayPlaneList() = default;
 
+CommitRequest::CommitRequest() = default;
+CommitRequest::CommitRequest(const CommitRequest& other) {
+  commit_state = other.commit_state;
+  plane_list = other.plane_list;
+}
+CommitRequest::~CommitRequest() = default;
+
 HardwareDisplayPlaneList::PageFlipInfo::PageFlipInfo(uint32_t crtc_id,
                                                      uint32_t framebuffer)
     : crtc_id(crtc_id), framebuffer(framebuffer) {}
@@ -429,4 +436,59 @@ void HardwareDisplayPlaneManager::DisableConnectedConnectorsToCrtcs(
   }
 }
 
+const HardwareDisplayPlaneManager::CrtcState&
+HardwareDisplayPlaneManager::GetCrtcStateForCrtcId(uint32_t crtc_id) {
+  return CrtcStateForCrtcId(crtc_id);
+}
+
+HardwareDisplayPlaneManager::CrtcState&
+HardwareDisplayPlaneManager::CrtcStateForCrtcId(uint32_t crtc_id) {
+  int crtc_index = LookupCrtcIndex(crtc_id);
+  DCHECK_GE(crtc_index, 0);
+  return crtc_state_[crtc_index];
+}
+
+void HardwareDisplayPlaneManager::UpdateCrtcStateAfterModeset(
+    const CommitRequest& commit_request) {
+  bool should_disable_overlay_planes = false;
+
+  for (const auto& crtc_request : commit_request.commit_state) {
+    bool is_enabled = !!crtc_request.primary_plane;
+
+    int connector_index = LookupConnectorIndex(crtc_request.connector_id);
+    DCHECK_GE(connector_index, 0);
+    ConnectorProperties& connector_props = connectors_props_[connector_index];
+    connector_props.crtc_id.value = is_enabled ? crtc_request.crtc_id : 0;
+
+    CrtcState& crtc_state = CrtcStateForCrtcId(crtc_request.crtc_id);
+    crtc_state.properties.active.value = static_cast<uint64_t>(is_enabled);
+
+    if (is_enabled) {
+      crtc_state.mode = crtc_request.mode;
+      crtc_state.modeset_framebuffer = crtc_request.primary_plane->buffer;
+    } else {
+      crtc_state.properties.mode_id.value = 0;
+      should_disable_overlay_planes = true;
+      // TODO(crbug/1135291): Use atomic APIs to reset cursor plane.
+      if (!drm_->SetCursor(crtc_request.crtc_id, 0, gfx::Size())) {
+        PLOG(ERROR) << "Failed to drmModeSetCursor: device:"
+                    << drm_->device_path().value()
+                    << " crtc:" << crtc_request.crtc_id;
+      }
+    }
+  }
+
+  if (should_disable_overlay_planes && commit_request.plane_list) {
+    // TODO(markyacoub): DisableOverlayPlanes can be part of the commit request.
+    bool status = DisableOverlayPlanes(commit_request.plane_list);
+    LOG_IF(ERROR, !status) << "Can't disable overlays when disabling HDC.";
+    commit_request.plane_list->plane_list.clear();
+  }
+}
+
+void HardwareDisplayPlaneManager::ResetModesetBufferOfCrtc(uint32_t crtc_id) {
+  CrtcState& crtc_state = CrtcStateForCrtcId(crtc_id);
+  crtc_state.modeset_framebuffer = nullptr;
+}
+
 }  // namespace ui

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

@@ -54,8 +54,63 @@ struct HardwareDisplayPlaneList {
   ScopedDrmAtomicReqPtr atomic_property_set;
 };
 
+// Container holding information for a single CRTC that need to be modeset.
+// TODO(markyacoub): PAGE_FLIP could re-use the same CrtcRequest. The difference
+// between MODESET and PAGE_FLIP are minimal.
+struct CrtcRequest {
+  uint32_t crtc_id = 0;
+  uint32_t connector_id = 0;
+  const DrmOverlayPlane* primary_plane = nullptr;
+  drmModeModeInfo mode = {};
+};
+
+// Container holding all parameters required to perform a modeset.
+struct CommitRequest {
+  CommitRequest();
+  CommitRequest(const CommitRequest& other);
+  ~CommitRequest();
+  std::vector<CrtcRequest> commit_state;
+  HardwareDisplayPlaneList* plane_list = nullptr;
+};
+
 class HardwareDisplayPlaneManager {
  public:
+  struct CrtcProperties {
+    // Unique identifier for the CRTC. This must be greater than 0 to be valid.
+    uint32_t id;
+    // Keeps track of the CRTC state. If a surface has been bound, then the
+    // value is set to true. Otherwise it is false.
+    DrmDevice::Property active;
+    DrmDevice::Property mode_id;
+    // Optional properties.
+    DrmDevice::Property ctm;
+    DrmDevice::Property gamma_lut;
+    DrmDevice::Property gamma_lut_size;
+    DrmDevice::Property degamma_lut;
+    DrmDevice::Property degamma_lut_size;
+    DrmDevice::Property out_fence_ptr;
+    DrmDevice::Property background_color;
+  };
+
+  struct CrtcState {
+    CrtcState();
+    ~CrtcState();
+    CrtcState(const CrtcState&) = delete;
+    CrtcState& operator=(const CrtcState&) = delete;
+    CrtcState(CrtcState&&);
+
+    drmModeModeInfo mode = {};
+    scoped_refptr<DrmFramebuffer> modeset_framebuffer;
+
+    CrtcProperties properties = {};
+
+    // Cached blobs for the properties since the CRTC properties are applied on
+    // the next page flip and we need to keep the properties valid until then.
+    ScopedDrmPropertyBlob ctm_blob;
+    ScopedDrmPropertyBlob gamma_lut_blob;
+    ScopedDrmPropertyBlob degamma_lut_blob;
+  };
+
   explicit HardwareDisplayPlaneManager(DrmDevice* drm);
   virtual ~HardwareDisplayPlaneManager();
 
@@ -63,14 +118,13 @@ class HardwareDisplayPlaneManager {
   // or crtcs found.
   bool Initialize();
 
-  // Performs modesetting, either atomic or legacy, depending on the device.
-  virtual bool Modeset(uint32_t crtc_id,
-                       uint32_t framebuffer_id,
-                       uint32_t connector_id,
-                       const drmModeModeInfo& mode,
-                       const HardwareDisplayPlaneList& plane_list) = 0;
-
-  virtual bool DisableModeset(uint32_t crtc_id, uint32_t connector) = 0;
+  // |commit_state| contains all the necessary information to build the
+  // atomic/legacy request. It acts as a thin wrapper that looks like the atomic
+  // request. It then gets converted into an atomic request for DRM atomic and
+  // has all the parameters for a legacy request.
+  // TODO(markyacoub): Consolidate this Commit() with the overloaded page flip
+  // Commit() down below.
+  virtual bool Commit(CommitRequest commit_request, uint32_t flags) = 0;
 
   // Clears old frame state out. Must be called before any AssignOverlayPlanes
   // calls.
@@ -146,45 +200,25 @@ class HardwareDisplayPlaneManager {
   // called whenever a DRM hotplug event is received via UDEV.
   void ResetConnectorsCache(const ScopedDrmResourcesPtr& resources);
 
+  // Get Immutable CRTC State.
+  const CrtcState& GetCrtcStateForCrtcId(uint32_t crtc_id);
+
+  // TODO(markyacoub): this seems hacky, this could be cleaned up a bit. Clarify
+  // which resources needed to be tracked internally in
+  // HardwareDisplayPlaneManager and which should be taken care of by the
+  // caller.
+  void ResetModesetBufferOfCrtc(uint32_t crtc_id);
+
  protected:
   struct ConnectorProperties {
     uint32_t id;
     DrmDevice::Property crtc_id;
   };
 
-  struct CrtcProperties {
-    // Unique identifier for the CRTC. This must be greater than 0 to be valid.
-    uint32_t id;
-    DrmDevice::Property active;
-    DrmDevice::Property mode_id;
-    // Optional properties.
-    DrmDevice::Property ctm;
-    DrmDevice::Property gamma_lut;
-    DrmDevice::Property gamma_lut_size;
-    DrmDevice::Property degamma_lut;
-    DrmDevice::Property degamma_lut_size;
-    DrmDevice::Property out_fence_ptr;
-    DrmDevice::Property background_color;
-  };
-
-  struct CrtcState {
-    CrtcState();
-    ~CrtcState();
-    CrtcState(CrtcState&&);
-
-    CrtcProperties properties = {};
-
-    // Cached blobs for the properties since the CRTC properties are applied on
-    // the next page flip and we need to keep the properties valid until then.
-    ScopedDrmPropertyBlob ctm_blob;
-    ScopedDrmPropertyBlob gamma_lut_blob;
-    ScopedDrmPropertyBlob degamma_lut_blob;
-
-    DISALLOW_COPY_AND_ASSIGN(CrtcState);
-  };
-
   bool InitializeCrtcState();
 
+  void UpdateCrtcStateAfterModeset(const CommitRequest& commit_request);
+
   // As the CRTC is being initialized, all connectors connected to it should
   // be disabled. This is a workaround for a bug on Hatch where Puff enables
   // a connector in dev mode before Chrome even starts. The kernel maps the HW
@@ -217,6 +251,9 @@ class HardwareDisplayPlaneManager {
   int LookupCrtcIndex(uint32_t crtc_id) const;
   int LookupConnectorIndex(uint32_t connector_idx) const;
 
+  // Get Mutable CRTC State.
+  CrtcState& CrtcStateForCrtcId(uint32_t crtc_id);
+
   // Returns true if |plane| can support |overlay| and compatible with
   // |crtc_index|.
   virtual bool IsCompatible(HardwareDisplayPlane* plane,

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

@@ -19,12 +19,8 @@ class HardwareDisplayPlaneManagerAtomic : public HardwareDisplayPlaneManager {
   ~HardwareDisplayPlaneManagerAtomic() override;
 
   // HardwareDisplayPlaneManager:
-  bool Modeset(uint32_t crtc_id,
-               uint32_t framebuffer_id,
-               uint32_t connector_id,
-               const drmModeModeInfo& mode,
-               const HardwareDisplayPlaneList& plane_list) override;
-  bool DisableModeset(uint32_t crtc_id, uint32_t connector) override;
+  bool Commit(CommitRequest commit_request, uint32_t flags) override;
+
   bool Commit(HardwareDisplayPlaneList* plane_list,
               bool should_modeset,
               scoped_refptr<PageFlipRequest> page_flip_request,
@@ -50,10 +46,26 @@ class HardwareDisplayPlaneManagerAtomic : public HardwareDisplayPlaneManager {
  private:
   bool InitializePlanes() override;
   std::unique_ptr<HardwareDisplayPlane> CreatePlane(uint32_t plane_id) override;
+  void SetAtomicPropsForCommit(HardwareDisplayPlaneList* plane_list,
+                               drmModeAtomicReq* atomic_request,
+                               bool test_only);
+
+  bool SetCrtcProps(drmModeAtomicReq* atomic_request,
+                    uint32_t crtc_id,
+                    bool set_active,
+                    uint32_t mode_id);
+  bool SetConnectorProps(drmModeAtomicReq* atomic_request,
+                         uint32_t connector_id,
+                         uint32_t crtc_id);
+  bool SetPlaneProps(drmModeAtomicReq* atomic_request,
+                     const DrmOverlayPlane* const primary,
+                     HardwareDisplayPlaneList* plane_list,
+                     uint32_t crtc_id);
+
   bool CommitColorMatrix(const CrtcProperties& crtc_props) override;
   bool CommitGammaCorrection(const CrtcProperties& crtc_props) override;
   bool AddOutFencePtrProperties(
-      drmModeAtomicReqPtr property_set,
+      drmModeAtomicReq* property_set,
       const std::vector<uint32_t>& crtcs,
       std::vector<base::ScopedFD>* out_fence_fds,
       std::vector<base::ScopedFD::Receiver>* out_fence_fd_receivers);

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

@@ -46,19 +46,28 @@ HardwareDisplayPlaneManagerLegacy::HardwareDisplayPlaneManagerLegacy(
 HardwareDisplayPlaneManagerLegacy::~HardwareDisplayPlaneManagerLegacy() =
     default;
 
-bool HardwareDisplayPlaneManagerLegacy::Modeset(
-    uint32_t crtc_id,
-    uint32_t framebuffer_id,
-    uint32_t connector_id,
-    const drmModeModeInfo& mode,
-    const HardwareDisplayPlaneList&) {
-  return drm_->SetCrtc(crtc_id, framebuffer_id,
-                       std::vector<uint32_t>(1, connector_id), mode);
-}
+bool HardwareDisplayPlaneManagerLegacy::Commit(CommitRequest commit_request,
+                                               uint32_t flags) {
+  bool status = true;
+
+  for (const auto& crtc_request : commit_request.commit_state) {
+    bool should_enable = crtc_request.primary_plane;
+
+    if (should_enable) {
+      status &= drm_->SetCrtc(
+          crtc_request.crtc_id,
+          crtc_request.primary_plane->buffer->opaque_framebuffer_id(),
+          std::vector<uint32_t>(1, crtc_request.connector_id),
+          crtc_request.mode);
+    } else {
+      drm_->DisableCrtc(crtc_request.crtc_id);
+    }
+  }
+
+  if (status)
+    UpdateCrtcStateAfterModeset(commit_request);
 
-bool HardwareDisplayPlaneManagerLegacy::DisableModeset(uint32_t crtc_id,
-                                                       uint32_t connector) {
-  return drm_->DisableCrtc(crtc_id);
+  return status;
 }
 
 bool HardwareDisplayPlaneManagerLegacy::Commit(

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

@@ -19,12 +19,8 @@ class HardwareDisplayPlaneManagerLegacy : public HardwareDisplayPlaneManager {
   ~HardwareDisplayPlaneManagerLegacy() override;
 
   // HardwareDisplayPlaneManager:
-  bool Modeset(uint32_t crtc_id,
-               uint32_t framebuffer_id,
-               uint32_t connector_id,
-               const drmModeModeInfo& mode,
-               const HardwareDisplayPlaneList& plane_list) override;
-  bool DisableModeset(uint32_t crtc_id, uint32_t connector) override;
+  bool Commit(CommitRequest commit_request, uint32_t flags) override;
+
   bool Commit(HardwareDisplayPlaneList* plane_list,
               bool should_modeset,
               scoped_refptr<PageFlipRequest> page_flip_request,

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

@@ -276,13 +276,23 @@ TEST_P(HardwareDisplayPlaneManagerTest, ResettingConnectorCache) {
                              plane_properties_, property_names_,
                              /*use_atomic=*/true);
 
-  constexpr uint32_t kFrameBuffer = 2;
   ui::HardwareDisplayPlaneList state;
-  // Check all 3 connectors exist
-  for (size_t i = 0; i < connector_and_crtc_count; ++i) {
-    EXPECT_TRUE(fake_drm_->plane_manager()->Modeset(
-        crtc_properties_[i].id, kFrameBuffer, connector_properties[i].id,
-        kDefaultMode, state));
+
+  {
+    ui::DrmOverlayPlane primary_plane(fake_buffer_, nullptr);
+
+    ui::CommitRequest commit_request;
+    commit_request.plane_list = &state;
+    fake_drm_->plane_manager()->BeginFrame(&state);
+    // Check all 3 connectors exist
+    for (size_t i = 0; i < connector_and_crtc_count; ++i) {
+      ui::CrtcRequest crtc_request{crtc_properties_[i].id,
+                                   connector_properties[i].id, &primary_plane,
+                                   kDefaultMode};
+      commit_request.commit_state.push_back(std::move(crtc_request));
+    }
+    EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+        std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
   }
 
   // Replace last connector and update state.
@@ -291,15 +301,23 @@ TEST_P(HardwareDisplayPlaneManagerTest, ResettingConnectorCache) {
                          plane_properties_, property_names_);
   fake_drm_->plane_manager()->ResetConnectorsCache(fake_drm_->GetResources());
 
-  EXPECT_TRUE(fake_drm_->plane_manager()->Modeset(
-      crtc_properties_[0].id, kFrameBuffer, kConnectorIdBase, kDefaultMode,
-      state));
-  EXPECT_TRUE(fake_drm_->plane_manager()->Modeset(
-      crtc_properties_[1].id, kFrameBuffer, kConnectorIdBase + 1, kDefaultMode,
-      state));
-  EXPECT_TRUE(fake_drm_->plane_manager()->Modeset(
-      crtc_properties_[2].id, kFrameBuffer, kConnectorIdBase + 3, kDefaultMode,
-      state));
+  {
+    ui::DrmOverlayPlane primary_plane(fake_buffer_, nullptr);
+    ui::CommitRequest commit_request;
+    commit_request.plane_list = &state;
+    fake_drm_->plane_manager()->BeginFrame(&state);
+    commit_request.commit_state.push_back({crtc_properties_[0].id,
+                                           kConnectorIdBase, &primary_plane,
+                                           kDefaultMode});
+    commit_request.commit_state.push_back({crtc_properties_[1].id,
+                                           kConnectorIdBase + 1, &primary_plane,
+                                           kDefaultMode});
+    commit_request.commit_state.push_back({crtc_properties_[2].id,
+                                           kConnectorIdBase + 3, &primary_plane,
+                                           kDefaultMode});
+    EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+        std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
+  }
 }
 
 TEST_P(HardwareDisplayPlaneManagerLegacyTest, Modeset) {
@@ -309,12 +327,18 @@ TEST_P(HardwareDisplayPlaneManagerLegacyTest, Modeset) {
                              /*use_atomic=*/false);
   fake_drm_->set_set_crtc_expectation(false);
 
-  constexpr uint32_t kFrameBuffer = 2;
   ui::HardwareDisplayPlaneList state;
-  EXPECT_FALSE(fake_drm_->plane_manager()->Modeset(
-      crtc_properties_[0].id, kFrameBuffer, connector_properties_[0].id,
-      kDefaultMode, state));
-  EXPECT_EQ(kFrameBuffer, fake_drm_->current_framebuffer());
+  ui::DrmOverlayPlane primary_plane(fake_buffer_, nullptr);
+  ui::CommitRequest commit_request;
+  commit_request.plane_list = &state;
+  commit_request.commit_state.push_back({crtc_properties_[0].id,
+                                         connector_properties_[0].id,
+                                         &primary_plane, kDefaultMode});
+  EXPECT_FALSE(fake_drm_->plane_manager()->Commit(
+      std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
+
+  EXPECT_EQ(primary_plane.buffer->framebuffer_id(),
+            fake_drm_->current_framebuffer());
   EXPECT_EQ(1, fake_drm_->get_set_crtc_call_count());
 }
 
@@ -324,8 +348,13 @@ TEST_P(HardwareDisplayPlaneManagerLegacyTest, DisableModeset) {
                              plane_properties_, property_names_,
                              /*use_atomic*/ false);
 
-  EXPECT_TRUE(
-      fake_drm_->plane_manager()->DisableModeset(crtc_properties_[0].id, 0));
+  ui::HardwareDisplayPlaneList state;
+  ui::CommitRequest commit_request;
+  commit_request.plane_list = &state;
+  commit_request.commit_state.push_back(
+      {crtc_properties_[0].id, connector_properties_[0].id, nullptr, {}});
+  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+      std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
 }
 
 TEST_P(HardwareDisplayPlaneManagerLegacyTest, SinglePlaneAssignment) {
@@ -448,11 +477,15 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, Modeset) {
                              plane_properties_, property_names_,
                              /*use_atomic=*/true);
 
-  constexpr uint32_t kFrameBuffer = 2;
   ui::HardwareDisplayPlaneList state;
-  EXPECT_TRUE(fake_drm_->plane_manager()->Modeset(
-      crtc_properties_[0].id, kFrameBuffer, connector_properties_[0].id,
-      kDefaultMode, state));
+  ui::DrmOverlayPlane primary_plane(fake_buffer_, nullptr);
+  ui::CommitRequest commit_request;
+  commit_request.plane_list = &state;
+  commit_request.commit_state.push_back({crtc_properties_[0].id,
+                                         connector_properties_[0].id,
+                                         &primary_plane, kDefaultMode});
+  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+      std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
 
   EXPECT_EQ(1, fake_drm_->get_commit_count());
 }
@@ -463,9 +496,15 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, DisableModeset) {
                              plane_properties_, property_names_,
                              /*use_atomic*/ true);
 
-  EXPECT_TRUE(fake_drm_->plane_manager()->DisableModeset(
-      crtc_properties_[0].id, connector_properties_[0].id));
-  EXPECT_EQ(1, fake_drm_->get_commit_count());
+  ui::HardwareDisplayPlaneList state;
+  ui::CommitRequest commit_request;
+  commit_request.plane_list = &state;
+  commit_request.commit_state.push_back(
+      {crtc_properties_[0].id, connector_properties_[0].id, nullptr, {}});
+  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+      std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
+
+  EXPECT_EQ(2, fake_drm_->get_commit_count());
 }
 
 TEST_P(HardwareDisplayPlaneManagerAtomicTest, CheckPropsAfterModeset) {
@@ -474,11 +513,15 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, CheckPropsAfterModeset) {
                              plane_properties_, property_names_,
                              /*use_atomic=*/true);
 
-  constexpr uint32_t kFrameBuffer = 2;
   ui::HardwareDisplayPlaneList state;
-  EXPECT_TRUE(fake_drm_->plane_manager()->Modeset(
-      crtc_properties_[0].id, kFrameBuffer, connector_properties_[0].id,
-      kDefaultMode, state));
+  ui::DrmOverlayPlane primary_plane(fake_buffer_, nullptr);
+  ui::CommitRequest commit_request;
+  commit_request.plane_list = &state;
+  commit_request.commit_state.push_back({crtc_properties_[0].id,
+                                         connector_properties_[0].id,
+                                         &primary_plane, kDefaultMode});
+  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+      std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
 
   // Test props values after modesetting.
   ui::DrmDevice::Property connector_prop_crtc_id;
@@ -508,15 +551,23 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, CheckPropsAfterDisable) {
                              plane_properties_, property_names_,
                              /*use_atomic=*/true);
 
-  constexpr uint32_t kFrameBuffer = 2;
   ui::HardwareDisplayPlaneList state;
-  EXPECT_TRUE(fake_drm_->plane_manager()->Modeset(
-      crtc_properties_[0].id, kFrameBuffer, connector_properties_[0].id,
-      kDefaultMode, state));
+  ui::DrmOverlayPlane primary_plane(fake_buffer_, nullptr);
+  ui::CommitRequest commit_request;
+  commit_request.plane_list = &state;
+  commit_request.commit_state.push_back({crtc_properties_[0].id,
+                                         connector_properties_[0].id,
+                                         &primary_plane, kDefaultMode});
+  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+      std::move(commit_request), DRM_MODE_ATOMIC_ALLOW_MODESET));
 
-  //  Test props values after disabling.
-  EXPECT_TRUE(fake_drm_->plane_manager()->DisableModeset(
-      crtc_properties_[0].id, connector_properties_[0].id));
+  // Test props values after disabling.
+  ui::CommitRequest commit_request_2;
+  commit_request_2.plane_list = &state;
+  commit_request_2.commit_state.push_back(
+      {crtc_properties_[0].id, connector_properties_[0].id, nullptr, {}});
+  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+      std::move(commit_request_2), DRM_MODE_ATOMIC_ALLOW_MODESET));
 
   ui::DrmDevice::Property crtc_prop_for_name;
   ui::ScopedDrmObjectPropertyPtr crtc_props =