Ozone: Upgrade Displays Modeset to use Atomic APIs.

Replace modesetting displays using Legacy APIs to use Atomic Commit APIs
for enabling and disabling screens.
The flow for modeset follows closely the flow for atomic page flip.
Change only apply to devices supporting Atomic APIs. Legacy APIs are
still used by older systems.
Tests are improved to accommodate for props being set on modeset and an
extra commit being called.

(Reland of "Ozone: Implement Atomic Modeset" reverted on crrev.com/c/2152766)

BUG=987274
TEST=Enable and Disable displays, ozone_unittests

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

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

@@ -76,8 +76,11 @@ bool CrtcController::Disable() {
 }
 
 bool CrtcController::AssignOverlayPlanes(HardwareDisplayPlaneList* plane_list,
-                                         const DrmOverlayPlaneList& overlays) {
-  DCHECK(!is_disabled_);
+                                         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_);
 
   const DrmOverlayPlane* primary = DrmOverlayPlane::GetPrimaryPlane(overlays);
   if (primary && !drm_->plane_manager()->ValidatePrimarySize(*primary, mode_)) {

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

@@ -51,7 +51,8 @@ class CrtcController {
   bool Disable();
 
   bool AssignOverlayPlanes(HardwareDisplayPlaneList* plane_list,
-                           const DrmOverlayPlaneList& planes);
+                           const DrmOverlayPlaneList& planes,
+                           bool is_modesetting);
 
   // Returns a vector of format modifiers for the given fourcc format
   // on this CRTCs primary plane. A format modifier describes the

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

@@ -63,6 +63,7 @@ class MockHardwareDisplayPlaneManager : public HardwareDisplayPlaneManager {
     return false;
   }
   bool Commit(HardwareDisplayPlaneList* plane_list,
+              bool should_modeset,
               scoped_refptr<PageFlipRequest> page_flip_request,
               std::unique_ptr<gfx::GpuFence>* out_fence) override {
     return false;

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

@@ -90,10 +90,20 @@ bool HardwareDisplayController::ModesetCrtc(const DrmOverlayPlane& primary,
                                             const drmModeModeInfo& mode) {
   DCHECK(primary.buffer.get());
   bool status = true;
-  for (const auto& controller : crtc_controllers_)
+
+  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_);
+  }
 
   is_disabled_ = false;
   ResetCursor();
@@ -176,12 +186,13 @@ bool HardwareDisplayController::ScheduleOrTestPageFlip(
 
   bool status = true;
   for (const auto& controller : crtc_controllers_) {
-    status &= controller->AssignOverlayPlanes(&owned_hardware_planes_,
-                                              pending_planes);
+    status &= controller->AssignOverlayPlanes(
+        &owned_hardware_planes_, pending_planes, /*is_modesetting=*/false);
   }
 
   status &= GetDrmDevice()->plane_manager()->Commit(
-      &owned_hardware_planes_, page_flip_request, out_fence);
+      &owned_hardware_planes_, /*should_modeset=*/false, page_flip_request,
+      out_fence);
 
   return status;
 }
