drm_hwcomposer: Cleanup DRM atomic commit

Create and use DrmPlane::AtomicSet() wrapper.

Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>

diff --git a/compositor/DrmDisplayCompositor.cpp b/compositor/DrmDisplayCompositor.cpp
index 142f574..bcb90cf 100644 (file)
--- a/compositor/DrmDisplayCompositor.cpp
+++ b/compositor/DrmDisplayCompositor.cpp
@@ -177,13 +177,9 @@ int DrmDisplayCompositor::DisablePlanes(DrmDisplayComposition *display_comp) {
                                                       ->composition_planes();
   for (DrmCompositionPlane &comp_plane : comp_planes) {
     DrmPlane *plane = comp_plane.plane();
-    ret = drmModeAtomicAddProperty(pset.get(), plane->id(),
-                                   plane->crtc_property().id(), 0) < 0 ||
-          drmModeAtomicAddProperty(pset.get(), plane->id(),
-                                   plane->fb_property().id(), 0) < 0;
-    if (ret) {
-      ALOGE("Failed to add plane %d disable to pset", plane->id());
-      return ret;
+    if (!plane->crtc_property().AtomicSet(*pset, 0) ||
+        !plane->fb_property().AtomicSet(*pset, 0)) {
+      return -EINVAL;
     }
   }
   DrmDevice *drm = resource_manager_->GetDrmDevice(display_);
@@ -219,40 +215,23 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
     return -ENODEV;
   }
 
-  auto pset_unique = MakeDrmModeAtomicReqUnique();
-  auto *pset = pset_unique.get();
+  auto pset = MakeDrmModeAtomicReqUnique();
   if (!pset) {
     ALOGE("Failed to allocate property set");
     return -ENOMEM;
   }
 
-  if (crtc->out_fence_ptr_property().id() != 0) {
-    ret = drmModeAtomicAddProperty(pset, crtc->id(),
-                                   crtc->out_fence_ptr_property().id(),
-                                   (uint64_t)&out_fences[crtc->pipe()]);
-    if (ret < 0) {
-      ALOGE("Failed to add OUT_FENCE_PTR property to pset: %d", ret);
-      return ret;
-    }
+  if (crtc->out_fence_ptr_property() &&
+      !crtc->out_fence_ptr_property()
+           .AtomicSet(*pset, (uint64_t)&out_fences[crtc->pipe()])) {
+    return -EINVAL;
   }
 
-  if (mode_.needs_modeset) {
-    ret = drmModeAtomicAddProperty(pset, crtc->id(),
-                                   crtc->active_property().id(), 1);
-    if (ret < 0) {
-      ALOGE("Failed to add crtc active to pset\n");
-      return ret;
-    }
-
-    ret = drmModeAtomicAddProperty(pset, crtc->id(), crtc->mode_property().id(),
-                                   mode_.blob_id) < 0 ||
-          drmModeAtomicAddProperty(pset, connector->id(),
-                                   connector->crtc_id_property().id(),
-                                   crtc->id()) < 0;
-    if (ret) {
-      ALOGE("Failed to add blob %d to pset", mode_.blob_id);
-      return ret;
-    }
+  if (mode_.needs_modeset &&
+      (!crtc->active_property().AtomicSet(*pset, 1) ||
+       !crtc->mode_property().AtomicSet(*pset, mode_.blob_id) ||
+       !connector->crtc_id_property().AtomicSet(*pset, crtc->id()))) {
+    return -EINVAL;
   }
 
   for (DrmCompositionPlane &comp_plane : comp_planes) {
@@ -292,7 +271,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
       source_crop = layer.source_crop;
       alpha = layer.alpha;
 
-      if (plane->blend_property().id()) {
+      if (plane->blend_property()) {
         switch (layer.blending) {
           case DrmHwcBlending::kPreMult:
             std::tie(blend, ret) = plane->blend_property().GetEnumValueWithName(
@@ -310,19 +289,14 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
         }
       }
 
-      if (plane->zpos_property().id() &&
-          !plane->zpos_property().is_immutable()) {
+      if (plane->zpos_property() && !plane->zpos_property().is_immutable()) {
         uint64_t min_zpos = 0;
 
         // Ignore ret and use min_zpos as 0 by default
         std::tie(std::ignore, min_zpos) = plane->zpos_property().range_min();
 
-        ret = drmModeAtomicAddProperty(pset, plane->id(),
-                                       plane->zpos_property().id(),
-                                       source_layers.front() + min_zpos) < 0;
-        if (ret) {
-          ALOGE("Failed to add zpos property %d to plane %d",
-                plane->zpos_property().id(), plane->id());
+        if (!plane->zpos_property().AtomicSet(*pset, source_layers.front() +
+                                                         min_zpos)) {
           break;
         }
       }
@@ -342,19 +316,12 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
         rotation |= DRM_MODE_ROTATE_0;
 
       if (fence_fd >= 0) {
-        uint32_t prop_id = plane->in_fence_fd_property().id();
-        if (prop_id == 0) {
-          ALOGE("Failed to get IN_FENCE_FD property id");
-          break;
-        }
-        ret = drmModeAtomicAddProperty(pset, plane->id(), prop_id, fence_fd);
-        if (ret < 0) {
-          ALOGE("Failed to add IN_FENCE_FD property to pset: %d", ret);
+        if (!plane->in_fence_fd_property().AtomicSet(*pset, fence_fd)) {
           break;
         }
       }
 
-      if (plane->color_encoding_propery().id()) {
+      if (plane->color_encoding_propery()) {
         switch (layer.dataspace & HAL_DATASPACE_STANDARD_MASK) {
           case HAL_DATASPACE_STANDARD_BT709:
             std::tie(color_encoding,
@@ -378,7 +345,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
         }
       }
 
-      if (plane->color_range_property().id()) {
+      if (plane->color_range_property()) {
         switch (layer.dataspace & HAL_DATASPACE_RANGE_MASK) {
           case HAL_DATASPACE_RANGE_FULL:
             std::tie(color_range,
@@ -396,103 +363,60 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
 
     // Disable the plane if there's no framebuffer
     if (fb_id == UINT32_MAX) {
-      ret = drmModeAtomicAddProperty(pset, plane->id(),
-                                     plane->crtc_property().id(), 0) < 0 ||
-            drmModeAtomicAddProperty(pset, plane->id(),
-                                     plane->fb_property().id(), 0) < 0;
-      if (ret) {
-        ALOGE("Failed to add plane %d disable to pset", plane->id());
+      if (!plane->crtc_property().AtomicSet(*pset, 0) ||
+          !plane->fb_property().AtomicSet(*pset, 0)) {
         break;
       }
       continue;
     }
 
-    ret = drmModeAtomicAddProperty(pset, plane->id(),
-                                   plane->crtc_property().id(), crtc->id()) < 0;
-    ret |= drmModeAtomicAddProperty(pset, plane->id(),
-                                    plane->fb_property().id(), fb_id) < 0;
-    ret |= drmModeAtomicAddProperty(pset, plane->id(),
-                                    plane->crtc_x_property().id(),
-                                    display_frame.left) < 0;
-    ret |= drmModeAtomicAddProperty(pset, plane->id(),
-                                    plane->crtc_y_property().id(),
-                                    display_frame.top) < 0;
-    ret |= drmModeAtomicAddProperty(pset, plane->id(),
-                                    plane->crtc_w_property().id(),
-                                    display_frame.right - display_frame.left) <
-           0;
-    ret |= drmModeAtomicAddProperty(pset, plane->id(),
-                                    plane->crtc_h_property().id(),
-                                    display_frame.bottom - display_frame.top) <
-           0;
-    ret |= drmModeAtomicAddProperty(pset, plane->id(),
-                                    plane->src_x_property().id(),
-                                    (int)(source_crop.left) << 16) < 0;
-    ret |= drmModeAtomicAddProperty(pset, plane->id(),
-                                    plane->src_y_property().id(),
-                                    (int)(source_crop.top) << 16) < 0;
-    ret |= drmModeAtomicAddProperty(pset, plane->id(),
-                                    plane->src_w_property().id(),
-                                    (int)(source_crop.right - source_crop.left)
-                                        << 16) < 0;
-    ret |= drmModeAtomicAddProperty(pset, plane->id(),
-                                    plane->src_h_property().id(),
-                                    (int)(source_crop.bottom - source_crop.top)
-                                        << 16) < 0;
-    if (ret) {
-      ALOGE("Failed to add plane %d to set", plane->id());
+    if (!plane->crtc_property().AtomicSet(*pset, crtc->id()) ||
+        !plane->fb_property().AtomicSet(*pset, fb_id) ||
+        !plane->crtc_x_property().AtomicSet(*pset, display_frame.left) ||
+        !plane->crtc_y_property().AtomicSet(*pset, display_frame.top) ||
+        !plane->crtc_w_property().AtomicSet(*pset, display_frame.right -
+                                                       display_frame.left) ||
+        !plane->crtc_h_property().AtomicSet(*pset, display_frame.bottom -
+                                                       display_frame.top) ||
+        !plane->src_x_property().AtomicSet(*pset, (int)(source_crop.left)
+                                                      << 16) ||
+        !plane->src_y_property().AtomicSet(*pset, (int)(source_crop.top)
+                                                      << 16) ||
+        !plane->src_w_property()
+             .AtomicSet(*pset, (int)(source_crop.right - source_crop.left)
+                                   << 16) ||
+        !plane->src_h_property()
+             .AtomicSet(*pset, (int)(source_crop.bottom - source_crop.top)
+                                   << 16)) {
       break;
     }
 
-    if (plane->rotation_property().id()) {
-      ret = drmModeAtomicAddProperty(pset, plane->id(),
-                                     plane->rotation_property().id(),
-                                     rotation) < 0;
-      if (ret) {
-        ALOGE("Failed to add rotation property %d to plane %d",
-              plane->rotation_property().id(), plane->id());
+    if (plane->rotation_property()) {
+      if (!plane->rotation_property().AtomicSet(*pset, rotation)) {
         break;
       }
     }
 
-    if (plane->alpha_property().id()) {
-      ret = drmModeAtomicAddProperty(pset, plane->id(),
-                                     plane->alpha_property().id(), alpha) < 0;
-      if (ret) {
-        ALOGE("Failed to add alpha property %d to plane %d",
-              plane->alpha_property().id(), plane->id());
+    if (plane->alpha_property()) {
+      if (!plane->alpha_property().AtomicSet(*pset, alpha)) {
         break;
       }
     }
 
-    if (plane->blend_property().id() && blend != UINT64_MAX) {
-      ret = drmModeAtomicAddProperty(pset, plane->id(),
-                                     plane->blend_property().id(), blend) < 0;
-      if (ret) {
-        ALOGE("Failed to add pixel blend mode property %d to plane %d",
-              plane->blend_property().id(), plane->id());
+    if (plane->blend_property() && blend != UINT64_MAX) {
+      if (!plane->blend_property().AtomicSet(*pset, blend)) {
         break;
       }
     }
 
-    if (plane->color_encoding_propery().id() && color_encoding != UINT64_MAX) {
-      ret = drmModeAtomicAddProperty(pset, plane->id(),
-                                     plane->color_encoding_propery().id(),
-                                     color_encoding) < 0;
-      if (ret) {
-        ALOGE("Failed to add COLOR_ENCODING property %d to plane %d",
-              plane->color_encoding_propery().id(), plane->id());
+    if (plane->color_encoding_propery() && color_encoding != UINT64_MAX) {
+      if (!plane->color_encoding_propery().AtomicSet(*pset, color_encoding)) {
         break;
       }
     }
 
-    if (plane->color_range_property().id() && color_range != UINT64_MAX) {
-      ret = drmModeAtomicAddProperty(pset, plane->id(),
-                                     plane->color_range_property().id(),
-                                     color_range) < 0;
-      if (ret) {
-        ALOGE("Failed to add COLOR_RANGE property %d to plane %d",
-              plane->color_range_property().id(), plane->id());
+    if (plane->color_range_property() && color_range != UINT64_MAX) {
+      if (!plane->color_range_property().AtomicSet(*pset, color_range)) {
         break;
       }
     }
@@ -503,7 +427,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
     if (test_only)
       flags |= DRM_MODE_ATOMIC_TEST_ONLY;
 
-    ret = drmModeAtomicCommit(drm->fd(), pset, flags, drm);
+    ret = drmModeAtomicCommit(drm->fd(), pset.get(), flags, drm);
     if (ret) {
       if (!test_only)
         ALOGE("Failed to commit pset ret=%d\n", ret);
@@ -532,7 +456,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
     mode_.needs_modeset = false;
   }
 
-  if (crtc->out_fence_ptr_property().id()) {
+  if (crtc->out_fence_ptr_property()) {
     display_comp->out_fence_ = UniqueFd((int)out_fences[crtc->pipe()]);
   }
 

diff --git a/drm/DrmDevice.cpp b/drm/DrmDevice.cpp
index bd4b1e4..01327f8 100644 (file)
--- a/drm/DrmDevice.cpp
+++ b/drm/DrmDevice.cpp
@@ -547,7 +547,7 @@ int DrmDevice::GetProperty(uint32_t obj_id, uint32_t obj_type,
   for (int i = 0; !found && (size_t)i < props->count_props; ++i) {
     drmModePropertyPtr p = drmModeGetProperty(fd(), props->props[i]);
     if (!strcmp(p->name, prop_name)) {
-      property->Init(p, props->prop_values[i]);
+      property->Init(obj_id, p, props->prop_values[i]);
       found = true;
     }
     drmModeFreeProperty(p);

diff --git a/drm/DrmProperty.cpp b/drm/DrmProperty.cpp
index 8e6f7e5..32f1c62 100644 (file)
--- a/drm/DrmProperty.cpp
+++ b/drm/DrmProperty.cpp
@@ -14,6 +14,9 @@
  * limitations under the License.
  */
 
+// NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
+#define LOG_TAG "hwc-drm-property"
+
 #include "DrmProperty.h"
 
 #include <xf86drmMode.h>
@@ -23,6 +26,7 @@
 #include <string>
 
 #include "DrmDevice.h"
+#include "utils/log.h"
 
 namespace android {
 
@@ -30,11 +34,13 @@ DrmProperty::DrmPropertyEnum::DrmPropertyEnum(drm_mode_property_enum *e)
     : value_(e->value), name_(e->name) {
 }
 
-DrmProperty::DrmProperty(drmModePropertyPtr p, uint64_t value) {
-  Init(p, value);
+DrmProperty::DrmProperty(uint32_t obj_id, drmModePropertyPtr p,
+                         uint64_t value) {
+  Init(obj_id, p, value);
 }
 
-void DrmProperty::Init(drmModePropertyPtr p, uint64_t value) {
+void DrmProperty::Init(uint32_t obj_id, drmModePropertyPtr p, uint64_t value) {
+  obj_id_ = obj_id;
   id_ = p->prop_id;
   flags_ = p->flags;
   name_ = p->name;
@@ -131,4 +137,19 @@ std::tuple<uint64_t, int> DrmProperty::GetEnumValueWithName(
 
   return std::make_tuple(UINT64_MAX, -EINVAL);
 }
+
+auto DrmProperty::AtomicSet(drmModeAtomicReq &pset, uint64_t value) const
+    -> bool {
+  if (id_ == 0) {
+    ALOGE("AtomicSet() is called on non-initialized property!");
+    return false;
+  }
+  if (drmModeAtomicAddProperty(&pset, obj_id_, id_, value) < 0) {
+    ALOGE("Failed to add obj_id: %u, prop_id: %u (%s) to pset", obj_id_, id_,
+          name_.c_str());
+    return false;
+  }
+  return true;
+}
+
 }  // namespace android

diff --git a/drm/DrmProperty.h b/drm/DrmProperty.h
index 70678fd..8db480a 100644 (file)
--- a/drm/DrmProperty.h
+++ b/drm/DrmProperty.h
@@ -37,11 +37,11 @@ enum DrmPropertyType {
 class DrmProperty {
  public:
   DrmProperty() = default;
-  DrmProperty(drmModePropertyPtr p, uint64_t value);
+  DrmProperty(uint32_t obj_id, drmModePropertyPtr p, uint64_t value);
   DrmProperty(const DrmProperty &) = delete;
   DrmProperty &operator=(const DrmProperty &) = delete;
 
-  void Init(drmModePropertyPtr p, uint64_t value);
+  auto Init(uint32_t obj_id, drmModePropertyPtr p, uint64_t value) -> void;
   std::tuple<uint64_t, int> GetEnumValueWithName(const std::string &name) const;
 
   uint32_t id() const;
@@ -54,6 +54,13 @@ class DrmProperty {
   std::tuple<int, uint64_t> range_min() const;
   std::tuple<int, uint64_t> range_max() const;
 
+  [[nodiscard]] auto AtomicSet(drmModeAtomicReq &pset, uint64_t value) const
+      -> bool;
+
+  operator bool() const {
+    return id_ != 0;
+  }
+
  private:
   class DrmPropertyEnum {
    public:
@@ -64,6 +71,7 @@ class DrmProperty {
     std::string name_;
   };
 
+  uint32_t obj_id_ = 0;
   uint32_t id_ = 0;
 
   DrmPropertyType type_ = DRM_PROPERTY_TYPE_INVALID;