Merge "Fix BufferHub state machine to return errors on invalid transitions." into oc-dr1-dev
am: 2b7b5c34d3
Change-Id: I311d3fa978ca1a7d74bc6403be59705dd9ea995e
diff --git a/libs/vr/libbufferhub/bufferhub_tests.cpp b/libs/vr/libbufferhub/bufferhub_tests.cpp
index fa61c4a..1daa5d6 100644
--- a/libs/vr/libbufferhub/bufferhub_tests.cpp
+++ b/libs/vr/libbufferhub/bufferhub_tests.cpp
@@ -62,6 +62,57 @@
EXPECT_GE(0, RETRY_EINTR(p->Poll(0)));
}
+TEST_F(LibBufferHubTest, TestStateTransitions) {
+ std::unique_ptr<BufferProducer> p = BufferProducer::Create(
+ kWidth, kHeight, kFormat, kUsage, sizeof(uint64_t));
+ ASSERT_TRUE(p.get() != nullptr);
+ std::unique_ptr<BufferConsumer> c =
+ BufferConsumer::Import(p->CreateConsumer());
+ ASSERT_TRUE(c.get() != nullptr);
+
+ uint64_t context;
+ LocalHandle fence;
+
+ // The producer buffer starts in gained state.
+
+ // Acquire, release, and gain in gained state should fail.
+ EXPECT_EQ(-EBUSY, c->Acquire(&fence, &context));
+ EXPECT_EQ(-EBUSY, c->Release(LocalHandle()));
+ EXPECT_EQ(-EALREADY, p->Gain(&fence));
+
+ // Post in gained state should succeed.
+ EXPECT_EQ(0, p->Post(LocalHandle(), kContext));
+
+ // Post, release, and gain in posted state should fail.
+ EXPECT_EQ(-EBUSY, p->Post(LocalHandle(), kContext));
+ EXPECT_EQ(-EBUSY, c->Release(LocalHandle()));
+ EXPECT_EQ(-EBUSY, p->Gain(&fence));
+
+ // Acquire in posted state should succeed.
+ EXPECT_LE(0, c->Acquire(&fence, &context));
+
+ // Acquire, post, and gain in acquired state should fail.
+ EXPECT_EQ(-EBUSY, c->Acquire(&fence, &context));
+ EXPECT_EQ(-EBUSY, p->Post(LocalHandle(), kContext));
+ EXPECT_EQ(-EBUSY, p->Gain(&fence));
+
+ // Release in acquired state should succeed.
+ EXPECT_EQ(0, c->Release(LocalHandle()));
+
+ // Release, acquire, and post in released state should fail.
+ EXPECT_EQ(-EBUSY, c->Release(LocalHandle()));
+ EXPECT_EQ(-EBUSY, c->Acquire(&fence, &context));
+ EXPECT_EQ(-EBUSY, p->Post(LocalHandle(), kContext));
+
+ // Gain in released state should succeed.
+ EXPECT_EQ(0, p->Gain(&fence));
+
+ // Acquire, release, and gain in gained state should fail.
+ EXPECT_EQ(-EBUSY, c->Acquire(&fence, &context));
+ EXPECT_EQ(-EBUSY, c->Release(LocalHandle()));
+ EXPECT_EQ(-EALREADY, p->Gain(&fence));
+}
+
TEST_F(LibBufferHubTest, TestWithCustomMetadata) {
struct Metadata {
int64_t field1;
diff --git a/services/vr/bufferhubd/consumer_channel.cpp b/services/vr/bufferhubd/consumer_channel.cpp
index 08b2790..ac6896a 100644
--- a/services/vr/bufferhubd/consumer_channel.cpp
+++ b/services/vr/bufferhubd/consumer_channel.cpp
@@ -8,9 +8,9 @@
#include <private/dvr/bufferhub_rpc.h>
#include "producer_channel.h"
-using android::pdx::ErrorStatus;
using android::pdx::BorrowedHandle;
using android::pdx::Channel;
+using android::pdx::ErrorStatus;
using android::pdx::Message;
using android::pdx::Status;
using android::pdx::rpc::DispatchRemoteMethod;
@@ -22,8 +22,6 @@
int channel_id,
const std::shared_ptr<Channel> producer)
: BufferHubChannel(service, buffer_id, channel_id, kConsumerType),
- handled_(true),
- ignored_(false),
producer_(producer) {
GetProducer()->AddConsumer(this);
}
@@ -34,7 +32,7 @@
channel_id(), buffer_id());
if (auto producer = GetProducer()) {
- if (!handled_) // Producer is waiting for our Release.
+ if (!released_) // Producer is waiting for our Release.
producer->OnConsumerIgnored();
producer->RemoveConsumer(this);
}
@@ -108,15 +106,20 @@
if (!producer)
return ErrorStatus(EPIPE);
- if (ignored_ || handled_) {
+ if (acquired_ || released_) {
ALOGE(
"ConsumerChannel::OnConsumerAcquire: Acquire when not posted: "
- "ignored=%d handled=%d channel_id=%d buffer_id=%d",
- ignored_, handled_, message.GetChannelId(), producer->buffer_id());
+ "ignored=%d acquired=%d released=%d channel_id=%d buffer_id=%d",
+ ignored_, acquired_, released_, message.GetChannelId(),
+ producer->buffer_id());
return ErrorStatus(EBUSY);
} else {
- ClearAvailable();
- return producer->OnConsumerAcquire(message, metadata_size);
+ auto status = producer->OnConsumerAcquire(message, metadata_size);
+ if (status) {
+ ClearAvailable();
+ acquired_ = true;
+ }
+ return status;
}
}
@@ -127,17 +130,21 @@
if (!producer)
return ErrorStatus(EPIPE);
- if (ignored_ || handled_) {
+ if (!acquired_ || released_) {
ALOGE(
"ConsumerChannel::OnConsumerRelease: Release when not acquired: "
- "ignored=%d handled=%d channel_id=%d buffer_id=%d",
- ignored_, handled_, message.GetChannelId(), producer->buffer_id());
+ "ignored=%d acquired=%d released=%d channel_id=%d buffer_id=%d",
+ ignored_, acquired_, released_, message.GetChannelId(),
+ producer->buffer_id());
return ErrorStatus(EBUSY);
} else {
- ClearAvailable();
auto status =
producer->OnConsumerRelease(message, std::move(release_fence));
- handled_ = !!status;
+ if (status) {
+ ClearAvailable();
+ acquired_ = false;
+ released_ = true;
+ }
return status;
}
}
@@ -149,12 +156,13 @@
return ErrorStatus(EPIPE);
ignored_ = ignored;
- if (ignored_ && !handled_) {
+ if (ignored_ && acquired_) {
// Update the producer if ignore is set after the consumer acquires the
// buffer.
ClearAvailable();
producer->OnConsumerIgnored();
- handled_ = false;
+ acquired_ = false;
+ released_ = true;
}
return {};
@@ -162,10 +170,12 @@
bool ConsumerChannel::OnProducerPosted() {
if (ignored_) {
- handled_ = true;
+ acquired_ = false;
+ released_ = true;
return false;
} else {
- handled_ = false;
+ acquired_ = false;
+ released_ = false;
SignalAvailable();
return true;
}
diff --git a/services/vr/bufferhubd/consumer_channel.h b/services/vr/bufferhubd/consumer_channel.h
index d84055c..208a002 100644
--- a/services/vr/bufferhubd/consumer_channel.h
+++ b/services/vr/bufferhubd/consumer_channel.h
@@ -38,8 +38,9 @@
LocalFence release_fence);
pdx::Status<void> OnConsumerSetIgnore(Message& message, bool ignore);
- bool handled_; // True if we have processed RELEASE.
- bool ignored_; // True if we are ignoring events.
+ bool acquired_{false};
+ bool released_{true};
+ bool ignored_{false}; // True if we are ignoring events.
std::weak_ptr<Channel> producer_;
ConsumerChannel(const ConsumerChannel&) = delete;
diff --git a/services/vr/bufferhubd/producer_channel.cpp b/services/vr/bufferhubd/producer_channel.cpp
index b9984a0..b2db795 100644
--- a/services/vr/bufferhubd/producer_channel.cpp
+++ b/services/vr/bufferhubd/producer_channel.cpp
@@ -122,7 +122,7 @@
}
Status<NativeBufferHandle<BorrowedHandle>> ProducerChannel::OnGetBuffer(
- Message& message) {
+ Message& /*message*/) {
ATRACE_NAME("ProducerChannel::OnGetBuffer");
ALOGD_IF(TRACE, "ProducerChannel::OnGetBuffer: buffer=%d", buffer_id());
return {NativeBufferHandle<BorrowedHandle>(buffer_, buffer_id())};
@@ -204,7 +204,7 @@
return {};
}
-Status<LocalFence> ProducerChannel::OnProducerGain(Message& message) {
+Status<LocalFence> ProducerChannel::OnProducerGain(Message& /*message*/) {
ATRACE_NAME("ProducerChannel::OnGain");
ALOGD_IF(TRACE, "ProducerChannel::OnGain: buffer_id=%d", buffer_id());
if (producer_owns_) {
@@ -224,7 +224,7 @@
}
Status<std::pair<BorrowedFence, BufferWrapper<std::uint8_t*>>>
-ProducerChannel::OnConsumerAcquire(Message& message,
+ProducerChannel::OnConsumerAcquire(Message& /*message*/,
std::size_t metadata_size) {
ATRACE_NAME("ProducerChannel::OnConsumerAcquire");
ALOGD_IF(TRACE, "ProducerChannel::OnConsumerAcquire: buffer_id=%d",