@@ -357,6 +368,7 @@ void HardwareDisplayController::OnModesetComplete(
   // pending planes to the same values so that the callback keeps the correct
   // state.
   page_flip_request_ = nullptr;
+  owned_hardware_planes_.legacy_page_flips.clear();
   current_planes_.clear();
   current_planes_.push_back(primary.Clone());
   time_of_last_flip_ = base::TimeTicks::Now();

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

@@ -30,11 +30,18 @@ namespace {
 const drmModeModeInfo kDefaultMode = {0, 6, 0, 0, 0, 0, 4,     0,
                                       0, 0, 0, 0, 0, 0, {'\0'}};
 
-constexpr uint32_t kCrtcIdBase = 1;
+constexpr uint32_t kCrtcIdBase = 100;
 constexpr uint32_t kPrimaryCrtc = kCrtcIdBase;
 constexpr uint32_t kSecondaryCrtc = kCrtcIdBase + 1;
-constexpr uint32_t kConnectorIdBase = 10;
-constexpr uint32_t kPlaneOffset = 1000;
+constexpr uint32_t kConnectorIdBase = 200;
+constexpr uint32_t kPlaneOffset = 300;
+constexpr uint32_t kInFormatsBlobPropId = 400;
+
+constexpr uint32_t kActivePropId = 1000;
+constexpr uint32_t kModePropId = 1001;
+constexpr uint32_t kCrtcIdPropId = 2000;
+constexpr uint32_t kTypePropId = 3010;
+constexpr uint32_t kInFormatsPropId = 3011;
 
 const gfx::Size kDefaultModeSize(kDefaultMode.hdisplay, kDefaultMode.vdisplay);
 const gfx::Size kOverlaySize(kDefaultMode.hdisplay / 2,
@@ -104,10 +111,6 @@ void HardwareDisplayControllerTest::TearDown() {
 }
 
 void HardwareDisplayControllerTest::InitializeDrmDevice(bool use_atomic) {
-  constexpr uint32_t kTypePropId = 3010;
-  constexpr uint32_t kInFormatsPropId = 3011;
-  constexpr uint32_t kInFormatsBlobPropId = 400;
-
   std::vector<ui::MockDrmDevice::CrtcProperties> crtc_properties(2);
   std::map<uint32_t, std::string> crtc_property_names = {
       {1000, "ACTIVE"},
@@ -116,7 +119,7 @@ void HardwareDisplayControllerTest::InitializeDrmDevice(bool use_atomic) {
 
   std::vector<ui::MockDrmDevice::ConnectorProperties> connector_properties(2);
   std::map<uint32_t, std::string> connector_property_names = {
-      {2000, "CRTC_ID"},
+      {kCrtcIdPropId, "CRTC_ID"},
   };
   for (size_t i = 0; i < connector_properties.size(); ++i) {
     connector_properties[i].id = kConnectorIdBase + i;
@@ -225,10 +228,78 @@ TEST_F(HardwareDisplayControllerTest, CheckModesettingResult) {
   EXPECT_FALSE(plane.buffer->HasOneRef());
 }
 
+TEST_F(HardwareDisplayControllerTest, CheckModesettingSetsProps) {
+  ui::DrmOverlayPlane plane1(CreateBuffer(), nullptr);
+
+  EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));
+
+  ui::DrmOverlayPlane plane2(CreateBuffer(), nullptr);
+  std::vector<ui::DrmOverlayPlane> planes = {};
+  planes.push_back(plane2.Clone());
+
+  SchedulePageFlip(std::move(planes));
+
+  // Test props values after modesetting.
+  ui::DrmDevice::Property connector_prop_crtc_id = {};
+  ui::ScopedDrmObjectPropertyPtr connector_props =
+      drm_->GetObjectProperties(kConnectorIdBase, DRM_MODE_OBJECT_CONNECTOR);
+  ui::GetDrmPropertyForName(drm_.get(), connector_props.get(), "CRTC_ID",
+                            &connector_prop_crtc_id);
+  EXPECT_EQ(kCrtcIdPropId, connector_prop_crtc_id.id);
+  EXPECT_EQ(kCrtcIdBase, connector_prop_crtc_id.value);
+
+  ui::DrmDevice::Property crtc_prop_for_name = {};
+  ui::ScopedDrmObjectPropertyPtr crtc_props =
+      drm_->GetObjectProperties(kPrimaryCrtc, DRM_MODE_OBJECT_CRTC);
+  GetDrmPropertyForName(drm_.get(), crtc_props.get(), "ACTIVE",
+                        &crtc_prop_for_name);
+  EXPECT_EQ(kActivePropId, crtc_prop_for_name.id);
+  EXPECT_EQ(1U, crtc_prop_for_name.value);
+
+  GetDrmPropertyForName(drm_.get(), crtc_props.get(), "MODE_ID",
+                        &crtc_prop_for_name);
+  EXPECT_EQ(kModePropId, crtc_prop_for_name.id);
+}
+
+TEST_F(HardwareDisplayControllerTest, CheckDisableResetsProps) {
+  ui::DrmOverlayPlane plane1(CreateBuffer(), nullptr);
+
+  EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));
+
+  ui::DrmOverlayPlane plane2(CreateBuffer(), nullptr);
+  std::vector<ui::DrmOverlayPlane> planes = {};
+  planes.push_back(plane2.Clone());
+
+  SchedulePageFlip(std::move(planes));
+
+  // Test props values after disabling.
+  controller_->Disable();
+
+  ui::DrmDevice::Property connector_prop_crtc_id;
+  ui::ScopedDrmObjectPropertyPtr connector_props =
+      drm_->GetObjectProperties(kConnectorIdBase, DRM_MODE_OBJECT_CONNECTOR);
+  ui::GetDrmPropertyForName(drm_.get(), connector_props.get(), "CRTC_ID",
+                            &connector_prop_crtc_id);
+  EXPECT_EQ(0U, connector_prop_crtc_id.value);
+
+  ui::DrmDevice::Property crtc_prop_for_name;
+  ui::ScopedDrmObjectPropertyPtr crtc_props =
+      drm_->GetObjectProperties(kPrimaryCrtc, DRM_MODE_OBJECT_CRTC);
+  GetDrmPropertyForName(drm_.get(), crtc_props.get(), "ACTIVE",
+                        &crtc_prop_for_name);
+  EXPECT_EQ(0U, crtc_prop_for_name.value);
+
+  crtc_props = drm_->GetObjectProperties(kPrimaryCrtc, DRM_MODE_OBJECT_CRTC);
+  GetDrmPropertyForName(drm_.get(), crtc_props.get(), "MODE_ID",
+                        &crtc_prop_for_name);
+  EXPECT_EQ(0U, crtc_prop_for_name.value);
+}
+
 TEST_F(HardwareDisplayControllerTest, CheckStateAfterPageFlip) {
   ui::DrmOverlayPlane plane1(CreateBuffer(), nullptr);
 
   EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));
+  EXPECT_EQ(1, drm_->get_commit_count());
 
   ui::DrmOverlayPlane plane2(CreateBuffer(), nullptr);
   std::vector<ui::DrmOverlayPlane> planes;
@@ -242,13 +313,14 @@ TEST_F(HardwareDisplayControllerTest, CheckStateAfterPageFlip) {
 
   EXPECT_EQ(gfx::SwapResult::SWAP_ACK, last_swap_result_);
   EXPECT_EQ(1, page_flips_);
-  EXPECT_EQ(1, drm_->get_commit_count());
+  EXPECT_EQ(2, drm_->get_commit_count());
   // Verify only the primary display have a valid framebuffer.
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset, "FB_ID"));
   EXPECT_EQ(0u, GetPlanePropertyValue(kPlaneOffset + 1, "FB_ID"));
 }
 
 TEST_F(HardwareDisplayControllerTest, CheckStateIfModesetFails) {
+  InitializeDrmDevice(/* use_atomic */ false);
   drm_->set_set_crtc_expectation(false);
 
   ui::DrmOverlayPlane plane(CreateBuffer(), nullptr);
@@ -256,20 +328,6 @@ TEST_F(HardwareDisplayControllerTest, CheckStateIfModesetFails) {
   EXPECT_FALSE(controller_->Modeset(plane, kDefaultMode));
 }
 
-TEST_F(HardwareDisplayControllerTest, CheckStateIfPageFlipFails) {
-  drm_->set_commit_expectation(false);
-
-  ui::DrmOverlayPlane plane1(CreateBuffer(), nullptr);
-
-  EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));
-
-  ui::DrmOverlayPlane plane2(CreateBuffer(), nullptr);
-  std::vector<ui::DrmOverlayPlane> planes;
-  planes.push_back(plane2.Clone());
-  EXPECT_DEATH_IF_SUPPORTED(SchedulePageFlip(std::move(planes)),
-                            "SchedulePageFlip failed");
-}
-
 TEST_F(HardwareDisplayControllerTest, CheckOverlayPresent) {
   ui::DrmOverlayPlane plane1(CreateBuffer(), nullptr);
   ui::DrmOverlayPlane plane2(
@@ -277,6 +335,7 @@ TEST_F(HardwareDisplayControllerTest, CheckOverlayPresent) {
       gfx::Rect(kOverlaySize), gfx::RectF(kDefaultModeSizeF), true, nullptr);
 
   EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));
