ui/ozone/..drm/: commit gamma correction on next page flip

DrmDisplay::SetColorSpace() ends up setting the de/gamma ramps instantly
via HardwareDisplayPlaneManager(Atomic)::CommitGammaCorrection(). When
transitioning SDR-HDR or viceversa, this causes a glitch because the
de/gamma ramp is committed separately from the page flip containing the
new content. This CL fixes that by adding a parameter |on_next_commit|
to DrmDisplay::CommitGammaCorrection() and wiring it down to the
appropriate HardwareDisplayPlaneManagerAtomic where, if set, the code
stores the new gamma/degamma ramps and pushes them on the next Commit().

Bug: b:158781255
Change-Id: I89ad16f15d40f1c9674ce78f68400b74d506b00d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264771Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782951}

ui/ozone/platform/drm/BUILD.gn

@@ -9,10 +9,10 @@ import("//ui/ozone/ozone.gni")
 visibility = [ "//ui/ozone/*" ]
 
 declare_args() {
-  # Enables commiting of CRTC properties on page flip events.
-  # This is not enabled by default because of a bug on Intel:
-  # https://crbug.com/854753
-  drm_commit_properties_on_page_flip = false
+  # TODO(crbug.com/1015184, crbug.com/854753): Enable by default once the CTM
+  # setting works as expected, i.e.: without glitches and independently of the
+  # de/gamna tables.
+  drm_commit_ctm_on_page_flip = true
 }
 
 source_set("gbm") {
@@ -171,8 +171,8 @@ source_set("gbm") {
 
   defines = [ "OZONE_IMPLEMENTATION" ]
 
-  if (drm_commit_properties_on_page_flip) {
-    defines += [ "COMMIT_PROPERTIES_ON_PAGE_FLIP" ]
+  if (drm_commit_ctm_on_page_flip) {
+    defines += [ "COMMIT_CTM_ON_PAGE_FLIP" ]
   }
 }
 
@@ -208,7 +208,7 @@ source_set("gbm_unittests") {
     "//ui/ozone/common",
   ]
 
-  if (drm_commit_properties_on_page_flip) {
-    defines = [ "COMMIT_PROPERTIES_ON_PAGE_FLIP" ]
+  if (drm_commit_ctm_on_page_flip) {
+    defines = [ "COMMIT_CTM_ON_PAGE_FLIP" ]
   }
 }

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

@@ -401,4 +401,9 @@ bool HardwareDisplayPlaneManager::InitializeCrtcState() {
   return true;
 }
 
+bool HardwareDisplayPlaneManager::CommitGammaCorrection(
+    const CrtcProperties& crtc_props) {
+  return true;
+}
+
 }  // namespace ui

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

@@ -219,7 +219,9 @@ class HardwareDisplayPlaneManager {
 
   virtual bool CommitColorMatrix(const CrtcProperties& crtc_props) = 0;
 
-  virtual bool CommitGammaCorrection(const CrtcProperties& crtc_props) = 0;
+  // Derived classes hook to commit the Gamma correction properties. By default
+  // it does nothing, and they will be taken on the next Commit() page flip.
+  virtual bool CommitGammaCorrection(const CrtcProperties& crtc_props);
 
   // Object containing the connection to the graphics device and wraps the API
   // calls to control it. Not owned.

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

@@ -124,16 +124,16 @@ bool HardwareDisplayPlaneManagerAtomic::Commit(
   for (uint32_t crtc : crtcs) {
     int idx = LookupCrtcIndex(crtc);
 
-#if defined(COMMIT_PROPERTIES_ON_PAGE_FLIP)
+#if defined(COMMIT_CTM_ON_PAGE_FLIP)
     // Apply all CRTC properties in the page-flip so we don't block the
     // swap chain for a vsync.
     // TODO(dnicoara): See if we can apply these properties async using
     // DRM_MODE_ATOMIC_ASYNC_UPDATE flag when committing.
-    AddPropertyIfValid(request, crtc, crtc_state_[idx].properties.degamma_lut);
-    AddPropertyIfValid(request, crtc, crtc_state_[idx].properties.gamma_lut);
     AddPropertyIfValid(request, crtc, crtc_state_[idx].properties.ctm);
 #endif
 
+    AddPropertyIfValid(request, crtc, crtc_state_[idx].properties.degamma_lut);
+    AddPropertyIfValid(request, crtc, crtc_state_[idx].properties.gamma_lut);
     AddPropertyIfValid(request, crtc,
                        crtc_state_[idx].properties.background_color);
   }
@@ -146,13 +146,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 +165,8 @@ bool HardwareDisplayPlaneManagerAtomic::Commit(
       }
     }
 
+    const uint32_t 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) {
@@ -317,7 +312,7 @@ HardwareDisplayPlaneManagerAtomic::CreatePlane(uint32_t plane_id) {
 bool HardwareDisplayPlaneManagerAtomic::CommitColorMatrix(
     const CrtcProperties& crtc_props) {
   DCHECK(crtc_props.ctm.id);
-#if !defined(COMMIT_PROPERTIES_ON_PAGE_FLIP)
+#if !defined(COMMIT_CTM_ON_PAGE_FLIP)
   ScopedDrmAtomicReqPtr property_set(drmModeAtomicAlloc());
   int ret = drmModeAtomicAddProperty(property_set.get(), crtc_props.id,
                                      crtc_props.ctm.id, crtc_props.ctm.value);
@@ -337,44 +332,6 @@ bool HardwareDisplayPlaneManagerAtomic::CommitColorMatrix(
 #endif
 }
 
-bool HardwareDisplayPlaneManagerAtomic::CommitGammaCorrection(
-    const CrtcProperties& crtc_props) {
-  DCHECK(crtc_props.degamma_lut.id || crtc_props.gamma_lut.id);
-#if !defined(COMMIT_PROPERTIES_ON_PAGE_FLIP)
-  ScopedDrmAtomicReqPtr property_set(drmModeAtomicAlloc());
-  if (crtc_props.degamma_lut.id) {
-    int ret = drmModeAtomicAddProperty(property_set.get(), crtc_props.id,
-                                       crtc_props.degamma_lut.id,
-                                       crtc_props.degamma_lut.value);
-    if (ret < 0) {
-      LOG(ERROR) << "Failed to set DEGAMMA_LUT property for crtc="
-                 << crtc_props.id;
-      return false;
-    }
-  }
-
-  if (crtc_props.gamma_lut.id) {
-    int ret = drmModeAtomicAddProperty(property_set.get(), crtc_props.id,
-                                       crtc_props.gamma_lut.id,
-                                       crtc_props.gamma_lut.value);
-    if (ret < 0) {
-      LOG(ERROR) << "Failed to set GAMMA_LUT property for crtc="
-                 << crtc_props.id;
-      return false;
-    }
-  }
-
-  // If we try to do this in a non-blocking fashion this can return EBUSY since
-  // there is a pending page flip. Do a blocking commit (the same as the legacy
-  // API) to ensure the properties are applied.
-  // TODO(dnicoara): Should cache these values locally and aggregate them with
-  // the page flip event otherwise this "steals" a vsync to apply the property.
-  return drm_->CommitProperties(property_set.get(), 0, 0, nullptr);
-#else
-  return true;
-#endif
-}
-
 bool HardwareDisplayPlaneManagerAtomic::AddOutFencePtrProperties(
     drmModeAtomicReqPtr property_set,
     const std::vector<uint32_t>& crtcs,

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

@@ -50,7 +50,6 @@ class HardwareDisplayPlaneManagerAtomic : public HardwareDisplayPlaneManager {
   bool InitializePlanes() override;
   std::unique_ptr<HardwareDisplayPlane> CreatePlane(uint32_t plane_id) override;
   bool CommitColorMatrix(const CrtcProperties& crtc_props) override;
-  bool CommitGammaCorrection(const CrtcProperties& crtc_props) override;
   bool AddOutFencePtrProperties(
       drmModeAtomicReqPtr property_set,
       const std::vector<uint32_t>& crtcs,

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

@@ -680,7 +680,7 @@ TEST_P(HardwareDisplayPlaneManagerTest, SetColorMatrix_Success) {
   if (use_atomic_) {
     ui::HardwareDisplayPlaneList state;
     PerformPageFlip(/*crtc_idx=*/0, &state);
-#if defined(COMMIT_PROPERTIES_ON_PAGE_FLIP)
+#if defined(COMMIT_CTM_ON_PAGE_FLIP)
     EXPECT_EQ(1, fake_drm_->get_commit_count());
 #else
     EXPECT_EQ(2, fake_drm_->get_commit_count());
@@ -830,11 +830,7 @@ TEST_P(HardwareDisplayPlaneManagerTest, SetGammaCorrection_Success) {
       crtc_properties_[0].id, {}, {}));
   if (use_atomic_) {
     PerformPageFlip(/*crtc_idx=*/0, &state);
-#if defined(COMMIT_PROPERTIES_ON_PAGE_FLIP)
     EXPECT_EQ(1, fake_drm_->get_commit_count());
-#else
-    EXPECT_EQ(2, fake_drm_->get_commit_count());
-#endif
     EXPECT_EQ(0u, GetCrtcPropertyValue(crtc_properties_[0].id, "GAMMA_LUT"));
     EXPECT_EQ(0u, GetCrtcPropertyValue(crtc_properties_[0].id, "DEGAMMA_LUT"));
   } else {
@@ -845,11 +841,7 @@ TEST_P(HardwareDisplayPlaneManagerTest, SetGammaCorrection_Success) {
       crtc_properties_[0].id, {{0, 0, 0}}, {{0, 0, 0}}));
   if (use_atomic_) {
     PerformPageFlip(/*crtc_idx=*/0, &state);
-#if defined(COMMIT_PROPERTIES_ON_PAGE_FLIP)
     EXPECT_EQ(2, fake_drm_->get_commit_count());
-#else
-    EXPECT_EQ(4, fake_drm_->get_commit_count());
-#endif
     EXPECT_NE(0u, GetCrtcPropertyValue(crtc_properties_[0].id, "GAMMA_LUT"));
     EXPECT_NE(0u, GetCrtcPropertyValue(crtc_properties_[0].id, "DEGAMMA_LUT"));
   } else {