OSDN Git Service

drm_hwcomposer: Rework autofd
authorRoman Stratiienko <r.stratiienko@gmail.com>
Sat, 20 Feb 2021 11:59:55 +0000 (13:59 +0200)
committerRoman Stratiienko <r.stratiienko@gmail.com>
Sat, 24 Jul 2021 14:59:09 +0000 (17:59 +0300)
Motivation:

Current implementation of UniqueFd can be used in a different ways,
making analytical tracking of FD lifecycle much harder than it may be.
Keep this part clean is very important, since any wrong code may open
a hard-to-detect runtime bugs and fd leaks, which may accidentally slip
into the production.

Implementation:

1. Combine UniqueFd anf OutputFd into single class.
2. Reduce the API to be minimal and sufficient.
3. Document the API and use cases.
4. Move to utils/UniqueFd.h.
5. dup(fd) was replaced with fcntl(fd, F_DUPFD_CLOEXEC)) to
   address clang-tidy findings. Find more information at [1]

[1]: https://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-dup.html

Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
13 files changed:
.ci/.gitlab-ci-clang-tidy-fine.sh
DrmHwcTwo.cpp
DrmHwcTwo.h
compositor/DrmDisplayComposition.h
compositor/DrmDisplayCompositor.cpp
compositor/DrmDisplayCompositor.h
drm/DrmDevice.cpp
drm/DrmDevice.h
drm/DrmEventListener.cpp
drm/DrmEventListener.h
include/autofd.h [deleted file]
include/drmhwcomposer.h
utils/UniqueFd.h [new file with mode: 0644]

index c23bf0a..c1805e8 100755 (executable)
@@ -4,6 +4,7 @@
 
 TIDY_FILES=(
 drm/DrmFbImporter.h
+utils/UniqueFd.h
 utils/log.h
 utils/properties.h
 )
index 4a7a416..4db60cc 100644 (file)
 
 #include "DrmHwcTwo.h"
 
+#include <fcntl.h>
 #include <hardware/hardware.h>
 #include <hardware/hwcomposer2.h>
 #include <sync/sync.h>
+#include <unistd.h>
 
 #include <cinttypes>
 #include <iostream>
@@ -597,22 +599,22 @@ HWC2::Error DrmHwcTwo::HwcDisplay::GetReleaseFences(uint32_t *num_elements,
     }
 
     layers[num_layers - 1] = l.first;
-    fences[num_layers - 1] = l.second.take_release_fence();
+    fences[num_layers - 1] = l.second.release_fence_.Release();
   }
   *num_elements = num_layers;
   return HWC2::Error::None;
 }
 