+  EXPECT_EQ(1, drm_->get_commit_count());
 
   std::vector<ui::DrmOverlayPlane> planes;
   planes.push_back(plane1.Clone());
@@ -286,7 +345,7 @@ TEST_F(HardwareDisplayControllerTest, CheckOverlayPresent) {
   drm_->RunCallbacks();
   EXPECT_EQ(gfx::SwapResult::SWAP_ACK, last_swap_result_);
   EXPECT_EQ(1, page_flips_);
-  EXPECT_EQ(1, drm_->get_commit_count());
+  EXPECT_EQ(2, drm_->get_commit_count());
   // Verify both planes on the primary display have a valid framebuffer.
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset, "FB_ID"));
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset + 1, "FB_ID"));
@@ -299,13 +358,14 @@ TEST_F(HardwareDisplayControllerTest, CheckOverlayTestMode) {
       gfx::Rect(kOverlaySize), gfx::RectF(kDefaultModeSizeF), true, nullptr);
 
   EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));
+  EXPECT_EQ(1, drm_->get_commit_count());
 
   std::vector<ui::DrmOverlayPlane> planes;
   planes.push_back(plane1.Clone());
   planes.push_back(plane2.Clone());
 
   SchedulePageFlip(ui::DrmOverlayPlane::Clone(planes));
-  EXPECT_EQ(1, drm_->get_commit_count());
+  EXPECT_EQ(2, drm_->get_commit_count());
   // Verify both planes on the primary display have a valid framebuffer.
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset, "FB_ID"));
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset + 1, "FB_ID"));
@@ -315,14 +375,14 @@ TEST_F(HardwareDisplayControllerTest, CheckOverlayTestMode) {
   drm_->RunCallbacks();
   EXPECT_EQ(gfx::SwapResult::SWAP_ACK, last_swap_result_);
   EXPECT_EQ(1, page_flips_);
-  EXPECT_EQ(2, drm_->get_commit_count());
+  EXPECT_EQ(3, drm_->get_commit_count());
 
   // Regular flips should continue on normally.
   SchedulePageFlip(ui::DrmOverlayPlane::Clone(planes));
   drm_->RunCallbacks();
   EXPECT_EQ(gfx::SwapResult::SWAP_ACK, last_swap_result_);
   EXPECT_EQ(2, page_flips_);
-  EXPECT_EQ(3, drm_->get_commit_count());
+  EXPECT_EQ(4, drm_->get_commit_count());
   // Verify both planes on the primary display have a valid framebuffer.
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset, "FB_ID"));
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset + 1, "FB_ID"));
@@ -353,7 +413,7 @@ TEST_F(HardwareDisplayControllerTest, PageflipMirroredControllers) {
 
   ui::DrmOverlayPlane plane1(CreateBuffer(), nullptr);
   EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));
