Remove active_buffer_bit_mask_ from BufferNode.
BufferNode already have metadata which contains active_clients_bit_mask,
which serves the same purpose as active_buffer_bit_mask_ but also have
other usage. This change removes the redundant active_buffer_bit_mask_
from BufferNode.
Test: buffer_node-test on marlin-eng and vega_xr build
Bug: 112007999
Change-Id: I7695dc5d6eb84a4a5c73148e9636871cc7776df4
diff --git a/services/vr/bufferhubd/buffer_channel.cpp b/services/vr/bufferhubd/buffer_channel.cpp
index cc9cf20..ee85746 100644
--- a/services/vr/bufferhubd/buffer_channel.cpp
+++ b/services/vr/bufferhubd/buffer_channel.cpp
@@ -1,3 +1,4 @@
+#include <errno.h>
#include <private/dvr/buffer_channel.h>
#include <private/dvr/producer_channel.h>
@@ -16,9 +17,8 @@
size_t user_metadata_size)
: BufferHubChannel(service, buffer_id, channel_id, kDetachedBufferType),
buffer_node_(
- std::make_shared<BufferNode>(std::move(buffer), user_metadata_size)),
- buffer_state_bit_(BufferHubDefs::FindFirstClearedBit()) {
- buffer_node_->set_buffer_state_bit(buffer_state_bit_);
+ std::make_shared<BufferNode>(std::move(buffer), user_metadata_size)) {
+ buffer_state_bit_ = buffer_node_->AddNewActiveClientsBitToMask();
}
BufferChannel::BufferChannel(BufferHubService* service, int buffer_id,
@@ -27,24 +27,28 @@
uint64_t usage, size_t user_metadata_size)
: BufferHubChannel(service, buffer_id, buffer_id, kDetachedBufferType),
buffer_node_(std::make_shared<BufferNode>(
- width, height, layer_count, format, usage, user_metadata_size)),
- buffer_state_bit_(BufferHubDefs::FindFirstClearedBit()) {
- buffer_node_->set_buffer_state_bit(buffer_state_bit_);
+ width, height, layer_count, format, usage, user_metadata_size)) {
+ buffer_state_bit_ = buffer_node_->AddNewActiveClientsBitToMask();
}
BufferChannel::BufferChannel(BufferHubService* service, int buffer_id,
int channel_id,
- std::shared_ptr<BufferNode> buffer_node,
- uint64_t buffer_state_bit)
+ std::shared_ptr<BufferNode> buffer_node)
: BufferHubChannel(service, buffer_id, channel_id, kDetachedBufferType),
- buffer_node_(buffer_node),
- buffer_state_bit_(buffer_state_bit) {
- buffer_node_->set_buffer_state_bit(buffer_state_bit_);
+ buffer_node_(buffer_node) {
+ buffer_state_bit_ = buffer_node_->AddNewActiveClientsBitToMask();
+ if (buffer_state_bit_ == 0ULL) {
+ ALOGE("BufferChannel::BufferChannel: %s", strerror(errno));
+ buffer_node_ = nullptr;
+ }
}
BufferChannel::~BufferChannel() {
ALOGD_IF(TRACE, "BufferChannel::~BufferChannel: channel_id=%d buffer_id=%d.",
channel_id(), buffer_id());
+ if (buffer_state_bit_ != 0ULL) {
+ buffer_node_->RemoveClientsBitFromMask(buffer_state_bit_);
+ }
Hangup();
}
@@ -101,38 +105,22 @@
/*released_fence_fd=*/BorrowedHandle{}};
}
-Status<RemoteChannelHandle> BufferChannel::OnDuplicate(
- Message& message) {
+Status<RemoteChannelHandle> BufferChannel::OnDuplicate(Message& message) {
ATRACE_NAME("BufferChannel::OnDuplicate");
- ALOGD_IF(TRACE, "BufferChannel::OnDuplicate: buffer=%d.",
- buffer_id());
+ ALOGD_IF(TRACE, "BufferChannel::OnDuplicate: buffer=%d.", buffer_id());
int channel_id;
auto status = message.PushChannel(0, nullptr, &channel_id);
- if (!status) {
- ALOGE(
- "BufferChannel::OnDuplicate: Failed to push buffer channel: %s",
- status.GetErrorMessage().c_str());
+ if (!status.ok()) {
+ ALOGE("BufferChannel::OnDuplicate: Failed to push buffer channel: %s",
+ status.GetErrorMessage().c_str());
return ErrorStatus(ENOMEM);
}
- // Try find the next buffer state bit which has not been claimed by any
- // other buffers yet.
- uint64_t buffer_state_bit =
- BufferHubDefs::FindNextClearedBit(buffer_node_->active_buffer_bit_mask() |
- BufferHubDefs::kProducerStateBit);
- if (buffer_state_bit == 0ULL) {
- ALOGE(
- "BufferChannel::OnDuplicate: reached the maximum mumber of channels "
- "per buffer node: 63.");
- return ErrorStatus(E2BIG);
- }
-
- auto channel =
- std::shared_ptr<BufferChannel>(new BufferChannel(
- service(), buffer_id(), channel_id, buffer_node_, buffer_state_bit));
- if (!channel) {
- ALOGE("BufferChannel::OnDuplicate: Invalid buffer.");
+ auto channel = std::shared_ptr<BufferChannel>(
+ new BufferChannel(service(), buffer_id(), channel_id, buffer_node_));
+ if (!channel->IsValid()) {
+ ALOGE("BufferChannel::OnDuplicate: Invalid buffer. %s", strerror(errno));
return ErrorStatus(EINVAL);
}
diff --git a/services/vr/bufferhubd/buffer_node.cpp b/services/vr/bufferhubd/buffer_node.cpp
index 782b9c2..bedec6f 100644
--- a/services/vr/bufferhubd/buffer_node.cpp
+++ b/services/vr/bufferhubd/buffer_node.cpp
@@ -1,12 +1,24 @@
+#include <errno.h>
#include <private/dvr/buffer_hub_defs.h>
#include <private/dvr/buffer_node.h>
namespace android {
namespace dvr {
+void BufferNode::InitializeMetadata() {
+ // Using placement new here to reuse shared memory instead of new allocation
+ // Initialize the atomic variables to zero.
+ BufferHubDefs::MetadataHeader* metadata_header = metadata_.metadata_header();
+ buffer_state_ = new (&metadata_header->buffer_state) std::atomic<uint64_t>(0);
+ fence_state_ = new (&metadata_header->fence_state) std::atomic<uint64_t>(0);
+ active_clients_bit_mask_ =
+ new (&metadata_header->active_clients_bit_mask) std::atomic<uint64_t>(0);
+}
+
BufferNode::BufferNode(IonBuffer buffer, size_t user_metadata_size)
: buffer_(std::move(buffer)) {
metadata_ = BufferHubMetadata::Create(user_metadata_size);
+ InitializeMetadata();
}
// Allocates a new BufferNode.
@@ -22,6 +34,37 @@
}
metadata_ = BufferHubMetadata::Create(user_metadata_size);
+ InitializeMetadata();
+}
+
+uint64_t BufferNode::GetActiveClientsBitMask() const {
+ return active_clients_bit_mask_->load(std::memory_order_acquire);
+}
+
+uint64_t BufferNode::AddNewActiveClientsBitToMask() {
+ uint64_t current_active_clients_bit_mask = GetActiveClientsBitMask();
+ uint64_t buffer_state_bit = 0ULL;
+ uint64_t updated_active_clients_bit_mask = 0ULL;
+ do {
+ buffer_state_bit =
+ BufferHubDefs::FindNextClearedBit(current_active_clients_bit_mask);
+ if (buffer_state_bit == 0ULL) {
+ ALOGE(
+ "BufferNode::AddNewActiveClientsBitToMask: reached the maximum "
+ "mumber of channels per buffer node: 32.");
+ errno = E2BIG;
+ return 0ULL;
+ }
+ updated_active_clients_bit_mask =
+ current_active_clients_bit_mask | buffer_state_bit;
+ } while (!(active_clients_bit_mask_->compare_exchange_weak(
+ current_active_clients_bit_mask, updated_active_clients_bit_mask,
+ std::memory_order_acq_rel, std::memory_order_acquire)));
+ return buffer_state_bit;
+}
+
+void BufferNode::RemoveClientsBitFromMask(const uint64_t& value) {
+ active_clients_bit_mask_->fetch_and(~value);
}
} // namespace dvr
diff --git a/services/vr/bufferhubd/include/private/dvr/buffer_channel.h b/services/vr/bufferhubd/include/private/dvr/buffer_channel.h
index 0ce991a..e9bdb37 100644
--- a/services/vr/bufferhubd/include/private/dvr/buffer_channel.h
+++ b/services/vr/bufferhubd/include/private/dvr/buffer_channel.h
@@ -42,20 +42,20 @@
uint32_t height, uint32_t layer_count, uint32_t format,
uint64_t usage, size_t user_metadata_size);
- // Creates a detached buffer from an existing BufferNode.
+ // Creates a detached buffer from an existing BufferNode. This method is used
+ // in OnDuplicate method.
BufferChannel(BufferHubService* service, int buffer_id, int channel_id,
- std::shared_ptr<BufferNode> buffer_node,
- uint64_t buffer_state_bit);
+ std::shared_ptr<BufferNode> buffer_node);
pdx::Status<BufferTraits<pdx::BorrowedHandle>> OnImport(
pdx::Message& message);
pdx::Status<pdx::RemoteChannelHandle> OnDuplicate(pdx::Message& message);
// The concrete implementation of the Buffer object.
- std::shared_ptr<BufferNode> buffer_node_;
+ std::shared_ptr<BufferNode> buffer_node_ = nullptr;
// The state bit of this buffer. Must be one the lower 63 bits.
- uint64_t buffer_state_bit_;
+ uint64_t buffer_state_bit_ = 0ULL;
};
} // namespace dvr
diff --git a/services/vr/bufferhubd/include/private/dvr/buffer_node.h b/services/vr/bufferhubd/include/private/dvr/buffer_node.h
index d6c6105..e1e8057 100644
--- a/services/vr/bufferhubd/include/private/dvr/buffer_node.h
+++ b/services/vr/bufferhubd/include/private/dvr/buffer_node.h
@@ -10,7 +10,7 @@
class BufferNode {
public:
// Creates a BufferNode from existing IonBuffers, i.e. creating from an
- // existing ProducerChannel.
+ // existing ProducerChannel. Allocate a new BufferHubMetadata.
BufferNode(IonBuffer buffer, size_t user_metadata_size);
// Allocates a new BufferNode.
@@ -21,26 +21,56 @@
bool IsValid() const { return buffer_.IsValid() && metadata_.IsValid(); }
size_t user_metadata_size() const { return metadata_.user_metadata_size(); }
- uint64_t active_buffer_bit_mask() const { return active_buffer_bit_mask_; }
- void set_buffer_state_bit(uint64_t buffer_state_bit) {
- active_buffer_bit_mask_ |= buffer_state_bit;
- }
- // Accessor of the IonBuffer.
+ // Accessors of the IonBuffer.
IonBuffer& buffer() { return buffer_; }
const IonBuffer& buffer() const { return buffer_; }
- // Accessor of the metadata.
+ // Accessors of metadata.
const BufferHubMetadata& metadata() const { return metadata_; }
+ // Gets the current value of active_clients_bit_mask in metadata_ with
+ // std::memory_order_acquire, so that all previous releases of
+ // active_clients_bit_mask from all threads will be returned here.
+ uint64_t GetActiveClientsBitMask() const;
+
+ // Find and add a new buffer_state_bit to active_clients_bit_mask in
+ // metadata_.
+ // Return the new buffer_state_bit that is added to active_clients_bit_mask.
+ // Return 0ULL if there are already 32 bp clients of the buffer.
+ uint64_t AddNewActiveClientsBitToMask();
+
+ // Removes the value from active_clients_bit_mask in metadata_ with
+ // std::memory_order_release, so that the change will be visible to any
+ // acquire of active_clients_bit_mask_ in any threads after the succeed of
+ // this operation.
+ void RemoveClientsBitFromMask(const uint64_t& value);
+
private:
+ // Helper method for constructors to initialize atomic metadata header
+ // variables in shared memory.
+ void InitializeMetadata();
+
// Gralloc buffer handles.
IonBuffer buffer_;
+
+ // Metadata in shared memory.
BufferHubMetadata metadata_;
- // All active buffer bits. Valid bits are the lower 63 bits, while the
- // highest bit is reserved for the exclusive writing and should not be set.
- uint64_t active_buffer_bit_mask_ = 0ULL;
+ // The following variables are atomic variables in metadata_ that are visible
+ // to Bn object and Bp objects. Please find more info in
+ // BufferHubDefs::MetadataHeader.
+
+ // buffer_state_ tracks the state of the buffer. Buffer can be in one of these
+ // four states: gained, posted, acquired, released.
+ std::atomic<uint64_t>* buffer_state_ = nullptr;
+
+ // TODO(b/112012161): add comments to fence_state_.
+ std::atomic<uint64_t>* fence_state_ = nullptr;
+
+ // active_clients_bit_mask_ tracks all the bp clients of the buffer. It is the
+ // union of all buffer_state_bit of all bp clients.
+ std::atomic<uint64_t>* active_clients_bit_mask_ = nullptr;
};
} // namespace dvr
diff --git a/services/vr/bufferhubd/tests/Android.bp b/services/vr/bufferhubd/tests/Android.bp
index bf8ea5b..a80691f 100644
--- a/services/vr/bufferhubd/tests/Android.bp
+++ b/services/vr/bufferhubd/tests/Android.bp
@@ -23,4 +23,31 @@
// TODO(b/117568153): Temporarily opt out using libcrt.
no_libcrt: true,
-}
\ No newline at end of file
+}
+
+cc_test {
+ name: "buffer_node-test",
+ srcs: ["buffer_node-test.cpp"],
+ cflags: [
+ "-DLOG_TAG=\"buffer_node-test\"",
+ "-DTRACE=0",
+ "-DATRACE_TAG=ATRACE_TAG_GRAPHICS",
+ ],
+ header_libs: ["libdvr_headers"],
+ static_libs: [
+ "libbufferhub",
+ "libbufferhubd",
+ "libgmock",
+ ],
+ shared_libs: [
+ "libbase",
+ "libbinder",
+ "liblog",
+ "libpdx_default_transport",
+ "libui",
+ "libutils",
+ ],
+ // TODO(b/117568153): Temporarily opt out using libcrt.
+ no_libcrt: true,
+}
+
diff --git a/services/vr/bufferhubd/tests/buffer_node-test.cpp b/services/vr/bufferhubd/tests/buffer_node-test.cpp
new file mode 100644
index 0000000..c2526fe
--- /dev/null
+++ b/services/vr/bufferhubd/tests/buffer_node-test.cpp
@@ -0,0 +1,89 @@
+#include <errno.h>
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <private/dvr/buffer_node.h>
+
+namespace android {
+namespace dvr {
+
+namespace {
+
+const uint32_t kWidth = 640;
+const uint32_t kHeight = 480;
+const uint32_t kLayerCount = 1;
+const uint32_t kFormat = 1;
+const uint64_t kUsage = 0;
+const size_t kUserMetadataSize = 0;
+
+class BufferNodeTest : public ::testing::Test {
+ protected:
+ void SetUp() override {
+ buffer_node = new BufferNode(kWidth, kHeight, kLayerCount, kFormat, kUsage,
+ kUserMetadataSize);
+ ASSERT_TRUE(buffer_node->IsValid());
+ }
+
+ void TearDown() override {
+ if (buffer_node != nullptr) {
+ delete buffer_node;
+ }
+ }
+
+ BufferNode* buffer_node = nullptr;
+};
+
+TEST_F(BufferNodeTest, TestCreateBufferNode) {
+ EXPECT_EQ(buffer_node->user_metadata_size(), kUserMetadataSize);
+}
+
+TEST_F(BufferNodeTest, TestAddNewActiveClientsBitToMask_twoNewClients) {
+ uint64_t new_buffer_state_bit_1 = buffer_node->AddNewActiveClientsBitToMask();
+ EXPECT_EQ(buffer_node->GetActiveClientsBitMask(), new_buffer_state_bit_1);
+
+ // Request and add a new buffer_state_bit again.
+ // Active clients bit mask should be the union of the two new
+ // buffer_state_bits.
+ uint64_t new_buffer_state_bit_2 = buffer_node->AddNewActiveClientsBitToMask();
+ EXPECT_EQ(buffer_node->GetActiveClientsBitMask(),
+ new_buffer_state_bit_1 | new_buffer_state_bit_2);
+}
+
+TEST_F(BufferNodeTest, TestAddNewActiveClientsBitToMask_32NewClients) {
+ uint64_t new_buffer_state_bit = 0ULL;
+ uint64_t current_mask = 0ULL;
+ uint64_t expected_mask = 0ULL;
+
+ for (int i = 0; i < 64; ++i) {
+ new_buffer_state_bit = buffer_node->AddNewActiveClientsBitToMask();
+ EXPECT_NE(new_buffer_state_bit, 0);
+ EXPECT_FALSE(new_buffer_state_bit & current_mask);
+ expected_mask = current_mask | new_buffer_state_bit;
+ current_mask = buffer_node->GetActiveClientsBitMask();
+ EXPECT_EQ(current_mask, expected_mask);
+ }
+
+ // Method should fail upon requesting for more than maximum allowable clients.
+ new_buffer_state_bit = buffer_node->AddNewActiveClientsBitToMask();
+ EXPECT_EQ(new_buffer_state_bit, 0ULL);
+ EXPECT_EQ(errno, E2BIG);
+}
+
+TEST_F(BufferNodeTest, TestRemoveActiveClientsBitFromMask) {
+ buffer_node->AddNewActiveClientsBitToMask();
+ uint64_t current_mask = buffer_node->GetActiveClientsBitMask();
+ uint64_t new_buffer_state_bit = buffer_node->AddNewActiveClientsBitToMask();
+ EXPECT_NE(buffer_node->GetActiveClientsBitMask(), current_mask);
+
+ buffer_node->RemoveClientsBitFromMask(new_buffer_state_bit);
+ EXPECT_EQ(buffer_node->GetActiveClientsBitMask(), current_mask);
+
+ // Remove the test_mask again to the active client bit mask should not modify
+ // the value of active clients bit mask.
+ buffer_node->RemoveClientsBitFromMask(new_buffer_state_bit);
+ EXPECT_EQ(buffer_node->GetActiveClientsBitMask(), current_mask);
+}
+
+} // namespace
+
+} // namespace dvr
+} // namespace android