-void DrmHwcTwo::HwcDisplay::AddFenceToPresentFence(int fd) {
-  if (fd < 0)
+void DrmHwcTwo::HwcDisplay::AddFenceToPresentFence(UniqueFd fd) {
+  if (!fd) {
     return;
+  }
 
-  if (present_fence_.get() >= 0) {
-    int old_fence = present_fence_.get();
-    present_fence_.Set(sync_merge("dc_present", old_fence, fd));
-    close(fd);
+  if (present_fence_) {
+    present_fence_ = UniqueFd(
+        sync_merge("dc_present", present_fence_.Get(), fd.Get()));
   } else {
-    present_fence_.Set(fd);
+    present_fence_ = std::move(fd);
   }
 }
 
@@ -763,10 +765,10 @@ HWC2::Error DrmHwcTwo::HwcDisplay::SetClientTarget(buffer_handle_t target,
                                                    int32_t dataspace,
                                                    hwc_region_t /*damage*/) {
   supported(__func__);
-  UniqueFd uf(acquire_fence);
 
   client_layer_.set_buffer(target);
-  client_layer_.set_acquire_fence(uf.get());
+  client_layer_.acquire_fence_ = UniqueFd(
+      fcntl(acquire_fence, F_DUPFD_CLOEXEC));
   client_layer_.SetLayerDataspace(dataspace);
 
   /* TODO: Do not update source_crop every call.
@@ -1047,10 +1049,9 @@ HWC2::Error DrmHwcTwo::HwcLayer::SetLayerBlendMode(int32_t mode) {
 HWC2::Error DrmHwcTwo::HwcLayer::SetLayerBuffer(buffer_handle_t buffer,
                                                 int32_t acquire_fence) {
   supported(__func__);
-  UniqueFd uf(acquire_fence);
 
   set_buffer(buffer);
-  set_acquire_fence(uf.get());
+  acquire_fence_ = UniqueFd(fcntl(acquire_fence, F_DUPFD_CLOEXEC));
   return HWC2::Error::None;
 }
 
@@ -1141,11 +1142,9 @@ void DrmHwcTwo::HwcLayer::PopulateDrmLayer(DrmHwcLayer *layer) {
       break;
   }
 
-  OutputFd release_fence = release_fence_output();
-
   layer->sf_handle = buffer_;
-  layer->acquire_fence = acquire_fence_.Release();
-  layer->release_fence = std::move(release_fence);
+  // TODO(rsglobal): Avoid extra fd duplication
+  layer->acquire_fence = UniqueFd(fcntl(acquire_fence_.Get(), F_DUPFD_CLOEXEC));
   layer->display_frame = display_frame_;
   layer->alpha = lround(65535.0F * alpha_);
   layer->source_crop = source_crop_;
index b4e1ce8..748fc81 100644 (file)
@@ -82,27 +82,6 @@ class DrmHwcTwo : public hwc2_device_t {
       buffer_ = buffer;
     }
 
-    int take_acquire_fence() {
-      return acquire_fence_.Release();
-    }
-    void set_acquire_fence(int acquire_fence) {
-      acquire_fence_.Set(dup(acquire_fence));
-    }
-
-    int release_fence() {
-      return release_fence_.get();
-    }
-    int take_release_fence() {
-      return release_fence_.Release();
-    }
-    void manage_release_fence() {
-      release_fence_.Set(release_fence_raw_);
-      release_fence_raw_ = -1;
-    }
-    OutputFd release_fence_output() {
-      return OutputFd(&release_fence_raw_);
-    }
-
     hwc_rect_t display_frame() {
       return display_frame_;
     }
@@ -138,6 +117,10 @@ class DrmHwcTwo : public hwc2_device_t {
     HWC2::Error SetLayerVisibleRegion(hwc_region_t visible);
     HWC2::Error SetLayerZOrder(uint32_t order);
 
+    UniqueFd acquire_fence_;
+
+    UniqueFd release_fence_;
+
    private:
     // sf_type_ stores the initial type given to us by surfaceflinger,
     // validated_type_ stores the type after running ValidateDisplay
@@ -146,9 +129,6 @@ class DrmHwcTwo : public hwc2_device_t {
 
     HWC2::BlendMode blending_ = HWC2::BlendMode::None;
     buffer_handle_t buffer_ = NULL;
-    UniqueFd acquire_fence_;
-    int release_fence_raw_ = -1;
-    UniqueFd release_fence_;
     hwc_rect_t display_frame_;
     float alpha_ = 1.0f;
     hwc_frect_t source_crop_;
@@ -316,7 +296,7 @@ class DrmHwcTwo : public hwc2_device_t {
     }
 
    private:
-    void AddFenceToPresentFence(int fd);
+    void AddFenceToPresentFence(UniqueFd fd);
 
     constexpr static size_t MATRIX_SIZE = 16;
 
index 27b34f2..1738630 100644 (file)
@@ -127,16 +127,10 @@ class DrmDisplayComposition {
     return planner_;
   }
 
-  int take_out_fence() {
-    return out_fence_.Release();
-  }
-
-  void set_out_fence(int out_fence) {
-    out_fence_.Set(out_fence);
-  }
-
   void Dump(std::ostringstream *out) const;
 
+  UniqueFd out_fence_;
+
  private:
   bool validate_composition_type(DrmCompositionType desired);
 
@@ -147,8 +141,6 @@ class DrmDisplayComposition {
   uint32_t dpms_mode_ = DRM_MODE_DPMS_ON;
   DrmMode display_mode_;
 
-  UniqueFd out_fence_ = -1;
-
   bool geometry_changed_ = true;
   std::vector<DrmHwcLayer> layers_;
   std::vector<DrmCompositionPlane> composition_planes_;
index 4e7fe0d..a1fe50f 100644 (file)
@@ -291,7 +291,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
         break;
       }
       fb_id = layer.FbIdHandle->GetFbId();
-      fence_fd = layer.acquire_fence.get();
+      fence_fd = layer.acquire_fence.Get();
       display_frame = layer.display_frame;
       source_crop = layer.source_crop;
       alpha = layer.alpha;
@@ -540,7 +540,7 @@ int DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
   }
 
   if (crtc->out_fence_ptr_property().id()) {
-    display_comp->set_out_fence((int)out_fences[crtc->pipe()]);
+    display_comp->out_fence_ = UniqueFd((int)out_fences[crtc->pipe()]);
   }
 
   return ret;
index aebb6ff..c0eed0c 100644 (file)
@@ -72,10 +72,11 @@ class DrmDisplayCompositor {
   void Dump(std::ostringstream *out) const;
   void Vsync(int display, int64_t timestamp);
   void ClearDisplay();
-  int TakeOutFence() {
-    if (!active_composition_)
-      return -1;
-    return active_composition_->take_out_fence();
+  UniqueFd TakeOutFence() {
+    if (!active_composition_) {
+      return UniqueFd();
+    }
+    return std::move(active_composition_->out_fence_);
   }
 
   FlatteningState GetFlatteningState() const;
index d9198d4..abc8edc 100644 (file)
@@ -122,7 +122,7 @@ DrmDevice::~DrmDevice() {
 
 std::tuple<int, int> DrmDevice::Init(const char *path, int num_displays) {
   /* TODO: Use drmOpenControl here instead */
-  fd_.Set(open(path, O_RDWR | O_CLOEXEC));
+  fd_ = UniqueFd(open(path, O_RDWR | O_CLOEXEC));
   if (fd() < 0) {
     ALOGE("Failed to open dri %s: %s", path, strerror(errno));
     return std::make_tuple(-ENODEV, 0);
@@ -585,9 +585,9 @@ int DrmDevice::GetConnectorProperty(const DrmConnector &connector,
 }
 
 std::string DrmDevice::GetName() const {
-  auto *ver = drmGetVersion(fd_.get());
+  auto *ver = drmGetVersion(fd());
   if (!ver) {
-    ALOGW("Failed to get drm version for fd=%d", fd_.get());
+    ALOGW("Failed to get drm version for fd=%d", fd());
     return "generic";
   }
 
index c92004b..dfca263 100644 (file)
@@ -28,6 +28,7 @@
 #include "DrmEventListener.h"
 #include "DrmFbImporter.h"
 #include "DrmPlane.h"
+#include "utils/UniqueFd.h"
 
 namespace android {
 
@@ -42,7 +43,7 @@ class DrmDevice {
   std::tuple<int, int> Init(const char *path, int num_displays);
 
   int fd() const {
-    return fd_.get();
+    return fd_.Get();
   }
 
   const std::vector<std::unique_ptr<DrmConnector>> &connectors() const {
index a6eee47..b303653 100644 (file)
@@ -39,11 +39,11 @@ DrmEventListener::DrmEventListener(DrmDevice *drm)
 }
 
 int DrmEventListener::Init() {
-  uevent_fd_.Set(
+  uevent_fd_ = UniqueFd(
       socket(PF_NETLINK, SOCK_DGRAM | SOCK_CLOEXEC, NETLINK_KOBJECT_UEVENT));
-  if (uevent_fd_.get() < 0) {
+  if (!uevent_fd_) {
     ALOGE("Failed to open uevent socket: %s", strerror(errno));
-    return uevent_fd_.get();
+    return -errno;
   }
 
   struct sockaddr_nl addr {};
@@ -51,7 +51,7 @@ int DrmEventListener::Init() {
   addr.nl_pid = 0;
   addr.nl_groups = 0xFFFFFFFF;
 
-  int ret = bind(uevent_fd_.get(), (struct sockaddr *)&addr, sizeof(addr));
+  int ret = bind(uevent_fd_.Get(), (struct sockaddr *)&addr, sizeof(addr));
   if (ret) {
     ALOGE("Failed to bind uevent socket: %s", strerror(errno));
     return -errno;
@@ -60,8 +60,8 @@ int DrmEventListener::Init() {
   // NOLINTNEXTLINE(readability-isolate-declaration)
   FD_ZERO(&fds_);
   FD_SET(drm_->fd(), &fds_);
-  FD_SET(uevent_fd_.get(), &fds_);
-  max_fd_ = std::max(drm_->fd(), uevent_fd_.get());
+  FD_SET(uevent_fd_.Get(), &fds_);
+  max_fd_ = std::max(drm_->fd(), uevent_fd_.Get());
 
   return InitWorker();
 }
@@ -96,7 +96,7 @@ void DrmEventListener::UEventHandler() {
     ALOGE("Failed to get monotonic clock on hotplug %d", ret);
 
   while (true) {
-    ret = read(uevent_fd_.get(), &buffer, sizeof(buffer));
+    ret = read(uevent_fd_.Get(), &buffer, sizeof(buffer));
     if (ret == 0)
       return;
 
@@ -139,7 +139,7 @@ void DrmEventListener::Routine() {
     drmHandleEvent(drm_->fd(), &event_context);
   }
 
-  if (FD_ISSET(uevent_fd_.get(), &fds_))
+  if (FD_ISSET(uevent_fd_.Get(), &fds_))
     UEventHandler();
 }
 }  // namespace android
index c880a8c..f1f7310 100644 (file)
@@ -17,7 +17,7 @@
 #ifndef ANDROID_DRM_EVENT_LISTENER_H_
 #define ANDROID_DRM_EVENT_LISTENER_H_
 
-#include "autofd.h"
+#include "utils/UniqueFd.h"
 #include "utils/Worker.h"
 
 namespace android {
diff --git a/include/autofd.h b/include/autofd.h
deleted file mode 100644 (file)
index 9af6c22..0000000
+++ /dev/null
@@ -1,106 +0,0 @@
-/*
- * Copyright (C) 2015 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#ifndef ANDROID_AUTO_FD_H_
-#define ANDROID_AUTO_FD_H_
-
-#include <unistd.h>
-
-namespace android {
-
-class UniqueFd {
- public:
-  UniqueFd() = default;
-  UniqueFd(int fd) : fd_(fd) {
-  }
-  UniqueFd(UniqueFd &&rhs) {
-    fd_ = rhs.fd_;
-    rhs.fd_ = -1;
-  }
-
-  UniqueFd &operator=(UniqueFd &&rhs) {
-    Set(rhs.Release());
-    return *this;
-  }
-
-  ~UniqueFd() {
-    if (fd_ >= 0)
-      close(fd_);
-  }
-
-  int Release() {
-    int old_fd = fd_;
-    fd_ = -1;
-    return old_fd;
-  }
-
-  int Set(int fd) {
-    if (fd_ >= 0)
-      close(fd_);
-    fd_ = fd;
-    return fd_;
-  }
-
-  void Close() {
-    if (fd_ >= 0)
-      close(fd_);
-    fd_ = -1;
-  }
-
-  int get() const {
-    return fd_;
-  }
-
- private:
-  int fd_ = -1;
-};
-
-struct OutputFd {
-  OutputFd() = default;
-  OutputFd(int *fd) : fd_(fd) {
-  }
-  OutputFd(OutputFd &&rhs) {
-    fd_ = rhs.fd_;
-    rhs.fd_ = NULL;
-  }
-
-  OutputFd &operator=(OutputFd &&rhs) {
-    fd_ = rhs.fd_;
-    rhs.fd_ = NULL;
-    return *this;
-  }
-
-  int Set(int fd) {
-    if (*fd_ >= 0)
-      close(*fd_);
-    *fd_ = fd;
-    return fd;
-  }
-
-  int get() {
-    return *fd_;
-  }
-
-  operator bool() const {
-    return fd_ != NULL;
-  }
-
- private:
-  int *fd_ = NULL;
-};
-}  // namespace android
-
-#endif
index 6955306..22af12b 100644 (file)
@@ -24,9 +24,9 @@
 
 #include <vector>
 
-#include "autofd.h"
 #include "drm/DrmFbImporter.h"
 #include "drmhwcgralloc.h"
+#include "utils/UniqueFd.h"
 
 namespace android {
 
@@ -61,7 +61,6 @@ struct DrmHwcLayer {
   android_dataspace_t dataspace;
 
   UniqueFd acquire_fence;
-  OutputFd release_fence;
 
   int ImportBuffer(DrmDevice *drmDevice);
 
diff --git a/utils/UniqueFd.h b/utils/UniqueFd.h
new file mode 100644 (file)
index 0000000..1a390ba
--- /dev/null
@@ -0,0 +1,111 @@
+/*
+ * Copyright (C) 2015, 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef UNIQUEFD_H_
+#define UNIQUEFD_H_
+
+#include <unistd.h>
+
+#include <memory>
+
+namespace android {
+
+/*
+ * Using UniqueFd:
+ * 1. Create UniqueFd object:
+ *    auto fd_obj = UniqueFd(open("SomeFile", xxx));
+ *
+ * 2. Check whether the fd_obj is empty:
+ *    if (!fd_obj) { return -errno; }
+ *
+ * 3. Accessing the file descriptor:
+ *    int ret = read(fd_obj.Get(), buf, buf_size);
+ *
+ * 4. Closing the file:
+ *    FD will be closed once execution leaves fd_obj scope (on any return,
+ *    exception, destruction of class/struct where object is member, etc.).
+ *    User can also force closing the fd_obj by calling:
+ *      fd_obj = UniqueFd();
+ *      // fd is closed and fd_obj is empty now.
+ *
+ * 5. File descriptor may be transferred to the code, which will close it after
+ *    using. This can be done in 2 ways:
+ *    a. Duplicate the fd, in this case both fds should be closed separately:
+ *      int out_fd = dup(fd_obj.Get();
+ *      ...
+ *      close(out_fd);
+ *    b. Transfer ownership, use this method if you do not need the fd anymore.
+ *      int out_fd = fd_obj.Release();
+ *      // fd_obj is empty now.
+ *      ...
+ *      close(out_fd);
+ *
+ * 6. Transferring fd into another UniqueFD object:
+ *    UniqueFd fd_obj_2 = std::move(fd_obj);
+ *    // fd_obj empty now
+ */
+
+constexpr int kEmptyFd = -1;
+
+class UniqueFd {
+ public:
+  UniqueFd() = default;
+  explicit UniqueFd(int fd) : fd_(fd){};
+
+  auto Release [[nodiscard]] () -> int {
+    return std::exchange(fd_, kEmptyFd);
+  }
+
+  auto Get [[nodiscard]] () const -> int {
+    return fd_;
+  }
+
+  explicit operator bool() const {
+    return fd_ != kEmptyFd;
+  }
+
+  ~UniqueFd() {
+    Set(kEmptyFd);
+  }
+
+  /* Allow move semantics */
+  UniqueFd(UniqueFd &&rhs) noexcept {
+    Set(rhs.Release());
+  }
+
+  auto operator=(UniqueFd &&rhs) noexcept -> UniqueFd & {
+    Set(rhs.Release());
+    return *this;
+  }
+
+  /* Disable copy semantics */
+  UniqueFd(const UniqueFd &) = delete;
+  auto operator=(const UniqueFd &) = delete;
+
+ private:
+  void Set(int new_fd) {
+    if (fd_ != kEmptyFd) {
+      close(fd_);
+    }
+    fd_ = new_fd;
+  }
+
+  int fd_ = kEmptyFd;
+};
+
+}  // namespace android
+
+#endif