-  EXPECT_EQ(2, drm_->get_set_crtc_call_count());
+  EXPECT_EQ(2, drm_->get_commit_count());
 
   ui::DrmOverlayPlane plane2(CreateBuffer(), nullptr);
   std::vector<ui::DrmOverlayPlane> planes;
@@ -362,7 +422,7 @@ TEST_F(HardwareDisplayControllerTest, PageflipMirroredControllers) {
   drm_->RunCallbacks();
   EXPECT_EQ(gfx::SwapResult::SWAP_ACK, last_swap_result_);
   EXPECT_EQ(1, page_flips_);
-  EXPECT_EQ(1, drm_->get_commit_count());
+  EXPECT_EQ(3, 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"));
@@ -506,10 +566,10 @@ TEST_F(HardwareDisplayControllerTest, ModesetWhilePageFlipping) {
 }
 
 TEST_F(HardwareDisplayControllerTest, FailPageFlipping) {
-  drm_->set_commit_expectation(false);
-
   ui::DrmOverlayPlane plane1(CreateBuffer(), nullptr);
   EXPECT_TRUE(controller_->Modeset(plane1, kDefaultMode));
+
+  drm_->set_commit_expectation(false);
   std::vector<ui::DrmOverlayPlane> planes;
   planes.push_back(plane1.Clone());
   EXPECT_DEATH_IF_SUPPORTED(SchedulePageFlip(std::move(planes)),

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

@@ -97,7 +97,7 @@ class HardwareDisplayPlaneManager {
                                    uint32_t crtc_id);
 
   // Commit the plane states in |plane_list|.
-  //
+  // if |should_modeset| is set, it only modesets without page flipping.
   // If |page_flip_request| is null, this tests the plane configuration without
   // submitting it.
   // The fence returned in |out_fence| will signal when the currently scanned
@@ -105,6 +105,7 @@ class HardwareDisplayPlaneManager {
   // |page_flip_request|. Note that the returned fence may be a nullptr
   // if the system doesn't support out fences.
   virtual bool Commit(HardwareDisplayPlaneList* plane_list,
+                      bool should_modeset,
                       scoped_refptr<PageFlipRequest> page_flip_request,
                       std::unique_ptr<gfx::GpuFence>* out_fence) = 0;
 

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

@@ -66,9 +66,32 @@ bool HardwareDisplayPlaneManagerAtomic::Modeset(
     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);
+    const HardwareDisplayPlaneList& plane_list) {
+  const int connector_idx = LookupConnectorIndex(connector_id);
+  DCHECK_GE(connector_idx, 0);
+  connectors_props_[connector_idx].crtc_id.value = crtc_id;
+  bool res =
+      AddPropertyIfValid(plane_list.atomic_property_set.get(), connector_id,
+                         connectors_props_[connector_idx].crtc_id);
+
+  const int crtc_idx = LookupCrtcIndex(crtc_id);
+  DCHECK_GE(crtc_idx, 0);
+  crtc_state_[crtc_idx].properties.active.value = 1UL;
+  ScopedDrmPropertyBlob mode_blob =
+      drm_->CreatePropertyBlob(&mode, sizeof(mode));
+  crtc_state_[crtc_idx].properties.mode_id.value =
+      mode_blob ? mode_blob->id() : 0;
+
+  res &= AddPropertyIfValid(plane_list.atomic_property_set.get(), crtc_id,
+                            crtc_state_[crtc_idx].properties.active);
+  res &= AddPropertyIfValid(plane_list.atomic_property_set.get(), crtc_id,
+                            crtc_state_[crtc_idx].properties.mode_id);
+
+  DCHECK(res);
+  return Commit(const_cast<HardwareDisplayPlaneList*>(&plane_list),
+                /*should_modeset=*/true,
+                /*page_flip_request=*/nullptr,
+                /*out_fence=*/nullptr);
 }
 
 bool HardwareDisplayPlaneManagerAtomic::DisableModeset(uint32_t crtc_id,
@@ -97,12 +120,19 @@ bool HardwareDisplayPlaneManagerAtomic::DisableModeset(uint32_t crtc_id,
 
 bool HardwareDisplayPlaneManagerAtomic::Commit(
     HardwareDisplayPlaneList* plane_list,
+    bool should_modeset,
     scoped_refptr<PageFlipRequest> page_flip_request,
     std::unique_ptr<gfx::GpuFence>* out_fence) {
-  bool test_only = !page_flip_request;
+  bool test_only = !should_modeset && !page_flip_request;
+
   for (HardwareDisplayPlane* plane : plane_list->old_plane_list) {
     if (!base::Contains(plane_list->plane_list, plane)) {
-      // This plane is being released, so we need to zero it.
+      // |plane| is shared state between |old_plane_list| and |plane_list|.
+      // When we call BeginFrame(), we reset in_use since we need to be able to
+      // allocate the planes as needed. The current frame might not need to use
+      // |plane|, thus |plane->in_use()| would be false even though the previous
+      // frame used it. It's existence in |old_plane_list| is sufficient to
+      // signal that |plane| was in use previously.
       plane->set_in_use(false);
       HardwareDisplayPlaneAtomic* atomic_plane =
           static_cast<HardwareDisplayPlaneAtomic*>(plane);
@@ -146,13 +176,6 @@ bool HardwareDisplayPlaneManagerAtomic::Commit(
     plane_list->plane_list.swap(plane_list->old_plane_list);
   }
 
-  uint32_t flags = 0;
-  if (test_only) {
-    flags = DRM_MODE_ATOMIC_TEST_ONLY;
-  } else {
-    flags = DRM_MODE_ATOMIC_NONBLOCK;
-  }
-
   // After we perform the atomic commit, and if the caller has requested an
   // out-fence, the out_fence_fds vector will contain any provided out-fence
   // fds for the crtcs, therefore the scope of out_fence_fds needs to outlive
@@ -172,6 +195,12 @@ bool HardwareDisplayPlaneManagerAtomic::Commit(
       }
     }
 
+    uint32_t flags = 0;
+    if (should_modeset)
+      flags = DRM_MODE_ATOMIC_ALLOW_MODESET;
+    else
+      flags = test_only ? DRM_MODE_ATOMIC_TEST_ONLY : DRM_MODE_ATOMIC_NONBLOCK;
+
     if (!drm_->CommitProperties(plane_list->atomic_property_set.get(), flags,
                                 crtcs.size(), page_flip_request)) {
       if (!test_only) {

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

@@ -26,6 +26,7 @@ class HardwareDisplayPlaneManagerAtomic : public HardwareDisplayPlaneManager {
                const HardwareDisplayPlaneList& plane_list) override;
   bool DisableModeset(uint32_t crtc_id, uint32_t connector) override;
   bool Commit(HardwareDisplayPlaneList* plane_list,
+              bool should_modeset,
               scoped_refptr<PageFlipRequest> page_flip_request,
               std::unique_ptr<gfx::GpuFence>* out_fence) override;
   bool DisableOverlayPlanes(HardwareDisplayPlaneList* plane_list) override;

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

@@ -64,6 +64,7 @@ bool HardwareDisplayPlaneManagerLegacy::DisableModeset(uint32_t crtc_id,
 
 bool HardwareDisplayPlaneManagerLegacy::Commit(
     HardwareDisplayPlaneList* plane_list,
+    bool should_modeset,
     scoped_refptr<PageFlipRequest> page_flip_request,
     std::unique_ptr<gfx::GpuFence>* out_fence) {
   bool test_only = !page_flip_request;

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

@@ -26,6 +26,7 @@ class HardwareDisplayPlaneManagerLegacy : public HardwareDisplayPlaneManager {
                const HardwareDisplayPlaneList& plane_list) override;
   bool DisableModeset(uint32_t crtc_id, uint32_t connector) override;
   bool Commit(HardwareDisplayPlaneList* plane_list,
+              bool should_modeset,
               scoped_refptr<PageFlipRequest> page_flip_request,
               std::unique_ptr<gfx::GpuFence>* out_fence) override;
   bool DisableOverlayPlanes(HardwareDisplayPlaneList* plane_list) override;

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

@@ -37,6 +37,7 @@ constexpr uint32_t kCrtcIdBase = 500;
 constexpr uint32_t kConnectorIdBase = 700;
 
 constexpr uint32_t kActivePropId = 1000;
+constexpr uint32_t kModePropId = 1001;
 constexpr uint32_t kBackgroundColorPropId = 1002;
 constexpr uint32_t kCtmPropId = 1003;
 constexpr uint32_t kGammaLutPropId = 1004;
@@ -121,7 +122,7 @@ void HardwareDisplayPlaneManagerTest::InitializeDrmState(
     size_t planes_per_crtc) {
   std::map<uint32_t, std::string> crtc_property_names = {
       {kActivePropId, "ACTIVE"},
-      {1001, "MODE_ID"},
+      {kModePropId, "MODE_ID"},
   };
 
   std::vector<ui::MockDrmDevice::ConnectorProperties> connector_properties(1);
@@ -226,8 +227,8 @@ void HardwareDisplayPlaneManagerTest::PerformPageFlip(
       state, assigns, crtc_properties_[crtc_idx].id));
   scoped_refptr<ui::PageFlipRequest> page_flip_request =
       base::MakeRefCounted<ui::PageFlipRequest>(base::TimeDelta());
-  ASSERT_TRUE(
-      fake_drm_->plane_manager()->Commit(state, page_flip_request, nullptr));
+  ASSERT_TRUE(fake_drm_->plane_manager()->Commit(
+      state, /*should_modeset*/ false, page_flip_request, nullptr));
 }
 
 uint64_t HardwareDisplayPlaneManagerTest::GetObjectPropertyValue(
@@ -296,8 +297,6 @@ TEST_P(HardwareDisplayPlaneManagerTest, ResettingConnectorCache) {
   EXPECT_TRUE(fake_drm_->plane_manager()->Modeset(
       crtc_properties_[1].id, kFrameBuffer, kConnectorIdBase + 1, kDefaultMode,
       state));
-  // TODO(markyacoub): Add a test that fails for kConnectorIdBase +2 when atomic
-  // modeset is enabled.
   EXPECT_TRUE(fake_drm_->plane_manager()->Modeset(
       crtc_properties_[2].id, kFrameBuffer, kConnectorIdBase + 3, kDefaultMode,
       state));
@@ -455,7 +454,7 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, Modeset) {
       crtc_properties_[0].id, kFrameBuffer, connector_properties_[0].id,
       kDefaultMode, state));
 
-  EXPECT_EQ(0, fake_drm_->get_commit_count());
+  EXPECT_EQ(1, fake_drm_->get_commit_count());
 }
 
 TEST_P(HardwareDisplayPlaneManagerAtomicTest, DisableModeset) {
@@ -469,6 +468,40 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, DisableModeset) {
   EXPECT_EQ(1, fake_drm_->get_commit_count());
 }
 
+TEST_P(HardwareDisplayPlaneManagerAtomicTest, CheckPropsAfterModeset) {
+  InitializeDrmState(/*crtc_count=*/1, /*planes_per_crtc=*/1);
+  fake_drm_->InitializeState(crtc_properties_, connector_properties_,
+                             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));
+
+  // Test props values after modesetting.
+  ui::DrmDevice::Property connector_prop_crtc_id;
+  ui::ScopedDrmObjectPropertyPtr connector_props =
+      fake_drm_->GetObjectProperties(kConnectorIdBase,
+                                     DRM_MODE_OBJECT_CONNECTOR);
+  ui::GetDrmPropertyForName(fake_drm_.get(), connector_props.get(), "CRTC_ID",
+                            &connector_prop_crtc_id);
+  EXPECT_EQ(kCrtcIdPropId, connector_prop_crtc_id.id);
+
+  ui::DrmDevice::Property crtc_prop_for_name;
+  ui::ScopedDrmObjectPropertyPtr crtc_props =
+      fake_drm_->GetObjectProperties(kCrtcIdBase, DRM_MODE_OBJECT_CRTC);
+  ui::GetDrmPropertyForName(fake_drm_.get(), crtc_props.get(), "ACTIVE",
+                            &crtc_prop_for_name);
+  EXPECT_EQ(kActivePropId, crtc_prop_for_name.id);
+  EXPECT_EQ(1U, crtc_prop_for_name.value);
+
+  ui::GetDrmPropertyForName(fake_drm_.get(), crtc_props.get(), "MODE_ID",
+                            &crtc_prop_for_name);
+  EXPECT_EQ(kModePropId, crtc_prop_for_name.id);
+}
+
 TEST_P(HardwareDisplayPlaneManagerAtomicTest, CheckPropsAfterDisable) {
   InitializeDrmState(/*crtc_count=*/1, /*planes_per_crtc=*/1);
   fake_drm_->InitializeState(crtc_properties_, connector_properties_,
@@ -570,8 +603,8 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, UnusedPlanesAreReleased) {
   fake_drm_->plane_manager()->BeginFrame(&hdpl);
   EXPECT_TRUE(fake_drm_->plane_manager()->AssignOverlayPlanes(
       &hdpl, assigns, crtc_properties_[0].id));
-  EXPECT_TRUE(
-      fake_drm_->plane_manager()->Commit(&hdpl, page_flip_request, nullptr));
+  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+      &hdpl, /*should_modeset*/ false, page_flip_request, nullptr));
   assigns.clear();
   assigns.push_back(ui::DrmOverlayPlane(primary_buffer, nullptr));
   fake_drm_->plane_manager()->BeginFrame(&hdpl);
@@ -580,8 +613,8 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, UnusedPlanesAreReleased) {
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset, "FB_ID"));
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset + 1, "FB_ID"));
 
-  EXPECT_TRUE(
-      fake_drm_->plane_manager()->Commit(&hdpl, page_flip_request, nullptr));
+  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+      &hdpl, /*should_modeset*/ false, page_flip_request, nullptr));
   EXPECT_NE(0u, GetPlanePropertyValue(kPlaneOffset, "FB_ID"));
   EXPECT_EQ(0u, GetPlanePropertyValue(kPlaneOffset + 1, "FB_ID"));
 }
@@ -914,8 +947,8 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest,
       base::MakeRefCounted<ui::PageFlipRequest>(base::TimeDelta());
 
   std::unique_ptr<gfx::GpuFence> out_fence;
-  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(&state_, page_flip_request,
-                                                 &out_fence));
+  EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+      &state_, /*should_modeset*/ false, page_flip_request, &out_fence));
   EXPECT_EQ(nullptr, out_fence);
 }
 
