drm_hwcomposer: Rework autofd
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>
diff --git a/.ci/.gitlab-ci-clang-tidy-fine.sh b/.ci/.gitlab-ci-clang-tidy-fine.sh
index c23bf0a..c1805e8 100755
--- a/.ci/.gitlab-ci-clang-tidy-fine.sh
+++ b/.ci/.gitlab-ci-clang-tidy-fine.sh
@@ -4,6 +4,7 @@
TIDY_FILES=(
drm/DrmFbImporter.h
+utils/UniqueFd.h
utils/log.h
utils/properties.h
)
diff --git a/DrmHwcTwo.cpp b/DrmHwcTwo.cpp
index 4a7a416..4db60cc 100644
--- a/DrmHwcTwo.cpp
+++ b/DrmHwcTwo.cpp
@@ -19,9 +19,11 @@
#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 @@
}
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 @@
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::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 @@
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_;
diff --git a/DrmHwcTwo.h b/DrmHwcTwo.h
index b4e1ce8..748fc81 100644
--- a/DrmHwcTwo.h
+++ b/DrmHwcTwo.h
@@ -82,27 +82,6 @@
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 @@
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 @@
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 @@
}
private:
- void AddFenceToPresentFence(int fd);
+ void AddFenceToPresentFence(UniqueFd fd);
constexpr static size_t MATRIX_SIZE = 16;
diff --git a/compositor/DrmDisplayComposition.h b/compositor/DrmDisplayComposition.h
index 27b34f2..1738630 100644
--- a/compositor/DrmDisplayComposition.h
+++ b/compositor/DrmDisplayComposition.h
@@ -127,16 +127,10 @@
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 @@
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_;
diff --git a/compositor/DrmDisplayCompositor.cpp b/compositor/DrmDisplayCompositor.cpp
index 4e7fe0d..a1fe50f 100644
--- a/compositor/DrmDisplayCompositor.cpp
+++ b/compositor/DrmDisplayCompositor.cpp
@@ -291,7 +291,7 @@
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 @@
}
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;
diff --git a/compositor/DrmDisplayCompositor.h b/compositor/DrmDisplayCompositor.h
index aebb6ff..c0eed0c 100644
--- a/compositor/DrmDisplayCompositor.h
+++ b/compositor/DrmDisplayCompositor.h
@@ -72,10 +72,11 @@
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;
diff --git a/drm/DrmDevice.cpp b/drm/DrmDevice.cpp
index d9198d4..abc8edc 100644
--- a/drm/DrmDevice.cpp
+++ b/drm/DrmDevice.cpp
@@ -122,7 +122,7 @@
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 @@
}
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";
}
diff --git a/drm/DrmDevice.h b/drm/DrmDevice.h
index c92004b..dfca263 100644
--- a/drm/DrmDevice.h
+++ b/drm/DrmDevice.h
@@ -28,6 +28,7 @@
#include "DrmEventListener.h"
#include "DrmFbImporter.h"
#include "DrmPlane.h"
+#include "utils/UniqueFd.h"
namespace android {
@@ -42,7 +43,7 @@
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 {
diff --git a/drm/DrmEventListener.cpp b/drm/DrmEventListener.cpp
index a6eee47..b303653 100644
--- a/drm/DrmEventListener.cpp
+++ b/drm/DrmEventListener.cpp
@@ -39,11 +39,11 @@
}
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 @@
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 @@
// 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 @@
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 @@
drmHandleEvent(drm_->fd(), &event_context);
}
- if (FD_ISSET(uevent_fd_.get(), &fds_))
+ if (FD_ISSET(uevent_fd_.Get(), &fds_))
UEventHandler();
}
} // namespace android
diff --git a/drm/DrmEventListener.h b/drm/DrmEventListener.h
index c880a8c..f1f7310 100644
--- a/drm/DrmEventListener.h
+++ b/drm/DrmEventListener.h
@@ -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
index 9af6c22..0000000
--- a/include/autofd.h
+++ /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
diff --git a/include/drmhwcomposer.h b/include/drmhwcomposer.h
index 6955306..22af12b 100644
--- a/include/drmhwcomposer.h
+++ b/include/drmhwcomposer.h
@@ -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 @@
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
index 0000000..1a390ba
--- /dev/null
+++ b/utils/UniqueFd.h
@@ -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