Move detached buffer off IonBuffer
1/ Migration DetachedBuffer's metadata to use ashmem-based
BufferHubMetadata.
2/ Avoid import the actual gralloc buffer into the IonBuffer. Instead,
just store the native_handle_t of the gralloc buffer in DetachedBuffer.
3/ Replace the usage of BufferDescription/NativeBufferHandle with
BufferTraits/NativeHandleWrapper, as they both depend on
IonBuffer. Currently dvr::ProdcuerBuffer and dvr::ConsumerBuffer
are still using BufferDescription/NativeBufferHandle so that we are
reimplementing them for DetachedBuffer to avoid chaning
dvr::ProdcuerBuffer and dvr::ConsumerBuffer at this point.
Bug: 112940221
Bug: 112011098
Bug: 70048475
Test: atest buffer_hub-test
Change-Id: I435180ba80a27b0ff35f0d95fcdbc23412978e22
diff --git a/libs/vr/libbufferhub/buffer_hub-test.cpp b/libs/vr/libbufferhub/buffer_hub-test.cpp
index cf4a0e9..a77dbf1 100644
--- a/libs/vr/libbufferhub/buffer_hub-test.cpp
+++ b/libs/vr/libbufferhub/buffer_hub-test.cpp
@@ -920,6 +920,7 @@
kUsage, kUserMetadataSize);
int b1_id = b1->id();
EXPECT_TRUE(b1->IsValid());
+ EXPECT_EQ(b1->user_metadata_size(), kUserMetadataSize);
auto status_or_handle = b1->Duplicate();
EXPECT_TRUE(status_or_handle);
@@ -935,6 +936,9 @@
std::unique_ptr<DetachedBuffer> b2 = DetachedBuffer::Import(std::move(h2));
EXPECT_FALSE(h2.valid());
ASSERT_TRUE(b2 != nullptr);
+ EXPECT_TRUE(b2->IsValid());
+ EXPECT_EQ(b2->user_metadata_size(), kUserMetadataSize);
+
int b2_id = b2->id();
// These two buffer instances are based on the same physical buffer under the
diff --git a/libs/vr/libbufferhub/buffer_hub_metadata.cpp b/libs/vr/libbufferhub/buffer_hub_metadata.cpp
index 8243fc5..788c22c 100644
--- a/libs/vr/libbufferhub/buffer_hub_metadata.cpp
+++ b/libs/vr/libbufferhub/buffer_hub_metadata.cpp
@@ -74,6 +74,9 @@
size_t metadata_size = ashmem_get_size_region(ashmem_handle.Get());
size_t user_metadata_size = metadata_size - kMetadataHeaderSize;
+ // Note that here the buffer state is mapped from shared memory as an atomic
+ // object. The std::atomic's constructor will not be called so that the
+ // original value stored in the memory region can be preserved.
auto metadata_header = static_cast<MetadataHeader*>(
mmap(nullptr, metadata_size, kAshmemProt, MAP_SHARED, ashmem_handle.Get(),
/*offset=*/0));
diff --git a/libs/vr/libbufferhub/detached_buffer.cpp b/libs/vr/libbufferhub/detached_buffer.cpp
index 420ce24..795ad19 100644
--- a/libs/vr/libbufferhub/detached_buffer.cpp
+++ b/libs/vr/libbufferhub/detached_buffer.cpp
@@ -16,8 +16,16 @@
namespace android {
namespace dvr {
+namespace {
+
+// TODO(b/112338294): Remove this string literal after refactoring BufferHub
+// to use Binder.
+static constexpr char kBufferHubClientPath[] = "system/buffer_hub/client";
+
+} // namespace
+
BufferHubClient::BufferHubClient()
- : Client(ClientChannelFactory::Create(BufferHubRPC::kClientPath)) {}
+ : Client(ClientChannelFactory::Create(kBufferHubClientPath)) {}
BufferHubClient::BufferHubClient(LocalChannelHandle channel_handle)
: Client(ClientChannel::Create(std::move(channel_handle))) {}
@@ -80,71 +88,53 @@
return -status.error();
}
- BufferDescription<LocalHandle> buffer_desc = status.take();
- if (buffer_desc.id() < 0) {
+ BufferTraits<LocalHandle> buffer_traits = status.take();
+ if (buffer_traits.id() < 0) {
ALOGE("DetachedBuffer::DetachedBuffer: Received an invalid id!");
return -EIO;
}
// Stash the buffer id to replace the value in id_.
- const int buffer_id = buffer_desc.id();
-
- // Import the buffer.
- IonBuffer ion_buffer;
- ALOGD_IF(TRACE, "DetachedBuffer::DetachedBuffer: id=%d.", buffer_id);
-
- if (const int ret = buffer_desc.ImportBuffer(&ion_buffer)) {
- ALOGE("Failed to import GraphicBuffer, error=%d", ret);
- return ret;
- }
+ const int buffer_id = buffer_traits.id();
// Import the metadata.
- IonBuffer metadata_buffer;
- if (const int ret = buffer_desc.ImportMetadata(&metadata_buffer)) {
- ALOGE("Failed to import metadata buffer, error=%d", ret);
- return ret;
+ metadata_ = BufferHubMetadata::Import(buffer_traits.take_metadata_handle());
+
+ if (!metadata_.IsValid()) {
+ ALOGE("DetachedBuffer::ImportGraphicBuffer: invalid metadata.");
+ return -ENOMEM;
}
- size_t metadata_buf_size = metadata_buffer.width();
+
+ if (metadata_.metadata_size() != buffer_traits.metadata_size()) {
+ ALOGE(
+ "DetachedBuffer::ImportGraphicBuffer: metadata buffer too small: "
+ "%zu, expected: %" PRIu64 ".",
+ metadata_.metadata_size(), buffer_traits.metadata_size());
+ return -ENOMEM;
+ }
+
+ size_t metadata_buf_size = buffer_traits.metadata_size();
if (metadata_buf_size < BufferHubDefs::kMetadataHeaderSize) {
- ALOGE("DetachedBuffer::ImportGraphicBuffer: metadata buffer too small: %zu",
+ ALOGE("DetachedBuffer::ImportGraphicBuffer: metadata too small: %zu",
metadata_buf_size);
return -EINVAL;
}
+ // Import the buffer: We only need to hold on the native_handle_t here so that
+ // GraphicBuffer instance can be created in future.
+ buffer_handle_ = buffer_traits.take_buffer_handle();
+
// If all imports succeed, replace the previous buffer and id.
id_ = buffer_id;
- buffer_ = std::move(ion_buffer);
- metadata_buffer_ = std::move(metadata_buffer);
- user_metadata_size_ = metadata_buf_size - BufferHubDefs::kMetadataHeaderSize;
-
- void* metadata_ptr = nullptr;
- if (const int ret =
- metadata_buffer_.Lock(BufferHubDefs::kMetadataUsage, /*x=*/0,
- /*y=*/0, metadata_buf_size,
- /*height=*/1, &metadata_ptr)) {
- ALOGE("DetachedBuffer::ImportGraphicBuffer: Failed to lock metadata.");
- return ret;
- }
+ buffer_state_bit_ = buffer_traits.buffer_state_bit();
// TODO(b/112012161) Set up shared fences.
-
- // Note that here the buffer state is mapped from shared memory as an atomic
- // object. The std::atomic's constructor will not be called so that the
- // original value stored in the memory region can be preserved.
- metadata_header_ = static_cast<BufferHubDefs::MetadataHeader*>(metadata_ptr);
- if (user_metadata_size_) {
- user_metadata_ptr_ = static_cast<void*>(metadata_header_ + 1);
- } else {
- user_metadata_ptr_ = nullptr;
- }
-
- id_ = buffer_desc.id();
- buffer_state_bit_ = buffer_desc.buffer_state_bit();
-
ALOGD_IF(TRACE,
"DetachedBuffer::ImportGraphicBuffer: id=%d, buffer_state=%" PRIx64
".",
- id(), metadata_header_->buffer_state.load());
+ id(),
+ metadata_.metadata_header()->buffer_state.load(
+ std::memory_order_acquire));
return 0;
}
@@ -166,7 +156,7 @@
client_.InvokeRemoteMethod<DetachedBufferRPC::Promote>();
if (status_or_handle.ok()) {
// Invalidate the buffer.
- buffer_ = {};
+ buffer_handle_ = {};
} else {
ALOGE("DetachedBuffer::Promote: Failed to promote buffer (id=%d): %s.", id_,
status_or_handle.GetErrorMessage().c_str());
diff --git a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h
index 8b2bf91..5a24253 100644
--- a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h
+++ b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h
@@ -3,6 +3,11 @@
#include <dvr/dvr_api.h>
#include <hardware/gralloc.h>
+#include <pdx/channel_handle.h>
+#include <pdx/file_handle.h>
+#include <pdx/rpc/remote_method.h>
+#include <pdx/rpc/serializable.h>
+#include <private/dvr/native_handle_wrapper.h>
#include <atomic>
@@ -82,8 +87,137 @@
} // namespace BufferHubDefs
+template <typename FileHandleType>
+class BufferTraits {
+ public:
+ BufferTraits() = default;
+ BufferTraits(const native_handle_t* buffer_handle,
+ const FileHandleType& metadata_handle, int id,
+ uint64_t buffer_state_bit, uint64_t metadata_size,
+ uint32_t width, uint32_t height, uint32_t layer_count,
+ uint32_t format, uint64_t usage, uint32_t stride,
+ const FileHandleType& acquire_fence_fd,
+ const FileHandleType& release_fence_fd)
+ : id_(id),
+ buffer_state_bit_(buffer_state_bit),
+ metadata_size_(metadata_size),
+ width_(width),
+ height_(height),
+ layer_count_(layer_count),
+ format_(format),
+ usage_(usage),
+ stride_(stride),
+ buffer_handle_(buffer_handle),
+ metadata_handle_(metadata_handle.Borrow()),
+ acquire_fence_fd_(acquire_fence_fd.Borrow()),
+ release_fence_fd_(release_fence_fd.Borrow()) {}
+
+ BufferTraits(BufferTraits&& other) = default;
+ BufferTraits& operator=(BufferTraits&& other) = default;
+
+ // ID of the buffer client. All BufferHubBuffer clients derived from the same
+ // buffer in bufferhubd share the same buffer id.
+ int id() const { return id_; }
+
+ // State mask of the buffer client. Each BufferHubBuffer client backed by the
+ // same buffer channel has uniqued state bit among its siblings. For a
+ // producer buffer the bit must be kProducerStateBit; for a consumer the bit
+ // must be one of the kConsumerStateMask.
+ uint64_t buffer_state_bit() const { return buffer_state_bit_; }
+ uint64_t metadata_size() const { return metadata_size_; }
+
+ uint32_t width() { return width_; }
+ uint32_t height() { return height_; }
+ uint32_t layer_count() { return layer_count_; }
+ uint32_t format() { return format_; }
+ uint64_t usage() { return usage_; }
+ uint32_t stride() { return stride_; }
+
+ const NativeHandleWrapper<FileHandleType>& buffer_handle() const {
+ return buffer_handle_;
+ }
+
+ NativeHandleWrapper<FileHandleType> take_buffer_handle() {
+ return std::move(buffer_handle_);
+ }
+ FileHandleType take_metadata_handle() { return std::move(metadata_handle_); }
+ FileHandleType take_acquire_fence() { return std::move(acquire_fence_fd_); }
+ FileHandleType take_release_fence() { return std::move(release_fence_fd_); }
+
+ private:
+ // BufferHub specific traits.
+ int id_ = -1;
+ uint64_t buffer_state_bit_;
+ uint64_t metadata_size_;
+
+ // Traits for a GraphicBuffer.
+ uint32_t width_;
+ uint32_t height_;
+ uint32_t layer_count_;
+ uint32_t format_;
+ uint64_t usage_;
+ uint32_t stride_;
+
+ // Native handle for the graphic buffer.
+ NativeHandleWrapper<FileHandleType> buffer_handle_;
+
+ // File handle of an ashmem that holds buffer metadata.
+ FileHandleType metadata_handle_;
+
+ // Pamameters for shared fences.
+ FileHandleType acquire_fence_fd_;
+ FileHandleType release_fence_fd_;
+
+ PDX_SERIALIZABLE_MEMBERS(BufferTraits<FileHandleType>, id_, buffer_state_bit_,
+ metadata_size_, stride_, width_, height_,
+ layer_count_, format_, usage_, buffer_handle_,
+ metadata_handle_, acquire_fence_fd_,
+ release_fence_fd_);
+
+ BufferTraits(const BufferTraits&) = delete;
+ void operator=(const BufferTraits&) = delete;
+};
+
+struct DetachedBufferRPC {
+ private:
+ enum {
+ kOpDetachedBufferBase = 1000,
+
+ // Allocates a standalone DetachedBuffer not associated with any producer
+ // consumer set.
+ kOpCreate,
+
+ // Imports the given channel handle to a DetachedBuffer, taking ownership.
+ kOpImport,
+
+ // Promotes a DetachedBuffer to become a ProducerBuffer. Once promoted the
+ // DetachedBuffer channel will be closed automatically on successful IPC
+ // return. Further IPCs towards this channel will return error.
+ kOpPromote,
+
+ // Creates a DetachedBuffer client from an existing one. The new client will
+ // share the same underlying gralloc buffer and ashmem region for metadata.
+ kOpDuplicate,
+ };
+
+ // Aliases.
+ using LocalChannelHandle = pdx::LocalChannelHandle;
+ using LocalHandle = pdx::LocalHandle;
+ using Void = pdx::rpc::Void;
+
+ public:
+ PDX_REMOTE_METHOD(Create, kOpCreate,
+ void(uint32_t width, uint32_t height, uint32_t layer_count,
+ uint32_t format, uint64_t usage,
+ size_t user_metadata_size));
+ PDX_REMOTE_METHOD(Import, kOpImport, BufferTraits<LocalHandle>(Void));
+ PDX_REMOTE_METHOD(Promote, kOpPromote, LocalChannelHandle(Void));
+ PDX_REMOTE_METHOD(Duplicate, kOpDuplicate, LocalChannelHandle(Void));
+
+ PDX_REMOTE_API(API, Create, Import, Promote, Duplicate);
+};
+
} // namespace dvr
} // namespace android
-
#endif // ANDROID_DVR_BUFFER_HUB_DEFS_H_
diff --git a/libs/vr/libbufferhub/include/private/dvr/bufferhub_rpc.h b/libs/vr/libbufferhub/include/private/dvr/bufferhub_rpc.h
index 0c7fc90..7330aa1 100644
--- a/libs/vr/libbufferhub/include/private/dvr/bufferhub_rpc.h
+++ b/libs/vr/libbufferhub/include/private/dvr/bufferhub_rpc.h
@@ -316,8 +316,6 @@
kOpConsumerRelease,
kOpProducerBufferDetach,
kOpConsumerBufferDetach,
- kOpDetachedBufferCreate,
- kOpDetachedBufferPromote,
kOpCreateProducerQueue,
kOpCreateConsumerQueue,
kOpGetQueueInfo,
@@ -326,7 +324,6 @@
kOpProducerQueueRemoveBuffer,
kOpConsumerQueueImportBuffers,
// TODO(b/77153033): Separate all those RPC operations into subclasses.
- kOpDetachedBufferBase = 1000,
};
// Aliases.
@@ -379,27 +376,6 @@
std::vector<std::pair<LocalChannelHandle, size_t>>(Void));
};
-struct DetachedBufferRPC final : public BufferHubRPC {
- private:
- enum {
- kOpCreate = kOpDetachedBufferBase,
- kOpImport,
- kOpPromote,
- kOpDuplicate,
- };
-
- public:
- PDX_REMOTE_METHOD(Create, kOpCreate,
- void(uint32_t width, uint32_t height, uint32_t layer_count,
- uint32_t format, uint64_t usage,
- size_t user_metadata_size));
- PDX_REMOTE_METHOD(Import, kOpImport, BufferDescription<LocalHandle>(Void));
- PDX_REMOTE_METHOD(Promote, kOpPromote, LocalChannelHandle(Void));
- PDX_REMOTE_METHOD(Duplicate, kOpDuplicate, LocalChannelHandle(Void));
-
- PDX_REMOTE_API(API, Create, Import, Promote, Duplicate);
-};
-
} // namespace dvr
} // namespace android
diff --git a/libs/vr/libbufferhub/include/private/dvr/detached_buffer.h b/libs/vr/libbufferhub/include/private/dvr/detached_buffer.h
index d280d94..ff61145 100644
--- a/libs/vr/libbufferhub/include/private/dvr/detached_buffer.h
+++ b/libs/vr/libbufferhub/include/private/dvr/detached_buffer.h
@@ -49,19 +49,28 @@
// same buffer in bufferhubd share the same buffer id.
int id() const { return id_; }
+ const native_handle_t* DuplicateHandle() {
+ return buffer_handle_.DuplicateHandle();
+ }
+
// Returns the current value of MetadataHeader::buffer_state.
- uint64_t buffer_state() { return metadata_header_->buffer_state.load(); }
+ uint64_t buffer_state() {
+ return metadata_.metadata_header()->buffer_state.load(
+ std::memory_order_acquire);
+ }
// A state mask which is unique to a buffer hub client among all its siblings
// sharing the same concrete graphic buffer.
uint64_t buffer_state_bit() const { return buffer_state_bit_; }
+ size_t user_metadata_size() const { return metadata_.user_metadata_size(); }
+
// Returns true if the buffer holds an open PDX channels towards bufferhubd.
bool IsConnected() const { return client_.IsValid(); }
- // Returns true if the buffer holds an valid gralloc buffer handle that's
+ // Returns true if the buffer holds an valid native buffer handle that's
// availble for the client to read from and/or write into.
- bool IsValid() const { return buffer_.IsValid(); }
+ bool IsValid() const { return buffer_handle_.IsValid(); }
// Returns the event mask for all the events that are pending on this buffer
// (see sys/poll.h for all possible bits).
@@ -81,7 +90,8 @@
// return. Further IPCs towards this channel will return error.
pdx::Status<pdx::LocalChannelHandle> Promote();
- // Creates a DetachedBuffer from an existing one.
+ // Creates a DetachedBuffer client from an existing one. The new client will
+ // share the same underlying gralloc buffer and ashmem region for metadata.
pdx::Status<pdx::LocalChannelHandle> Duplicate();
private:
@@ -96,14 +106,12 @@
int id_;
uint64_t buffer_state_bit_;
- // The concrete Ion buffers.
- IonBuffer buffer_;
- IonBuffer metadata_buffer_;
+ // Wrapps the gralloc buffer handle of this buffer.
+ NativeHandleWrapper<pdx::LocalHandle> buffer_handle_;
- // buffer metadata.
- size_t user_metadata_size_ = 0;
- BufferHubDefs::MetadataHeader* metadata_header_ = nullptr;
- void* user_metadata_ptr_ = nullptr;
+ // An ashmem-based metadata object. The same shared memory are mapped to the
+ // bufferhubd daemon and all buffer clients.
+ BufferHubMetadata metadata_;
// PDX backend.
BufferHubClient client_;
diff --git a/libs/vr/libbufferhub/include/private/dvr/native_handle_wrapper.h b/libs/vr/libbufferhub/include/private/dvr/native_handle_wrapper.h
new file mode 100644
index 0000000..3086982
--- /dev/null
+++ b/libs/vr/libbufferhub/include/private/dvr/native_handle_wrapper.h
@@ -0,0 +1,100 @@
+#ifndef ANDROID_DVR_NATIVE_HANDLE_WRAPPER_H_
+#define ANDROID_DVR_NATIVE_HANDLE_WRAPPER_H_
+
+#include <cutils/native_handle.h>
+
+#include <vector>
+
+namespace android {
+namespace dvr {
+
+// A PDX-friendly wrapper to maintain the life cycle of a native_handle_t
+// object.
+//
+// See https://source.android.com/devices/architecture/hidl/types#handle_t for
+// more information about native_handle_t.
+template <typename FileHandleType>
+class NativeHandleWrapper {
+ public:
+ NativeHandleWrapper() = default;
+ NativeHandleWrapper(NativeHandleWrapper&& other) = default;
+ NativeHandleWrapper& operator=(NativeHandleWrapper&& other) = default;
+
+ // Create a new NativeHandleWrapper by duplicating the handle.
+ explicit NativeHandleWrapper(const native_handle_t* handle) {
+ const int fd_count = handle->numFds;
+ const int int_count = handle->numInts;
+
+ // Populate the fd and int vectors: native_handle->data[] is an array of fds
+ // followed by an array of opaque ints.
+ for (int i = 0; i < fd_count; i++) {
+ fds_.emplace_back(FileHandleType::AsDuplicate(handle->data[i]));
+ }
+ for (int i = 0; i < int_count; i++) {
+ ints_.push_back(handle->data[fd_count + i]);
+ }
+ }
+
+ size_t int_count() const { return ints_.size(); }
+ size_t fd_count() const { return fds_.size(); }
+ bool IsValid() const { return ints_.size() != 0 || fds_.size() != 0; }
+
+ // Duplicate a native handle from the wrapper.
+ const native_handle_t* DuplicateHandle() const {
+ if (!IsValid()) {
+ return nullptr;
+ }
+
+ // numFds + numInts ints.
+ std::vector<FileHandleType> fds;
+ for (const auto& fd : fds_) {
+ if (!fd.IsValid()) {
+ return nullptr;
+ }
+ fds.emplace_back(fd.Duplicate());
+ }
+
+ return FromFdsAndInts(std::move(fds), ints_);
+ }
+
+ // Takes the native handle out of the wrapper.
+ const native_handle_t* TakeHandle() {
+ if (!IsValid()) {
+ return nullptr;
+ }
+
+ return FromFdsAndInts(std::move(fds_), std::move(ints_));
+ }
+
+ private:
+ NativeHandleWrapper(const NativeHandleWrapper&) = delete;
+ void operator=(const NativeHandleWrapper&) = delete;
+
+ static const native_handle_t* FromFdsAndInts(std::vector<FileHandleType> fds,
+ std::vector<int> ints) {
+ native_handle_t* handle = native_handle_create(fds.size(), ints.size());
+ if (!handle) {
+ ALOGE("NativeHandleWrapper::TakeHandle: Failed to create new handle.");
+ return nullptr;
+ }
+
+ // numFds + numInts ints.
+ for (int i = 0; i < handle->numFds; i++) {
+ handle->data[i] = fds[i].Release();
+ }
+ memcpy(&handle->data[handle->numFds], ints.data(),
+ sizeof(int) * handle->numInts);
+
+ return handle;
+ }
+
+ std::vector<int> ints_;
+ std::vector<FileHandleType> fds_;
+
+ PDX_SERIALIZABLE_MEMBERS(NativeHandleWrapper<FileHandleType>, ints_, fds_);
+};
+
+} // namespace dvr
+} // namespace android
+
+#endif // ANDROID_DVR_NATIVE_HANDLE_WRAPPER_H_