@@ -1148,7 +1181,7 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, OverlaySourceCrop) {
 
   {
     ui::DrmOverlayPlaneList assigns;
-    assigns.push_back(ui::DrmOverlayPlane(fake_buffer_, 0));
+    assigns.push_back(ui::DrmOverlayPlane(fake_buffer_, nullptr));
 
     fake_drm_->plane_manager()->BeginFrame(&state_);
     EXPECT_TRUE(fake_drm_->plane_manager()->AssignOverlayPlanes(
@@ -1157,8 +1190,8 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, OverlaySourceCrop) {
     std::unique_ptr<gfx::GpuFence> out_fence;
     scoped_refptr<ui::PageFlipRequest> page_flip_request =
         base::MakeRefCounted<ui::PageFlipRequest>(base::TimeDelta());
-    EXPECT_TRUE(fake_drm_->plane_manager()->Commit(&state_, page_flip_request,
-                                                   &out_fence));
+    EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+        &state_, /*should_modeset*/ false, page_flip_request, &out_fence));
 
     EXPECT_EQ(2u << 16, GetPlanePropertyValue(kPlaneOffset, "SRC_W"));
     EXPECT_EQ(2u << 16, GetPlanePropertyValue(kPlaneOffset, "SRC_H"));
@@ -1178,8 +1211,8 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, OverlaySourceCrop) {
     scoped_refptr<ui::PageFlipRequest> page_flip_request =
         base::MakeRefCounted<ui::PageFlipRequest>(base::TimeDelta());
     std::unique_ptr<gfx::GpuFence> out_fence;
-    EXPECT_TRUE(fake_drm_->plane_manager()->Commit(&state_, page_flip_request,
-                                                   &out_fence));
+    EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+        &state_, /*should_modeset*/ false, page_flip_request, &out_fence));
 
     EXPECT_EQ(1u << 16, GetPlanePropertyValue(kPlaneOffset, "SRC_W"));
     EXPECT_EQ(2u << 16, GetPlanePropertyValue(kPlaneOffset, "SRC_H"));
@@ -1199,8 +1232,8 @@ TEST_P(HardwareDisplayPlaneManagerAtomicTest, OverlaySourceCrop) {
     scoped_refptr<ui::PageFlipRequest> page_flip_request =
         base::MakeRefCounted<ui::PageFlipRequest>(base::TimeDelta());
     std::unique_ptr<gfx::GpuFence> out_fence;
-    EXPECT_TRUE(fake_drm_->plane_manager()->Commit(&state_, page_flip_request,
-                                                   &out_fence));
+    EXPECT_TRUE(fake_drm_->plane_manager()->Commit(
+        &state_, /*should_modeset*/ false, page_flip_request, &out_fence));
 
     EXPECT_EQ(2u << 16, GetPlanePropertyValue(kPlaneOffset, "SRC_W"));
     EXPECT_EQ(1u << 16, GetPlanePropertyValue(kPlaneOffset, "SRC_H"));

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

@@ -430,8 +430,10 @@ bool MockDrmDevice::CommitProperties(
     return false;
 
   for (uint32_t i = 0; i < request->cursor; ++i) {
-    EXPECT_TRUE(ValidatePropertyValue(request->items[i].property_id,
-                                      request->items[i].value));
+    bool res = ValidatePropertyValue(request->items[i].property_id,
+                                     request->items[i].value);
+    if (!res)
+      return false;
   }
 
   if (page_flip_request)
@@ -442,9 +444,11 @@ bool MockDrmDevice::CommitProperties(
 
   // Only update values if not testing.
   for (uint32_t i = 0; i < request->cursor; ++i) {
-    EXPECT_TRUE(UpdateProperty(request->items[i].object_id,
-                               request->items[i].property_id,
-                               request->items[i].value));
+    bool res =
+        UpdateProperty(request->items[i].object_id,
+                       request->items[i].property_id, request->items[i].value);
+    if (!res)
+      return false;
   }
 
   return true;

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

@@ -34,12 +34,21 @@ namespace {
 const drmModeModeInfo kDefaultMode = {0, 6, 0, 0, 0, 0, 4,     0,
                                       0, 0, 0, 0, 0, 0, {'\0'}};
 
-const uint32_t kPrimaryCrtc = 1;
-const uint32_t kPrimaryConnector = 2;
-const uint32_t kPrimaryDisplayId = 3;
-const uint32_t kSecondaryCrtc = 4;
-const uint32_t kSecondaryConnector = 5;
-const uint32_t kSecondaryDisplayId = 6;
+const uint32_t kPrimaryDisplayId = 1;
+const uint32_t kSecondaryDisplayId = 2;
+
+constexpr uint32_t kCrtcIdBase = 100;
+constexpr uint32_t kPrimaryCrtc = kCrtcIdBase;
+constexpr uint32_t kSecondaryCrtc = kCrtcIdBase + 1;
+
+constexpr uint32_t kConnectorIdBase = 200;
+constexpr uint32_t kPrimaryConnector = kConnectorIdBase;
+constexpr uint32_t kSecondaryConnector = kConnectorIdBase + 1;
+constexpr uint32_t kPlaneIdBase = 300;
+constexpr uint32_t kInFormatsBlobPropIdBase = 400;
+
+constexpr uint32_t kTypePropId = 3010;
+constexpr uint32_t kInFormatsPropId = 3011;
 
 drmModeModeInfo ConstructMode(uint16_t hdisplay, uint16_t vdisplay) {
   return {0, hdisplay, 0, 0, 0, 0, vdisplay, 0, 0, 0, 0, 0, 0, 0, {'\0'}};
@@ -49,6 +58,14 @@ drmModeModeInfo ConstructMode(uint16_t hdisplay, uint16_t vdisplay) {
 
 class ScreenManagerTest : public testing::Test {
  public:
+  struct PlaneState {
+    std::vector<uint32_t> formats;
+  };
+
+  struct CrtcState {
+    std::vector<PlaneState> planes;
+  };
+
   ScreenManagerTest() = default;
   ~ScreenManagerTest() override = default;
 
@@ -62,6 +79,95 @@ class ScreenManagerTest : public testing::Test {
                      kDefaultMode.vdisplay);
   }
 
+  void InitializeDrmState(const std::vector<CrtcState>& crtc_states) {
+    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(2);
+    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 = kPrimaryConnector + 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.
+        {kTypePropId, "type"},
+        {kInFormatsPropId, "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 = kPrimaryCrtc + 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 == kTypePropId) {
+            value = plane_idx == 0 ? DRM_PLANE_TYPE_PRIMARY
+                                   : DRM_PLANE_TYPE_OVERLAY;
+          } else if (pair.first == kInFormatsPropId) {
+            value = property_id++;
+            drm_->SetPropertyBlob(ui::MockDrmDevice::AllocateInFormatsBlob(
+                value, crtc_states[crtc_idx].planes[plane_idx].formats,
+                std::vector<drm_format_modifier>()));
+          }
+
+          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,
+                          /* use_atomic= */ true);
+  }
+
   void SetUp() override {
     auto gbm = std::make_unique<ui::MockGbmDevice>();
     drm_ = new ui::MockDrmDevice(std::move(gbm));
@@ -245,6 +351,24 @@ TEST_F(ScreenManagerTest, CheckForControllersInMirroredMode) {
 }
 
 TEST_F(ScreenManagerTest, CheckMirrorModeTransitions) {
+  std::vector<CrtcState> crtc_states = {
+      {
+          /* .planes = */
+          {
+              {/* .formats = */ {DRM_FORMAT_XRGB8888}},
+              {/* .formats = */ {DRM_FORMAT_XRGB8888, DRM_FORMAT_NV12}},
+          },
+      },
+      {
+          /* .planes = */
+          {
+              {/* .formats = */ {DRM_FORMAT_XRGB8888}},
+              {/* .formats = */ {DRM_FORMAT_XRGB8888, DRM_FORMAT_NV12}},
+          },
+      },
+  };
+  InitializeDrmState(crtc_states);
+
   screen_manager_->AddDisplayController(drm_, kPrimaryCrtc, kPrimaryConnector);
   screen_manager_->AddDisplayController(drm_, kSecondaryCrtc,
                                         kSecondaryConnector);
@@ -450,6 +574,24 @@ TEST_F(ScreenManagerTest, ReuseFramebufferIfDisabledThenReEnabled) {
 }
 
 TEST_F(ScreenManagerTest, CheckMirrorModeAfterBeginReEnabled) {
+  std::vector<CrtcState> crtc_states = {
+      {
+          /* .planes = */
+          {
+              {/* .formats = */ {DRM_FORMAT_XRGB8888}},
+              {/* .formats = */ {DRM_FORMAT_XRGB8888, DRM_FORMAT_NV12}},
+          },
+      },
+      {
+          /* .planes = */
+          {
+              {/* .formats = */ {DRM_FORMAT_XRGB8888}},
+              {/* .formats = */ {DRM_FORMAT_XRGB8888, DRM_FORMAT_NV12}},
+          },
+      },
+  };
+  InitializeDrmState(crtc_states);
+
   screen_manager_->AddDisplayController(drm_, kPrimaryCrtc, kPrimaryConnector);
   screen_manager_->AddDisplayController(drm_, kSecondaryCrtc,
                                         kSecondaryConnector);