From d9f2abe6e29d84e32c2b1e91748d2b699d32f98b Mon Sep 17 00:00:00 2001 From: Jiwen 'Steve' Cai Date: Sat, 20 Oct 2018 17:03:13 -0700 Subject: Basic implementation for bufferhub HIDL service Bug: 118124442 Test: device can boot with android.frameworks.bufferhub@1.0-service running Test: BufferHubBuffer_test Change-Id: I6d1013d9be8268e3776f8fdbdd2eb79e3d73a74e --- services/bufferhub/BufferHubService.cpp | 41 +++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 services/bufferhub/BufferHubService.cpp (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp new file mode 100644 index 0000000000..8be85a503a --- /dev/null +++ b/services/bufferhub/BufferHubService.cpp @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2018 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. + */ + +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +using ::android::status_t; +using ::android::hardware::Void; + +Return BufferHubService::allocateBuffer(const HardwareBufferDescription& /*description*/, + allocateBuffer_cb /*hidl_cb*/) { + return Void(); +} + +Return> BufferHubService::importBuffer(const hidl_handle& /*nativeHandle*/) { + return nullptr; +} + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android -- cgit v1.2.3-59-g8ed1b From ca70b7ba83ff4c0a9a91b52296ef089ab062e762 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Wed, 31 Oct 2018 13:20:12 -0700 Subject: Implement IBufferClient hwbinder interface Some boiler plate code for future use. Test: "atest BufferHubBuffer_test" passed. Bug: b/116681016 Change-Id: I12854ac6f553777451584e86a81f2e6064a12696 --- libs/ui/tests/BufferHubBuffer_test.cpp | 15 +++++--- services/bufferhub/Android.bp | 1 + services/bufferhub/BufferClient.cpp | 39 ++++++++++++++++++++ services/bufferhub/BufferHubService.cpp | 15 +++++--- .../bufferhub/include/bufferhub/BufferClient.h | 41 ++++++++++++++++++++++ .../bufferhub/include/bufferhub/BufferHubService.h | 11 +++--- 6 files changed, 107 insertions(+), 15 deletions(-) create mode 100644 services/bufferhub/BufferClient.cpp create mode 100644 services/bufferhub/include/bufferhub/BufferClient.h (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp index a247e60103..606aee60cb 100644 --- a/libs/ui/tests/BufferHubBuffer_test.cpp +++ b/libs/ui/tests/BufferHubBuffer_test.cpp @@ -16,6 +16,7 @@ #define LOG_TAG "BufferHubBufferTest" +#include #include #include #include @@ -33,11 +34,11 @@ const int kFormat = HAL_PIXEL_FORMAT_RGBA_8888; const int kUsage = 0; const size_t kUserMetadataSize = 0; -} // namespace - using dvr::BufferHubDefs::IsBufferGained; using dvr::BufferHubDefs::kFirstClientBitMask; using dvr::BufferHubDefs::kMetadataHeaderSize; +using frameworks::bufferhub::V1_0::BufferHubStatus; +using frameworks::bufferhub::V1_0::IBufferClient; using frameworks::bufferhub::V1_0::IBufferHub; using hardware::hidl_handle; using hidl::base::V1_0::IBase; @@ -129,8 +130,14 @@ TEST_F(BufferHubBufferTest, ConnectHidlServer) { // TODO(b/116681016): Fill in real test once the interface gets implemented.. hidl_handle handle; - sp interface = bufferhub->importBuffer(handle); - EXPECT_EQ(nullptr, interface.get()); + EXPECT_TRUE(bufferhub + ->importBuffer(handle, + [](const auto& client, const auto& ret) { + EXPECT_EQ(client, nullptr); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + }) + .isOk()); } +} // namespace } // namespace android diff --git a/services/bufferhub/Android.bp b/services/bufferhub/Android.bp index d03d833fd7..ca65e02538 100644 --- a/services/bufferhub/Android.bp +++ b/services/bufferhub/Android.bp @@ -22,6 +22,7 @@ cc_library_shared { "-Wextra", ], srcs: [ + "BufferClient.cpp", "BufferHubService.cpp", "BufferNode.cpp", ], diff --git a/services/bufferhub/BufferClient.cpp b/services/bufferhub/BufferClient.cpp new file mode 100644 index 0000000000..b3662b265b --- /dev/null +++ b/services/bufferhub/BufferClient.cpp @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2018 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. + */ + +#include +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +using hardware::hidl_handle; +using hardware::Void; + +Return BufferClient::duplicate(duplicate_cb _hidl_cb) { + // TODO(b/118614157): implement token generation and registration + _hidl_cb(/*token=*/hidl_handle(), /*status=*/BufferHubStatus::NO_ERROR); + return Void(); +} + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android \ No newline at end of file diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 8be85a503a..86598e00ac 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -22,16 +22,21 @@ namespace bufferhub { namespace V1_0 { namespace implementation { -using ::android::status_t; -using ::android::hardware::Void; +using hardware::Void; Return BufferHubService::allocateBuffer(const HardwareBufferDescription& /*description*/, - allocateBuffer_cb /*hidl_cb*/) { + const uint32_t /*userMetadataSize*/, + allocateBuffer_cb _hidl_cb) { + // TODO(b/118614333): implement buffer allocation + _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::NO_ERROR); return Void(); } -Return> BufferHubService::importBuffer(const hidl_handle& /*nativeHandle*/) { - return nullptr; +Return BufferHubService::importBuffer(const hidl_handle& /*nativeHandle*/, + importBuffer_cb _hidl_cb) { + // TODO(b/118614157): implement buffer import + _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::NO_ERROR); + return Void(); } } // namespace implementation diff --git a/services/bufferhub/include/bufferhub/BufferClient.h b/services/bufferhub/include/bufferhub/BufferClient.h new file mode 100644 index 0000000000..14ea95c415 --- /dev/null +++ b/services/bufferhub/include/bufferhub/BufferClient.h @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2018 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_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_CLIENT_H +#define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_CLIENT_H + +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +using hardware::Return; + +class BufferClient : public IBufferClient { +public: + Return duplicate(duplicate_cb _hidl_cb) override; +}; + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android + +#endif \ No newline at end of file diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index b273e5b93c..5e0cff0a69 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -17,8 +17,8 @@ #ifndef ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H +#include #include -#include namespace android { namespace frameworks { @@ -26,17 +26,16 @@ namespace bufferhub { namespace V1_0 { namespace implementation { -using ::android::sp; using ::android::hardware::hidl_handle; using ::android::hardware::Return; using ::android::hardware::graphics::common::V1_2::HardwareBufferDescription; -using ::android::hidl::base::V1_0::IBase; class BufferHubService : public IBufferHub { public: - Return allocateBuffer(const HardwareBufferDescription& /*description*/, - allocateBuffer_cb /*hidl_cb*/) override; - Return> importBuffer(const hidl_handle& /*nativeHandle*/) override; + Return allocateBuffer(const HardwareBufferDescription& description, + const uint32_t userMetadataSize, + allocateBuffer_cb _hidl_cb) override; + Return importBuffer(const hidl_handle& nativeHandle, importBuffer_cb _hidl_cb) override; }; } // namespace implementation -- cgit v1.2.3-59-g8ed1b From 93c9490762bf8908bb6e15b343c8d65f893af4e5 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Thu, 1 Nov 2018 12:22:05 -0700 Subject: Implement allocateBuffer for BufferHubService Return ALLOCATION_FAILED when failed to create BufferNode, NO_ERROR on success. Allocation and memory management logics are in BufferNode, and enforced via the shared_ptr and clientList. Memcpy is used to convert between AHardwareBuffer_Desc and HardwareBufferDescription (i.e. hidl_vec). Test: BufferHubBuffer_test (passed) Change-Id: I5e17aa7330c5f94656e62dc4bea8ab6c705ab7a4 Fix: 118614333 --- libs/ui/tests/Android.bp | 3 ++- libs/ui/tests/BufferHubBuffer_test.cpp | 31 +++++++++++++--------- services/bufferhub/Android.bp | 6 +++++ services/bufferhub/BufferHubService.cpp | 26 +++++++++++++++--- .../bufferhub/include/bufferhub/BufferClient.h | 11 ++++++++ .../bufferhub/include/bufferhub/BufferHubService.h | 10 ++++++- 6 files changed, 68 insertions(+), 19 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/tests/Android.bp b/libs/ui/tests/Android.bp index 1521e1d758..4c9c1768e0 100644 --- a/libs/ui/tests/Android.bp +++ b/libs/ui/tests/Android.bp @@ -32,7 +32,8 @@ cc_test { name: "BufferHubBuffer_test", header_libs: [ "libbufferhub_headers", - "libdvr_headers" + "libdvr_headers", + "libnativewindow_headers", ], shared_libs: [ "android.frameworks.bufferhub@1.0", diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp index 606aee60cb..143335a091 100644 --- a/libs/ui/tests/BufferHubBuffer_test.cpp +++ b/libs/ui/tests/BufferHubBuffer_test.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -41,6 +42,7 @@ using frameworks::bufferhub::V1_0::BufferHubStatus; using frameworks::bufferhub::V1_0::IBufferClient; using frameworks::bufferhub::V1_0::IBufferHub; using hardware::hidl_handle; +using hardware::graphics::common::V1_2::HardwareBufferDescription; using hidl::base::V1_0::IBase; using pdx::LocalChannelHandle; @@ -124,19 +126,22 @@ TEST_F(BufferHubBufferTest, DuplicateBufferHubBuffer) { return; } -TEST_F(BufferHubBufferTest, ConnectHidlServer) { - sp bufferhub = IBufferHub::getService(); - ASSERT_NE(nullptr, bufferhub.get()); - - // TODO(b/116681016): Fill in real test once the interface gets implemented.. - hidl_handle handle; - EXPECT_TRUE(bufferhub - ->importBuffer(handle, - [](const auto& client, const auto& ret) { - EXPECT_EQ(client, nullptr); - EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); - }) - .isOk()); +TEST_F(BufferHubBufferTest, AllocateBuffer) { + // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. + sp bufferHub = IBufferHub::getService(); + ASSERT_NE(nullptr, bufferHub.get()); + + // Stride is an output, rfu0 and rfu1 are reserved data slot for future use. + AHardwareBuffer_Desc aDesc = {kWidth, kHeight, kLayerCount, kFormat, + kUsage, /*stride=*/0UL, /*rfu0=*/0UL, /*rfu1=*/0ULL}; + HardwareBufferDescription desc; + memcpy(&desc, &aDesc, sizeof(HardwareBufferDescription)); + + IBufferHub::allocateBuffer_cb callback = [](const auto& client, const auto& status) { + EXPECT_EQ(status, BufferHubStatus::NO_ERROR); + EXPECT_NE(nullptr, client.get()); + }; + EXPECT_TRUE(bufferHub->allocateBuffer(desc, kUserMetadataSize, callback).isOk()); } } // namespace diff --git a/services/bufferhub/Android.bp b/services/bufferhub/Android.bp index ca65e02538..871561f171 100644 --- a/services/bufferhub/Android.bp +++ b/services/bufferhub/Android.bp @@ -52,6 +52,12 @@ cc_binary { srcs: [ "main_bufferhub.cpp" ], + header_libs: [ + "libbufferhub_headers", + "libdvr_headers", + "libnativewindow_headers", + "libpdx_headers", + ], shared_libs: [ "android.frameworks.bufferhub@1.0", "libbufferhubservice", diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 86598e00ac..b72b556c6e 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -14,7 +14,9 @@ * limitations under the License. */ +#include #include +#include namespace android { namespace frameworks { @@ -24,11 +26,27 @@ namespace implementation { using hardware::Void; -Return BufferHubService::allocateBuffer(const HardwareBufferDescription& /*description*/, - const uint32_t /*userMetadataSize*/, +Return BufferHubService::allocateBuffer(const HardwareBufferDescription& description, + const uint32_t userMetadataSize, allocateBuffer_cb _hidl_cb) { - // TODO(b/118614333): implement buffer allocation - _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::NO_ERROR); + AHardwareBuffer_Desc desc; + memcpy(&desc, &description, sizeof(AHardwareBuffer_Desc)); + + std::shared_ptr node = + std::make_shared(desc.width, desc.height, desc.layers, desc.format, + desc.usage, userMetadataSize); + if (node == nullptr || !node->IsValid()) { + ALOGE("%s: creating BufferNode failed.", __FUNCTION__); + _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::ALLOCATION_FAILED); + return Void(); + } + + sp client = new BufferClient(node); + // Add it to list for bookkeeping and dumpsys. + std::lock_guard lock(mClientListMutex); + mClientList.push_back(client); + + _hidl_cb(/*bufferClient=*/client, /*status=*/BufferHubStatus::NO_ERROR); return Void(); } diff --git a/services/bufferhub/include/bufferhub/BufferClient.h b/services/bufferhub/include/bufferhub/BufferClient.h index 14ea95c415..c6eb791b1f 100644 --- a/services/bufferhub/include/bufferhub/BufferClient.h +++ b/services/bufferhub/include/bufferhub/BufferClient.h @@ -18,6 +18,7 @@ #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_CLIENT_H #include +#include namespace android { namespace frameworks { @@ -27,9 +28,19 @@ namespace implementation { using hardware::Return; +// Forward declaration to avoid circular dependency +class BufferHubService; + class BufferClient : public IBufferClient { public: + // Creates a server-side buffer client from an existing BufferNode. Note that + // this funciton takes ownership of the shared_ptr. + explicit BufferClient(const std::shared_ptr& node) : mBufferNode(node){}; + Return duplicate(duplicate_cb _hidl_cb) override; + +private: + std::shared_ptr mBufferNode; }; } // namespace implementation diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 5e0cff0a69..1bbd807069 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -17,8 +17,11 @@ #ifndef ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H -#include +#include + #include +#include +#include namespace android { namespace frameworks { @@ -36,6 +39,11 @@ public: const uint32_t userMetadataSize, allocateBuffer_cb _hidl_cb) override; Return importBuffer(const hidl_handle& nativeHandle, importBuffer_cb _hidl_cb) override; + +private: + // List of active BufferClient for bookkeeping. + std::mutex mClientListMutex; + std::vector> mClientList GUARDED_BY(mClientListMutex); }; } // namespace implementation -- cgit v1.2.3-59-g8ed1b From 18d90eaf05d8b3a21a68d84ebf23ea2326067aad Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Tue, 6 Nov 2018 15:46:44 -0800 Subject: Implement duplicate buffer Allow BufferClient to generate a opaque token in hidl_handle (i.e. native_handle_t) format that could be used for IBufferHub::import. The service will keep a mapping from token to client. Token comparison is handled in BufferHubService.cpp, as it's opaque to the users. Test: BufferHubBuffer_test (passed) Bug: 118614157 Change-Id: Ie2b0b650dba90fe2ed2f8091dd88acb9768f261f --- libs/ui/tests/BufferHubBuffer_test.cpp | 28 ++++++++++++++++++++ services/bufferhub/Android.bp | 2 ++ services/bufferhub/BufferClient.cpp | 30 ++++++++++++++++++++-- services/bufferhub/BufferHubService.cpp | 22 +++++++++++++++- .../bufferhub/include/bufferhub/BufferClient.h | 10 +++++++- .../bufferhub/include/bufferhub/BufferHubService.h | 17 +++++++++--- 6 files changed, 102 insertions(+), 7 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp index 143335a091..6af8033e76 100644 --- a/libs/ui/tests/BufferHubBuffer_test.cpp +++ b/libs/ui/tests/BufferHubBuffer_test.cpp @@ -144,5 +144,33 @@ TEST_F(BufferHubBufferTest, AllocateBuffer) { EXPECT_TRUE(bufferHub->allocateBuffer(desc, kUserMetadataSize, callback).isOk()); } +TEST_F(BufferHubBufferTest, DuplicateBuffer) { + // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. + sp bufferhub = IBufferHub::getService(); + ASSERT_NE(nullptr, bufferhub.get()); + + // Stride is an output, rfu0 and rfu1 are reserved data slot for future use. + AHardwareBuffer_Desc aDesc = {kWidth, kHeight, kLayerCount, kFormat, + kUsage, /*stride=*/0UL, /*rfu0=*/0UL, /*rfu1=*/0ULL}; + HardwareBufferDescription desc; + memcpy(&desc, &aDesc, sizeof(HardwareBufferDescription)); + + sp client; + IBufferHub::allocateBuffer_cb alloc_cb = [&](const auto& outClient, const auto& status) { + ASSERT_EQ(status, BufferHubStatus::NO_ERROR); + ASSERT_NE(nullptr, outClient.get()); + client = outClient; + }; + ASSERT_TRUE(bufferhub->allocateBuffer(desc, kUserMetadataSize, alloc_cb).isOk()); + + IBufferClient::duplicate_cb dup_cb = [](const auto& token, const auto& status) { + ASSERT_EQ(status, BufferHubStatus::NO_ERROR); + ASSERT_NE(token.getNativeHandle(), nullptr); + EXPECT_EQ(token->numInts, 1); + EXPECT_EQ(token->numFds, 0); + }; + EXPECT_TRUE(client->duplicate(dup_cb).isOk()); +} + } // namespace } // namespace android diff --git a/services/bufferhub/Android.bp b/services/bufferhub/Android.bp index 871561f171..8b4333342d 100644 --- a/services/bufferhub/Android.bp +++ b/services/bufferhub/Android.bp @@ -34,6 +34,7 @@ cc_library_shared { ], shared_libs: [ "android.frameworks.bufferhub@1.0", + "libcutils", "libhidlbase", "libhidltransport", "libhwbinder", @@ -61,6 +62,7 @@ cc_binary { shared_libs: [ "android.frameworks.bufferhub@1.0", "libbufferhubservice", + "libcutils", "libhidltransport", "libhwbinder", "liblog", diff --git a/services/bufferhub/BufferClient.cpp b/services/bufferhub/BufferClient.cpp index b3662b265b..37fd75fe80 100644 --- a/services/bufferhub/BufferClient.cpp +++ b/services/bufferhub/BufferClient.cpp @@ -15,6 +15,7 @@ */ #include +#include #include namespace android { @@ -26,9 +27,34 @@ namespace implementation { using hardware::hidl_handle; using hardware::Void; +BufferClient* BufferClient::create(BufferHubService* service, + const std::shared_ptr& node) { + if (!service) { + ALOGE("%s: service cannot be nullptr.", __FUNCTION__); + return nullptr; + } else if (!node) { + ALOGE("%s: node cannot be nullptr.", __FUNCTION__); + return nullptr; + } + return new BufferClient(service, node); +} + Return BufferClient::duplicate(duplicate_cb _hidl_cb) { - // TODO(b/118614157): implement token generation and registration - _hidl_cb(/*token=*/hidl_handle(), /*status=*/BufferHubStatus::NO_ERROR); + if (!mBufferNode) { + // Should never happen + ALOGE("%s: node is missing.", __FUNCTION__); + _hidl_cb(/*token=*/hidl_handle(), /*status=*/BufferHubStatus::BUFFER_FREED); + return Void(); + } + + sp service = mService.promote(); + if (service == nullptr) { + // Should never happen. Kill the process. + LOG_FATAL("%s: service died.", __FUNCTION__); + } + + const hidl_handle token = service->registerToken(this); + _hidl_cb(/*token=*/token, /*status=*/BufferHubStatus::NO_ERROR); return Void(); } diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index b72b556c6e..3bfd9cbe7e 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -16,6 +16,7 @@ #include #include +#include #include namespace android { @@ -41,7 +42,7 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d return Void(); } - sp client = new BufferClient(node); + sp client = BufferClient::create(this, node); // Add it to list for bookkeeping and dumpsys. std::lock_guard lock(mClientListMutex); mClientList.push_back(client); @@ -57,6 +58,25 @@ Return BufferHubService::importBuffer(const hidl_handle& /*nativeHandle*/, return Void(); } +hidl_handle BufferHubService::registerToken(const BufferClient* client) { + uint32_t token; + std::lock_guard lock(mTokenMapMutex); + do { + token = mTokenEngine(); + } while (mTokenMap.find(token) != mTokenMap.end()); + + // native_handle_t use int[], so here need one slots to fit in uint32_t + native_handle_t* handle = native_handle_create(/*numFds=*/0, /*numInts=*/1); + handle->data[0] = token; + + // returnToken owns the native_handle_t* thus doing lifecycle management + hidl_handle returnToken; + returnToken.setTo(handle, /*shoudOwn=*/true); + + mTokenMap.emplace(token, client); + return returnToken; +} + } // namespace implementation } // namespace V1_0 } // namespace bufferhub diff --git a/services/bufferhub/include/bufferhub/BufferClient.h b/services/bufferhub/include/bufferhub/BufferClient.h index c6eb791b1f..5456ec33f0 100644 --- a/services/bufferhub/include/bufferhub/BufferClient.h +++ b/services/bufferhub/include/bufferhub/BufferClient.h @@ -17,6 +17,8 @@ #ifndef ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_CLIENT_H #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_CLIENT_H +#include + #include #include @@ -26,6 +28,7 @@ namespace bufferhub { namespace V1_0 { namespace implementation { +using hardware::hidl_handle; using hardware::Return; // Forward declaration to avoid circular dependency @@ -35,11 +38,16 @@ class BufferClient : public IBufferClient { public: // Creates a server-side buffer client from an existing BufferNode. Note that // this funciton takes ownership of the shared_ptr. - explicit BufferClient(const std::shared_ptr& node) : mBufferNode(node){}; + // Returns a raw pointer to the BufferClient on success, nullptr on failure. + static BufferClient* create(BufferHubService* service, const std::shared_ptr& node); Return duplicate(duplicate_cb _hidl_cb) override; private: + BufferClient(wp service, const std::shared_ptr& node) + : mService(service), mBufferNode(node){}; + + wp mService; std::shared_ptr mBufferNode; }; diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 1bbd807069..dbbee8fccf 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -18,6 +18,7 @@ #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H #include +#include #include #include @@ -29,9 +30,9 @@ namespace bufferhub { namespace V1_0 { namespace implementation { -using ::android::hardware::hidl_handle; -using ::android::hardware::Return; -using ::android::hardware::graphics::common::V1_2::HardwareBufferDescription; +using hardware::hidl_handle; +using hardware::Return; +using hardware::graphics::common::V1_2::HardwareBufferDescription; class BufferHubService : public IBufferHub { public: @@ -40,10 +41,20 @@ public: allocateBuffer_cb _hidl_cb) override; Return importBuffer(const hidl_handle& nativeHandle, importBuffer_cb _hidl_cb) override; + // Non-binder functions + // Internal help function for IBufferClient::duplicate. + hidl_handle registerToken(const BufferClient* client); + private: // List of active BufferClient for bookkeeping. std::mutex mClientListMutex; std::vector> mClientList GUARDED_BY(mClientListMutex); + + // TODO(b/118180214): use a more secure implementation + std::mt19937 mTokenEngine; + // The mapping from token to the client creates it. + std::mutex mTokenMapMutex; + std::map mTokenMap GUARDED_BY(mTokenMapMutex); }; } // namespace implementation -- cgit v1.2.3-59-g8ed1b From ffde786ffce1afcd7ee574e7b45026d651ac1779 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Thu, 8 Nov 2018 16:29:13 -0800 Subject: Add a service-unique ID to BufferNode All the BufferNode allocated by bufferhub hwservice now will have a unique ID generated and managed by the UniqueIdGenerator. The idGenerator is placed in global static namespace because 1) therefore it is process-unique 2) BufferNode could still access it without a reference to the service instance. Added UniqueIdGenerator_test. Test: UniqueIdGenerator_test, BufferNode_test, buffer_hub-test, BufferHubBuffer_test passed. Fix: 118844348 Change-Id: I9c9adebab3af7b9de71dbe8728d3f24ed231338d --- services/bufferhub/Android.bp | 1 + services/bufferhub/BufferHubService.cpp | 2 +- services/bufferhub/BufferNode.cpp | 15 +++++- services/bufferhub/UniqueIdGenerator.cpp | 55 +++++++++++++++++++++ .../bufferhub/include/bufferhub/BufferHubService.h | 3 ++ services/bufferhub/include/bufferhub/BufferNode.h | 11 ++++- .../include/bufferhub/UniqueIdGenerator.h | 57 ++++++++++++++++++++++ services/bufferhub/tests/Android.bp | 16 ++++++ .../bufferhub/tests/UniqueIdGenerator_test.cpp | 45 +++++++++++++++++ 9 files changed, 201 insertions(+), 4 deletions(-) create mode 100644 services/bufferhub/UniqueIdGenerator.cpp create mode 100644 services/bufferhub/include/bufferhub/UniqueIdGenerator.h create mode 100644 services/bufferhub/tests/UniqueIdGenerator_test.cpp (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/Android.bp b/services/bufferhub/Android.bp index 8b4333342d..28a7501e0a 100644 --- a/services/bufferhub/Android.bp +++ b/services/bufferhub/Android.bp @@ -25,6 +25,7 @@ cc_library_shared { "BufferClient.cpp", "BufferHubService.cpp", "BufferNode.cpp", + "UniqueIdGenerator.cpp", ], header_libs: [ "libbufferhub_headers", diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 3bfd9cbe7e..fc5ad1dbbf 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -35,7 +35,7 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d std::shared_ptr node = std::make_shared(desc.width, desc.height, desc.layers, desc.format, - desc.usage, userMetadataSize); + desc.usage, userMetadataSize, nodeIdGenerator.getId()); if (node == nullptr || !node->IsValid()) { ALOGE("%s: creating BufferNode failed.", __FUNCTION__); _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::ALLOCATION_FAILED); diff --git a/services/bufferhub/BufferNode.cpp b/services/bufferhub/BufferNode.cpp index 53dd702792..715d0a1f67 100644 --- a/services/bufferhub/BufferNode.cpp +++ b/services/bufferhub/BufferNode.cpp @@ -1,5 +1,6 @@ #include +#include #include #include #include @@ -22,7 +23,8 @@ void BufferNode::InitializeMetadata() { // Allocates a new BufferNode. BufferNode::BufferNode(uint32_t width, uint32_t height, uint32_t layer_count, uint32_t format, - uint64_t usage, size_t user_metadata_size) { + uint64_t usage, size_t user_metadata_size, uint32_t id) + : mId(id) { uint32_t out_stride = 0; // graphicBufferId is not used in GraphicBufferAllocator::allocate // TODO(b/112338294) After move to the service folder, stop using the @@ -54,14 +56,23 @@ BufferNode::BufferNode(uint32_t width, uint32_t height, uint32_t layer_count, ui InitializeMetadata(); } -// Free the handle BufferNode::~BufferNode() { + // Free the handle if (buffer_handle_ != nullptr) { status_t ret = GraphicBufferAllocator::get().free(buffer_handle_); if (ret != OK) { ALOGE("%s: Failed to free handle; Got error: %d", __FUNCTION__, ret); } } + + // Free the id, if valid + if (id() != UniqueIdGenerator::kInvalidId) { + if (nodeIdGenerator.freeId(id())) { + ALOGI("%s: id #%u is freed.", __FUNCTION__, id()); + } else { + ALOGE("%s: Cannot free nonexistent id #%u", __FUNCTION__, id()); + } + } } uint64_t BufferNode::GetActiveClientsBitMask() const { diff --git a/services/bufferhub/UniqueIdGenerator.cpp b/services/bufferhub/UniqueIdGenerator.cpp new file mode 100644 index 0000000000..362a026f6c --- /dev/null +++ b/services/bufferhub/UniqueIdGenerator.cpp @@ -0,0 +1,55 @@ +/* + * Copyright (C) 2018 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. + */ + +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +constexpr uint32_t UniqueIdGenerator::kInvalidId; + +uint32_t UniqueIdGenerator::getId() { + std::lock_guard lock(mIdsInUseMutex); + + do { + if (++mLastId >= std::numeric_limits::max()) { + mLastId = kInvalidId + 1; + } + } while (mIdsInUse.find(mLastId) != mIdsInUse.end()); + + mIdsInUse.insert(mLastId); + return mLastId; +} + +bool UniqueIdGenerator::freeId(uint32_t id) { + std::lock_guard lock(mIdsInUseMutex); + auto iter = mIdsInUse.find(id); + if (iter != mIdsInUse.end()) { + mIdsInUse.erase(iter); + return true; + } + + return false; +} + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index dbbee8fccf..5441750e9a 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -22,6 +22,7 @@ #include #include +#include #include namespace android { @@ -34,6 +35,8 @@ using hardware::hidl_handle; using hardware::Return; using hardware::graphics::common::V1_2::HardwareBufferDescription; +static UniqueIdGenerator nodeIdGenerator; + class BufferHubService : public IBufferHub { public: Return allocateBuffer(const HardwareBufferDescription& description, diff --git a/services/bufferhub/include/bufferhub/BufferNode.h b/services/bufferhub/include/bufferhub/BufferNode.h index ffeacaccc3..c490e7c709 100644 --- a/services/bufferhub/include/bufferhub/BufferNode.h +++ b/services/bufferhub/include/bufferhub/BufferNode.h @@ -2,6 +2,7 @@ #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_NODE_H_ #include +#include #include namespace android { @@ -14,13 +15,16 @@ class BufferNode { public: // Allocates a new BufferNode. BufferNode(uint32_t width, uint32_t height, uint32_t layer_count, uint32_t format, - uint64_t usage, size_t user_metadata_size); + uint64_t usage, size_t user_metadata_size, + uint32_t id = UniqueIdGenerator::kInvalidId); ~BufferNode(); // Returns whether the object holds a valid metadata. bool IsValid() const { return metadata_.IsValid(); } + uint32_t id() const { return mId; } + size_t user_metadata_size() const { return metadata_.user_metadata_size(); } // Accessors of the buffer description and handle @@ -59,6 +63,11 @@ private: // Metadata in shared memory. BufferHubMetadata metadata_; + // A system-unique id generated by bufferhub from 1 to std::numeric_limits::max(). + // BufferNodes not created by bufferhub will have id = 0, meaning "not specified". + // TODO(b/118891412): remove default id = 0 and update comments after pdx is no longer in use + const uint32_t mId = 0; + // The following variables are atomic variables in metadata_ that are visible // to Bn object and Bp objects. Please find more info in // BufferHubDefs::MetadataHeader. diff --git a/services/bufferhub/include/bufferhub/UniqueIdGenerator.h b/services/bufferhub/include/bufferhub/UniqueIdGenerator.h new file mode 100644 index 0000000000..d2e702f7e0 --- /dev/null +++ b/services/bufferhub/include/bufferhub/UniqueIdGenerator.h @@ -0,0 +1,57 @@ +/* + * Copyright (C) 2018 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_FRAMEWORKS_BUFFERHUB_V1_0_ID_GENERATOR_H +#define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_ID_GENERATOR_H + +#include +#include + +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +// A thread-safe incremental uint32_t id generator. +class UniqueIdGenerator { +public: + // 0 is considered invalid + static constexpr uint32_t kInvalidId = 0UL; + + // Gets next available id. If next id is greater than std::numeric_limits::max() (2 ^ + // 32 - 1), it will try to get an id start from 1 again. + uint32_t getId(); + + // Free a specific id. Return true on freed, false on not found. + bool freeId(uint32_t id); + +private: + std::mutex mIdsInUseMutex; + // Start from kInvalidID to avoid generating it. + uint32_t mLastId = kInvalidId; + std::set mIdsInUse GUARDED_BY(mIdsInUseMutex); +}; + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android + +#endif // ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_ID_GENERATOR_H diff --git a/services/bufferhub/tests/Android.bp b/services/bufferhub/tests/Android.bp index cef31f6ba9..8a30ef5820 100644 --- a/services/bufferhub/tests/Android.bp +++ b/services/bufferhub/tests/Android.bp @@ -21,4 +21,20 @@ cc_test { ], // TODO(b/117568153): Temporarily opt out using libcrt. no_libcrt: true, +} + +cc_test { + name: "UniqueIdGenerator_test", + srcs: ["UniqueIdGenerator_test.cpp"], + cflags: [ + "-DLOG_TAG=\"UniqueIdGenerator_test\"", + "-DTRACE=0", + "-DATRACE_TAG=ATRACE_TAG_GRAPHICS", + ], + shared_libs: [ + "libbufferhubservice", + ], + static_libs: [ + "libgmock", + ], } \ No newline at end of file diff --git a/services/bufferhub/tests/UniqueIdGenerator_test.cpp b/services/bufferhub/tests/UniqueIdGenerator_test.cpp new file mode 100644 index 0000000000..c4d83e0c1f --- /dev/null +++ b/services/bufferhub/tests/UniqueIdGenerator_test.cpp @@ -0,0 +1,45 @@ +#include +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +namespace { + +class UniqueIdGeneratorTest : public testing::Test { +protected: + UniqueIdGenerator mIdGenerator; +}; + +TEST_F(UniqueIdGeneratorTest, TestGenerateAndFreeID) { + uint32_t id = mIdGenerator.getId(); + EXPECT_NE(id, UniqueIdGenerator::kInvalidId); + + EXPECT_TRUE(mIdGenerator.freeId(id)); + EXPECT_FALSE(mIdGenerator.freeId(id)); +} + +TEST_F(UniqueIdGeneratorTest, TestGenerateUniqueIncrementalID) { + // 10 IDs should not overflow the UniqueIdGenerator to cause a roll back to start, so the + // resulting IDs should still keep incresing. + const size_t kTestSize = 10U; + uint32_t ids[kTestSize]; + for (int i = 0; i < kTestSize; ++i) { + ids[i] = mIdGenerator.getId(); + EXPECT_NE(ids[i], UniqueIdGenerator::kInvalidId); + if (i >= 1) { + EXPECT_GT(ids[i], ids[i - 1]); + } + } +} + +} // namespace + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android \ No newline at end of file -- cgit v1.2.3-59-g8ed1b From d6cd6ba6d387d1d6e1c8cd8077ff6ae596fb7065 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Thu, 15 Nov 2018 16:46:55 -0800 Subject: Add garbage collecion function to BufferHubService Now BufferHubService will cleanup unused BufferClient automatically. If the mClientList storing weak pointer, when user side's strong pointer dies, the refcount will become 0 and the allocated memory in the service will get freed. Test: BufferHubBuffer_test (passed) Bug: 119623209 Change-Id: I1390f878bf44afd7c1a5cccaeb34a9d13dab3db0 --- services/bufferhub/BufferHubService.cpp | 2 +- services/bufferhub/include/bufferhub/BufferHubService.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index fc5ad1dbbf..1a38dd80b7 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -58,7 +58,7 @@ Return BufferHubService::importBuffer(const hidl_handle& /*nativeHandle*/, return Void(); } -hidl_handle BufferHubService::registerToken(const BufferClient* client) { +hidl_handle BufferHubService::registerToken(const wp& client) { uint32_t token; std::lock_guard lock(mTokenMapMutex); do { diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 5441750e9a..e3f657fdd7 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -46,18 +46,18 @@ public: // Non-binder functions // Internal help function for IBufferClient::duplicate. - hidl_handle registerToken(const BufferClient* client); + hidl_handle registerToken(const wp& client); private: // List of active BufferClient for bookkeeping. std::mutex mClientListMutex; - std::vector> mClientList GUARDED_BY(mClientListMutex); + std::vector> mClientList GUARDED_BY(mClientListMutex); // TODO(b/118180214): use a more secure implementation std::mt19937 mTokenEngine; // The mapping from token to the client creates it. std::mutex mTokenMapMutex; - std::map mTokenMap GUARDED_BY(mTokenMapMutex); + std::map> mTokenMap GUARDED_BY(mTokenMapMutex); }; } // namespace implementation -- cgit v1.2.3-59-g8ed1b From 467e08fb6a370fe26064da06ea57dbb693d599d4 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Fri, 9 Nov 2018 15:58:51 -0800 Subject: Implement importBuffer for BufferHubService Allow user importBuffer by a previously generated handle from IBufferClient::duplicate. Passing a nullptr or invalid token to the service will get a nullptr IBufferClient and BufferHubStatus::INVALID_TOKEN. Test: BufferHubBuffer_test (passed) Change-Id: I0aa969a9706e42039ac851acb991b3aceb924c27 Fix: 118614157 --- libs/ui/tests/Android.bp | 1 + libs/ui/tests/BufferHubBuffer_test.cpp | 57 +++++++++++++++++++++- services/bufferhub/BufferHubService.cpp | 40 +++++++++++++-- .../bufferhub/include/bufferhub/BufferClient.h | 8 ++- .../bufferhub/include/bufferhub/BufferHubService.h | 2 +- 5 files changed, 100 insertions(+), 8 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/tests/Android.bp b/libs/ui/tests/Android.bp index b7ad4e5a80..00f30a67b2 100644 --- a/libs/ui/tests/Android.bp +++ b/libs/ui/tests/Android.bp @@ -53,6 +53,7 @@ cc_test { ], shared_libs: [ "android.frameworks.bufferhub@1.0", + "libcutils", "libhidlbase", "libhwbinder", "libpdx_default_transport", diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp index d30636f8cc..553ee0be8e 100644 --- a/libs/ui/tests/BufferHubBuffer_test.cpp +++ b/libs/ui/tests/BufferHubBuffer_test.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -144,7 +145,7 @@ TEST_F(BufferHubBufferTest, AllocateBuffer) { EXPECT_TRUE(bufferHub->allocateBuffer(desc, kUserMetadataSize, callback).isOk()); } -TEST_F(BufferHubBufferTest, DuplicateBuffer) { +TEST_F(BufferHubBufferTest, DuplicateAndImportBuffer) { // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. sp bufferhub = IBufferHub::getService(); ASSERT_NE(nullptr, bufferhub.get()); @@ -170,11 +171,63 @@ TEST_F(BufferHubBufferTest, DuplicateBuffer) { token = outToken; ret = status; }; - EXPECT_TRUE(client->duplicate(dup_cb).isOk()); + ASSERT_TRUE(client->duplicate(dup_cb).isOk()); EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); ASSERT_NE(token.getNativeHandle(), nullptr); EXPECT_EQ(token->numInts, 1); EXPECT_EQ(token->numFds, 0); + + sp client2; + IBufferHub::importBuffer_cb import_cb = [&](const auto& outClient, const auto& status) { + ret = status; + client2 = outClient; + }; + ASSERT_TRUE(bufferhub->importBuffer(token, import_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + EXPECT_NE(nullptr, client2.get()); + // TODO(b/116681016): once BufferNode.id() is exposed via BufferHubBuffer, check origin.id = + // improted.id here. +} + +// nullptr must not crash the service +TEST_F(BufferHubBufferTest, ImportNullToken) { + // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. + sp bufferhub = IBufferHub::getService(); + ASSERT_NE(nullptr, bufferhub.get()); + + hidl_handle nullToken; + sp client; + BufferHubStatus ret; + IBufferHub::importBuffer_cb import_cb = [&](const auto& outClient, const auto& status) { + client = outClient; + ret = status; + }; + ASSERT_TRUE(bufferhub->importBuffer(nullToken, import_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::INVALID_TOKEN); + EXPECT_EQ(nullptr, client.get()); +} + +// This test has a very little chance to fail (number of existing tokens / 2 ^ 32) +TEST_F(BufferHubBufferTest, ImportInvalidToken) { + // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. + sp bufferhub = IBufferHub::getService(); + ASSERT_NE(nullptr, bufferhub.get()); + + native_handle_t* tokenHandle = native_handle_create(/*numFds=*/0, /*numInts=*/1); + tokenHandle->data[0] = 0; + + hidl_handle invalidToken(tokenHandle); + sp client; + BufferHubStatus ret; + IBufferHub::importBuffer_cb import_cb = [&](const auto& outClient, const auto& status) { + client = outClient; + ret = status; + }; + ASSERT_TRUE(bufferhub->importBuffer(invalidToken, import_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::INVALID_TOKEN); + EXPECT_EQ(nullptr, client.get()); + + native_handle_delete(tokenHandle); } } // namespace diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 1a38dd80b7..6f97f0d8c0 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -51,10 +51,44 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d return Void(); } -Return BufferHubService::importBuffer(const hidl_handle& /*nativeHandle*/, +Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, importBuffer_cb _hidl_cb) { - // TODO(b/118614157): implement buffer import - _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::NO_ERROR); + if (!tokenHandle.getNativeHandle() || tokenHandle->numFds != 0 || tokenHandle->numInts != 1) { + // nullptr handle or wrong format + _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::INVALID_TOKEN); + return Void(); + } + + uint32_t token = tokenHandle->data[0]; + + wp originClientWp; + { + std::lock_guard lock(mTokenMapMutex); + auto iter = mTokenMap.find(token); + if (iter == mTokenMap.end()) { + // Invalid token + _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::INVALID_TOKEN); + return Void(); + } + + originClientWp = iter->second; + mTokenMap.erase(iter); + } + + // Check if original client is dead + sp originClient = originClientWp.promote(); + if (!originClient) { + // Should not happen since token should be removed if already gone + ALOGE("%s: original client %p gone!", __FUNCTION__, originClientWp.unsafe_get()); + _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::BUFFER_FREED); + return Void(); + } + + sp client = new BufferClient(*originClient); + + std::lock_guard lock(mClientListMutex); + mClientList.push_back(client); + _hidl_cb(/*bufferClient=*/client, /*status=*/BufferHubStatus::NO_ERROR); return Void(); } diff --git a/services/bufferhub/include/bufferhub/BufferClient.h b/services/bufferhub/include/bufferhub/BufferClient.h index 5456ec33f0..769ec86776 100644 --- a/services/bufferhub/include/bufferhub/BufferClient.h +++ b/services/bufferhub/include/bufferhub/BufferClient.h @@ -37,15 +37,19 @@ class BufferHubService; class BufferClient : public IBufferClient { public: // Creates a server-side buffer client from an existing BufferNode. Note that - // this funciton takes ownership of the shared_ptr. + // this function takes ownership of the shared_ptr. // Returns a raw pointer to the BufferClient on success, nullptr on failure. static BufferClient* create(BufferHubService* service, const std::shared_ptr& node); + // Creates a BufferClient from an existing BufferClient. Will share the same BufferNode. + explicit BufferClient(const BufferClient& other) + : mService(other.mService), mBufferNode(other.mBufferNode) {} + Return duplicate(duplicate_cb _hidl_cb) override; private: BufferClient(wp service, const std::shared_ptr& node) - : mService(service), mBufferNode(node){}; + : mService(service), mBufferNode(node) {} wp mService; std::shared_ptr mBufferNode; diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index e3f657fdd7..6535659ae7 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -42,7 +42,7 @@ public: Return allocateBuffer(const HardwareBufferDescription& description, const uint32_t userMetadataSize, allocateBuffer_cb _hidl_cb) override; - Return importBuffer(const hidl_handle& nativeHandle, importBuffer_cb _hidl_cb) override; + Return importBuffer(const hidl_handle& tokenHandle, importBuffer_cb _hidl_cb) override; // Non-binder functions // Internal help function for IBufferClient::duplicate. -- cgit v1.2.3-59-g8ed1b From a7422fe2e0afc5bce205db37460061e0ad7e3fb4 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Mon, 19 Nov 2018 15:21:32 -0800 Subject: Implement IBufferClient::close on BufferClient The close() function will mark a BufferClient as closed (set mClosed = true), remove it from service's mClientList, and remove all of its token in service's mTokenMap. Add test cases to make sure it works in all situations. Test: BufferHubBuffer_test (passed) Bug: 119623209 Change-Id: Ic1d17ced97b67ef5432c9d341469d8e6105e2717 --- libs/ui/tests/BufferHubBuffer_test.cpp | 93 +++++++++++++++++++++- services/bufferhub/BufferClient.cpp | 32 +++++++- services/bufferhub/BufferHubService.cpp | 24 ++++++ .../bufferhub/include/bufferhub/BufferClient.h | 8 ++ .../bufferhub/include/bufferhub/BufferHubService.h | 5 ++ 5 files changed, 155 insertions(+), 7 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp index 553ee0be8e..5f9fa672ef 100644 --- a/libs/ui/tests/BufferHubBuffer_test.cpp +++ b/libs/ui/tests/BufferHubBuffer_test.cpp @@ -127,7 +127,7 @@ TEST_F(BufferHubBufferTest, DuplicateBufferHubBuffer) { return; } -TEST_F(BufferHubBufferTest, AllocateBuffer) { +TEST_F(BufferHubBufferTest, AllocateAndFreeBuffer) { // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. sp bufferHub = IBufferHub::getService(); ASSERT_NE(nullptr, bufferHub.get()); @@ -138,11 +138,51 @@ TEST_F(BufferHubBufferTest, AllocateBuffer) { HardwareBufferDescription desc; memcpy(&desc, &aDesc, sizeof(HardwareBufferDescription)); - IBufferHub::allocateBuffer_cb callback = [](const auto& client, const auto& status) { - EXPECT_EQ(status, BufferHubStatus::NO_ERROR); - EXPECT_NE(nullptr, client.get()); + sp client; + BufferHubStatus ret; + IBufferHub::allocateBuffer_cb callback = [&](const auto& outClient, const auto& outStatus) { + client = outClient; + ret = outStatus; + }; + EXPECT_TRUE(bufferHub->allocateBuffer(desc, kUserMetadataSize, callback).isOk()); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + ASSERT_NE(nullptr, client.get()); + + EXPECT_EQ(BufferHubStatus::NO_ERROR, client->close()); + EXPECT_EQ(BufferHubStatus::CLIENT_CLOSED, client->close()); +} + +TEST_F(BufferHubBufferTest, DuplicateFreedBuffer) { + // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. + sp bufferHub = IBufferHub::getService(); + ASSERT_NE(nullptr, bufferHub.get()); + + // Stride is an output, rfu0 and rfu1 are reserved data slot for future use. + AHardwareBuffer_Desc aDesc = {kWidth, kHeight, kLayerCount, kFormat, + kUsage, /*stride=*/0UL, /*rfu0=*/0UL, /*rfu1=*/0ULL}; + HardwareBufferDescription desc; + memcpy(&desc, &aDesc, sizeof(HardwareBufferDescription)); + + sp client; + BufferHubStatus ret; + IBufferHub::allocateBuffer_cb callback = [&](const auto& outClient, const auto& outStatus) { + client = outClient; + ret = outStatus; }; EXPECT_TRUE(bufferHub->allocateBuffer(desc, kUserMetadataSize, callback).isOk()); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + ASSERT_NE(nullptr, client.get()); + + EXPECT_EQ(BufferHubStatus::NO_ERROR, client->close()); + + hidl_handle token; + IBufferClient::duplicate_cb dup_cb = [&](const auto& outToken, const auto& status) { + token = outToken; + ret = status; + }; + EXPECT_TRUE(client->duplicate(dup_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::CLIENT_CLOSED); + EXPECT_EQ(token.getNativeHandle(), nullptr); } TEST_F(BufferHubBufferTest, DuplicateAndImportBuffer) { @@ -230,5 +270,50 @@ TEST_F(BufferHubBufferTest, ImportInvalidToken) { native_handle_delete(tokenHandle); } +TEST_F(BufferHubBufferTest, ImportFreedBuffer) { + // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. + sp bufferhub = IBufferHub::getService(); + ASSERT_NE(nullptr, bufferhub.get()); + + // Stride is an output, rfu0 and rfu1 are reserved data slot for future use. + AHardwareBuffer_Desc aDesc = {kWidth, kHeight, kLayerCount, kFormat, + kUsage, /*stride=*/0UL, /*rfu0=*/0UL, /*rfu1=*/0ULL}; + HardwareBufferDescription desc; + memcpy(&desc, &aDesc, sizeof(HardwareBufferDescription)); + + sp client; + BufferHubStatus ret; + IBufferHub::allocateBuffer_cb alloc_cb = [&](const auto& outClient, const auto& status) { + client = outClient; + ret = status; + }; + ASSERT_TRUE(bufferhub->allocateBuffer(desc, kUserMetadataSize, alloc_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + ASSERT_NE(nullptr, client.get()); + + hidl_handle token; + IBufferClient::duplicate_cb dup_cb = [&](const auto& outToken, const auto& status) { + token = outToken; + ret = status; + }; + ASSERT_TRUE(client->duplicate(dup_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + ASSERT_NE(token.getNativeHandle(), nullptr); + EXPECT_EQ(token->numInts, 1); + EXPECT_EQ(token->numFds, 0); + + // Close the client. Now the token should be invalid. + client->close(); + + sp client2; + IBufferHub::importBuffer_cb import_cb = [&](const auto& outClient, const auto& status) { + client2 = outClient; + ret = status; + }; + EXPECT_TRUE(bufferhub->importBuffer(token, import_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::INVALID_TOKEN); + EXPECT_EQ(nullptr, client2.get()); +} + } // namespace } // namespace android diff --git a/services/bufferhub/BufferClient.cpp b/services/bufferhub/BufferClient.cpp index 37fd75fe80..745951745a 100644 --- a/services/bufferhub/BufferClient.cpp +++ b/services/bufferhub/BufferClient.cpp @@ -39,7 +39,29 @@ BufferClient* BufferClient::create(BufferHubService* service, return new BufferClient(service, node); } +BufferClient::~BufferClient() { + close(); +} + +Return BufferClient::close() { + std::lock_guard lock(mClosedMutex); + if (mClosed) { + return BufferHubStatus::CLIENT_CLOSED; + } + + getService()->onClientClosed(this); + mBufferNode.reset(); + mClosed = true; + return BufferHubStatus::NO_ERROR; +} + Return BufferClient::duplicate(duplicate_cb _hidl_cb) { + std::lock_guard lock(mClosedMutex); + if (mClosed) { + _hidl_cb(/*token=*/hidl_handle(), /*status=*/BufferHubStatus::CLIENT_CLOSED); + return Void(); + } + if (!mBufferNode) { // Should never happen ALOGE("%s: node is missing.", __FUNCTION__); @@ -47,15 +69,19 @@ Return BufferClient::duplicate(duplicate_cb _hidl_cb) { return Void(); } + const hidl_handle token = getService()->registerToken(this); + _hidl_cb(/*token=*/token, /*status=*/BufferHubStatus::NO_ERROR); + return Void(); +} + +sp BufferClient::getService() { sp service = mService.promote(); if (service == nullptr) { // Should never happen. Kill the process. LOG_FATAL("%s: service died.", __FUNCTION__); } - const hidl_handle token = service->registerToken(this); - _hidl_cb(/*token=*/token, /*status=*/BufferHubStatus::NO_ERROR); - return Void(); + return service; } } // namespace implementation diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 6f97f0d8c0..409f2ea8b0 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -111,6 +111,30 @@ hidl_handle BufferHubService::registerToken(const wp& client) { return returnToken; } +void BufferHubService::onClientClosed(const BufferClient* client) { + removeTokenByClient(client); + + std::lock_guard lock(mClientListMutex); + auto iter = std::find(mClientList.begin(), mClientList.end(), client); + if (iter != mClientList.end()) { + mClientList.erase(iter); + } +} + +void BufferHubService::removeTokenByClient(const BufferClient* client) { + std::lock_guard lock(mTokenMapMutex); + auto iter = mTokenMap.begin(); + while (iter != mTokenMap.end()) { + if (iter->second == client) { + auto oldIter = iter; + ++iter; + mTokenMap.erase(oldIter); + } else { + ++iter; + } + } +} + } // namespace implementation } // namespace V1_0 } // namespace bufferhub diff --git a/services/bufferhub/include/bufferhub/BufferClient.h b/services/bufferhub/include/bufferhub/BufferClient.h index 769ec86776..7f5d3a6aa1 100644 --- a/services/bufferhub/include/bufferhub/BufferClient.h +++ b/services/bufferhub/include/bufferhub/BufferClient.h @@ -44,14 +44,22 @@ public: // Creates a BufferClient from an existing BufferClient. Will share the same BufferNode. explicit BufferClient(const BufferClient& other) : mService(other.mService), mBufferNode(other.mBufferNode) {} + ~BufferClient(); + Return close() override; Return duplicate(duplicate_cb _hidl_cb) override; private: BufferClient(wp service, const std::shared_ptr& node) : mService(service), mBufferNode(node) {} + sp getService(); + wp mService; + + std::mutex mClosedMutex; + bool mClosed GUARDED_BY(mClosedMutex) = false; + std::shared_ptr mBufferNode; }; diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 6535659ae7..1c328b33a2 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -48,7 +48,12 @@ public: // Internal help function for IBufferClient::duplicate. hidl_handle registerToken(const wp& client); + void onClientClosed(const BufferClient* client); + private: + // Helper function to remove all the token belongs to a specific client. + void removeTokenByClient(const BufferClient* client); + // List of active BufferClient for bookkeeping. std::mutex mClientListMutex; std::vector> mClientList GUARDED_BY(mClientListMutex); -- cgit v1.2.3-59-g8ed1b From cd74d78173b9ccd60c14906a22ddbf723647b4f8 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Mon, 26 Nov 2018 13:51:25 -0800 Subject: Change mClientList to std::set mClientSet Change-Id: I34af8a96916032630c05ea3b4e9987a6e09d6f51 Fix: 119819092 Test: BufferHubBuffer_test (passed) --- services/bufferhub/BufferHubService.cpp | 16 ++++++++-------- services/bufferhub/include/bufferhub/BufferHubService.h | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 409f2ea8b0..81bd1422ba 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -44,8 +44,8 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d sp client = BufferClient::create(this, node); // Add it to list for bookkeeping and dumpsys. - std::lock_guard lock(mClientListMutex); - mClientList.push_back(client); + std::lock_guard lock(mClientSetMutex); + mClientSet.emplace(client); _hidl_cb(/*bufferClient=*/client, /*status=*/BufferHubStatus::NO_ERROR); return Void(); @@ -86,8 +86,8 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, sp client = new BufferClient(*originClient); - std::lock_guard lock(mClientListMutex); - mClientList.push_back(client); + std::lock_guard lock(mClientSetMutex); + mClientSet.emplace(client); _hidl_cb(/*bufferClient=*/client, /*status=*/BufferHubStatus::NO_ERROR); return Void(); } @@ -114,10 +114,10 @@ hidl_handle BufferHubService::registerToken(const wp& client) { void BufferHubService::onClientClosed(const BufferClient* client) { removeTokenByClient(client); - std::lock_guard lock(mClientListMutex); - auto iter = std::find(mClientList.begin(), mClientList.end(), client); - if (iter != mClientList.end()) { - mClientList.erase(iter); + std::lock_guard lock(mClientSetMutex); + auto iter = std::find(mClientSet.begin(), mClientSet.end(), client); + if (iter != mClientSet.end()) { + mClientSet.erase(iter); } } diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 1c328b33a2..add3d89cbf 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -55,8 +55,8 @@ private: void removeTokenByClient(const BufferClient* client); // List of active BufferClient for bookkeeping. - std::mutex mClientListMutex; - std::vector> mClientList GUARDED_BY(mClientListMutex); + std::mutex mClientSetMutex; + std::set> mClientSet GUARDED_BY(mClientSetMutex); // TODO(b/118180214): use a more secure implementation std::mt19937 mTokenEngine; -- cgit v1.2.3-59-g8ed1b From 1c16df541e8b1a67a042fbb20b6e2c76446e9748 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Mon, 19 Nov 2018 16:27:27 -0800 Subject: Fix freeId() failed in ~BufferNode And rename UniqueIdGenerator to BufferHubIdGenerator. Created a static getter and Use scoped static initialization for UniqueIdGenerator. Test: run BufferHubBuffer_test, then "adb logcat | grep bufferhub -i" to verify that there are no error messages. Fix: 118844348 Change-Id: Ia75ae4dac13cc709f39161c803401bcbb90c70e2 --- services/bufferhub/Android.bp | 2 +- services/bufferhub/BufferHubIdGenerator.cpp | 61 +++++++++++++++++++++ services/bufferhub/BufferHubService.cpp | 3 +- services/bufferhub/BufferNode.cpp | 4 +- services/bufferhub/UniqueIdGenerator.cpp | 55 ------------------- .../include/bufferhub/BufferHubIdGenerator.h | 63 ++++++++++++++++++++++ .../bufferhub/include/bufferhub/BufferHubService.h | 4 +- services/bufferhub/include/bufferhub/BufferNode.h | 4 +- .../include/bufferhub/UniqueIdGenerator.h | 57 -------------------- services/bufferhub/tests/Android.bp | 6 +-- .../bufferhub/tests/BufferHubIdGenerator_test.cpp | 45 ++++++++++++++++ .../bufferhub/tests/UniqueIdGenerator_test.cpp | 45 ---------------- 12 files changed, 180 insertions(+), 169 deletions(-) create mode 100644 services/bufferhub/BufferHubIdGenerator.cpp delete mode 100644 services/bufferhub/UniqueIdGenerator.cpp create mode 100644 services/bufferhub/include/bufferhub/BufferHubIdGenerator.h delete mode 100644 services/bufferhub/include/bufferhub/UniqueIdGenerator.h create mode 100644 services/bufferhub/tests/BufferHubIdGenerator_test.cpp delete mode 100644 services/bufferhub/tests/UniqueIdGenerator_test.cpp (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/Android.bp b/services/bufferhub/Android.bp index b747dbca4b..f9aaa2050f 100644 --- a/services/bufferhub/Android.bp +++ b/services/bufferhub/Android.bp @@ -24,9 +24,9 @@ cc_library_shared { ], srcs: [ "BufferClient.cpp", + "BufferHubIdGenerator.cpp", "BufferHubService.cpp", "BufferNode.cpp", - "UniqueIdGenerator.cpp", ], header_libs: [ "libbufferhub_headers", diff --git a/services/bufferhub/BufferHubIdGenerator.cpp b/services/bufferhub/BufferHubIdGenerator.cpp new file mode 100644 index 0000000000..6444a033e1 --- /dev/null +++ b/services/bufferhub/BufferHubIdGenerator.cpp @@ -0,0 +1,61 @@ +/* + * Copyright (C) 2018 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. + */ + +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +constexpr uint32_t BufferHubIdGenerator::kInvalidId; + +BufferHubIdGenerator& BufferHubIdGenerator::getInstance() { + static BufferHubIdGenerator generator; + + return generator; +} + +uint32_t BufferHubIdGenerator::getId() { + std::lock_guard lock(mIdsInUseMutex); + + do { + if (++mLastId >= std::numeric_limits::max()) { + mLastId = kInvalidId + 1; + } + } while (mIdsInUse.find(mLastId) != mIdsInUse.end()); + + mIdsInUse.insert(mLastId); + return mLastId; +} + +bool BufferHubIdGenerator::freeId(uint32_t id) { + std::lock_guard lock(mIdsInUseMutex); + auto iter = mIdsInUse.find(id); + if (iter != mIdsInUse.end()) { + mIdsInUse.erase(iter); + return true; + } + + return false; +} + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 6f97f0d8c0..6eaf0abe8e 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -35,7 +35,8 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d std::shared_ptr node = std::make_shared(desc.width, desc.height, desc.layers, desc.format, - desc.usage, userMetadataSize, nodeIdGenerator.getId()); + desc.usage, userMetadataSize, + BufferHubIdGenerator::getInstance().getId()); if (node == nullptr || !node->IsValid()) { ALOGE("%s: creating BufferNode failed.", __FUNCTION__); _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::ALLOCATION_FAILED); diff --git a/services/bufferhub/BufferNode.cpp b/services/bufferhub/BufferNode.cpp index 715d0a1f67..ec84849e22 100644 --- a/services/bufferhub/BufferNode.cpp +++ b/services/bufferhub/BufferNode.cpp @@ -66,8 +66,8 @@ BufferNode::~BufferNode() { } // Free the id, if valid - if (id() != UniqueIdGenerator::kInvalidId) { - if (nodeIdGenerator.freeId(id())) { + if (id() != BufferHubIdGenerator::kInvalidId) { + if (BufferHubIdGenerator::getInstance().freeId(id())) { ALOGI("%s: id #%u is freed.", __FUNCTION__, id()); } else { ALOGE("%s: Cannot free nonexistent id #%u", __FUNCTION__, id()); diff --git a/services/bufferhub/UniqueIdGenerator.cpp b/services/bufferhub/UniqueIdGenerator.cpp deleted file mode 100644 index 362a026f6c..0000000000 --- a/services/bufferhub/UniqueIdGenerator.cpp +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright (C) 2018 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. - */ - -#include - -namespace android { -namespace frameworks { -namespace bufferhub { -namespace V1_0 { -namespace implementation { - -constexpr uint32_t UniqueIdGenerator::kInvalidId; - -uint32_t UniqueIdGenerator::getId() { - std::lock_guard lock(mIdsInUseMutex); - - do { - if (++mLastId >= std::numeric_limits::max()) { - mLastId = kInvalidId + 1; - } - } while (mIdsInUse.find(mLastId) != mIdsInUse.end()); - - mIdsInUse.insert(mLastId); - return mLastId; -} - -bool UniqueIdGenerator::freeId(uint32_t id) { - std::lock_guard lock(mIdsInUseMutex); - auto iter = mIdsInUse.find(id); - if (iter != mIdsInUse.end()) { - mIdsInUse.erase(iter); - return true; - } - - return false; -} - -} // namespace implementation -} // namespace V1_0 -} // namespace bufferhub -} // namespace frameworks -} // namespace android diff --git a/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h b/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h new file mode 100644 index 0000000000..c5b2cdef33 --- /dev/null +++ b/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h @@ -0,0 +1,63 @@ +/* + * Copyright (C) 2018 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_FRAMEWORKS_BUFFERHUB_V1_0_ID_GENERATOR_H +#define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_ID_GENERATOR_H + +#include +#include + +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +// A thread-safe incremental uint32_t id generator. +class BufferHubIdGenerator { +public: + // 0 is considered invalid + static constexpr uint32_t kInvalidId = 0UL; + + // Get the singleton instance of this class + static BufferHubIdGenerator& getInstance(); + + // Gets next available id. If next id is greater than std::numeric_limits::max() (2 ^ + // 32 - 1), it will try to get an id start from 1 again. + uint32_t getId(); + + // Free a specific id. Return true on freed, false on not found. + bool freeId(uint32_t id); + +private: + BufferHubIdGenerator() = default; + ~BufferHubIdGenerator() = default; + + std::mutex mIdsInUseMutex; + // Start from kInvalidID to avoid generating it. + uint32_t mLastId = kInvalidId; + std::set mIdsInUse GUARDED_BY(mIdsInUseMutex); +}; + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android + +#endif // ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_ID_GENERATOR_H diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 6535659ae7..63997aeff9 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -22,7 +22,7 @@ #include #include -#include +#include #include namespace android { @@ -35,8 +35,6 @@ using hardware::hidl_handle; using hardware::Return; using hardware::graphics::common::V1_2::HardwareBufferDescription; -static UniqueIdGenerator nodeIdGenerator; - class BufferHubService : public IBufferHub { public: Return allocateBuffer(const HardwareBufferDescription& description, diff --git a/services/bufferhub/include/bufferhub/BufferNode.h b/services/bufferhub/include/bufferhub/BufferNode.h index c490e7c709..94ef505d41 100644 --- a/services/bufferhub/include/bufferhub/BufferNode.h +++ b/services/bufferhub/include/bufferhub/BufferNode.h @@ -2,7 +2,7 @@ #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_NODE_H_ #include -#include +#include #include namespace android { @@ -16,7 +16,7 @@ public: // Allocates a new BufferNode. BufferNode(uint32_t width, uint32_t height, uint32_t layer_count, uint32_t format, uint64_t usage, size_t user_metadata_size, - uint32_t id = UniqueIdGenerator::kInvalidId); + uint32_t id = BufferHubIdGenerator::kInvalidId); ~BufferNode(); diff --git a/services/bufferhub/include/bufferhub/UniqueIdGenerator.h b/services/bufferhub/include/bufferhub/UniqueIdGenerator.h deleted file mode 100644 index d2e702f7e0..0000000000 --- a/services/bufferhub/include/bufferhub/UniqueIdGenerator.h +++ /dev/null @@ -1,57 +0,0 @@ -/* - * Copyright (C) 2018 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_FRAMEWORKS_BUFFERHUB_V1_0_ID_GENERATOR_H -#define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_ID_GENERATOR_H - -#include -#include - -#include - -namespace android { -namespace frameworks { -namespace bufferhub { -namespace V1_0 { -namespace implementation { - -// A thread-safe incremental uint32_t id generator. -class UniqueIdGenerator { -public: - // 0 is considered invalid - static constexpr uint32_t kInvalidId = 0UL; - - // Gets next available id. If next id is greater than std::numeric_limits::max() (2 ^ - // 32 - 1), it will try to get an id start from 1 again. - uint32_t getId(); - - // Free a specific id. Return true on freed, false on not found. - bool freeId(uint32_t id); - -private: - std::mutex mIdsInUseMutex; - // Start from kInvalidID to avoid generating it. - uint32_t mLastId = kInvalidId; - std::set mIdsInUse GUARDED_BY(mIdsInUseMutex); -}; - -} // namespace implementation -} // namespace V1_0 -} // namespace bufferhub -} // namespace frameworks -} // namespace android - -#endif // ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_ID_GENERATOR_H diff --git a/services/bufferhub/tests/Android.bp b/services/bufferhub/tests/Android.bp index 8a30ef5820..39678865cb 100644 --- a/services/bufferhub/tests/Android.bp +++ b/services/bufferhub/tests/Android.bp @@ -24,10 +24,10 @@ cc_test { } cc_test { - name: "UniqueIdGenerator_test", - srcs: ["UniqueIdGenerator_test.cpp"], + name: "BufferHubIdGenerator_test", + srcs: ["BufferHubIdGenerator_test.cpp"], cflags: [ - "-DLOG_TAG=\"UniqueIdGenerator_test\"", + "-DLOG_TAG=\"BufferHubIdGenerator_test\"", "-DTRACE=0", "-DATRACE_TAG=ATRACE_TAG_GRAPHICS", ], diff --git a/services/bufferhub/tests/BufferHubIdGenerator_test.cpp b/services/bufferhub/tests/BufferHubIdGenerator_test.cpp new file mode 100644 index 0000000000..4eddfe0e9b --- /dev/null +++ b/services/bufferhub/tests/BufferHubIdGenerator_test.cpp @@ -0,0 +1,45 @@ +#include +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +namespace { + +class BufferHubIdGeneratorTest : public testing::Test { +protected: + BufferHubIdGenerator* mIdGenerator = &BufferHubIdGenerator::getInstance(); +}; + +TEST_F(BufferHubIdGeneratorTest, TestGenerateAndFreeID) { + uint32_t id = mIdGenerator->getId(); + EXPECT_NE(id, BufferHubIdGenerator::kInvalidId); + + EXPECT_TRUE(mIdGenerator->freeId(id)); + EXPECT_FALSE(mIdGenerator->freeId(id)); +} + +TEST_F(BufferHubIdGeneratorTest, TestGenerateUniqueIncrementalID) { + // 10 IDs should not overflow the UniqueIdGenerator to cause a roll back to start, so the + // resulting IDs should still keep incresing. + const size_t kTestSize = 10U; + uint32_t ids[kTestSize]; + for (int i = 0; i < kTestSize; ++i) { + ids[i] = mIdGenerator->getId(); + EXPECT_NE(ids[i], BufferHubIdGenerator::kInvalidId); + if (i >= 1) { + EXPECT_GT(ids[i], ids[i - 1]); + } + } +} + +} // namespace + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android \ No newline at end of file diff --git a/services/bufferhub/tests/UniqueIdGenerator_test.cpp b/services/bufferhub/tests/UniqueIdGenerator_test.cpp deleted file mode 100644 index c4d83e0c1f..0000000000 --- a/services/bufferhub/tests/UniqueIdGenerator_test.cpp +++ /dev/null @@ -1,45 +0,0 @@ -#include -#include - -namespace android { -namespace frameworks { -namespace bufferhub { -namespace V1_0 { -namespace implementation { - -namespace { - -class UniqueIdGeneratorTest : public testing::Test { -protected: - UniqueIdGenerator mIdGenerator; -}; - -TEST_F(UniqueIdGeneratorTest, TestGenerateAndFreeID) { - uint32_t id = mIdGenerator.getId(); - EXPECT_NE(id, UniqueIdGenerator::kInvalidId); - - EXPECT_TRUE(mIdGenerator.freeId(id)); - EXPECT_FALSE(mIdGenerator.freeId(id)); -} - -TEST_F(UniqueIdGeneratorTest, TestGenerateUniqueIncrementalID) { - // 10 IDs should not overflow the UniqueIdGenerator to cause a roll back to start, so the - // resulting IDs should still keep incresing. - const size_t kTestSize = 10U; - uint32_t ids[kTestSize]; - for (int i = 0; i < kTestSize; ++i) { - ids[i] = mIdGenerator.getId(); - EXPECT_NE(ids[i], UniqueIdGenerator::kInvalidId); - if (i >= 1) { - EXPECT_GT(ids[i], ids[i - 1]); - } - } -} - -} // namespace - -} // namespace implementation -} // namespace V1_0 -} // namespace bufferhub -} // namespace frameworks -} // namespace android \ No newline at end of file -- cgit v1.2.3-59-g8ed1b From f8f4a45fbefe998270cf62fb866774b028f7082a Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Thu, 29 Nov 2018 16:26:30 -0800 Subject: Implement new definition of IBufferHub Now allocateBuffer and importBuffer will return BufferTraits description to client side. The hidl_handle BufferInfo requires some complex logic to pack and unpack, which would be part of another CL. Test: VtsFwkBufferHubV1_0TargetTest (Passed) Bug: 116681016 Change-Id: Iff99b08360a5de2f300546ffd8e9b215518a83c8 --- services/bufferhub/BufferHubService.cpp | 43 +++++++++++++++++++--- .../bufferhub/include/bufferhub/BufferClient.h | 3 ++ 2 files changed, 40 insertions(+), 6 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index b0869fe755..26638126d3 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -39,7 +39,8 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d BufferHubIdGenerator::getInstance().getId()); if (node == nullptr || !node->IsValid()) { ALOGE("%s: creating BufferNode failed.", __FUNCTION__); - _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::ALLOCATION_FAILED); + _hidl_cb(/*status=*/BufferHubStatus::ALLOCATION_FAILED, /*bufferClient=*/nullptr, + /*bufferTraits=*/{}); return Void(); } @@ -48,7 +49,13 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d std::lock_guard lock(mClientSetMutex); mClientSet.emplace(client); - _hidl_cb(/*bufferClient=*/client, /*status=*/BufferHubStatus::NO_ERROR); + BufferTraits bufferTraits = {/*bufferDesc=*/description, + /*bufferHandle=*/hidl_handle(node->buffer_handle()), + // TODO(b/116681016): return real data to client + /*bufferInfo=*/hidl_handle()}; + + _hidl_cb(/*status=*/BufferHubStatus::NO_ERROR, /*bufferClient=*/client, + /*bufferTraits=*/bufferTraits); return Void(); } @@ -56,7 +63,8 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, importBuffer_cb _hidl_cb) { if (!tokenHandle.getNativeHandle() || tokenHandle->numFds != 0 || tokenHandle->numInts != 1) { // nullptr handle or wrong format - _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::INVALID_TOKEN); + _hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr, + /*bufferTraits=*/{}); return Void(); } @@ -68,7 +76,8 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, auto iter = mTokenMap.find(token); if (iter == mTokenMap.end()) { // Invalid token - _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::INVALID_TOKEN); + _hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr, + /*bufferTraits=*/{}); return Void(); } @@ -81,15 +90,37 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, if (!originClient) { // Should not happen since token should be removed if already gone ALOGE("%s: original client %p gone!", __FUNCTION__, originClientWp.unsafe_get()); - _hidl_cb(/*bufferClient=*/nullptr, /*status=*/BufferHubStatus::BUFFER_FREED); + _hidl_cb(/*status=*/BufferHubStatus::BUFFER_FREED, /*bufferClient=*/nullptr, + /*bufferTraits=*/{}); return Void(); } sp client = new BufferClient(*originClient); + uint32_t clientStateMask = client->getBufferNode()->AddNewActiveClientsBitToMask(); + if (clientStateMask == 0U) { + // Reach max client count + ALOGE("%s: import failed, BufferNode#%u reached maximum clients.", __FUNCTION__, + client->getBufferNode()->id()); + _hidl_cb(/*status=*/BufferHubStatus::MAX_CLIENT, /*bufferClient=*/nullptr, + /*bufferTraits=*/{}); + return Void(); + } std::lock_guard lock(mClientSetMutex); mClientSet.emplace(client); - _hidl_cb(/*bufferClient=*/client, /*status=*/BufferHubStatus::NO_ERROR); + + std::shared_ptr node = client->getBufferNode(); + + HardwareBufferDescription bufferDesc; + memcpy(&bufferDesc, &node->buffer_desc(), sizeof(HardwareBufferDescription)); + + BufferTraits bufferTraits = {/*bufferDesc=*/bufferDesc, + /*bufferHandle=*/hidl_handle(node->buffer_handle()), + // TODO(b/116681016): return real data to client + /*bufferInfo=*/hidl_handle()}; + + _hidl_cb(/*status=*/BufferHubStatus::NO_ERROR, /*bufferClient=*/client, + /*bufferTraits=*/bufferTraits); return Void(); } diff --git a/services/bufferhub/include/bufferhub/BufferClient.h b/services/bufferhub/include/bufferhub/BufferClient.h index 7f5d3a6aa1..66ed4bd13f 100644 --- a/services/bufferhub/include/bufferhub/BufferClient.h +++ b/services/bufferhub/include/bufferhub/BufferClient.h @@ -49,6 +49,9 @@ public: Return close() override; Return duplicate(duplicate_cb _hidl_cb) override; + // Non-binder functions + const std::shared_ptr& getBufferNode() const { return mBufferNode; } + private: BufferClient(wp service, const std::shared_ptr& node) : mService(service), mBufferNode(node) {} -- cgit v1.2.3-59-g8ed1b From fe097c76f3989e7f852597d748bb1d88e5da91c6 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Fri, 7 Dec 2018 15:46:51 -0800 Subject: Add lshal debug function to bufferhub Using similar logic from services/vr/bufferhubd/buffer_hub.cpp. Test: adb shell lshal debug android.frameworks.bufferhub@1.0::IBufferHub Fix: 116526156 Change-Id: Ie13660752a3a9325f518e8144f548de361b5c00c --- services/bufferhub/BufferHubService.cpp | 141 ++++++++++++++++++++- .../bufferhub/include/bufferhub/BufferHubService.h | 4 + 2 files changed, 143 insertions(+), 2 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 26638126d3..54796a2896 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -14,10 +14,17 @@ * limitations under the License. */ +#include +#include + #include #include #include #include +#include + +using ::android::BufferHubDefs::MetadataHeader; +using ::android::hardware::Void; namespace android { namespace frameworks { @@ -25,8 +32,6 @@ namespace bufferhub { namespace V1_0 { namespace implementation { -using hardware::Void; - Return BufferHubService::allocateBuffer(const HardwareBufferDescription& description, const uint32_t userMetadataSize, allocateBuffer_cb _hidl_cb) { @@ -124,6 +129,138 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, return Void(); } +Return BufferHubService::debug(const hidl_handle& fd, const hidl_vec& args) { + if (fd.getNativeHandle() == nullptr || fd->numFds < 1) { + ALOGE("%s: missing fd for writing.", __FUNCTION__); + return Void(); + } + + FILE* out = fdopen(dup(fd->data[0]), "w"); + + if (args.size() != 0) { + fprintf(out, + "Note: lshal bufferhub currently does not support args. Input arguments are " + "ignored.\n"); + } + + std::ostringstream stream; + + // Get the number of clients of each buffer. + // Map from bufferId to bufferNode_clientCount pair. + std::map, uint32_t>> clientCount; + { + std::lock_guard lock(mClientSetMutex); + for (auto iter = mClientSet.begin(); iter != mClientSet.end(); ++iter) { + sp client = iter->promote(); + if (client != nullptr) { + const std::shared_ptr node = client->getBufferNode(); + auto mapIter = clientCount.find(node->id()); + if (mapIter != clientCount.end()) { + ++mapIter->second.second; + } else { + clientCount.emplace(node->id(), + std::pair, uint32_t>(node, 1U)); + } + } + } + } + + stream << "Active Buffers:\n"; + stream << std::right; + stream << std::setw(6) << "Id"; + stream << " "; + stream << std::setw(9) << "Clients"; + stream << " "; + stream << std::setw(14) << "Geometry"; + stream << " "; + stream << std::setw(6) << "Format"; + stream << " "; + stream << std::setw(10) << "Usage"; + stream << " "; + stream << std::setw(10) << "State"; + stream << " "; + stream << std::setw(10) << "Index"; + stream << std::endl; + + for (auto iter = clientCount.begin(); iter != clientCount.end(); ++iter) { + const std::shared_ptr node = std::move(iter->second.first); + const uint32_t clientCount = iter->second.second; + AHardwareBuffer_Desc desc = node->buffer_desc(); + + MetadataHeader* metadataHeader = + const_cast(&node->metadata())->metadata_header(); + const uint32_t state = metadataHeader->buffer_state.load(std::memory_order_acquire); + const uint64_t index = metadataHeader->queue_index; + + stream << std::right; + stream << std::setw(6) << /*Id=*/node->id(); + stream << " "; + stream << std::setw(9) << /*Clients=*/clientCount; + stream << " "; + if (desc.format == HAL_PIXEL_FORMAT_BLOB) { + std::string size = std::to_string(desc.width) + " B"; + stream << std::setw(14) << /*Geometry=*/size; + } else { + std::string dimensions = std::to_string(desc.width) + "x" + + std::to_string(desc.height) + "x" + std::to_string(desc.layers); + stream << std::setw(14) << /*Geometry=*/dimensions; + } + stream << " "; + stream << std::setw(6) << /*Format=*/desc.format; + stream << " "; + stream << "0x" << std::hex << std::setfill('0'); + stream << std::setw(8) << /*Usage=*/desc.usage; + stream << std::dec << std::setfill(' '); + stream << " "; + stream << "0x" << std::hex << std::setfill('0'); + stream << std::setw(8) << /*State=*/state; + stream << " "; + stream << std::setw(8) << /*Index=*/index; + stream << std::endl; + } + + stream << std::endl; + + // Get the number of tokens of each buffer. + // Map from bufferId to tokenCount + std::map tokenCount; + { + std::lock_guard lock(mTokenMapMutex); + for (auto iter = mTokenMap.begin(); iter != mTokenMap.end(); ++iter) { + sp client = iter->second.promote(); + if (client != nullptr) { + const std::shared_ptr node = client->getBufferNode(); + auto mapIter = tokenCount.find(node->id()); + if (mapIter != tokenCount.end()) { + ++mapIter->second; + } else { + tokenCount.emplace(node->id(), 1U); + } + } + } + } + + stream << "Unused Tokens:\n"; + stream << std::right; + stream << std::setw(8) << "Buffer Id"; + stream << " "; + stream << std::setw(6) << "Tokens"; + stream << std::endl; + + for (auto iter = tokenCount.begin(); iter != tokenCount.end(); ++iter) { + stream << std::right; + stream << std::setw(8) << /*Buffer Id=*/iter->first; + stream << " "; + stream << std::setw(6) << /*Tokens=*/iter->second; + stream << std::endl; + } + + fprintf(out, "%s", stream.str().c_str()); + + fclose(out); + return Void(); +} + hidl_handle BufferHubService::registerToken(const wp& client) { uint32_t token; std::lock_guard lock(mTokenMapMutex); diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index f2c8ef8965..255f329bcf 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -32,6 +32,8 @@ namespace V1_0 { namespace implementation { using hardware::hidl_handle; +using hardware::hidl_string; +using hardware::hidl_vec; using hardware::Return; using hardware::graphics::common::V1_2::HardwareBufferDescription; @@ -42,6 +44,8 @@ public: allocateBuffer_cb _hidl_cb) override; Return importBuffer(const hidl_handle& tokenHandle, importBuffer_cb _hidl_cb) override; + Return debug(const hidl_handle& fd, const hidl_vec& args) override; + // Non-binder functions // Internal help function for IBufferClient::duplicate. hidl_handle registerToken(const wp& client); -- cgit v1.2.3-59-g8ed1b From 55b26a6101460780acaa756ef43fbc73d1b82898 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Wed, 19 Dec 2018 11:03:14 -0800 Subject: Refactor bufferhub token to be more secure Now BufferHubService will use HMAC hashing from openssl library to enhance the security of the token. The token handle will use its first int to store token id, then the HMAC hash of the token id, so that BufferHubService could store token in mTokenMap using token id as the key to improve looking up performance. Test: VtsHalBufferHubV1_0TargetTest Change-Id: Ica9639bc1504a477c6a1b1f703399bb07b3a2d2c Fix: 118180214 --- services/bufferhub/Android.bp | 2 + services/bufferhub/BufferHubService.cpp | 82 +++++++++++++++++----- .../bufferhub/include/bufferhub/BufferHubService.h | 24 +++++-- 3 files changed, 84 insertions(+), 24 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/Android.bp b/services/bufferhub/Android.bp index 72d210cbb0..a4b81feffc 100644 --- a/services/bufferhub/Android.bp +++ b/services/bufferhub/Android.bp @@ -34,6 +34,7 @@ cc_library_shared { ], shared_libs: [ "android.frameworks.bufferhub@1.0", + "libcrypto", "libcutils", "libhidlbase", "libhidltransport", @@ -60,6 +61,7 @@ cc_binary { shared_libs: [ "android.frameworks.bufferhub@1.0", "libbufferhubservice", + "libcrypto", "libcutils", "libhidltransport", "libhwbinder", diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 54796a2896..43235f24c3 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -14,13 +14,16 @@ * limitations under the License. */ +#include #include +#include #include #include #include #include #include +#include #include using ::android::BufferHubDefs::MetadataHeader; @@ -32,6 +35,13 @@ namespace bufferhub { namespace V1_0 { namespace implementation { +BufferHubService::BufferHubService() { + std::mt19937_64 randomEngine; + randomEngine.seed(time(nullptr)); + + mKey = randomEngine(); +} + Return BufferHubService::allocateBuffer(const HardwareBufferDescription& description, const uint32_t userMetadataSize, allocateBuffer_cb _hidl_cb) { @@ -66,27 +76,49 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, importBuffer_cb _hidl_cb) { - if (!tokenHandle.getNativeHandle() || tokenHandle->numFds != 0 || tokenHandle->numInts != 1) { + if (!tokenHandle.getNativeHandle() || tokenHandle->numFds != 0 || tokenHandle->numInts <= 1) { // nullptr handle or wrong format _hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr, /*bufferTraits=*/{}); return Void(); } - uint32_t token = tokenHandle->data[0]; + int tokenId = tokenHandle->data[0]; wp originClientWp; { - std::lock_guard lock(mTokenMapMutex); - auto iter = mTokenMap.find(token); + std::lock_guard lock(mTokenMutex); + auto iter = mTokenMap.find(tokenId); if (iter == mTokenMap.end()) { - // Invalid token + // Token Id not exist + ALOGD("%s: token #%d not found.", __FUNCTION__, tokenId); + _hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr, + /*bufferTraits=*/{}); + return Void(); + } + + const std::vector& tokenHMAC = iter->second.first; + + int numIntsForHMAC = (int)ceil(tokenHMAC.size() * sizeof(uint8_t) / (double)sizeof(int)); + if (tokenHandle->numInts - 1 != numIntsForHMAC) { + // HMAC size not match + ALOGD("%s: token #%d HMAC size not match. Expected: %d Actual: %d", __FUNCTION__, + tokenId, numIntsForHMAC, tokenHandle->numInts - 1); _hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr, /*bufferTraits=*/{}); return Void(); } - originClientWp = iter->second; + size_t hmacSize = tokenHMAC.size() * sizeof(uint8_t); + if (memcmp(tokenHMAC.data(), &tokenHandle->data[1], hmacSize) != 0) { + // HMAC not match + ALOGD("%s: token #%d HMAC not match.", __FUNCTION__, tokenId); + _hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr, + /*bufferTraits=*/{}); + return Void(); + } + + originClientWp = iter->second.second; mTokenMap.erase(iter); } @@ -225,9 +257,9 @@ Return BufferHubService::debug(const hidl_handle& fd, const hidl_vec tokenCount; { - std::lock_guard lock(mTokenMapMutex); + std::lock_guard lock(mTokenMutex); for (auto iter = mTokenMap.begin(); iter != mTokenMap.end(); ++iter) { - sp client = iter->second.promote(); + sp client = iter->second.second.promote(); if (client != nullptr) { const std::shared_ptr node = client->getBufferNode(); auto mapIter = tokenCount.find(node->id()); @@ -262,21 +294,35 @@ Return BufferHubService::debug(const hidl_handle& fd, const hidl_vec& client) { - uint32_t token; - std::lock_guard lock(mTokenMapMutex); + // Find next available token id + std::lock_guard lock(mTokenMutex); do { - token = mTokenEngine(); - } while (mTokenMap.find(token) != mTokenMap.end()); + ++mLastTokenId; + } while (mTokenMap.find(mLastTokenId) != mTokenMap.end()); + + std::array hmac; + uint32_t hmacSize = 0U; - // native_handle_t use int[], so here need one slots to fit in uint32_t - native_handle_t* handle = native_handle_create(/*numFds=*/0, /*numInts=*/1); - handle->data[0] = token; + HMAC(/*evp_md=*/EVP_sha256(), /*key=*/&mKey, /*key_len=*/kKeyLen, + /*data=*/(uint8_t*)&mLastTokenId, /*data_len=*/mTokenIdSize, + /*out=*/hmac.data(), /*out_len=*/&hmacSize); + + int numIntsForHMAC = (int)ceil(hmacSize / (double)sizeof(int)); + native_handle_t* handle = native_handle_create(/*numFds=*/0, /*numInts=*/1 + numIntsForHMAC); + handle->data[0] = mLastTokenId; + // Set all the the bits of last int to 0 since it might not be fully overwritten + handle->data[numIntsForHMAC] = 0; + memcpy(&handle->data[1], hmac.data(), hmacSize); // returnToken owns the native_handle_t* thus doing lifecycle management hidl_handle returnToken; returnToken.setTo(handle, /*shoudOwn=*/true); - mTokenMap.emplace(token, client); + std::vector hmacVec; + hmacVec.resize(hmacSize); + memcpy(hmacVec.data(), hmac.data(), hmacSize); + mTokenMap.emplace(mLastTokenId, std::pair(hmacVec, client)); + return returnToken; } @@ -291,10 +337,10 @@ void BufferHubService::onClientClosed(const BufferClient* client) { } void BufferHubService::removeTokenByClient(const BufferClient* client) { - std::lock_guard lock(mTokenMapMutex); + std::lock_guard lock(mTokenMutex); auto iter = mTokenMap.begin(); while (iter != mTokenMap.end()) { - if (iter->second == client) { + if (iter->second.second == client) { auto oldIter = iter; ++iter; mTokenMap.erase(oldIter); diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 255f329bcf..0195bcf498 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -17,8 +17,10 @@ #ifndef ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H +#include #include -#include +#include +#include #include #include @@ -39,6 +41,8 @@ using hardware::graphics::common::V1_2::HardwareBufferDescription; class BufferHubService : public IBufferHub { public: + BufferHubService(); + Return allocateBuffer(const HardwareBufferDescription& description, const uint32_t userMetadataSize, allocateBuffer_cb _hidl_cb) override; @@ -60,11 +64,19 @@ private: std::mutex mClientSetMutex; std::set> mClientSet GUARDED_BY(mClientSetMutex); - // TODO(b/118180214): use a more secure implementation - std::mt19937 mTokenEngine; - // The mapping from token to the client creates it. - std::mutex mTokenMapMutex; - std::map> mTokenMap GUARDED_BY(mTokenMapMutex); + // Token generation related + // A random number used as private key for HMAC + uint64_t mKey; + static constexpr size_t kKeyLen = sizeof(uint64_t); + + std::mutex mTokenMutex; + // The first TokenId will be 1. TokenId could be negative. + int mLastTokenId GUARDED_BY(mTokenMutex) = 0; + static constexpr size_t mTokenIdSize = sizeof(int); + // A map from token id to the token-buffer_client pair. Using token id as the key to reduce + // looking up time + std::map, const wp>> mTokenMap + GUARDED_BY(mTokenMutex); }; } // namespace implementation -- cgit v1.2.3-59-g8ed1b From efce32efd5d9fdbf83796329ac66089f5ba0ce38 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Wed, 12 Dec 2018 14:34:16 -0800 Subject: Implement bufferInfo handle packing logic The service now will use buildBufferInfo() function to build the handle and return real data to client. Test: VtsHalBufferHubV1_0TargetTest Bug: 116681016 Change-Id: I45baa91cc4f91f817c82b1c59243f48440a5558a --- libs/ui/include/ui/BufferHubDefs.h | 23 +++++++++++++++ services/bufferhub/BufferHubService.cpp | 33 +++++++++++++++++++--- .../bufferhub/include/bufferhub/BufferHubService.h | 4 +++ 3 files changed, 56 insertions(+), 4 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/include/ui/BufferHubDefs.h b/libs/ui/include/ui/BufferHubDefs.h index d259fefb8f..069f0dc25e 100644 --- a/libs/ui/include/ui/BufferHubDefs.h +++ b/libs/ui/include/ui/BufferHubDefs.h @@ -158,6 +158,29 @@ struct __attribute__((aligned(8))) MetadataHeader { static_assert(sizeof(MetadataHeader) == 128, "Unexpected MetadataHeader size"); static constexpr size_t kMetadataHeaderSize = sizeof(MetadataHeader); +/** + * android.frameworks.bufferhub@1.0::BufferTraits.bufferInfo is an opaque handle. See + * https://cs.corp.google.com/android/frameworks/hardware/interfaces/bufferhub/1.0/types.hal for + * more details about android.frameworks.bufferhub@1.0::BufferTraits. + * + * This definition could be changed, but implementation of BufferHubService::buildBufferInfo + * (frameworks/native/services/bufferhub), VtsHalBufferHubV1_0TargetTest + * (frameworks/hardware/interfaces/bufferhub) and BufferHubBuffer::readBufferTraits (libui) will + * also need to be updated. + * + * It's definition should follow the following format: + * { + * NumFds = 1, + * NumInts = 3, + * data[0] = Ashmem fd for BufferHubMetadata, + * data[1] = buffer id, + * data[2] = client state bit mask, + * data[3] = user metadata size, + * } + */ +static constexpr int kBufferInfoNumFds = 1; +static constexpr int kBufferInfoNumInts = 3; + } // namespace BufferHubDefs } // namespace android diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 43235f24c3..6389521e6b 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -25,6 +25,7 @@ #include #include #include +#include using ::android::BufferHubDefs::MetadataHeader; using ::android::hardware::Void; @@ -64,10 +65,12 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d std::lock_guard lock(mClientSetMutex); mClientSet.emplace(client); + hidl_handle bufferInfo = + buildBufferInfo(node->id(), node->AddNewActiveClientsBitToMask(), + node->user_metadata_size(), node->metadata().ashmem_fd()); BufferTraits bufferTraits = {/*bufferDesc=*/description, /*bufferHandle=*/hidl_handle(node->buffer_handle()), - // TODO(b/116681016): return real data to client - /*bufferInfo=*/hidl_handle()}; + /*bufferInfo=*/bufferInfo}; _hidl_cb(/*status=*/BufferHubStatus::NO_ERROR, /*bufferClient=*/client, /*bufferTraits=*/bufferTraits); @@ -151,10 +154,12 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, HardwareBufferDescription bufferDesc; memcpy(&bufferDesc, &node->buffer_desc(), sizeof(HardwareBufferDescription)); + hidl_handle bufferInfo = + buildBufferInfo(node->id(), clientStateMask, node->user_metadata_size(), + node->metadata().ashmem_fd()); BufferTraits bufferTraits = {/*bufferDesc=*/bufferDesc, /*bufferHandle=*/hidl_handle(node->buffer_handle()), - // TODO(b/116681016): return real data to client - /*bufferInfo=*/hidl_handle()}; + /*bufferInfo=*/bufferInfo}; _hidl_cb(/*status=*/BufferHubStatus::NO_ERROR, /*bufferClient=*/client, /*bufferTraits=*/bufferTraits); @@ -336,6 +341,26 @@ void BufferHubService::onClientClosed(const BufferClient* client) { } } +// Implementation of this function should be consistent with the definition of bufferInfo handle in +// ui/BufferHubDefs.h. +hidl_handle BufferHubService::buildBufferInfo(uint32_t bufferId, uint32_t clientBitMask, + uint32_t userMetadataSize, const int metadataFd) { + native_handle_t* infoHandle = native_handle_create(BufferHubDefs::kBufferInfoNumFds, + BufferHubDefs::kBufferInfoNumInts); + + infoHandle->data[0] = dup(metadataFd); + // Use memcpy to convert to int without missing digit. + // TOOD(b/121345852): use bit_cast to unpack bufferInfo when C++20 becomes available. + memcpy(&infoHandle->data[1], &bufferId, sizeof(bufferId)); + memcpy(&infoHandle->data[2], &clientBitMask, sizeof(clientBitMask)); + memcpy(&infoHandle->data[3], &userMetadataSize, sizeof(userMetadataSize)); + + hidl_handle bufferInfo; + bufferInfo.setTo(infoHandle, /*shouldOwn=*/true); + + return bufferInfo; +} + void BufferHubService::removeTokenByClient(const BufferClient* client) { std::lock_guard lock(mTokenMutex); auto iter = mTokenMap.begin(); diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 0195bcf498..04ba5b76f9 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -57,6 +57,10 @@ public: void onClientClosed(const BufferClient* client); private: + // Helper function to build BufferTraits.bufferInfo handle + hidl_handle buildBufferInfo(uint32_t bufferId, uint32_t clientBitMask, + uint32_t userMetadataSize, const int metadataFd); + // Helper function to remove all the token belongs to a specific client. void removeTokenByClient(const BufferClient* client); -- cgit v1.2.3-59-g8ed1b From 6b4b1e5b1f7ad32dcadc49a2455370003b20897a Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Tue, 18 Dec 2018 14:03:44 -0800 Subject: Change bufferhub's bufferId to int After discussion we decided that our new system should still use int as type of buffer id. Now the Id generator will generate int ids >= 0 instead of uint32_t ids > 0. Remove redundant log in destructor of BufferNode, now only log when freeId failed. Update BufferHubIdGenerator_test.cpp to fit in current API. Add code to cleanup generated ids in TestGenerateUniqueIncrementalID. Test: BufferHubServer_test Change-Id: I2018bfd009a3c311a99b9114762d0f4fbf1e3fe2 Fix: 118844348 --- services/bufferhub/BufferHubIdGenerator.cpp | 16 +++++++--------- services/bufferhub/BufferHubService.cpp | 4 ++-- services/bufferhub/BufferNode.cpp | 10 +++------- .../include/bufferhub/BufferHubIdGenerator.h | 22 ++++++++++------------ .../bufferhub/include/bufferhub/BufferHubService.h | 4 ++-- services/bufferhub/include/bufferhub/BufferNode.h | 13 ++++++------- .../bufferhub/tests/BufferHubIdGenerator_test.cpp | 19 +++++++++++-------- 7 files changed, 41 insertions(+), 47 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/BufferHubIdGenerator.cpp b/services/bufferhub/BufferHubIdGenerator.cpp index 6444a033e1..2c12f0e26d 100644 --- a/services/bufferhub/BufferHubIdGenerator.cpp +++ b/services/bufferhub/BufferHubIdGenerator.cpp @@ -15,6 +15,7 @@ */ #include +#include namespace android { namespace frameworks { @@ -22,20 +23,18 @@ namespace bufferhub { namespace V1_0 { namespace implementation { -constexpr uint32_t BufferHubIdGenerator::kInvalidId; - BufferHubIdGenerator& BufferHubIdGenerator::getInstance() { static BufferHubIdGenerator generator; return generator; } -uint32_t BufferHubIdGenerator::getId() { +int BufferHubIdGenerator::getId() { std::lock_guard lock(mIdsInUseMutex); do { - if (++mLastId >= std::numeric_limits::max()) { - mLastId = kInvalidId + 1; + if (++mLastId >= std::numeric_limits::max()) { + mLastId = 0; } } while (mIdsInUse.find(mLastId) != mIdsInUse.end()); @@ -43,15 +42,14 @@ uint32_t BufferHubIdGenerator::getId() { return mLastId; } -bool BufferHubIdGenerator::freeId(uint32_t id) { +void BufferHubIdGenerator::freeId(int id) { std::lock_guard lock(mIdsInUseMutex); auto iter = mIdsInUse.find(id); if (iter != mIdsInUse.end()) { mIdsInUse.erase(iter); - return true; + } else { + ALOGW("%s: Cannot free nonexistent id #%d", __FUNCTION__, id); } - - return false; } } // namespace implementation diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 6389521e6b..ad49cd62ed 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -343,15 +343,15 @@ void BufferHubService::onClientClosed(const BufferClient* client) { // Implementation of this function should be consistent with the definition of bufferInfo handle in // ui/BufferHubDefs.h. -hidl_handle BufferHubService::buildBufferInfo(uint32_t bufferId, uint32_t clientBitMask, +hidl_handle BufferHubService::buildBufferInfo(int bufferId, uint32_t clientBitMask, uint32_t userMetadataSize, const int metadataFd) { native_handle_t* infoHandle = native_handle_create(BufferHubDefs::kBufferInfoNumFds, BufferHubDefs::kBufferInfoNumInts); infoHandle->data[0] = dup(metadataFd); + infoHandle->data[1] = bufferId; // Use memcpy to convert to int without missing digit. // TOOD(b/121345852): use bit_cast to unpack bufferInfo when C++20 becomes available. - memcpy(&infoHandle->data[1], &bufferId, sizeof(bufferId)); memcpy(&infoHandle->data[2], &clientBitMask, sizeof(clientBitMask)); memcpy(&infoHandle->data[3], &userMetadataSize, sizeof(userMetadataSize)); diff --git a/services/bufferhub/BufferNode.cpp b/services/bufferhub/BufferNode.cpp index da19a6fb1d..51063903b5 100644 --- a/services/bufferhub/BufferNode.cpp +++ b/services/bufferhub/BufferNode.cpp @@ -30,7 +30,7 @@ void BufferNode::InitializeMetadata() { // Allocates a new BufferNode. BufferNode::BufferNode(uint32_t width, uint32_t height, uint32_t layer_count, uint32_t format, - uint64_t usage, size_t user_metadata_size, uint32_t id) + uint64_t usage, size_t user_metadata_size, int id) : mId(id) { uint32_t out_stride = 0; // graphicBufferId is not used in GraphicBufferAllocator::allocate @@ -73,12 +73,8 @@ BufferNode::~BufferNode() { } // Free the id, if valid - if (id() != BufferHubIdGenerator::kInvalidId) { - if (BufferHubIdGenerator::getInstance().freeId(id())) { - ALOGI("%s: id #%u is freed.", __FUNCTION__, id()); - } else { - ALOGE("%s: Cannot free nonexistent id #%u", __FUNCTION__, id()); - } + if (mId >= 0) { + BufferHubIdGenerator::getInstance().freeId(mId); } } diff --git a/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h b/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h index b51fcda3cd..ef7c077feb 100644 --- a/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h +++ b/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h @@ -28,30 +28,28 @@ namespace bufferhub { namespace V1_0 { namespace implementation { -// A thread-safe incremental uint32_t id generator. +// A thread-safe, non-negative, incremental, int id generator. class BufferHubIdGenerator { public: - // 0 is considered invalid - static constexpr uint32_t kInvalidId = 0U; - // Get the singleton instance of this class static BufferHubIdGenerator& getInstance(); - // Gets next available id. If next id is greater than std::numeric_limits::max() (2 ^ - // 32 - 1), it will try to get an id start from 1 again. - uint32_t getId(); + // Gets next available id. If next id is greater than std::numeric_limits::max(), it + // will try to get an id start from 0 again. + int getId(); - // Free a specific id. Return true on freed, false on not found. - bool freeId(uint32_t id); + // Free a specific id. + void freeId(int id); private: BufferHubIdGenerator() = default; ~BufferHubIdGenerator() = default; + // Start from -1 so all valid ids will be >= 0 + int mLastId = -1; + std::mutex mIdsInUseMutex; - // Start from kInvalidID to avoid generating it. - uint32_t mLastId = kInvalidId; - std::set mIdsInUse GUARDED_BY(mIdsInUseMutex); + std::set mIdsInUse GUARDED_BY(mIdsInUseMutex); }; } // namespace implementation diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 04ba5b76f9..23e664e036 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -58,8 +58,8 @@ public: private: // Helper function to build BufferTraits.bufferInfo handle - hidl_handle buildBufferInfo(uint32_t bufferId, uint32_t clientBitMask, - uint32_t userMetadataSize, const int metadataFd); + hidl_handle buildBufferInfo(int bufferId, uint32_t clientBitMask, uint32_t userMetadataSize, + const int metadataFd); // Helper function to remove all the token belongs to a specific client. void removeTokenByClient(const BufferClient* client); diff --git a/services/bufferhub/include/bufferhub/BufferNode.h b/services/bufferhub/include/bufferhub/BufferNode.h index cf56c33ec0..b7a195b0a1 100644 --- a/services/bufferhub/include/bufferhub/BufferNode.h +++ b/services/bufferhub/include/bufferhub/BufferNode.h @@ -16,15 +16,14 @@ class BufferNode { public: // Allocates a new BufferNode. BufferNode(uint32_t width, uint32_t height, uint32_t layer_count, uint32_t format, - uint64_t usage, size_t user_metadata_size, - uint32_t id = BufferHubIdGenerator::kInvalidId); + uint64_t usage, size_t user_metadata_size, int id = -1); ~BufferNode(); // Returns whether the object holds a valid metadata. bool IsValid() const { return metadata_.IsValid(); } - uint32_t id() const { return mId; } + int id() const { return mId; } size_t user_metadata_size() const { return metadata_.user_metadata_size(); } @@ -64,10 +63,10 @@ private: // Metadata in shared memory. BufferHubMetadata metadata_; - // A system-unique id generated by bufferhub from 1 to std::numeric_limits::max(). - // BufferNodes not created by bufferhub will have id = 0, meaning "not specified". - // TODO(b/118891412): remove default id = 0 and update comments after pdx is no longer in use - const uint32_t mId = 0; + // A system-unique id generated by bufferhub from 0 to std::numeric_limits::max(). + // BufferNodes not created by bufferhub will have id < 0, meaning "not specified". + // TODO(b/118891412): remove default id = -1 and update comments after pdx is no longer in use + const int mId = -1; // The following variables are atomic variables in metadata_ that are visible // to Bn object and Bp objects. Please find more info in diff --git a/services/bufferhub/tests/BufferHubIdGenerator_test.cpp b/services/bufferhub/tests/BufferHubIdGenerator_test.cpp index fe010137fd..fb6de0d18e 100644 --- a/services/bufferhub/tests/BufferHubIdGenerator_test.cpp +++ b/services/bufferhub/tests/BufferHubIdGenerator_test.cpp @@ -15,25 +15,28 @@ protected: }; TEST_F(BufferHubIdGeneratorTest, TestGenerateAndFreeID) { - uint32_t id = mIdGenerator->getId(); - EXPECT_NE(id, BufferHubIdGenerator::kInvalidId); + int id = mIdGenerator->getId(); + EXPECT_GE(id, 0); - EXPECT_TRUE(mIdGenerator->freeId(id)); - EXPECT_FALSE(mIdGenerator->freeId(id)); + mIdGenerator->freeId(id); } TEST_F(BufferHubIdGeneratorTest, TestGenerateUniqueIncrementalID) { // 10 IDs should not overflow the UniqueIdGenerator to cause a roll back to start, so the // resulting IDs should still keep incresing. - const size_t kTestSize = 10U; - uint32_t ids[kTestSize]; - for (size_t i = 0UL; i < kTestSize; ++i) { + const int kTestSize = 10; + int ids[kTestSize]; + for (int i = 0; i < kTestSize; ++i) { ids[i] = mIdGenerator->getId(); - EXPECT_NE(ids[i], BufferHubIdGenerator::kInvalidId); + EXPECT_GE(ids[i], 0); if (i >= 1) { EXPECT_GT(ids[i], ids[i - 1]); } } + + for (int i = 0; i < kTestSize; ++i) { + mIdGenerator->freeId(ids[i]); + } } } // namespace -- cgit v1.2.3-59-g8ed1b From f653d0ba2b70e6e36bcea1e2547affd902ed188c Mon Sep 17 00:00:00 2001 From: Jiwen 'Steve' Cai Date: Tue, 15 Jan 2019 10:52:20 -0800 Subject: Fix lshal for BufferHub Service Bug: 122867618 Test: adb shell lshal debug android.frameworks.bufferhub@1.0::IBufferHub Change-Id: I2cde444b724ec7707e1579c560a362a6d5c6e918 --- services/bufferhub/BufferHubService.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index ad49cd62ed..a40443d8d9 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -216,7 +216,7 @@ Return BufferHubService::debug(const hidl_handle& fd, const hidl_vec BufferHubService::debug(const hidl_handle& fd, const hidl_vec Date: Tue, 15 Jan 2019 15:02:15 -0800 Subject: Add BufferHubEventFd to bufferhub system Now BufferNode will create a new BufferHubEventFd instance during allocation, BufferHubService will pass that fd to client side, and BufferHubBuffer will now extract and import it. User can now access the eventFd by a public function of BufferHubBuffer. Update BufferHubBuffer_test to check if the two event fds are actually linked to each other. Test: VtsHalBufferHubV1_0TargetTest, BufferHubServer_test, BufferHub_test Fix: 68770788 Change-Id: I7f6e07d17615d2b45c0e7e086c481292c5798e97 --- libs/ui/BufferHubBuffer.cpp | 16 +++++++++---- libs/ui/include/ui/BufferHubBuffer.h | 6 +++++ libs/ui/include/ui/BufferHubDefs.h | 11 +++++---- libs/ui/tests/BufferHubBuffer_test.cpp | 27 ++++++++++++++++++++++ services/bufferhub/BufferHubService.cpp | 18 ++++++++------- .../bufferhub/include/bufferhub/BufferHubService.h | 2 +- services/bufferhub/include/bufferhub/BufferNode.h | 7 ++++++ 7 files changed, 68 insertions(+), 19 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/BufferHubBuffer.cpp b/libs/ui/BufferHubBuffer.cpp index 6310f29152..9669135ad6 100644 --- a/libs/ui/BufferHubBuffer.cpp +++ b/libs/ui/BufferHubBuffer.cpp @@ -167,19 +167,26 @@ int BufferHubBuffer::initWithBufferTraits(const BufferTraits& bufferTraits) { return -EINVAL; } - int bufferId = bufferTraits.bufferInfo->data[1]; + int bufferId = bufferTraits.bufferInfo->data[2]; if (bufferId < 0) { ALOGE("%s: Received an invalid (negative) id!", __FUNCTION__); return -EINVAL; } uint32_t clientBitMask; - memcpy(&clientBitMask, &bufferTraits.bufferInfo->data[2], sizeof(clientBitMask)); + memcpy(&clientBitMask, &bufferTraits.bufferInfo->data[3], sizeof(clientBitMask)); if (clientBitMask == 0U) { ALOGE("%s: Received a invalid client state mask!", __FUNCTION__); return -EINVAL; } + const int eventFd = bufferTraits.bufferInfo->data[1]; + if (eventFd < 0) { + ALOGE("%s: Received a invalid event fd!", __FUNCTION__); + return -EINVAL; + } + mEventFd = BufferHubEventFd(eventFd); + // Import the metadata. Dup since hidl_handle owns the fd unique_fd ashmemFd(fcntl(bufferTraits.bufferInfo->data[0], F_DUPFD_CLOEXEC, 0)); mMetadata = BufferHubMetadata::Import(std::move(ashmemFd)); @@ -190,7 +197,7 @@ int BufferHubBuffer::initWithBufferTraits(const BufferTraits& bufferTraits) { } uint32_t userMetadataSize; - memcpy(&userMetadataSize, &bufferTraits.bufferInfo->data[3], sizeof(userMetadataSize)); + memcpy(&userMetadataSize, &bufferTraits.bufferInfo->data[4], sizeof(userMetadataSize)); if (mMetadata.user_metadata_size() != userMetadataSize) { ALOGE("%s: user metadata size not match: expected %u, actual %zu.", __FUNCTION__, userMetadataSize, mMetadata.user_metadata_size()); @@ -314,9 +321,8 @@ int BufferHubBuffer::Release() { } bool BufferHubBuffer::IsValid() const { - // TODO(b/68770788): check eventFd once implemented return mBufferHandle.getNativeHandle() != nullptr && mId >= 0 && mClientStateMask != 0U && - mMetadata.IsValid() && mBufferClient != nullptr; + mEventFd.get() >= 0 && mMetadata.IsValid() && mBufferClient != nullptr; } native_handle_t* BufferHubBuffer::Duplicate() { diff --git a/libs/ui/include/ui/BufferHubBuffer.h b/libs/ui/include/ui/BufferHubBuffer.h index c6a4a232d1..42d9320545 100644 --- a/libs/ui/include/ui/BufferHubBuffer.h +++ b/libs/ui/include/ui/BufferHubBuffer.h @@ -21,6 +21,7 @@ #include #include #include +#include #include namespace android { @@ -55,6 +56,8 @@ public: return native_handle_clone(mBufferHandle.getNativeHandle()); } + const BufferHubEventFd& eventFd() const { return mEventFd; } + // Returns the current value of MetadataHeader::buffer_state. uint32_t buffer_state() { return mMetadata.metadata_header()->buffer_state.load(std::memory_order_acquire); @@ -123,6 +126,9 @@ private: // Wraps the gralloc buffer handle of this buffer. hardware::hidl_handle mBufferHandle; + // Event fd used for signalling buffer state changes. Shared by all clients of the same buffer. + BufferHubEventFd mEventFd; + // An ashmem-based metadata object. The same shared memory are mapped to the // bufferhubd daemon and all buffer clients. BufferHubMetadata mMetadata; diff --git a/libs/ui/include/ui/BufferHubDefs.h b/libs/ui/include/ui/BufferHubDefs.h index 069f0dc25e..43d900c034 100644 --- a/libs/ui/include/ui/BufferHubDefs.h +++ b/libs/ui/include/ui/BufferHubDefs.h @@ -170,15 +170,16 @@ static constexpr size_t kMetadataHeaderSize = sizeof(MetadataHeader); * * It's definition should follow the following format: * { - * NumFds = 1, + * NumFds = 2, * NumInts = 3, * data[0] = Ashmem fd for BufferHubMetadata, - * data[1] = buffer id, - * data[2] = client state bit mask, - * data[3] = user metadata size, + * data[1] = event fd, + * data[2] = buffer id, + * data[3] = client state bit mask, + * data[4] = user metadata size, * } */ -static constexpr int kBufferInfoNumFds = 1; +static constexpr int kBufferInfoNumFds = 2; static constexpr int kBufferInfoNumInts = 3; } // namespace BufferHubDefs diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp index cd744cd106..77ea19cdfb 100644 --- a/libs/ui/tests/BufferHubBuffer_test.cpp +++ b/libs/ui/tests/BufferHubBuffer_test.cpp @@ -16,6 +16,8 @@ #define LOG_TAG "BufferHubBufferTest" +#include + #include #include #include @@ -23,6 +25,7 @@ #include #include #include +#include namespace android { @@ -160,6 +163,30 @@ TEST_F(BufferHubBufferTest, DuplicateAndImportBuffer) { // Both buffer instances should be in released state currently. EXPECT_TRUE(IsBufferReleased(b1->buffer_state())); EXPECT_TRUE(IsBufferReleased(b2->buffer_state())); + + // The event fd should behave like duped event fds. + const BufferHubEventFd& eventFd1 = b1->eventFd(); + const BufferHubEventFd& eventFd2 = b2->eventFd(); + + base::unique_fd epollFd(epoll_create(64)); + ASSERT_GE(epollFd.get(), 0); + + // Add eventFd1 to epoll set, and signal eventFd2. + epoll_event e = {.events = EPOLLIN | EPOLLET, .data = {.u32 = 0}}; + ASSERT_EQ(epoll_ctl(epollFd.get(), EPOLL_CTL_ADD, eventFd1.get(), &e), 0); + + std::array events; + EXPECT_EQ(epoll_wait(epollFd.get(), events.data(), events.size(), 0), 0); + + eventFd2.signal(); + EXPECT_EQ(epoll_wait(epollFd.get(), events.data(), events.size(), 0), 1); + + // The epoll fd is edge triggered, so it only responds to the eventFd once. + EXPECT_EQ(epoll_wait(epollFd.get(), events.data(), events.size(), 0), 0); + + eventFd2.signal(); + eventFd2.clear(); + EXPECT_EQ(epoll_wait(epollFd.get(), events.data(), events.size(), 0), 0); } TEST_F(BufferHubBufferTest, ImportFreedBuffer) { diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index a40443d8d9..0b29a80e00 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -65,9 +65,9 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d std::lock_guard lock(mClientSetMutex); mClientSet.emplace(client); - hidl_handle bufferInfo = - buildBufferInfo(node->id(), node->AddNewActiveClientsBitToMask(), - node->user_metadata_size(), node->metadata().ashmem_fd()); + hidl_handle bufferInfo = buildBufferInfo(node->id(), node->AddNewActiveClientsBitToMask(), + node->user_metadata_size(), node->eventFd().get(), + node->metadata().ashmem_fd()); BufferTraits bufferTraits = {/*bufferDesc=*/description, /*bufferHandle=*/hidl_handle(node->buffer_handle()), /*bufferInfo=*/bufferInfo}; @@ -156,7 +156,7 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, hidl_handle bufferInfo = buildBufferInfo(node->id(), clientStateMask, node->user_metadata_size(), - node->metadata().ashmem_fd()); + node->eventFd().get(), node->metadata().ashmem_fd()); BufferTraits bufferTraits = {/*bufferDesc=*/bufferDesc, /*bufferHandle=*/hidl_handle(node->buffer_handle()), /*bufferInfo=*/bufferInfo}; @@ -345,16 +345,18 @@ void BufferHubService::onClientClosed(const BufferClient* client) { // Implementation of this function should be consistent with the definition of bufferInfo handle in // ui/BufferHubDefs.h. hidl_handle BufferHubService::buildBufferInfo(int bufferId, uint32_t clientBitMask, - uint32_t userMetadataSize, const int metadataFd) { + uint32_t userMetadataSize, const int eventFd, + const int metadataFd) { native_handle_t* infoHandle = native_handle_create(BufferHubDefs::kBufferInfoNumFds, BufferHubDefs::kBufferInfoNumInts); infoHandle->data[0] = dup(metadataFd); - infoHandle->data[1] = bufferId; + infoHandle->data[1] = dup(eventFd); + infoHandle->data[2] = bufferId; // Use memcpy to convert to int without missing digit. // TOOD(b/121345852): use bit_cast to unpack bufferInfo when C++20 becomes available. - memcpy(&infoHandle->data[2], &clientBitMask, sizeof(clientBitMask)); - memcpy(&infoHandle->data[3], &userMetadataSize, sizeof(userMetadataSize)); + memcpy(&infoHandle->data[3], &clientBitMask, sizeof(clientBitMask)); + memcpy(&infoHandle->data[4], &userMetadataSize, sizeof(userMetadataSize)); hidl_handle bufferInfo; bufferInfo.setTo(infoHandle, /*shouldOwn=*/true); diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 23e664e036..9015e05e46 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -59,7 +59,7 @@ public: private: // Helper function to build BufferTraits.bufferInfo handle hidl_handle buildBufferInfo(int bufferId, uint32_t clientBitMask, uint32_t userMetadataSize, - const int metadataFd); + const int eventFd, const int metadataFd); // Helper function to remove all the token belongs to a specific client. void removeTokenByClient(const BufferClient* client); diff --git a/services/bufferhub/include/bufferhub/BufferNode.h b/services/bufferhub/include/bufferhub/BufferNode.h index b7a195b0a1..4729e1cc73 100644 --- a/services/bufferhub/include/bufferhub/BufferNode.h +++ b/services/bufferhub/include/bufferhub/BufferNode.h @@ -4,6 +4,7 @@ #include #include #include +#include #include namespace android { @@ -31,6 +32,9 @@ public: const native_handle_t* buffer_handle() const { return buffer_handle_; } const AHardwareBuffer_Desc& buffer_desc() const { return buffer_desc_; } + // Accessor of event fd. + const BufferHubEventFd& eventFd() const { return mEventFd; } + // Accessors of metadata. const BufferHubMetadata& metadata() const { return metadata_; } @@ -60,6 +64,9 @@ private: native_handle_t* buffer_handle_; AHardwareBuffer_Desc buffer_desc_; + // Eventfd used for signalling buffer events among the clients of the buffer. + BufferHubEventFd mEventFd; + // Metadata in shared memory. BufferHubMetadata metadata_; -- cgit v1.2.3-59-g8ed1b From 0f69e5e0dce5f5a4c34f28184898282397103ff7 Mon Sep 17 00:00:00 2001 From: Tianyu Jiang Date: Wed, 23 Jan 2019 17:17:59 -0800 Subject: Clarify the output of the lshal debug. Change-Id: Ib7f360187dfc85e114956d94c186cbb7be03e506 Fix: 123313838 Test: adb shell lshal debug android.frameworks.bufferhub@1.0::IBufferHub --- services/bufferhub/BufferHubService.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index a40443d8d9..ccefe2ff45 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -206,7 +206,7 @@ Return BufferHubService::debug(const hidl_handle& fd, const hidl_vec BufferHubService::debug(const hidl_handle& fd, const hidl_vecid(); stream << " "; - stream << std::setw(9) << /*Clients=*/clientCount; + stream << std::setw(9) << /*#Clients=*/clientCount; stream << " "; if (desc.format == HAL_PIXEL_FORMAT_BLOB) { std::string size = std::to_string(desc.width) + " B"; @@ -282,14 +282,14 @@ Return BufferHubService::debug(const hidl_handle& fd, const hidl_vecfirst; stream << " "; - stream << std::setw(6) << /*Tokens=*/iter->second; + stream << std::setw(7) << /*#Tokens=*/iter->second; stream << std::endl; } -- cgit v1.2.3-59-g8ed1b From 8adb319152c639d382625789c7be558be7f7c9b5 Mon Sep 17 00:00:00 2001 From: Tianyu Jiang Date: Tue, 29 Jan 2019 10:54:24 -0800 Subject: Remove unnecessary copies of hidl_handle and its contents Fix: 123582513 Test: BufferHub_test BufferHubServer_test VtsHalBufferHubV1_0TargetTest Change-Id: I4d186d5ce34bc3b9714d94bfbcd6b0ce34294a6d --- services/bufferhub/BufferHubService.cpp | 48 +++++++++++++--------- .../bufferhub/include/bufferhub/BufferHubService.h | 4 +- 2 files changed, 31 insertions(+), 21 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index ae357cda8a..6b802fbf83 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -65,15 +65,20 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d std::lock_guard lock(mClientSetMutex); mClientSet.emplace(client); - hidl_handle bufferInfo = buildBufferInfo(node->id(), node->AddNewActiveClientsBitToMask(), - node->user_metadata_size(), node->eventFd().get(), - node->metadata().ashmem_fd()); + // Allocate memory for bufferInfo of type hidl_handle on the stack. See + // http://aosp/286282 for the usage of NATIVE_HANDLE_DECLARE_STORAGE. + NATIVE_HANDLE_DECLARE_STORAGE(bufferInfoStorage, BufferHubDefs::kBufferInfoNumFds, + BufferHubDefs::kBufferInfoNumInts); + hidl_handle bufferInfo = + buildBufferInfo(bufferInfoStorage, node->id(), node->AddNewActiveClientsBitToMask(), + node->user_metadata_size(), node->metadata().ashmem_fd(), + node->eventFd().get()); BufferTraits bufferTraits = {/*bufferDesc=*/description, /*bufferHandle=*/hidl_handle(node->buffer_handle()), - /*bufferInfo=*/bufferInfo}; + /*bufferInfo=*/std::move(bufferInfo)}; _hidl_cb(/*status=*/BufferHubStatus::NO_ERROR, /*bufferClient=*/client, - /*bufferTraits=*/bufferTraits); + /*bufferTraits=*/std::move(bufferTraits)); return Void(); } @@ -154,15 +159,19 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, HardwareBufferDescription bufferDesc; memcpy(&bufferDesc, &node->buffer_desc(), sizeof(HardwareBufferDescription)); - hidl_handle bufferInfo = - buildBufferInfo(node->id(), clientStateMask, node->user_metadata_size(), - node->eventFd().get(), node->metadata().ashmem_fd()); + // Allocate memory for bufferInfo of type hidl_handle on the stack. See + // http://aosp/286282 for the usage of NATIVE_HANDLE_DECLARE_STORAGE. + NATIVE_HANDLE_DECLARE_STORAGE(bufferInfoStorage, BufferHubDefs::kBufferInfoNumFds, + BufferHubDefs::kBufferInfoNumInts); + hidl_handle bufferInfo = buildBufferInfo(bufferInfoStorage, node->id(), clientStateMask, + node->user_metadata_size(), + node->metadata().ashmem_fd(), node->eventFd().get()); BufferTraits bufferTraits = {/*bufferDesc=*/bufferDesc, /*bufferHandle=*/hidl_handle(node->buffer_handle()), - /*bufferInfo=*/bufferInfo}; + /*bufferInfo=*/std::move(bufferInfo)}; _hidl_cb(/*status=*/BufferHubStatus::NO_ERROR, /*bufferClient=*/client, - /*bufferTraits=*/bufferTraits); + /*bufferTraits=*/std::move(bufferTraits)); return Void(); } @@ -344,14 +353,15 @@ void BufferHubService::onClientClosed(const BufferClient* client) { // Implementation of this function should be consistent with the definition of bufferInfo handle in // ui/BufferHubDefs.h. -hidl_handle BufferHubService::buildBufferInfo(int bufferId, uint32_t clientBitMask, - uint32_t userMetadataSize, const int eventFd, - const int metadataFd) { - native_handle_t* infoHandle = native_handle_create(BufferHubDefs::kBufferInfoNumFds, - BufferHubDefs::kBufferInfoNumInts); - - infoHandle->data[0] = dup(metadataFd); - infoHandle->data[1] = dup(eventFd); +hidl_handle BufferHubService::buildBufferInfo(char* bufferInfoStorage, int bufferId, + uint32_t clientBitMask, uint32_t userMetadataSize, + int metadataFd, int eventFd) { + native_handle_t* infoHandle = + native_handle_init(bufferInfoStorage, BufferHubDefs::kBufferInfoNumFds, + BufferHubDefs::kBufferInfoNumInts); + + infoHandle->data[0] = metadataFd; + infoHandle->data[1] = eventFd; infoHandle->data[2] = bufferId; // Use memcpy to convert to int without missing digit. // TOOD(b/121345852): use bit_cast to unpack bufferInfo when C++20 becomes available. @@ -359,7 +369,7 @@ hidl_handle BufferHubService::buildBufferInfo(int bufferId, uint32_t clientBitMa memcpy(&infoHandle->data[4], &userMetadataSize, sizeof(userMetadataSize)); hidl_handle bufferInfo; - bufferInfo.setTo(infoHandle, /*shouldOwn=*/true); + bufferInfo.setTo(infoHandle, /*shouldOwn=*/false); return bufferInfo; } diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 9015e05e46..edad20b194 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -58,8 +58,8 @@ public: private: // Helper function to build BufferTraits.bufferInfo handle - hidl_handle buildBufferInfo(int bufferId, uint32_t clientBitMask, uint32_t userMetadataSize, - const int eventFd, const int metadataFd); + hidl_handle buildBufferInfo(char* bufferInfoStorage, int bufferId, uint32_t clientBitMask, + uint32_t userMetadataSize, int metadataFd, int eventFd); // Helper function to remove all the token belongs to a specific client. void removeTokenByClient(const BufferClient* client); -- cgit v1.2.3-59-g8ed1b From 727ede4a0fd4fbc6621833b21579da255eb1fb68 Mon Sep 17 00:00:00 2001 From: Tianyu Jiang Date: Fri, 1 Feb 2019 11:44:51 -0800 Subject: Fix non camelCase function names Android Framework C++ Code Style Guidelines says that function names should be camelCase: http://go/droidcppstyle Test: m, mma in frameworks/native Bug: 68273829 Change-Id: I2f661c06b31b2e72cd0eee3d91b95531b60ec939 --- libs/ui/BufferHubBuffer.cpp | 62 ++--- libs/ui/BufferHubMetadata.cpp | 8 +- libs/ui/GraphicBuffer.cpp | 2 +- libs/ui/include/ui/BufferHubBuffer.h | 28 +-- libs/ui/include/ui/BufferHubDefs.h | 16 +- libs/ui/include/ui/BufferHubMetadata.h | 14 +- libs/ui/tests/BufferHubBuffer_test.cpp | 272 ++++++++++----------- libs/ui/tests/BufferHubMetadata_test.cpp | 101 ++++---- libs/ui/tests/GraphicBuffer_test.cpp | 8 +- libs/vr/libbufferhub/buffer_hub-test.cpp | 46 ++-- libs/vr/libbufferhub/consumer_buffer.cpp | 6 +- .../include/private/dvr/buffer_hub_defs.h | 32 +-- libs/vr/libbufferhub/producer_buffer.cpp | 20 +- .../libbufferhubqueue/buffer_hub_queue_client.cpp | 4 +- services/bufferhub/BufferHubService.cpp | 22 +- services/bufferhub/BufferNode.cpp | 20 +- services/bufferhub/include/bufferhub/BufferNode.h | 16 +- services/bufferhub/tests/BufferNode_test.cpp | 47 ++-- services/vr/bufferhubd/producer_channel.cpp | 22 +- services/vr/bufferhubd/producer_queue_channel.cpp | 2 +- 20 files changed, 373 insertions(+), 375 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/BufferHubBuffer.cpp b/libs/ui/BufferHubBuffer.cpp index c3144b924d..7693fcbfe0 100644 --- a/libs/ui/BufferHubBuffer.cpp +++ b/libs/ui/BufferHubBuffer.cpp @@ -24,12 +24,12 @@ #include using ::android::base::unique_fd; -using ::android::BufferHubDefs::AnyClientAcquired; -using ::android::BufferHubDefs::AnyClientGained; -using ::android::BufferHubDefs::IsClientAcquired; -using ::android::BufferHubDefs::IsClientGained; -using ::android::BufferHubDefs::IsClientPosted; -using ::android::BufferHubDefs::IsClientReleased; +using ::android::BufferHubDefs::isAnyClientAcquired; +using ::android::BufferHubDefs::isAnyClientGained; +using ::android::BufferHubDefs::isClientAcquired; +using ::android::BufferHubDefs::isClientGained; +using ::android::BufferHubDefs::isClientPosted; +using ::android::BufferHubDefs::isClientReleased; using ::android::frameworks::bufferhub::V1_0::BufferHubStatus; using ::android::frameworks::bufferhub::V1_0::BufferTraits; using ::android::frameworks::bufferhub::V1_0::IBufferClient; @@ -39,22 +39,22 @@ using ::android::hardware::graphics::common::V1_2::HardwareBufferDescription; namespace android { -std::unique_ptr BufferHubBuffer::Create(uint32_t width, uint32_t height, +std::unique_ptr BufferHubBuffer::create(uint32_t width, uint32_t height, uint32_t layerCount, uint32_t format, uint64_t usage, size_t userMetadataSize) { auto buffer = std::unique_ptr( new BufferHubBuffer(width, height, layerCount, format, usage, userMetadataSize)); - return buffer->IsValid() ? std::move(buffer) : nullptr; + return buffer->isValid() ? std::move(buffer) : nullptr; } -std::unique_ptr BufferHubBuffer::Import(const native_handle_t* token) { +std::unique_ptr BufferHubBuffer::import(const native_handle_t* token) { if (token == nullptr) { ALOGE("%s: token cannot be nullptr!", __FUNCTION__); return nullptr; } auto buffer = std::unique_ptr(new BufferHubBuffer(token)); - return buffer->IsValid() ? std::move(buffer) : nullptr; + return buffer->isValid() ? std::move(buffer) : nullptr; } BufferHubBuffer::BufferHubBuffer(uint32_t width, uint32_t height, uint32_t layerCount, @@ -169,8 +169,8 @@ int BufferHubBuffer::initWithBufferTraits(const BufferTraits& bufferTraits) { // Import fds. Dup fds because hidl_handle owns the fds. unique_fd ashmemFd(fcntl(bufferTraits.bufferInfo->data[0], F_DUPFD_CLOEXEC, 0)); - mMetadata = BufferHubMetadata::Import(std::move(ashmemFd)); - if (!mMetadata.IsValid()) { + mMetadata = BufferHubMetadata::import(std::move(ashmemFd)); + if (!mMetadata.isValid()) { ALOGE("%s: Received an invalid metadata.", __FUNCTION__); return -EINVAL; } @@ -196,20 +196,20 @@ int BufferHubBuffer::initWithBufferTraits(const BufferTraits& bufferTraits) { uint32_t userMetadataSize; memcpy(&userMetadataSize, &bufferTraits.bufferInfo->data[4], sizeof(userMetadataSize)); - if (mMetadata.user_metadata_size() != userMetadataSize) { + if (mMetadata.userMetadataSize() != userMetadataSize) { ALOGE("%s: user metadata size not match: expected %u, actual %zu.", __FUNCTION__, - userMetadataSize, mMetadata.user_metadata_size()); + userMetadataSize, mMetadata.userMetadataSize()); return -EINVAL; } - size_t metadataSize = static_cast(mMetadata.metadata_size()); + size_t metadataSize = static_cast(mMetadata.metadataSize()); if (metadataSize < BufferHubDefs::kMetadataHeaderSize) { ALOGE("%s: metadata too small: %zu", __FUNCTION__, metadataSize); return -EINVAL; } // Populate shortcuts to the atomics in metadata. - auto metadata_header = mMetadata.metadata_header(); + auto metadata_header = mMetadata.metadataHeader(); mBufferState = &metadata_header->buffer_state; mFenceState = &metadata_header->fence_state; mActiveClientsBitMask = &metadata_header->active_clients_bit_mask; @@ -235,16 +235,16 @@ int BufferHubBuffer::initWithBufferTraits(const BufferTraits& bufferTraits) { return 0; } -int BufferHubBuffer::Gain() { +int BufferHubBuffer::gain() { uint32_t currentBufferState = mBufferState->load(std::memory_order_acquire); - if (IsClientGained(currentBufferState, mClientStateMask)) { + if (isClientGained(currentBufferState, mClientStateMask)) { ALOGV("%s: Buffer is already gained by this client %" PRIx32 ".", __FUNCTION__, mClientStateMask); return 0; } do { - if (AnyClientGained(currentBufferState & (~mClientStateMask)) || - AnyClientAcquired(currentBufferState)) { + if (isAnyClientGained(currentBufferState & (~mClientStateMask)) || + isAnyClientAcquired(currentBufferState)) { ALOGE("%s: Buffer is in use, id=%d mClientStateMask=%" PRIx32 " state=%" PRIx32 ".", __FUNCTION__, mId, mClientStateMask, currentBufferState); return -EBUSY; @@ -258,11 +258,11 @@ int BufferHubBuffer::Gain() { return 0; } -int BufferHubBuffer::Post() { +int BufferHubBuffer::post() { uint32_t currentBufferState = mBufferState->load(std::memory_order_acquire); uint32_t updatedBufferState = (~mClientStateMask) & BufferHubDefs::kHighBitsMask; do { - if (!IsClientGained(currentBufferState, mClientStateMask)) { + if (!isClientGained(currentBufferState, mClientStateMask)) { ALOGE("%s: Cannot post a buffer that is not gained by this client. buffer_id=%d " "mClientStateMask=%" PRIx32 " state=%" PRIx32 ".", __FUNCTION__, mId, mClientStateMask, currentBufferState); @@ -277,16 +277,16 @@ int BufferHubBuffer::Post() { return 0; } -int BufferHubBuffer::Acquire() { +int BufferHubBuffer::acquire() { uint32_t currentBufferState = mBufferState->load(std::memory_order_acquire); - if (IsClientAcquired(currentBufferState, mClientStateMask)) { + if (isClientAcquired(currentBufferState, mClientStateMask)) { ALOGV("%s: Buffer is already acquired by this client %" PRIx32 ".", __FUNCTION__, mClientStateMask); return 0; } uint32_t updatedBufferState = 0U; do { - if (!IsClientPosted(currentBufferState, mClientStateMask)) { + if (!isClientPosted(currentBufferState, mClientStateMask)) { ALOGE("%s: Cannot acquire a buffer that is not in posted state. buffer_id=%d " "mClientStateMask=%" PRIx32 " state=%" PRIx32 ".", __FUNCTION__, mId, mClientStateMask, currentBufferState); @@ -301,9 +301,9 @@ int BufferHubBuffer::Acquire() { return 0; } -int BufferHubBuffer::Release() { +int BufferHubBuffer::release() { uint32_t currentBufferState = mBufferState->load(std::memory_order_acquire); - if (IsClientReleased(currentBufferState, mClientStateMask)) { + if (isClientReleased(currentBufferState, mClientStateMask)) { ALOGV("%s: Buffer is already released by this client %" PRIx32 ".", __FUNCTION__, mClientStateMask); return 0; @@ -318,17 +318,17 @@ int BufferHubBuffer::Release() { return 0; } -bool BufferHubBuffer::IsReleased() const { +bool BufferHubBuffer::isReleased() const { return (mBufferState->load(std::memory_order_acquire) & mActiveClientsBitMask->load(std::memory_order_acquire)) == 0; } -bool BufferHubBuffer::IsValid() const { +bool BufferHubBuffer::isValid() const { return mBufferHandle.getNativeHandle() != nullptr && mId >= 0 && mClientStateMask != 0U && - mEventFd.get() >= 0 && mMetadata.IsValid() && mBufferClient != nullptr; + mEventFd.get() >= 0 && mMetadata.isValid() && mBufferClient != nullptr; } -native_handle_t* BufferHubBuffer::Duplicate() { +native_handle_t* BufferHubBuffer::duplicate() { if (mBufferClient == nullptr) { ALOGE("%s: missing BufferClient!", __FUNCTION__); return nullptr; diff --git a/libs/ui/BufferHubMetadata.cpp b/libs/ui/BufferHubMetadata.cpp index 816707db9d..05bc7ddfbe 100644 --- a/libs/ui/BufferHubMetadata.cpp +++ b/libs/ui/BufferHubMetadata.cpp @@ -34,7 +34,7 @@ using BufferHubDefs::kMetadataHeaderSize; using BufferHubDefs::MetadataHeader; /* static */ -BufferHubMetadata BufferHubMetadata::Create(size_t userMetadataSize) { +BufferHubMetadata BufferHubMetadata::create(size_t userMetadataSize) { // The size the of metadata buffer is used as the "width" parameter during allocation. Thus it // cannot overflow uint32_t. if (userMetadataSize >= (std::numeric_limits::max() - kMetadataHeaderSize)) { @@ -59,11 +59,11 @@ BufferHubMetadata BufferHubMetadata::Create(size_t userMetadataSize) { return {}; } - return BufferHubMetadata::Import(std::move(ashmemFd)); + return BufferHubMetadata::import(std::move(ashmemFd)); } /* static */ -BufferHubMetadata BufferHubMetadata::Import(unique_fd ashmemFd) { +BufferHubMetadata BufferHubMetadata::import(unique_fd ashmemFd) { if (!ashmem_valid(ashmemFd.get())) { ALOGE("BufferHubMetadata::Import: invalid ashmem fd."); return {}; @@ -94,7 +94,7 @@ BufferHubMetadata::BufferHubMetadata(size_t userMetadataSize, unique_fd ashmemFd BufferHubMetadata::~BufferHubMetadata() { if (mMetadataHeader != nullptr) { - int ret = munmap(mMetadataHeader, metadata_size()); + int ret = munmap(mMetadataHeader, metadataSize()); ALOGE_IF(ret != 0, "BufferHubMetadata::~BufferHubMetadata: failed to unmap ashmem, error=%d.", errno); mMetadataHeader = nullptr; diff --git a/libs/ui/GraphicBuffer.cpp b/libs/ui/GraphicBuffer.cpp index 41ae2531cd..f487dfa81b 100644 --- a/libs/ui/GraphicBuffer.cpp +++ b/libs/ui/GraphicBuffer.cpp @@ -100,7 +100,7 @@ GraphicBuffer::GraphicBuffer(std::unique_ptr buffer) : GraphicB return; } - mInitCheck = initWithHandle(buffer->DuplicateHandle(), /*method=*/TAKE_UNREGISTERED_HANDLE, + mInitCheck = initWithHandle(buffer->duplicateHandle(), /*method=*/TAKE_UNREGISTERED_HANDLE, buffer->desc().width, buffer->desc().height, static_cast(buffer->desc().format), buffer->desc().layers, buffer->desc().usage, buffer->desc().stride); diff --git a/libs/ui/include/ui/BufferHubBuffer.h b/libs/ui/include/ui/BufferHubBuffer.h index 06955e4161..eac8c8437b 100644 --- a/libs/ui/include/ui/BufferHubBuffer.h +++ b/libs/ui/include/ui/BufferHubBuffer.h @@ -29,14 +29,14 @@ namespace android { class BufferHubBuffer { public: // Allocates a standalone BufferHubBuffer. - static std::unique_ptr Create(uint32_t width, uint32_t height, + static std::unique_ptr create(uint32_t width, uint32_t height, uint32_t layerCount, uint32_t format, uint64_t usage, size_t userMetadataSize); // Imports the given token to a BufferHubBuffer. Not taking ownership of the token. Caller // should close and destroy the token after calling this function regardless of output. // TODO(b/122543147): use a movable wrapper for token - static std::unique_ptr Import(const native_handle_t* token); + static std::unique_ptr import(const native_handle_t* token); BufferHubBuffer(const BufferHubBuffer&) = delete; void operator=(const BufferHubBuffer&) = delete; @@ -52,51 +52,51 @@ public: // Duplicate the underlying Gralloc buffer handle. Caller is responsible to free the handle // after use. - native_handle_t* DuplicateHandle() { + native_handle_t* duplicateHandle() { return native_handle_clone(mBufferHandle.getNativeHandle()); } const BufferHubEventFd& eventFd() const { return mEventFd; } // Returns the current value of MetadataHeader::buffer_state. - uint32_t buffer_state() const { return mBufferState->load(std::memory_order_acquire); } + uint32_t bufferState() const { return mBufferState->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. - uint32_t client_state_mask() const { return mClientStateMask; } + uint32_t clientStateMask() const { return mClientStateMask; } - size_t user_metadata_size() const { return mMetadata.user_metadata_size(); } + size_t userMetadataSize() const { return mMetadata.userMetadataSize(); } // Returns true if the BufferClient is still alive. - bool IsConnected() const { return mBufferClient->ping().isOk(); } + bool isConnected() const { return mBufferClient->ping().isOk(); } // Returns true if the buffer is valid: non-null buffer handle, valid id, valid client bit mask, // valid metadata and valid buffer client - bool IsValid() const; + bool isValid() const; // Gains the buffer for exclusive write permission. Read permission is implied once a buffer is // gained. // The buffer can be gained as long as there is no other client in acquired or gained state. - int Gain(); + int gain(); // Posts the gained buffer for other buffer clients to use the buffer. // The buffer can be posted iff the buffer state for this client is gained. // After posting the buffer, this client is put to released state and does not have access to // the buffer for this cycle of the usage of the buffer. - int Post(); + int post(); // Acquires the buffer for shared read permission. // The buffer can be acquired iff the buffer state for this client is posted. - int Acquire(); + int acquire(); // Releases the buffer. // The buffer can be released from any buffer state. // After releasing the buffer, this client no longer have any permissions to the buffer for the // current cycle of the usage of the buffer. - int Release(); + int release(); // Returns whether the buffer is released by all active clients or not. - bool IsReleased() const; + bool isReleased() const; // Creates a token that stands for this BufferHubBuffer client and could be used for Import to // create another BufferHubBuffer. The new BufferHubBuffer will share the same underlying @@ -104,7 +104,7 @@ public: // should free it after use. // Returns a valid token on success, nullptr on failure. // TODO(b/122543147): use a movable wrapper for token - native_handle_t* Duplicate(); + native_handle_t* duplicate(); private: BufferHubBuffer(uint32_t width, uint32_t height, uint32_t layerCount, uint32_t format, diff --git a/libs/ui/include/ui/BufferHubDefs.h b/libs/ui/include/ui/BufferHubDefs.h index ff970cbd64..722a060597 100644 --- a/libs/ui/include/ui/BufferHubDefs.h +++ b/libs/ui/include/ui/BufferHubDefs.h @@ -63,19 +63,19 @@ static constexpr uint32_t kHighBitsMask = ~kLowbitsMask; static constexpr uint32_t kFirstClientBitMask = (1U << kMaxNumberOfClients) + 1U; // Returns true if any of the client is in gained state. -static inline bool AnyClientGained(uint32_t state) { +static inline bool isAnyClientGained(uint32_t state) { uint32_t high_bits = state >> kMaxNumberOfClients; uint32_t low_bits = state & kLowbitsMask; return high_bits == low_bits && low_bits != 0U; } // Returns true if the input client is in gained state. -static inline bool IsClientGained(uint32_t state, uint32_t client_bit_mask) { +static inline bool isClientGained(uint32_t state, uint32_t client_bit_mask) { return state == client_bit_mask; } // Returns true if any of the client is in posted state. -static inline bool AnyClientPosted(uint32_t state) { +static inline bool isAnyClientPosted(uint32_t state) { uint32_t high_bits = state >> kMaxNumberOfClients; uint32_t low_bits = state & kLowbitsMask; uint32_t posted_or_acquired = high_bits ^ low_bits; @@ -83,7 +83,7 @@ static inline bool AnyClientPosted(uint32_t state) { } // Returns true if the input client is in posted state. -static inline bool IsClientPosted(uint32_t state, uint32_t client_bit_mask) { +static inline bool isClientPosted(uint32_t state, uint32_t client_bit_mask) { uint32_t client_bits = state & client_bit_mask; if (client_bits == 0U) return false; uint32_t low_bits = client_bits & kLowbitsMask; @@ -91,7 +91,7 @@ static inline bool IsClientPosted(uint32_t state, uint32_t client_bit_mask) { } // Return true if any of the client is in acquired state. -static inline bool AnyClientAcquired(uint32_t state) { +static inline bool isAnyClientAcquired(uint32_t state) { uint32_t high_bits = state >> kMaxNumberOfClients; uint32_t low_bits = state & kLowbitsMask; uint32_t posted_or_acquired = high_bits ^ low_bits; @@ -99,7 +99,7 @@ static inline bool AnyClientAcquired(uint32_t state) { } // Return true if the input client is in acquired state. -static inline bool IsClientAcquired(uint32_t state, uint32_t client_bit_mask) { +static inline bool isClientAcquired(uint32_t state, uint32_t client_bit_mask) { uint32_t client_bits = state & client_bit_mask; if (client_bits == 0U) return false; uint32_t high_bits = client_bits & kHighBitsMask; @@ -107,13 +107,13 @@ static inline bool IsClientAcquired(uint32_t state, uint32_t client_bit_mask) { } // Returns true if the input client is in released state. -static inline bool IsClientReleased(uint32_t state, uint32_t client_bit_mask) { +static inline bool isClientReleased(uint32_t state, uint32_t client_bit_mask) { return (state & client_bit_mask) == 0U; } // Returns the next available buffer client's client_state_masks. // @params union_bits. Union of all existing clients' client_state_masks. -static inline uint32_t FindNextAvailableClientStateMask(uint32_t union_bits) { +static inline uint32_t findNextAvailableClientStateMask(uint32_t union_bits) { uint32_t low_union = union_bits & kLowbitsMask; if (low_union == kLowbitsMask) return 0U; uint32_t incremented = low_union + 1U; diff --git a/libs/ui/include/ui/BufferHubMetadata.h b/libs/ui/include/ui/BufferHubMetadata.h index 212189497a..3482507399 100644 --- a/libs/ui/include/ui/BufferHubMetadata.h +++ b/libs/ui/include/ui/BufferHubMetadata.h @@ -33,12 +33,12 @@ public: // @param userMetadataSize Size in bytes of the user defined metadata. The entire metadata // shared memory region to be allocated is the size of canonical // BufferHubDefs::MetadataHeader plus userMetadataSize. - static BufferHubMetadata Create(size_t userMetadataSize); + static BufferHubMetadata create(size_t userMetadataSize); // Imports an existing BufferHubMetadata from an ashmem FD. // // @param ashmemFd Ashmem file descriptor representing an ashmem region. - static BufferHubMetadata Import(unique_fd ashmemFd); + static BufferHubMetadata import(unique_fd ashmemFd); BufferHubMetadata() = default; @@ -63,13 +63,13 @@ public: // Returns true if the metadata is valid, i.e. the metadata has a valid ashmem fd and the ashmem // has been mapped into virtual address space. - bool IsValid() const { return mAshmemFd.get() != -1 && mMetadataHeader != nullptr; } + bool isValid() const { return mAshmemFd.get() != -1 && mMetadataHeader != nullptr; } - size_t user_metadata_size() const { return mUserMetadataSize; } - size_t metadata_size() const { return mUserMetadataSize + BufferHubDefs::kMetadataHeaderSize; } + size_t userMetadataSize() const { return mUserMetadataSize; } + size_t metadataSize() const { return mUserMetadataSize + BufferHubDefs::kMetadataHeaderSize; } - const unique_fd& ashmem_fd() const { return mAshmemFd; } - BufferHubDefs::MetadataHeader* metadata_header() { return mMetadataHeader; } + const unique_fd& ashmemFd() const { return mAshmemFd; } + BufferHubDefs::MetadataHeader* metadataHeader() { return mMetadataHeader; } private: BufferHubMetadata(size_t userMetadataSize, unique_fd ashmemFd, diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp index 4ad2c4c2ce..3087a90787 100644 --- a/libs/ui/tests/BufferHubBuffer_test.cpp +++ b/libs/ui/tests/BufferHubBuffer_test.cpp @@ -32,13 +32,13 @@ namespace android { namespace { -using ::android::BufferHubDefs::AnyClientAcquired; -using ::android::BufferHubDefs::AnyClientGained; -using ::android::BufferHubDefs::AnyClientPosted; -using ::android::BufferHubDefs::IsClientAcquired; -using ::android::BufferHubDefs::IsClientGained; -using ::android::BufferHubDefs::IsClientPosted; -using ::android::BufferHubDefs::IsClientReleased; +using ::android::BufferHubDefs::isAnyClientAcquired; +using ::android::BufferHubDefs::isAnyClientGained; +using ::android::BufferHubDefs::isAnyClientPosted; +using ::android::BufferHubDefs::isClientAcquired; +using ::android::BufferHubDefs::isClientGained; +using ::android::BufferHubDefs::isClientPosted; +using ::android::BufferHubDefs::isClientReleased; using ::android::BufferHubDefs::kMetadataHeaderSize; using ::testing::IsNull; using ::testing::NotNull; @@ -81,88 +81,88 @@ private: }; void BufferHubBufferStateTransitionTest::CreateTwoClientsOfABuffer() { - b1 = BufferHubBuffer::Create(kWidth, kHeight, kLayerCount, kFormat, kUsage, kUserMetadataSize); + b1 = BufferHubBuffer::create(kWidth, kHeight, kLayerCount, kFormat, kUsage, kUserMetadataSize); ASSERT_THAT(b1, NotNull()); - b1ClientMask = b1->client_state_mask(); + b1ClientMask = b1->clientStateMask(); ASSERT_NE(b1ClientMask, 0U); - native_handle_t* token = b1->Duplicate(); + native_handle_t* token = b1->duplicate(); ASSERT_THAT(token, NotNull()); // TODO(b/122543147): use a movalbe wrapper for token - b2 = BufferHubBuffer::Import(token); + b2 = BufferHubBuffer::import(token); native_handle_close(token); native_handle_delete(token); ASSERT_THAT(b2, NotNull()); - b2ClientMask = b2->client_state_mask(); + b2ClientMask = b2->clientStateMask(); ASSERT_NE(b2ClientMask, 0U); ASSERT_NE(b2ClientMask, b1ClientMask); } TEST_F(BufferHubBufferTest, CreateBufferFails) { // Buffer Creation will fail: BLOB format requires height to be 1. - auto b1 = BufferHubBuffer::Create(kWidth, /*height=*/2, kLayerCount, + auto b1 = BufferHubBuffer::create(kWidth, /*height=*/2, kLayerCount, /*format=*/HAL_PIXEL_FORMAT_BLOB, kUsage, kUserMetadataSize); EXPECT_THAT(b1, IsNull()); // Buffer Creation will fail: user metadata size too large. - auto b2 = BufferHubBuffer::Create(kWidth, kHeight, kLayerCount, kFormat, kUsage, + auto b2 = BufferHubBuffer::create(kWidth, kHeight, kLayerCount, kFormat, kUsage, /*userMetadataSize=*/std::numeric_limits::max()); EXPECT_THAT(b2, IsNull()); // Buffer Creation will fail: user metadata size too large. const size_t userMetadataSize = std::numeric_limits::max() - kMetadataHeaderSize; - auto b3 = BufferHubBuffer::Create(kWidth, kHeight, kLayerCount, kFormat, kUsage, + auto b3 = BufferHubBuffer::create(kWidth, kHeight, kLayerCount, kFormat, kUsage, userMetadataSize); EXPECT_THAT(b3, IsNull()); } TEST_F(BufferHubBufferTest, CreateBuffer) { - auto b1 = BufferHubBuffer::Create(kWidth, kHeight, kLayerCount, kFormat, kUsage, + auto b1 = BufferHubBuffer::create(kWidth, kHeight, kLayerCount, kFormat, kUsage, kUserMetadataSize); ASSERT_THAT(b1, NotNull()); - EXPECT_TRUE(b1->IsConnected()); - EXPECT_TRUE(b1->IsValid()); + EXPECT_TRUE(b1->isConnected()); + EXPECT_TRUE(b1->isValid()); EXPECT_TRUE(cmpAHardwareBufferDesc(b1->desc(), kDesc)); - EXPECT_EQ(b1->user_metadata_size(), kUserMetadataSize); + EXPECT_EQ(b1->userMetadataSize(), kUserMetadataSize); } TEST_F(BufferHubBufferTest, DuplicateAndImportBuffer) { - auto b1 = BufferHubBuffer::Create(kWidth, kHeight, kLayerCount, kFormat, kUsage, + auto b1 = BufferHubBuffer::create(kWidth, kHeight, kLayerCount, kFormat, kUsage, kUserMetadataSize); ASSERT_THAT(b1, NotNull()); - EXPECT_TRUE(b1->IsValid()); + EXPECT_TRUE(b1->isValid()); - native_handle_t* token = b1->Duplicate(); + native_handle_t* token = b1->duplicate(); EXPECT_TRUE(token); // The detached buffer should still be valid. - EXPECT_TRUE(b1->IsConnected()); - EXPECT_TRUE(b1->IsValid()); + EXPECT_TRUE(b1->isConnected()); + EXPECT_TRUE(b1->isValid()); - std::unique_ptr b2 = BufferHubBuffer::Import(token); + std::unique_ptr b2 = BufferHubBuffer::import(token); native_handle_close(token); native_handle_delete(token); ASSERT_THAT(b2, NotNull()); - EXPECT_TRUE(b2->IsValid()); + EXPECT_TRUE(b2->isValid()); EXPECT_TRUE(cmpAHardwareBufferDesc(b1->desc(), b2->desc())); - EXPECT_EQ(b1->user_metadata_size(), b2->user_metadata_size()); + EXPECT_EQ(b1->userMetadataSize(), b2->userMetadataSize()); // These two buffer instances are based on the same physical buffer under the // hood, so they should share the same id. EXPECT_EQ(b1->id(), b2->id()); - // We use client_state_mask() to tell those two instances apart. - EXPECT_NE(b1->client_state_mask(), b2->client_state_mask()); + // We use clientStateMask() to tell those two instances apart. + EXPECT_NE(b1->clientStateMask(), b2->clientStateMask()); // Both buffer instances should be in released state currently. - EXPECT_TRUE(b1->IsReleased()); - EXPECT_TRUE(b2->IsReleased()); + EXPECT_TRUE(b1->isReleased()); + EXPECT_TRUE(b2->isReleased()); // The event fd should behave like duped event fds. const BufferHubEventFd& eventFd1 = b1->eventFd(); @@ -192,19 +192,19 @@ TEST_F(BufferHubBufferTest, DuplicateAndImportBuffer) { } TEST_F(BufferHubBufferTest, ImportFreedBuffer) { - auto b1 = BufferHubBuffer::Create(kWidth, kHeight, kLayerCount, kFormat, kUsage, + auto b1 = BufferHubBuffer::create(kWidth, kHeight, kLayerCount, kFormat, kUsage, kUserMetadataSize); ASSERT_THAT(b1, NotNull()); - EXPECT_TRUE(b1->IsValid()); + EXPECT_TRUE(b1->isValid()); - native_handle_t* token = b1->Duplicate(); + native_handle_t* token = b1->duplicate(); EXPECT_TRUE(token); // Explicitly destroy b1. Backend buffer should be freed and token becomes invalid b1.reset(); // TODO(b/122543147): use a movalbe wrapper for token - std::unique_ptr b2 = BufferHubBuffer::Import(token); + std::unique_ptr b2 = BufferHubBuffer::import(token); native_handle_close(token); native_handle_delete(token); @@ -214,7 +214,7 @@ TEST_F(BufferHubBufferTest, ImportFreedBuffer) { // nullptr must not crash the service TEST_F(BufferHubBufferTest, ImportNullToken) { - auto b1 = BufferHubBuffer::Import(nullptr); + auto b1 = BufferHubBuffer::import(nullptr); EXPECT_THAT(b1, IsNull()); } @@ -222,185 +222,185 @@ TEST_F(BufferHubBufferTest, ImportInvalidToken) { native_handle_t* token = native_handle_create(/*numFds=*/0, /*numInts=*/1); token->data[0] = 0; - auto b1 = BufferHubBuffer::Import(token); + auto b1 = BufferHubBuffer::import(token); native_handle_delete(token); EXPECT_THAT(b1, IsNull()); } TEST_F(BufferHubBufferStateTransitionTest, GainBuffer_fromReleasedState) { - ASSERT_TRUE(b1->IsReleased()); + ASSERT_TRUE(b1->isReleased()); // Successful gaining the buffer should change the buffer state bit of b1 to // gained state, other client state bits to released state. - EXPECT_EQ(b1->Gain(), 0); - EXPECT_TRUE(IsClientGained(b1->buffer_state(), b1ClientMask)); + EXPECT_EQ(b1->gain(), 0); + EXPECT_TRUE(isClientGained(b1->bufferState(), b1ClientMask)); } TEST_F(BufferHubBufferStateTransitionTest, GainBuffer_fromGainedState) { - ASSERT_EQ(b1->Gain(), 0); - auto currentBufferState = b1->buffer_state(); - ASSERT_TRUE(IsClientGained(currentBufferState, b1ClientMask)); + ASSERT_EQ(b1->gain(), 0); + auto currentBufferState = b1->bufferState(); + ASSERT_TRUE(isClientGained(currentBufferState, b1ClientMask)); // Gaining from gained state by the same client should not return error. - EXPECT_EQ(b1->Gain(), 0); + EXPECT_EQ(b1->gain(), 0); // Gaining from gained state by another client should return error. - EXPECT_EQ(b2->Gain(), -EBUSY); + EXPECT_EQ(b2->gain(), -EBUSY); } TEST_F(BufferHubBufferStateTransitionTest, GainBuffer_fromAcquiredState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_EQ(b2->Acquire(), 0); - ASSERT_TRUE(AnyClientAcquired(b1->buffer_state())); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_EQ(b2->acquire(), 0); + ASSERT_TRUE(isAnyClientAcquired(b1->bufferState())); // Gaining from acquired state should fail. - EXPECT_EQ(b1->Gain(), -EBUSY); - EXPECT_EQ(b2->Gain(), -EBUSY); + EXPECT_EQ(b1->gain(), -EBUSY); + EXPECT_EQ(b2->gain(), -EBUSY); } TEST_F(BufferHubBufferStateTransitionTest, GainBuffer_fromOtherClientInPostedState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_TRUE(AnyClientPosted(b1->buffer_state())); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_TRUE(isAnyClientPosted(b1->bufferState())); // Gaining a buffer who has other posted client should succeed. - EXPECT_EQ(b1->Gain(), 0); + EXPECT_EQ(b1->gain(), 0); } TEST_F(BufferHubBufferStateTransitionTest, GainBuffer_fromSelfInPostedState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_TRUE(AnyClientPosted(b1->buffer_state())); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_TRUE(isAnyClientPosted(b1->bufferState())); // A posted client should be able to gain the buffer when there is no other clients in // acquired state. - EXPECT_EQ(b2->Gain(), 0); + EXPECT_EQ(b2->gain(), 0); } TEST_F(BufferHubBufferStateTransitionTest, PostBuffer_fromOtherInGainedState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_TRUE(IsClientGained(b1->buffer_state(), b1ClientMask)); + ASSERT_EQ(b1->gain(), 0); + ASSERT_TRUE(isClientGained(b1->bufferState(), b1ClientMask)); - EXPECT_EQ(b2->Post(), -EBUSY); + EXPECT_EQ(b2->post(), -EBUSY); } TEST_F(BufferHubBufferStateTransitionTest, PostBuffer_fromSelfInGainedState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_TRUE(IsClientGained(b1->buffer_state(), b1ClientMask)); + ASSERT_EQ(b1->gain(), 0); + ASSERT_TRUE(isClientGained(b1->bufferState(), b1ClientMask)); - EXPECT_EQ(b1->Post(), 0); - auto currentBufferState = b1->buffer_state(); - EXPECT_TRUE(IsClientReleased(currentBufferState, b1ClientMask)); - EXPECT_TRUE(IsClientPosted(currentBufferState, b2ClientMask)); + EXPECT_EQ(b1->post(), 0); + auto currentBufferState = b1->bufferState(); + EXPECT_TRUE(isClientReleased(currentBufferState, b1ClientMask)); + EXPECT_TRUE(isClientPosted(currentBufferState, b2ClientMask)); } TEST_F(BufferHubBufferStateTransitionTest, PostBuffer_fromPostedState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_TRUE(AnyClientPosted(b1->buffer_state())); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_TRUE(isAnyClientPosted(b1->bufferState())); // Post from posted state should fail. - EXPECT_EQ(b1->Post(), -EBUSY); - EXPECT_EQ(b2->Post(), -EBUSY); + EXPECT_EQ(b1->post(), -EBUSY); + EXPECT_EQ(b2->post(), -EBUSY); } TEST_F(BufferHubBufferStateTransitionTest, PostBuffer_fromAcquiredState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_EQ(b2->Acquire(), 0); - ASSERT_TRUE(AnyClientAcquired(b1->buffer_state())); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_EQ(b2->acquire(), 0); + ASSERT_TRUE(isAnyClientAcquired(b1->bufferState())); // Posting from acquired state should fail. - EXPECT_EQ(b1->Post(), -EBUSY); - EXPECT_EQ(b2->Post(), -EBUSY); + EXPECT_EQ(b1->post(), -EBUSY); + EXPECT_EQ(b2->post(), -EBUSY); } TEST_F(BufferHubBufferStateTransitionTest, PostBuffer_fromReleasedState) { - ASSERT_TRUE(b1->IsReleased()); + ASSERT_TRUE(b1->isReleased()); // Posting from released state should fail. - EXPECT_EQ(b1->Post(), -EBUSY); - EXPECT_EQ(b2->Post(), -EBUSY); + EXPECT_EQ(b1->post(), -EBUSY); + EXPECT_EQ(b2->post(), -EBUSY); } TEST_F(BufferHubBufferStateTransitionTest, AcquireBuffer_fromSelfInPostedState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_TRUE(IsClientPosted(b1->buffer_state(), b2ClientMask)); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_TRUE(isClientPosted(b1->bufferState(), b2ClientMask)); // Acquire from posted state should pass. - EXPECT_EQ(b2->Acquire(), 0); + EXPECT_EQ(b2->acquire(), 0); } TEST_F(BufferHubBufferStateTransitionTest, AcquireBuffer_fromOtherInPostedState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_TRUE(IsClientPosted(b1->buffer_state(), b2ClientMask)); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_TRUE(isClientPosted(b1->bufferState(), b2ClientMask)); // Acquire from released state should fail, although there are other clients // in posted state. - EXPECT_EQ(b1->Acquire(), -EBUSY); + EXPECT_EQ(b1->acquire(), -EBUSY); } TEST_F(BufferHubBufferStateTransitionTest, AcquireBuffer_fromSelfInAcquiredState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_EQ(b2->Acquire(), 0); - auto currentBufferState = b1->buffer_state(); - ASSERT_TRUE(IsClientAcquired(currentBufferState, b2ClientMask)); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_EQ(b2->acquire(), 0); + auto currentBufferState = b1->bufferState(); + ASSERT_TRUE(isClientAcquired(currentBufferState, b2ClientMask)); // Acquiring from acquired state by the same client should not error out. - EXPECT_EQ(b2->Acquire(), 0); + EXPECT_EQ(b2->acquire(), 0); } TEST_F(BufferHubBufferStateTransitionTest, AcquireBuffer_fromReleasedState) { - ASSERT_TRUE(b1->IsReleased()); + ASSERT_TRUE(b1->isReleased()); // Acquiring form released state should fail. - EXPECT_EQ(b1->Acquire(), -EBUSY); - EXPECT_EQ(b2->Acquire(), -EBUSY); + EXPECT_EQ(b1->acquire(), -EBUSY); + EXPECT_EQ(b2->acquire(), -EBUSY); } TEST_F(BufferHubBufferStateTransitionTest, AcquireBuffer_fromGainedState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_TRUE(AnyClientGained(b1->buffer_state())); + ASSERT_EQ(b1->gain(), 0); + ASSERT_TRUE(isAnyClientGained(b1->bufferState())); // Acquiring from gained state should fail. - EXPECT_EQ(b1->Acquire(), -EBUSY); - EXPECT_EQ(b2->Acquire(), -EBUSY); + EXPECT_EQ(b1->acquire(), -EBUSY); + EXPECT_EQ(b2->acquire(), -EBUSY); } TEST_F(BufferHubBufferStateTransitionTest, ReleaseBuffer_fromSelfInReleasedState) { - ASSERT_TRUE(b1->IsReleased()); + ASSERT_TRUE(b1->isReleased()); - EXPECT_EQ(b1->Release(), 0); + EXPECT_EQ(b1->release(), 0); } TEST_F(BufferHubBufferStateTransitionTest, ReleaseBuffer_fromSelfInGainedState) { - ASSERT_TRUE(b1->IsReleased()); - ASSERT_EQ(b1->Gain(), 0); - ASSERT_TRUE(AnyClientGained(b1->buffer_state())); + ASSERT_TRUE(b1->isReleased()); + ASSERT_EQ(b1->gain(), 0); + ASSERT_TRUE(isAnyClientGained(b1->bufferState())); - EXPECT_EQ(b1->Release(), 0); + EXPECT_EQ(b1->release(), 0); } TEST_F(BufferHubBufferStateTransitionTest, ReleaseBuffer_fromSelfInPostedState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_TRUE(AnyClientPosted(b1->buffer_state())); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_TRUE(isAnyClientPosted(b1->bufferState())); - EXPECT_EQ(b2->Release(), 0); + EXPECT_EQ(b2->release(), 0); } TEST_F(BufferHubBufferStateTransitionTest, ReleaseBuffer_fromSelfInAcquiredState) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_EQ(b2->Acquire(), 0); - ASSERT_TRUE(AnyClientAcquired(b1->buffer_state())); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_EQ(b2->acquire(), 0); + ASSERT_TRUE(isAnyClientAcquired(b1->bufferState())); - EXPECT_EQ(b2->Release(), 0); + EXPECT_EQ(b2->release(), 0); } TEST_F(BufferHubBufferStateTransitionTest, BasicUsage) { @@ -408,60 +408,60 @@ TEST_F(BufferHubBufferStateTransitionTest, BasicUsage) { // Test if this set of basic operation succeed: // Producer post three times to the consumer, and released by consumer. for (int i = 0; i < 3; ++i) { - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); - ASSERT_EQ(b2->Acquire(), 0); - ASSERT_EQ(b2->Release(), 0); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); + ASSERT_EQ(b2->acquire(), 0); + ASSERT_EQ(b2->release(), 0); } } TEST_F(BufferHubBufferTest, createNewConsumerAfterGain) { // Create a poducer buffer and gain. std::unique_ptr b1 = - BufferHubBuffer::Create(kWidth, kHeight, kLayerCount, kFormat, kUsage, + BufferHubBuffer::create(kWidth, kHeight, kLayerCount, kFormat, kUsage, kUserMetadataSize); ASSERT_THAT(b1, NotNull()); - ASSERT_EQ(b1->Gain(), 0); + ASSERT_EQ(b1->gain(), 0); // Create a consumer of the buffer and test if the consumer can acquire the // buffer if producer posts. // TODO(b/122543147): use a movalbe wrapper for token - native_handle_t* token = b1->Duplicate(); + native_handle_t* token = b1->duplicate(); ASSERT_TRUE(token); - std::unique_ptr b2 = BufferHubBuffer::Import(token); + std::unique_ptr b2 = BufferHubBuffer::import(token); native_handle_close(token); native_handle_delete(token); ASSERT_THAT(b2, NotNull()); - ASSERT_NE(b1->client_state_mask(), b2->client_state_mask()); + ASSERT_NE(b1->clientStateMask(), b2->clientStateMask()); - ASSERT_EQ(b1->Post(), 0); - EXPECT_EQ(b2->Acquire(), 0); + ASSERT_EQ(b1->post(), 0); + EXPECT_EQ(b2->acquire(), 0); } TEST_F(BufferHubBufferTest, createNewConsumerAfterPost) { // Create a poducer buffer and post. std::unique_ptr b1 = - BufferHubBuffer::Create(kWidth, kHeight, kLayerCount, kFormat, kUsage, + BufferHubBuffer::create(kWidth, kHeight, kLayerCount, kFormat, kUsage, kUserMetadataSize); - ASSERT_EQ(b1->Gain(), 0); - ASSERT_EQ(b1->Post(), 0); + ASSERT_EQ(b1->gain(), 0); + ASSERT_EQ(b1->post(), 0); // Create a consumer of the buffer and test if the consumer can acquire the // buffer if producer posts. // TODO(b/122543147): use a movalbe wrapper for token - native_handle_t* token = b1->Duplicate(); + native_handle_t* token = b1->duplicate(); ASSERT_TRUE(token); - std::unique_ptr b2 = BufferHubBuffer::Import(token); + std::unique_ptr b2 = BufferHubBuffer::import(token); native_handle_close(token); native_handle_delete(token); ASSERT_THAT(b2, NotNull()); - ASSERT_NE(b1->client_state_mask(), b2->client_state_mask()); + ASSERT_NE(b1->clientStateMask(), b2->clientStateMask()); - EXPECT_EQ(b2->Acquire(), 0); + EXPECT_EQ(b2->acquire(), 0); } } // namespace diff --git a/libs/ui/tests/BufferHubMetadata_test.cpp b/libs/ui/tests/BufferHubMetadata_test.cpp index b7f0b4b140..f02c4fc178 100644 --- a/libs/ui/tests/BufferHubMetadata_test.cpp +++ b/libs/ui/tests/BufferHubMetadata_test.cpp @@ -25,74 +25,73 @@ constexpr size_t kEmptyUserMetadataSize = 0; class BufferHubMetadataTest : public ::testing::Test {}; TEST_F(BufferHubMetadataTest, Create_UserMetdataSizeTooBig) { - BufferHubMetadata m1 = - BufferHubMetadata::Create(std::numeric_limits::max()); - EXPECT_FALSE(m1.IsValid()); + BufferHubMetadata m1 = BufferHubMetadata::create(std::numeric_limits::max()); + EXPECT_FALSE(m1.isValid()); } TEST_F(BufferHubMetadataTest, Create_Success) { - BufferHubMetadata m1 = BufferHubMetadata::Create(kEmptyUserMetadataSize); - EXPECT_TRUE(m1.IsValid()); - EXPECT_NE(m1.metadata_header(), nullptr); + BufferHubMetadata m1 = BufferHubMetadata::create(kEmptyUserMetadataSize); + EXPECT_TRUE(m1.isValid()); + EXPECT_NE(m1.metadataHeader(), nullptr); } TEST_F(BufferHubMetadataTest, Import_Success) { - BufferHubMetadata m1 = BufferHubMetadata::Create(kEmptyUserMetadataSize); - EXPECT_TRUE(m1.IsValid()); - EXPECT_NE(m1.metadata_header(), nullptr); - - unique_fd h2 = unique_fd(dup(m1.ashmem_fd().get())); - EXPECT_NE(h2.get(), -1); - - BufferHubMetadata m2 = BufferHubMetadata::Import(std::move(h2)); - EXPECT_EQ(h2.get(), -1); - EXPECT_TRUE(m1.IsValid()); - BufferHubDefs::MetadataHeader* mh1 = m1.metadata_header(); - EXPECT_NE(mh1, nullptr); - - // Check if the newly allocated buffer is initialized in released state (i.e. - // state equals to 0U). - EXPECT_TRUE(mh1->buffer_state.load() == 0U); - - EXPECT_TRUE(m2.IsValid()); - BufferHubDefs::MetadataHeader* mh2 = m2.metadata_header(); - EXPECT_NE(mh2, nullptr); - - // Check if the newly allocated buffer is initialized in released state (i.e. - // state equals to 0U). - EXPECT_TRUE(mh2->buffer_state.load() == 0U); + BufferHubMetadata m1 = BufferHubMetadata::create(kEmptyUserMetadataSize); + EXPECT_TRUE(m1.isValid()); + EXPECT_NE(m1.metadataHeader(), nullptr); + + unique_fd h2 = unique_fd(dup(m1.ashmemFd().get())); + EXPECT_NE(h2.get(), -1); + + BufferHubMetadata m2 = BufferHubMetadata::import(std::move(h2)); + EXPECT_EQ(h2.get(), -1); + EXPECT_TRUE(m1.isValid()); + BufferHubDefs::MetadataHeader* mh1 = m1.metadataHeader(); + EXPECT_NE(mh1, nullptr); + + // Check if the newly allocated buffer is initialized in released state (i.e. + // state equals to 0U). + EXPECT_TRUE(mh1->buffer_state.load() == 0U); + + EXPECT_TRUE(m2.isValid()); + BufferHubDefs::MetadataHeader* mh2 = m2.metadataHeader(); + EXPECT_NE(mh2, nullptr); + + // Check if the newly allocated buffer is initialized in released state (i.e. + // state equals to 0U). + EXPECT_TRUE(mh2->buffer_state.load() == 0U); } TEST_F(BufferHubMetadataTest, MoveMetadataInvalidatesOldOne) { - BufferHubMetadata m1 = BufferHubMetadata::Create(sizeof(int)); - EXPECT_TRUE(m1.IsValid()); - EXPECT_NE(m1.metadata_header(), nullptr); - EXPECT_NE(m1.ashmem_fd().get(), -1); - EXPECT_EQ(m1.user_metadata_size(), sizeof(int)); + BufferHubMetadata m1 = BufferHubMetadata::create(sizeof(int)); + EXPECT_TRUE(m1.isValid()); + EXPECT_NE(m1.metadataHeader(), nullptr); + EXPECT_NE(m1.ashmemFd().get(), -1); + EXPECT_EQ(m1.userMetadataSize(), sizeof(int)); - BufferHubMetadata m2 = std::move(m1); + BufferHubMetadata m2 = std::move(m1); - // After the move, the metadata header (a raw pointer) should be reset in the older buffer. - EXPECT_EQ(m1.metadata_header(), nullptr); - EXPECT_NE(m2.metadata_header(), nullptr); + // After the move, the metadata header (a raw pointer) should be reset in the older buffer. + EXPECT_EQ(m1.metadataHeader(), nullptr); + EXPECT_NE(m2.metadataHeader(), nullptr); - EXPECT_EQ(m1.ashmem_fd().get(), -1); - EXPECT_NE(m2.ashmem_fd().get(), -1); + EXPECT_EQ(m1.ashmemFd().get(), -1); + EXPECT_NE(m2.ashmemFd().get(), -1); - EXPECT_EQ(m1.user_metadata_size(), 0U); - EXPECT_EQ(m2.user_metadata_size(), sizeof(int)); + EXPECT_EQ(m1.userMetadataSize(), 0U); + EXPECT_EQ(m2.userMetadataSize(), sizeof(int)); - BufferHubMetadata m3{std::move(m2)}; + BufferHubMetadata m3{std::move(m2)}; - // After the move, the metadata header (a raw pointer) should be reset in the older buffer. - EXPECT_EQ(m2.metadata_header(), nullptr); - EXPECT_NE(m3.metadata_header(), nullptr); + // After the move, the metadata header (a raw pointer) should be reset in the older buffer. + EXPECT_EQ(m2.metadataHeader(), nullptr); + EXPECT_NE(m3.metadataHeader(), nullptr); - EXPECT_EQ(m2.ashmem_fd().get(), -1); - EXPECT_NE(m3.ashmem_fd().get(), -1); + EXPECT_EQ(m2.ashmemFd().get(), -1); + EXPECT_NE(m3.ashmemFd().get(), -1); - EXPECT_EQ(m2.user_metadata_size(), 0U); - EXPECT_EQ(m3.user_metadata_size(), sizeof(int)); + EXPECT_EQ(m2.userMetadataSize(), 0U); + EXPECT_EQ(m3.userMetadataSize(), sizeof(int)); } } // namespace dvr diff --git a/libs/ui/tests/GraphicBuffer_test.cpp b/libs/ui/tests/GraphicBuffer_test.cpp index 5b46454c60..c767ce02fc 100644 --- a/libs/ui/tests/GraphicBuffer_test.cpp +++ b/libs/ui/tests/GraphicBuffer_test.cpp @@ -37,10 +37,10 @@ class GraphicBufferTest : public testing::Test {}; TEST_F(GraphicBufferTest, CreateFromBufferHubBuffer) { std::unique_ptr b1 = - BufferHubBuffer::Create(kTestWidth, kTestHeight, kTestLayerCount, kTestFormat, + BufferHubBuffer::create(kTestWidth, kTestHeight, kTestLayerCount, kTestFormat, kTestUsage, /*userMetadataSize=*/0); ASSERT_NE(b1, nullptr); - EXPECT_TRUE(b1->IsValid()); + EXPECT_TRUE(b1->isValid()); sp gb(new GraphicBuffer(std::move(b1))); EXPECT_TRUE(gb->isBufferHubBuffer()); @@ -61,10 +61,10 @@ TEST_F(GraphicBufferTest, InvalidBufferIdForNoneBufferHubBuffer) { TEST_F(GraphicBufferTest, BufferIdMatchesBufferHubBufferId) { std::unique_ptr b1 = - BufferHubBuffer::Create(kTestWidth, kTestHeight, kTestLayerCount, kTestFormat, + BufferHubBuffer::create(kTestWidth, kTestHeight, kTestLayerCount, kTestFormat, kTestUsage, /*userMetadataSize=*/0); EXPECT_NE(b1, nullptr); - EXPECT_TRUE(b1->IsValid()); + EXPECT_TRUE(b1->isValid()); int b1_id = b1->id(); EXPECT_GE(b1_id, 0); diff --git a/libs/vr/libbufferhub/buffer_hub-test.cpp b/libs/vr/libbufferhub/buffer_hub-test.cpp index 527a27d5dd..27ab0242ad 100644 --- a/libs/vr/libbufferhub/buffer_hub-test.cpp +++ b/libs/vr/libbufferhub/buffer_hub-test.cpp @@ -20,12 +20,12 @@ namespace { return result; \ })() -using android::BufferHubDefs::AnyClientAcquired; -using android::BufferHubDefs::AnyClientGained; -using android::BufferHubDefs::AnyClientPosted; -using android::BufferHubDefs::IsClientAcquired; -using android::BufferHubDefs::IsClientPosted; -using android::BufferHubDefs::IsClientReleased; +using android::BufferHubDefs::isAnyClientAcquired; +using android::BufferHubDefs::isAnyClientGained; +using android::BufferHubDefs::isAnyClientPosted; +using android::BufferHubDefs::isClientAcquired; +using android::BufferHubDefs::isClientPosted; +using android::BufferHubDefs::isClientReleased; using android::BufferHubDefs::kFirstClientBitMask; using android::dvr::ConsumerBuffer; using android::dvr::ProducerBuffer; @@ -268,7 +268,7 @@ TEST_F(LibBufferHubTest, TestAsyncStateTransitions) { // Post in gained state should succeed. EXPECT_EQ(0, p->PostAsync(&metadata, invalid_fence)); EXPECT_EQ(p->buffer_state(), c->buffer_state()); - EXPECT_TRUE(AnyClientPosted(p->buffer_state())); + EXPECT_TRUE(isAnyClientPosted(p->buffer_state())); // Post and gain in posted state should fail. EXPECT_EQ(-EBUSY, p->PostAsync(&metadata, invalid_fence)); @@ -280,7 +280,7 @@ TEST_F(LibBufferHubTest, TestAsyncStateTransitions) { EXPECT_EQ(0, c->AcquireAsync(&metadata, &invalid_fence)); EXPECT_FALSE(invalid_fence.IsValid()); EXPECT_EQ(p->buffer_state(), c->buffer_state()); - EXPECT_TRUE(AnyClientAcquired(p->buffer_state())); + EXPECT_TRUE(isAnyClientAcquired(p->buffer_state())); // Acquire, post, and gain in acquired state should fail. EXPECT_EQ(-EBUSY, c->AcquireAsync(&metadata, &invalid_fence)); @@ -304,7 +304,7 @@ TEST_F(LibBufferHubTest, TestAsyncStateTransitions) { EXPECT_EQ(0, p->GainAsync(&metadata, &invalid_fence)); EXPECT_FALSE(invalid_fence.IsValid()); EXPECT_EQ(p->buffer_state(), c->buffer_state()); - EXPECT_TRUE(AnyClientGained(p->buffer_state())); + EXPECT_TRUE(isAnyClientGained(p->buffer_state())); // Acquire and gain in gained state should fail. EXPECT_EQ(-EBUSY, c->AcquireAsync(&metadata, &invalid_fence)); @@ -329,7 +329,7 @@ TEST_F(LibBufferHubTest, TestGainPostedBuffer) { ASSERT_TRUE(c.get() != nullptr); ASSERT_EQ(0, p->GainAsync()); ASSERT_EQ(0, p->Post(LocalHandle())); - ASSERT_TRUE(AnyClientPosted(p->buffer_state())); + ASSERT_TRUE(isAnyClientPosted(p->buffer_state())); // Gain in posted state should only succeed with gain_posted_buffer = true. LocalHandle invalid_fence; @@ -346,7 +346,7 @@ TEST_F(LibBufferHubTest, TestGainPostedBufferAsync) { ASSERT_TRUE(c.get() != nullptr); ASSERT_EQ(0, p->GainAsync()); ASSERT_EQ(0, p->Post(LocalHandle())); - ASSERT_TRUE(AnyClientPosted(p->buffer_state())); + ASSERT_TRUE(isAnyClientPosted(p->buffer_state())); // GainAsync in posted state should only succeed with gain_posted_buffer // equals true. @@ -364,9 +364,9 @@ TEST_F(LibBufferHubTest, TestGainPostedBuffer_noConsumer) { ASSERT_EQ(0, p->Post(LocalHandle())); // Producer state bit is in released state after post, other clients shall be // in posted state although there is no consumer of this buffer yet. - ASSERT_TRUE(IsClientReleased(p->buffer_state(), p->client_state_mask())); + ASSERT_TRUE(isClientReleased(p->buffer_state(), p->client_state_mask())); ASSERT_TRUE(p->is_released()); - ASSERT_TRUE(AnyClientPosted(p->buffer_state())); + ASSERT_TRUE(isAnyClientPosted(p->buffer_state())); // Gain in released state should succeed. LocalHandle invalid_fence; @@ -393,14 +393,14 @@ TEST_F(LibBufferHubTest, TestMaxConsumers) { // Post the producer should trigger all consumers to be available. EXPECT_EQ(0, p->PostAsync(&metadata, invalid_fence)); - EXPECT_TRUE(IsClientReleased(p->buffer_state(), p->client_state_mask())); + EXPECT_TRUE(isClientReleased(p->buffer_state(), p->client_state_mask())); for (size_t i = 0; i < kMaxConsumerCount; ++i) { EXPECT_TRUE( - IsClientPosted(cs[i]->buffer_state(), cs[i]->client_state_mask())); + isClientPosted(cs[i]->buffer_state(), cs[i]->client_state_mask())); EXPECT_LT(0, RETRY_EINTR(PollBufferEvent(cs[i]))); EXPECT_EQ(0, cs[i]->AcquireAsync(&metadata, &invalid_fence)); EXPECT_TRUE( - IsClientAcquired(p->buffer_state(), cs[i]->client_state_mask())); + isClientAcquired(p->buffer_state(), cs[i]->client_state_mask())); } // All consumers have to release before the buffer is considered to be @@ -424,22 +424,22 @@ TEST_F(LibBufferHubTest, TestCreateConsumerWhenBufferGained) { kWidth, kHeight, kFormat, kUsage, sizeof(uint64_t)); ASSERT_TRUE(p.get() != nullptr); EXPECT_EQ(0, p->GainAsync()); - EXPECT_TRUE(AnyClientGained(p->buffer_state())); + EXPECT_TRUE(isAnyClientGained(p->buffer_state())); std::unique_ptr c = ConsumerBuffer::Import(p->CreateConsumer()); ASSERT_TRUE(c.get() != nullptr); - EXPECT_TRUE(AnyClientGained(c->buffer_state())); + EXPECT_TRUE(isAnyClientGained(c->buffer_state())); DvrNativeBufferMetadata metadata; LocalHandle invalid_fence; // Post the gained buffer should signal already created consumer. EXPECT_EQ(0, p->PostAsync(&metadata, invalid_fence)); - EXPECT_TRUE(AnyClientPosted(p->buffer_state())); + EXPECT_TRUE(isAnyClientPosted(p->buffer_state())); EXPECT_LT(0, RETRY_EINTR(PollBufferEvent(c))); EXPECT_EQ(0, c->AcquireAsync(&metadata, &invalid_fence)); - EXPECT_TRUE(AnyClientAcquired(c->buffer_state())); + EXPECT_TRUE(isAnyClientAcquired(c->buffer_state())); } TEST_F(LibBufferHubTest, TestCreateTheFirstConsumerAfterPostingBuffer) { @@ -447,7 +447,7 @@ TEST_F(LibBufferHubTest, TestCreateTheFirstConsumerAfterPostingBuffer) { kWidth, kHeight, kFormat, kUsage, sizeof(uint64_t)); ASSERT_TRUE(p.get() != nullptr); EXPECT_EQ(0, p->GainAsync()); - EXPECT_TRUE(AnyClientGained(p->buffer_state())); + EXPECT_TRUE(isAnyClientGained(p->buffer_state())); DvrNativeBufferMetadata metadata; LocalHandle invalid_fence; @@ -462,7 +462,7 @@ TEST_F(LibBufferHubTest, TestCreateTheFirstConsumerAfterPostingBuffer) { std::unique_ptr c = ConsumerBuffer::Import(p->CreateConsumer()); ASSERT_TRUE(c.get() != nullptr); - EXPECT_TRUE(IsClientPosted(c->buffer_state(), c->client_state_mask())); + EXPECT_TRUE(isClientPosted(c->buffer_state(), c->client_state_mask())); EXPECT_EQ(0, c->AcquireAsync(&metadata, &invalid_fence)); } @@ -500,7 +500,7 @@ TEST_F(LibBufferHubTest, TestCreateConsumerWhenBufferReleased) { EXPECT_TRUE(p->is_released()); EXPECT_EQ(0, p->GainAsync(&metadata, &invalid_fence)); - EXPECT_TRUE(AnyClientGained(p->buffer_state())); + EXPECT_TRUE(isAnyClientGained(p->buffer_state())); } TEST_F(LibBufferHubTest, TestWithCustomMetadata) { diff --git a/libs/vr/libbufferhub/consumer_buffer.cpp b/libs/vr/libbufferhub/consumer_buffer.cpp index b6ca64eef2..115e8666e5 100644 --- a/libs/vr/libbufferhub/consumer_buffer.cpp +++ b/libs/vr/libbufferhub/consumer_buffer.cpp @@ -38,7 +38,7 @@ int ConsumerBuffer::LocalAcquire(DvrNativeBufferMetadata* out_meta, // The buffer can be acquired iff the buffer state for this client is posted. uint32_t current_buffer_state = buffer_state_->load(std::memory_order_acquire); - if (!BufferHubDefs::IsClientPosted(current_buffer_state, + if (!BufferHubDefs::isClientPosted(current_buffer_state, client_state_mask())) { ALOGE( "%s: Failed to acquire the buffer. The buffer is not posted, id=%d " @@ -58,7 +58,7 @@ int ConsumerBuffer::LocalAcquire(DvrNativeBufferMetadata* out_meta, " when trying to acquire the buffer and modify the buffer state to " "%" PRIx32 ". About to try again if the buffer is still posted.", __FUNCTION__, current_buffer_state, updated_buffer_state); - if (!BufferHubDefs::IsClientPosted(current_buffer_state, + if (!BufferHubDefs::isClientPosted(current_buffer_state, client_state_mask())) { ALOGE( "%s: Failed to acquire the buffer. The buffer is no longer posted, " @@ -144,7 +144,7 @@ int ConsumerBuffer::LocalRelease(const DvrNativeBufferMetadata* meta, // released state. uint32_t current_buffer_state = buffer_state_->load(std::memory_order_acquire); - if (BufferHubDefs::IsClientReleased(current_buffer_state, + if (BufferHubDefs::isClientReleased(current_buffer_state, client_state_mask())) { return 0; } 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 bab7367caa..e610e18849 100644 --- a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h +++ b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h @@ -27,38 +27,38 @@ static constexpr uint32_t kHighBitsMask = android::BufferHubDefs::kHighBitsMask; static constexpr uint32_t kFirstClientBitMask = android::BufferHubDefs::kFirstClientBitMask; -static inline bool AnyClientGained(uint32_t state) { - return android::BufferHubDefs::AnyClientGained(state); +static inline bool isAnyClientGained(uint32_t state) { + return android::BufferHubDefs::isAnyClientGained(state); } -static inline bool IsClientGained(uint32_t state, uint32_t client_bit_mask) { - return android::BufferHubDefs::IsClientGained(state, client_bit_mask); +static inline bool isClientGained(uint32_t state, uint32_t client_bit_mask) { + return android::BufferHubDefs::isClientGained(state, client_bit_mask); } -static inline bool AnyClientPosted(uint32_t state) { - return android::BufferHubDefs::AnyClientPosted(state); +static inline bool isAnyClientPosted(uint32_t state) { + return android::BufferHubDefs::isAnyClientPosted(state); } -static inline bool IsClientPosted(uint32_t state, uint32_t client_bit_mask) { - return android::BufferHubDefs::IsClientPosted(state, client_bit_mask); +static inline bool isClientPosted(uint32_t state, uint32_t client_bit_mask) { + return android::BufferHubDefs::isClientPosted(state, client_bit_mask); } -static inline bool AnyClientAcquired(uint32_t state) { - return android::BufferHubDefs::AnyClientAcquired(state); +static inline bool isAnyClientAcquired(uint32_t state) { + return android::BufferHubDefs::isAnyClientAcquired(state); } -static inline bool IsClientAcquired(uint32_t state, uint32_t client_bit_mask) { - return android::BufferHubDefs::IsClientAcquired(state, client_bit_mask); +static inline bool isClientAcquired(uint32_t state, uint32_t client_bit_mask) { + return android::BufferHubDefs::isClientAcquired(state, client_bit_mask); } -static inline bool IsClientReleased(uint32_t state, uint32_t client_bit_mask) { - return android::BufferHubDefs::IsClientReleased(state, client_bit_mask); +static inline bool isClientReleased(uint32_t state, uint32_t client_bit_mask) { + return android::BufferHubDefs::isClientReleased(state, client_bit_mask); } // Returns the next available buffer client's client_state_masks. // @params union_bits. Union of all existing clients' client_state_masks. -static inline uint32_t FindNextAvailableClientStateMask(uint32_t union_bits) { - return android::BufferHubDefs::FindNextAvailableClientStateMask(union_bits); +static inline uint32_t findNextAvailableClientStateMask(uint32_t union_bits) { + return android::BufferHubDefs::findNextAvailableClientStateMask(union_bits); } using MetadataHeader = android::BufferHubDefs::MetadataHeader; diff --git a/libs/vr/libbufferhub/producer_buffer.cpp b/libs/vr/libbufferhub/producer_buffer.cpp index edfdddf363..3d88ba5dbe 100644 --- a/libs/vr/libbufferhub/producer_buffer.cpp +++ b/libs/vr/libbufferhub/producer_buffer.cpp @@ -82,7 +82,7 @@ int ProducerBuffer::LocalPost(const DvrNativeBufferMetadata* meta, // The buffer can be posted iff the buffer state for this client is gained. uint32_t current_buffer_state = buffer_state_->load(std::memory_order_acquire); - if (!BufferHubDefs::IsClientGained(current_buffer_state, + if (!BufferHubDefs::isClientGained(current_buffer_state, client_state_mask())) { ALOGE("%s: not gained, id=%d state=%" PRIx32 ".", __FUNCTION__, id(), current_buffer_state); @@ -103,7 +103,7 @@ int ProducerBuffer::LocalPost(const DvrNativeBufferMetadata* meta, "%" PRIx32 ". About to try again if the buffer is still gained by this client.", __FUNCTION__, current_buffer_state, updated_buffer_state); - if (!BufferHubDefs::IsClientGained(current_buffer_state, + if (!BufferHubDefs::isClientGained(current_buffer_state, client_state_mask())) { ALOGE( "%s: Failed to post the buffer. The buffer is no longer gained, " @@ -166,14 +166,14 @@ int ProducerBuffer::LocalGain(DvrNativeBufferMetadata* out_meta, ALOGD_IF(TRACE, "%s: buffer=%d, state=%" PRIx32 ".", __FUNCTION__, id(), current_buffer_state); - if (BufferHubDefs::IsClientGained(current_buffer_state, + if (BufferHubDefs::isClientGained(current_buffer_state, client_state_mask())) { ALOGV("%s: already gained id=%d.", __FUNCTION__, id()); return 0; } - if (BufferHubDefs::AnyClientAcquired(current_buffer_state) || - BufferHubDefs::AnyClientGained(current_buffer_state) || - (BufferHubDefs::AnyClientPosted( + if (BufferHubDefs::isAnyClientAcquired(current_buffer_state) || + BufferHubDefs::isAnyClientGained(current_buffer_state) || + (BufferHubDefs::isAnyClientPosted( current_buffer_state & active_clients_bit_mask_->load(std::memory_order_acquire)) && !gain_posted_buffer)) { @@ -195,9 +195,9 @@ int ProducerBuffer::LocalGain(DvrNativeBufferMetadata* out_meta, "clients.", __FUNCTION__, current_buffer_state, updated_buffer_state); - if (BufferHubDefs::AnyClientAcquired(current_buffer_state) || - BufferHubDefs::AnyClientGained(current_buffer_state) || - (BufferHubDefs::AnyClientPosted( + if (BufferHubDefs::isAnyClientAcquired(current_buffer_state) || + BufferHubDefs::isAnyClientGained(current_buffer_state) || + (BufferHubDefs::isAnyClientPosted( current_buffer_state & active_clients_bit_mask_->load(std::memory_order_acquire)) && !gain_posted_buffer)) { @@ -291,7 +291,7 @@ Status ProducerBuffer::Detach() { // TODO(b/112338294) Keep here for reference. Remove it after new logic is // written. /* uint32_t buffer_state = buffer_state_->load(std::memory_order_acquire); - if (!BufferHubDefs::IsClientGained( + if (!BufferHubDefs::isClientGained( buffer_state, BufferHubDefs::kFirstClientStateMask)) { // Can only detach a ProducerBuffer when it's in gained state. ALOGW("ProducerBuffer::Detach: The buffer (id=%d, state=0x%" PRIx32 diff --git a/libs/vr/libbufferhubqueue/buffer_hub_queue_client.cpp b/libs/vr/libbufferhubqueue/buffer_hub_queue_client.cpp index d7833f382b..2d3fa4aec0 100644 --- a/libs/vr/libbufferhubqueue/buffer_hub_queue_client.cpp +++ b/libs/vr/libbufferhubqueue/buffer_hub_queue_client.cpp @@ -532,7 +532,7 @@ Status ProducerQueue::AddBuffer( Status ProducerQueue::InsertBuffer( const std::shared_ptr& buffer) { if (buffer == nullptr || - !BufferHubDefs::IsClientGained(buffer->buffer_state(), + !BufferHubDefs::isClientGained(buffer->buffer_state(), buffer->client_state_mask())) { ALOGE( "ProducerQueue::InsertBuffer: Can only insert a buffer when it's in " @@ -638,7 +638,7 @@ Status> ProducerQueue::DequeueUnacquiredBuffer( static_cast(*slot)); return ErrorStatus(EIO); } - if (!BufferHubDefs::AnyClientAcquired(buffer->buffer_state())) { + if (!BufferHubDefs::isAnyClientAcquired(buffer->buffer_state())) { *slot = *iter; unavailable_buffers_slot_.erase(iter); unavailable_buffers_slot_.push_back(*slot); diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 6b802fbf83..90ac1c2535 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -53,7 +53,7 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d std::make_shared(desc.width, desc.height, desc.layers, desc.format, desc.usage, userMetadataSize, BufferHubIdGenerator::getInstance().getId()); - if (node == nullptr || !node->IsValid()) { + if (node == nullptr || !node->isValid()) { ALOGE("%s: creating BufferNode failed.", __FUNCTION__); _hidl_cb(/*status=*/BufferHubStatus::ALLOCATION_FAILED, /*bufferClient=*/nullptr, /*bufferTraits=*/{}); @@ -70,11 +70,11 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d NATIVE_HANDLE_DECLARE_STORAGE(bufferInfoStorage, BufferHubDefs::kBufferInfoNumFds, BufferHubDefs::kBufferInfoNumInts); hidl_handle bufferInfo = - buildBufferInfo(bufferInfoStorage, node->id(), node->AddNewActiveClientsBitToMask(), - node->user_metadata_size(), node->metadata().ashmem_fd(), + buildBufferInfo(bufferInfoStorage, node->id(), node->addNewActiveClientsBitToMask(), + node->userMetadataSize(), node->metadata().ashmemFd(), node->eventFd().get()); BufferTraits bufferTraits = {/*bufferDesc=*/description, - /*bufferHandle=*/hidl_handle(node->buffer_handle()), + /*bufferHandle=*/hidl_handle(node->bufferHandle()), /*bufferInfo=*/std::move(bufferInfo)}; _hidl_cb(/*status=*/BufferHubStatus::NO_ERROR, /*bufferClient=*/client, @@ -141,7 +141,7 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, } sp client = new BufferClient(*originClient); - uint32_t clientStateMask = client->getBufferNode()->AddNewActiveClientsBitToMask(); + uint32_t clientStateMask = client->getBufferNode()->addNewActiveClientsBitToMask(); if (clientStateMask == 0U) { // Reach max client count ALOGE("%s: import failed, BufferNode#%u reached maximum clients.", __FUNCTION__, @@ -157,17 +157,17 @@ Return BufferHubService::importBuffer(const hidl_handle& tokenHandle, std::shared_ptr node = client->getBufferNode(); HardwareBufferDescription bufferDesc; - memcpy(&bufferDesc, &node->buffer_desc(), sizeof(HardwareBufferDescription)); + memcpy(&bufferDesc, &node->bufferDesc(), sizeof(HardwareBufferDescription)); // Allocate memory for bufferInfo of type hidl_handle on the stack. See // http://aosp/286282 for the usage of NATIVE_HANDLE_DECLARE_STORAGE. NATIVE_HANDLE_DECLARE_STORAGE(bufferInfoStorage, BufferHubDefs::kBufferInfoNumFds, BufferHubDefs::kBufferInfoNumInts); hidl_handle bufferInfo = buildBufferInfo(bufferInfoStorage, node->id(), clientStateMask, - node->user_metadata_size(), - node->metadata().ashmem_fd(), node->eventFd().get()); + node->userMetadataSize(), node->metadata().ashmemFd(), + node->eventFd().get()); BufferTraits bufferTraits = {/*bufferDesc=*/bufferDesc, - /*bufferHandle=*/hidl_handle(node->buffer_handle()), + /*bufferHandle=*/hidl_handle(node->bufferHandle()), /*bufferInfo=*/std::move(bufferInfo)}; _hidl_cb(/*status=*/BufferHubStatus::NO_ERROR, /*bufferClient=*/client, @@ -231,10 +231,10 @@ Return BufferHubService::debug(const hidl_handle& fd, const hidl_vec node = std::move(iter->second.first); const uint32_t clientCount = iter->second.second; - AHardwareBuffer_Desc desc = node->buffer_desc(); + AHardwareBuffer_Desc desc = node->bufferDesc(); MetadataHeader* metadataHeader = - const_cast(&node->metadata())->metadata_header(); + const_cast(&node->metadata())->metadataHeader(); const uint32_t state = metadataHeader->buffer_state.load(std::memory_order_acquire); const uint64_t index = metadataHeader->queue_index; diff --git a/services/bufferhub/BufferNode.cpp b/services/bufferhub/BufferNode.cpp index dfa2de0173..4f877b264a 100644 --- a/services/bufferhub/BufferNode.cpp +++ b/services/bufferhub/BufferNode.cpp @@ -11,10 +11,10 @@ namespace bufferhub { namespace V1_0 { namespace implementation { -void BufferNode::InitializeMetadata() { +void BufferNode::initializeMetadata() { // Using placement new here to reuse shared memory instead of new allocation // Initialize the atomic variables to zero. - BufferHubDefs::MetadataHeader* metadataHeader = mMetadata.metadata_header(); + BufferHubDefs::MetadataHeader* metadataHeader = mMetadata.metadataHeader(); mBufferState = new (&metadataHeader->buffer_state) std::atomic(0); mFenceState = new (&metadataHeader->fence_state) std::atomic(0); mActiveClientsBitMask = new (&metadataHeader->active_clients_bit_mask) std::atomic(0); @@ -54,12 +54,12 @@ BufferNode::BufferNode(uint32_t width, uint32_t height, uint32_t layer_count, ui mBufferDesc.usage = usage; mBufferDesc.stride = out_stride; - mMetadata = BufferHubMetadata::Create(user_metadata_size); - if (!mMetadata.IsValid()) { + mMetadata = BufferHubMetadata::create(user_metadata_size); + if (!mMetadata.isValid()) { ALOGE("%s: Failed to allocate metadata.", __FUNCTION__); return; } - InitializeMetadata(); + initializeMetadata(); } BufferNode::~BufferNode() { @@ -77,17 +77,17 @@ BufferNode::~BufferNode() { } } -uint32_t BufferNode::GetActiveClientsBitMask() const { +uint32_t BufferNode::getActiveClientsBitMask() const { return mActiveClientsBitMask->load(std::memory_order_acquire); } -uint32_t BufferNode::AddNewActiveClientsBitToMask() { - uint32_t currentActiveClientsBitMask = GetActiveClientsBitMask(); +uint32_t BufferNode::addNewActiveClientsBitToMask() { + uint32_t currentActiveClientsBitMask = getActiveClientsBitMask(); uint32_t client_state_mask = 0U; uint32_t updatedActiveClientsBitMask = 0U; do { client_state_mask = - BufferHubDefs::FindNextAvailableClientStateMask(currentActiveClientsBitMask); + BufferHubDefs::findNextAvailableClientStateMask(currentActiveClientsBitMask); if (client_state_mask == 0U) { ALOGE("%s: reached the maximum number of channels per buffer node: %d.", __FUNCTION__, BufferHubDefs::kMaxNumberOfClients); @@ -102,7 +102,7 @@ uint32_t BufferNode::AddNewActiveClientsBitToMask() { return client_state_mask; } -void BufferNode::RemoveClientsBitFromMask(const uint32_t& value) { +void BufferNode::removeClientsBitFromMask(const uint32_t& value) { mActiveClientsBitMask->fetch_and(~value); } diff --git a/services/bufferhub/include/bufferhub/BufferNode.h b/services/bufferhub/include/bufferhub/BufferNode.h index 112a21c9af..04970fd14d 100644 --- a/services/bufferhub/include/bufferhub/BufferNode.h +++ b/services/bufferhub/include/bufferhub/BufferNode.h @@ -22,15 +22,15 @@ public: ~BufferNode(); // Returns whether the object holds a valid metadata. - bool IsValid() const { return mMetadata.IsValid(); } + bool isValid() const { return mMetadata.isValid(); } int id() const { return mId; } - size_t user_metadata_size() const { return mMetadata.user_metadata_size(); } + size_t userMetadataSize() const { return mMetadata.userMetadataSize(); } // Accessors of the buffer description and handle - const native_handle_t* buffer_handle() const { return mBufferHandle; } - const AHardwareBuffer_Desc& buffer_desc() const { return mBufferDesc; } + const native_handle_t* bufferHandle() const { return mBufferHandle; } + const AHardwareBuffer_Desc& bufferDesc() const { return mBufferDesc; } // Accessor of event fd. const BufferHubEventFd& eventFd() const { return mEventFd; } @@ -41,24 +41,24 @@ public: // Gets the current value of mActiveClientsBitMask in mMetadata with // std::memory_order_acquire, so that all previous releases of // mActiveClientsBitMask from all threads will be returned here. - uint32_t GetActiveClientsBitMask() const; + uint32_t getActiveClientsBitMask() const; // Find and add a new client state mask to mActiveClientsBitMask in // mMetadata. // Return the new client state mask that is added to mActiveClientsBitMask. // Return 0U if there are already 16 clients of the buffer. - uint32_t AddNewActiveClientsBitToMask(); + uint32_t addNewActiveClientsBitToMask(); // Removes the value from active_clients_bit_mask in mMetadata with // std::memory_order_release, so that the change will be visible to any // acquire of mActiveClientsBitMask in any threads after the succeed of // this operation. - void RemoveClientsBitFromMask(const uint32_t& value); + void removeClientsBitFromMask(const uint32_t& value); private: // Helper method for constructors to initialize atomic metadata header // variables in shared memory. - void InitializeMetadata(); + void initializeMetadata(); // Gralloc buffer handles. native_handle_t* mBufferHandle; diff --git a/services/bufferhub/tests/BufferNode_test.cpp b/services/bufferhub/tests/BufferNode_test.cpp index ccb1197498..b9f1c81c63 100644 --- a/services/bufferhub/tests/BufferNode_test.cpp +++ b/services/bufferhub/tests/BufferNode_test.cpp @@ -28,7 +28,7 @@ protected: void SetUp() override { buffer_node = new BufferNode(kWidth, kHeight, kLayerCount, kFormat, kUsage, kUserMetadataSize); - ASSERT_TRUE(buffer_node->IsValid()); + ASSERT_TRUE(buffer_node->isValid()); } void TearDown() override { @@ -41,65 +41,64 @@ protected: }; TEST_F(BufferNodeTest, TestCreateBufferNode) { - EXPECT_EQ(buffer_node->user_metadata_size(), kUserMetadataSize); + EXPECT_EQ(buffer_node->userMetadataSize(), kUserMetadataSize); // Test the handle just allocated is good (i.e. able to be imported) GraphicBufferMapper& mapper = GraphicBufferMapper::get(); const native_handle_t* outHandle; status_t ret = - mapper.importBuffer(buffer_node->buffer_handle(), buffer_node->buffer_desc().width, - buffer_node->buffer_desc().height, - buffer_node->buffer_desc().layers, - buffer_node->buffer_desc().format, buffer_node->buffer_desc().usage, - buffer_node->buffer_desc().stride, &outHandle); + mapper.importBuffer(buffer_node->bufferHandle(), buffer_node->bufferDesc().width, + buffer_node->bufferDesc().height, buffer_node->bufferDesc().layers, + buffer_node->bufferDesc().format, buffer_node->bufferDesc().usage, + buffer_node->bufferDesc().stride, &outHandle); EXPECT_EQ(ret, OK); EXPECT_THAT(outHandle, NotNull()); } -TEST_F(BufferNodeTest, TestAddNewActiveClientsBitToMask_twoNewClients) { - uint32_t new_client_state_mask_1 = buffer_node->AddNewActiveClientsBitToMask(); - EXPECT_EQ(buffer_node->GetActiveClientsBitMask(), new_client_state_mask_1); +TEST_F(BufferNodeTest, TestaddNewActiveClientsBitToMask_twoNewClients) { + uint32_t new_client_state_mask_1 = buffer_node->addNewActiveClientsBitToMask(); + EXPECT_EQ(buffer_node->getActiveClientsBitMask(), new_client_state_mask_1); // Request and add a new client_state_mask again. // Active clients bit mask should be the union of the two new // client_state_masks. - uint32_t new_client_state_mask_2 = buffer_node->AddNewActiveClientsBitToMask(); - EXPECT_EQ(buffer_node->GetActiveClientsBitMask(), + uint32_t new_client_state_mask_2 = buffer_node->addNewActiveClientsBitToMask(); + EXPECT_EQ(buffer_node->getActiveClientsBitMask(), new_client_state_mask_1 | new_client_state_mask_2); } -TEST_F(BufferNodeTest, TestAddNewActiveClientsBitToMask_32NewClients) { +TEST_F(BufferNodeTest, TestaddNewActiveClientsBitToMask_32NewClients) { uint32_t new_client_state_mask = 0U; uint32_t current_mask = 0U; uint32_t expected_mask = 0U; for (int i = 0; i < BufferHubDefs::kMaxNumberOfClients; ++i) { - new_client_state_mask = buffer_node->AddNewActiveClientsBitToMask(); + new_client_state_mask = buffer_node->addNewActiveClientsBitToMask(); EXPECT_NE(new_client_state_mask, 0U); EXPECT_FALSE(new_client_state_mask & current_mask); expected_mask = current_mask | new_client_state_mask; - current_mask = buffer_node->GetActiveClientsBitMask(); + current_mask = buffer_node->getActiveClientsBitMask(); EXPECT_EQ(current_mask, expected_mask); } // Method should fail upon requesting for more than maximum allowable clients. - new_client_state_mask = buffer_node->AddNewActiveClientsBitToMask(); + new_client_state_mask = buffer_node->addNewActiveClientsBitToMask(); EXPECT_EQ(new_client_state_mask, 0U); EXPECT_EQ(errno, E2BIG); } TEST_F(BufferNodeTest, TestRemoveActiveClientsBitFromMask) { - buffer_node->AddNewActiveClientsBitToMask(); - uint32_t current_mask = buffer_node->GetActiveClientsBitMask(); - uint32_t new_client_state_mask = buffer_node->AddNewActiveClientsBitToMask(); - EXPECT_NE(buffer_node->GetActiveClientsBitMask(), current_mask); + buffer_node->addNewActiveClientsBitToMask(); + uint32_t current_mask = buffer_node->getActiveClientsBitMask(); + uint32_t new_client_state_mask = buffer_node->addNewActiveClientsBitToMask(); + EXPECT_NE(buffer_node->getActiveClientsBitMask(), current_mask); - buffer_node->RemoveClientsBitFromMask(new_client_state_mask); - EXPECT_EQ(buffer_node->GetActiveClientsBitMask(), current_mask); + buffer_node->removeClientsBitFromMask(new_client_state_mask); + 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_client_state_mask); - EXPECT_EQ(buffer_node->GetActiveClientsBitMask(), current_mask); + buffer_node->removeClientsBitFromMask(new_client_state_mask); + EXPECT_EQ(buffer_node->getActiveClientsBitMask(), current_mask); } } // namespace diff --git a/services/vr/bufferhubd/producer_channel.cpp b/services/vr/bufferhubd/producer_channel.cpp index 164d9e6b19..f3e54a0ed2 100644 --- a/services/vr/bufferhubd/producer_channel.cpp +++ b/services/vr/bufferhubd/producer_channel.cpp @@ -253,7 +253,7 @@ Status ProducerChannel::CreateConsumerStateMask() { uint32_t current_active_clients_bit_mask = active_clients_bit_mask_->load(std::memory_order_acquire); uint32_t consumer_state_mask = - BufferHubDefs::FindNextAvailableClientStateMask( + BufferHubDefs::findNextAvailableClientStateMask( current_active_clients_bit_mask | orphaned_consumer_bit_mask_); if (consumer_state_mask == 0U) { ALOGE("%s: reached the maximum mumber of consumers per producer: 63.", @@ -279,7 +279,7 @@ Status ProducerChannel::CreateConsumerStateMask() { "condition.", __FUNCTION__, updated_active_clients_bit_mask, current_active_clients_bit_mask); - consumer_state_mask = BufferHubDefs::FindNextAvailableClientStateMask( + consumer_state_mask = BufferHubDefs::findNextAvailableClientStateMask( current_active_clients_bit_mask | orphaned_consumer_bit_mask_); if (consumer_state_mask == 0U) { ALOGE("%s: reached the maximum mumber of consumers per producer: %d.", @@ -337,13 +337,13 @@ Status ProducerChannel::CreateConsumer( // consumer to a buffer that is available to producer (a.k.a a fully-released // buffer) or a gained buffer. if (current_buffer_state == 0U || - BufferHubDefs::AnyClientGained(current_buffer_state)) { + BufferHubDefs::isAnyClientGained(current_buffer_state)) { return {status.take()}; } // Signal the new consumer when adding it to a posted producer. bool update_buffer_state = true; - if (!BufferHubDefs::IsClientPosted(current_buffer_state, + if (!BufferHubDefs::isClientPosted(current_buffer_state, consumer_state_mask)) { uint32_t updated_buffer_state = current_buffer_state ^ @@ -360,7 +360,7 @@ Status ProducerChannel::CreateConsumer( "released.", __FUNCTION__, current_buffer_state, updated_buffer_state); if (current_buffer_state == 0U || - BufferHubDefs::AnyClientGained(current_buffer_state)) { + BufferHubDefs::isAnyClientGained(current_buffer_state)) { ALOGI("%s: buffer is gained or fully released, state=%" PRIx32 ".", __FUNCTION__, current_buffer_state); update_buffer_state = false; @@ -371,7 +371,7 @@ Status ProducerChannel::CreateConsumer( (consumer_state_mask & BufferHubDefs::kHighBitsMask); } } - if (update_buffer_state || BufferHubDefs::IsClientPosted( + if (update_buffer_state || BufferHubDefs::isClientPosted( buffer_state_->load(std::memory_order_acquire), consumer_state_mask)) { consumer->OnProducerPosted(); @@ -457,7 +457,7 @@ Status ProducerChannel::OnProducerGain(Message& /*message*/) { buffer_id()); uint32_t buffer_state = buffer_state_->load(std::memory_order_acquire); - if (!BufferHubDefs::IsClientGained( + if (!BufferHubDefs::isClientGained( buffer_state, BufferHubDefs::kFirstClientStateMask)) { // Can only detach a ProducerBuffer when it's in gained state. ALOGW( @@ -616,9 +616,9 @@ void ProducerChannel::RemoveConsumer(ConsumerChannel* channel) { const uint32_t current_buffer_state = buffer_state_->load(std::memory_order_acquire); - if (BufferHubDefs::IsClientPosted(current_buffer_state, + if (BufferHubDefs::isClientPosted(current_buffer_state, consumer_state_mask) || - BufferHubDefs::IsClientAcquired(current_buffer_state, + BufferHubDefs::isClientAcquired(current_buffer_state, consumer_state_mask)) { // The consumer client is being destoryed without releasing. This could // happen in corner cases when the consumer crashes. Here we mark it @@ -627,9 +627,9 @@ void ProducerChannel::RemoveConsumer(ConsumerChannel* channel) { return; } - if (BufferHubDefs::IsClientReleased(current_buffer_state, + if (BufferHubDefs::isClientReleased(current_buffer_state, consumer_state_mask) || - BufferHubDefs::AnyClientGained(current_buffer_state)) { + BufferHubDefs::isAnyClientGained(current_buffer_state)) { // The consumer is being close while it is suppose to signal a release // fence. Signal the dummy fence here. if (fence_state_->load(std::memory_order_acquire) & consumer_state_mask) { diff --git a/services/vr/bufferhubd/producer_queue_channel.cpp b/services/vr/bufferhubd/producer_queue_channel.cpp index 6b33f5094c..004dc7cb4f 100644 --- a/services/vr/bufferhubd/producer_queue_channel.cpp +++ b/services/vr/bufferhubd/producer_queue_channel.cpp @@ -323,7 +323,7 @@ Status ProducerQueueChannel::OnProducerQueueInsertBuffer( // memory to indicate which client is the last producer of the buffer. // Currently, the first client is the only producer to the buffer. // Thus, it checks whether the first client gains the buffer below. - if (!BufferHubDefs::IsClientGained(buffer_state, + if (!BufferHubDefs::isClientGained(buffer_state, BufferHubDefs::kFirstClientBitMask)) { // Rejects the request if the requested buffer is not in Gained state. ALOGE( -- cgit v1.2.3-59-g8ed1b From dfe0f4c69ad4f547db7109e7b4a1db42448d1233 Mon Sep 17 00:00:00 2001 From: Jiwen 'Steve' Cai Date: Tue, 15 Jan 2019 21:49:07 -0800 Subject: Fix BufferHubBuffer black screen 1/ BufferHubService should read back fields of HardwareBufferDescription of allocated BufferNode and send them back to the client. 2/ Update some minor naming for better readability. Bug: 122959735 Test: cts-tradefed run commandAndExit cts -m CtsNativeHardwareTestCases Change-Id: I10a176810d78c754fe0b69d4128078fc17d50035 --- libs/ui/BufferHubBuffer.cpp | 8 ++++---- services/bufferhub/BufferHubService.cpp | 8 +++++++- 2 files changed, 11 insertions(+), 5 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/BufferHubBuffer.cpp b/libs/ui/BufferHubBuffer.cpp index 405bcb62e1..604665bc91 100644 --- a/libs/ui/BufferHubBuffer.cpp +++ b/libs/ui/BufferHubBuffer.cpp @@ -79,10 +79,10 @@ BufferHubBuffer::BufferHubBuffer(uint32_t width, uint32_t height, uint32_t layer sp client; BufferTraits bufferTraits; IBufferHub::allocateBuffer_cb alloc_cb = [&](const auto& status, const auto& outClient, - const auto& traits) { + const auto& outTraits) { ret = status; client = std::move(outClient); - bufferTraits = std::move(traits); + bufferTraits = std::move(outTraits); }; if (!bufferhub->allocateBuffer(desc, static_cast(userMetadataSize), alloc_cb) @@ -116,10 +116,10 @@ BufferHubBuffer::BufferHubBuffer(const sp& token) { sp client; BufferTraits bufferTraits; IBufferHub::importBuffer_cb import_cb = [&](const auto& status, const auto& outClient, - const auto& traits) { + const auto& outTraits) { ret = status; client = std::move(outClient); - bufferTraits = std::move(traits); + bufferTraits = std::move(outTraits); }; // hidl_handle(native_handle_t*) simply creates a raw pointer reference withouth ownership diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 90ac1c2535..ade08e7d17 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -73,7 +73,13 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d buildBufferInfo(bufferInfoStorage, node->id(), node->addNewActiveClientsBitToMask(), node->userMetadataSize(), node->metadata().ashmemFd(), node->eventFd().get()); - BufferTraits bufferTraits = {/*bufferDesc=*/description, + // During the gralloc allocation carried out by BufferNode, gralloc allocator will populate the + // fields of its HardwareBufferDescription (i.e. strides) according to the actual + // gralloc implementation. We need to read those fields back and send them to the client via + // BufferTraits. + HardwareBufferDescription allocatedBufferDesc; + memcpy(&allocatedBufferDesc, &node->bufferDesc(), sizeof(AHardwareBuffer_Desc)); + BufferTraits bufferTraits = {/*bufferDesc=*/allocatedBufferDesc, /*bufferHandle=*/hidl_handle(node->bufferHandle()), /*bufferInfo=*/std::move(bufferInfo)}; -- cgit v1.2.3-59-g8ed1b From f377a76a47cdcfd15437c94d547a30a9da82fb7c Mon Sep 17 00:00:00 2001 From: Tianyu Jiang Date: Wed, 13 Feb 2019 13:46:42 -0800 Subject: Fix code style violation in the variables in BufferHubDefs::MetadataHeader Violation: variables should be lowerCaseCamel in the following directories: frameworks/native/libs/ui and frameworks/native/services/bufferhub Test: m, mma Bug: 68273829 Change-Id: I7dc56ec17089456f98b018032f04f779b12632b2 --- libs/ui/BufferHubBuffer.cpp | 25 ++++---- libs/ui/include/ui/BufferHubBuffer.h | 2 +- libs/ui/include/ui/BufferHubDefs.h | 66 +++++++++++----------- libs/ui/tests/BufferHubMetadata_test.cpp | 4 +- libs/vr/libbufferhub/buffer_hub_base.cpp | 6 +- .../include/private/dvr/buffer_hub_base.h | 4 +- services/bufferhub/BufferHubService.cpp | 4 +- services/bufferhub/BufferNode.cpp | 6 +- services/vr/bufferhubd/producer_channel.cpp | 13 ++--- 9 files changed, 64 insertions(+), 66 deletions(-) (limited to 'services/bufferhub/BufferHubService.cpp') diff --git a/libs/ui/BufferHubBuffer.cpp b/libs/ui/BufferHubBuffer.cpp index 604665bc91..da91a979fe 100644 --- a/libs/ui/BufferHubBuffer.cpp +++ b/libs/ui/BufferHubBuffer.cpp @@ -78,15 +78,14 @@ BufferHubBuffer::BufferHubBuffer(uint32_t width, uint32_t height, uint32_t layer BufferHubStatus ret; sp client; BufferTraits bufferTraits; - IBufferHub::allocateBuffer_cb alloc_cb = [&](const auto& status, const auto& outClient, - const auto& outTraits) { + IBufferHub::allocateBuffer_cb allocCb = [&](const auto& status, const auto& outClient, + const auto& outTraits) { ret = status; client = std::move(outClient); bufferTraits = std::move(outTraits); }; - if (!bufferhub->allocateBuffer(desc, static_cast(userMetadataSize), alloc_cb) - .isOk()) { + if (!bufferhub->allocateBuffer(desc, static_cast(userMetadataSize), allocCb).isOk()) { ALOGE("%s: allocateBuffer transaction failed!", __FUNCTION__); return; } else if (ret != BufferHubStatus::NO_ERROR) { @@ -115,8 +114,8 @@ BufferHubBuffer::BufferHubBuffer(const sp& token) { BufferHubStatus ret; sp client; BufferTraits bufferTraits; - IBufferHub::importBuffer_cb import_cb = [&](const auto& status, const auto& outClient, - const auto& outTraits) { + IBufferHub::importBuffer_cb importCb = [&](const auto& status, const auto& outClient, + const auto& outTraits) { ret = status; client = std::move(outClient); bufferTraits = std::move(outTraits); @@ -124,7 +123,7 @@ BufferHubBuffer::BufferHubBuffer(const sp& token) { // hidl_handle(native_handle_t*) simply creates a raw pointer reference withouth ownership // transfer. - if (!bufferhub->importBuffer(hidl_handle(token.get()->handle()), import_cb).isOk()) { + if (!bufferhub->importBuffer(hidl_handle(token.get()->handle()), importCb).isOk()) { ALOGE("%s: importBuffer transaction failed!", __FUNCTION__); return; } else if (ret != BufferHubStatus::NO_ERROR) { @@ -210,9 +209,9 @@ int BufferHubBuffer::initWithBufferTraits(const BufferTraits& bufferTraits) { // Populate shortcuts to the atomics in metadata. auto metadataHeader = mMetadata.metadataHeader(); - mBufferState = &metadataHeader->buffer_state; - mFenceState = &metadataHeader->fence_state; - mActiveClientsBitMask = &metadataHeader->active_clients_bit_mask; + mBufferState = &metadataHeader->bufferState; + mFenceState = &metadataHeader->fenceState; + mActiveClientsBitMask = &metadataHeader->activeClientsBitMask; // The C++ standard recommends (but does not require) that lock-free atomic operations are // also address-free, that is, suitable for communication between processes using shared // memory. @@ -230,7 +229,7 @@ int BufferHubBuffer::initWithBufferTraits(const BufferTraits& bufferTraits) { mClientStateMask = clientBitMask; // TODO(b/112012161) Set up shared fences. - ALOGD("%s: id=%d, buffer_state=%" PRIx32 ".", __FUNCTION__, mId, + ALOGD("%s: id=%d, mBufferState=%" PRIx32 ".", __FUNCTION__, mId, mBufferState->load(std::memory_order_acquire)); return 0; } @@ -336,12 +335,12 @@ sp BufferHubBuffer::duplicate() { hidl_handle token; BufferHubStatus ret; - IBufferClient::duplicate_cb dup_cb = [&](const auto& outToken, const auto& status) { + IBufferClient::duplicate_cb dupCb = [&](const auto& outToken, const auto& status) { token = std::move(outToken); ret = status; }; - if (!mBufferClient->duplicate(dup_cb).isOk()) { + if (!mBufferClient->duplicate(dupCb).isOk()) { ALOGE("%s: duplicate transaction failed!", __FUNCTION__); return nullptr; } else if (ret != BufferHubStatus::NO_ERROR) { diff --git a/libs/ui/include/ui/BufferHubBuffer.h b/libs/ui/include/ui/BufferHubBuffer.h index 5c0903251a..5ba189c2c3 100644 --- a/libs/ui/include/ui/BufferHubBuffer.h +++ b/libs/ui/include/ui/BufferHubBuffer.h @@ -57,7 +57,7 @@ public: const BufferHubEventFd& eventFd() const { return mEventFd; } - // Returns the current value of MetadataHeader::buffer_state. + // Returns the current value of MetadataHeader::bufferState. uint32_t bufferState() const { return mBufferState->load(std::memory_order_acquire); } // A state mask which is unique to a buffer hub client among all its siblings sharing the same diff --git a/libs/ui/include/ui/BufferHubDefs.h b/libs/ui/include/ui/BufferHubDefs.h index 722a060597..10f274f569 100644 --- a/libs/ui/include/ui/BufferHubDefs.h +++ b/libs/ui/include/ui/BufferHubDefs.h @@ -32,7 +32,7 @@ namespace BufferHubDefs { // Single buffer clients (up to 16) ownership signal. // 32-bit atomic unsigned int. // Each client takes 2 bits. The first bit locates in the first 16 bits of -// buffer_state; the second bit locates in the last 16 bits of buffer_state. +// bufferState; the second bit locates in the last 16 bits of bufferState. // Client states: // Gained state 11. Exclusive write state. // Posted state 10. @@ -64,9 +64,9 @@ static constexpr uint32_t kFirstClientBitMask = (1U << kMaxNumberOfClients) + 1U // Returns true if any of the client is in gained state. static inline bool isAnyClientGained(uint32_t state) { - uint32_t high_bits = state >> kMaxNumberOfClients; - uint32_t low_bits = state & kLowbitsMask; - return high_bits == low_bits && low_bits != 0U; + uint32_t highBits = state >> kMaxNumberOfClients; + uint32_t lowBits = state & kLowbitsMask; + return highBits == lowBits && lowBits != 0U; } // Returns true if the input client is in gained state. @@ -76,34 +76,34 @@ static inline bool isClientGained(uint32_t state, uint32_t client_bit_mask) { // Returns true if any of the client is in posted state. static inline bool isAnyClientPosted(uint32_t state) { - uint32_t high_bits = state >> kMaxNumberOfClients; - uint32_t low_bits = state & kLowbitsMask; - uint32_t posted_or_acquired = high_bits ^ low_bits; - return posted_or_acquired & high_bits; + uint32_t highBits = state >> kMaxNumberOfClients; + uint32_t lowBits = state & kLowbitsMask; + uint32_t postedOrAcquired = highBits ^ lowBits; + return postedOrAcquired & highBits; } // Returns true if the input client is in posted state. static inline bool isClientPosted(uint32_t state, uint32_t client_bit_mask) { - uint32_t client_bits = state & client_bit_mask; - if (client_bits == 0U) return false; - uint32_t low_bits = client_bits & kLowbitsMask; - return low_bits == 0U; + uint32_t clientBits = state & client_bit_mask; + if (clientBits == 0U) return false; + uint32_t lowBits = clientBits & kLowbitsMask; + return lowBits == 0U; } // Return true if any of the client is in acquired state. static inline bool isAnyClientAcquired(uint32_t state) { - uint32_t high_bits = state >> kMaxNumberOfClients; - uint32_t low_bits = state & kLowbitsMask; - uint32_t posted_or_acquired = high_bits ^ low_bits; - return posted_or_acquired & low_bits; + uint32_t highBits = state >> kMaxNumberOfClients; + uint32_t lowBits = state & kLowbitsMask; + uint32_t postedOrAcquired = highBits ^ lowBits; + return postedOrAcquired & lowBits; } // Return true if the input client is in acquired state. static inline bool isClientAcquired(uint32_t state, uint32_t client_bit_mask) { - uint32_t client_bits = state & client_bit_mask; - if (client_bits == 0U) return false; - uint32_t high_bits = client_bits & kHighBitsMask; - return high_bits == 0U; + uint32_t clientBits = state & client_bit_mask; + if (clientBits == 0U) return false; + uint32_t highBits = clientBits & kHighBitsMask; + return highBits == 0U; } // Returns true if the input client is in released state. @@ -114,12 +114,12 @@ static inline bool isClientReleased(uint32_t state, uint32_t client_bit_mask) { // Returns the next available buffer client's client_state_masks. // @params union_bits. Union of all existing clients' client_state_masks. static inline uint32_t findNextAvailableClientStateMask(uint32_t union_bits) { - uint32_t low_union = union_bits & kLowbitsMask; - if (low_union == kLowbitsMask) return 0U; - uint32_t incremented = low_union + 1U; - uint32_t difference = incremented ^ low_union; - uint32_t new_low_bit = (difference + 1U) >> 1; - return new_low_bit + (new_low_bit << kMaxNumberOfClients); + uint32_t lowUnion = union_bits & kLowbitsMask; + if (lowUnion == kLowbitsMask) return 0U; + uint32_t incremented = lowUnion + 1U; + uint32_t difference = incremented ^ lowUnion; + uint32_t newLowBit = (difference + 1U) >> 1; + return newLowBit + (newLowBit << kMaxNumberOfClients); } struct __attribute__((aligned(8))) MetadataHeader { @@ -129,22 +129,22 @@ struct __attribute__((aligned(8))) MetadataHeader { // platform (include Apps and vendor HAL). // Every client takes up one bit from the higher 32 bits and one bit from the lower 32 bits in - // buffer_state. - std::atomic buffer_state; + // bufferState. + std::atomic bufferState; - // Every client takes up one bit in fence_state. Only the lower 32 bits are valid. The upper 32 + // Every client takes up one bit in fenceState. Only the lower 32 bits are valid. The upper 32 // bits are there for easier manipulation, but the value should be ignored. - std::atomic fence_state; + std::atomic fenceState; // Every client takes up one bit from the higher 32 bits and one bit from the lower 32 bits in - // active_clients_bit_mask. - std::atomic active_clients_bit_mask; + // activeClientsBitMask. + std::atomic activeClientsBitMask; // Explicit padding 4 bytes. uint32_t padding; // The index of the buffer queue where the buffer belongs to. - uint64_t queue_index; + uint64_t queueIndex; // Public data format, which should be updated with caution. See more details in dvr_api.h DvrNativeBufferMetadata metadata; diff --git a/libs/ui/tests/BufferHubMetadata_test.cpp b/libs/ui/tests/BufferHubMetadata_test.cpp index f02c4fc178..eb978cabc6 100644 --- a/libs/ui/tests/BufferHubMetadata_test.cpp +++ b/libs/ui/tests/BufferHubMetadata_test.cpp @@ -51,7 +51,7 @@ TEST_F(BufferHubMetadataTest, Import_Success) { // Check if the newly allocated buffer is initialized in released state (i.e. // state equals to 0U). - EXPECT_TRUE(mh1->buffer_state.load() == 0U); + EXPECT_TRUE(mh1->bufferState.load() == 0U); EXPECT_TRUE(m2.isValid()); BufferHubDefs::MetadataHeader* mh2 = m2.metadataHeader(); @@ -59,7 +59,7 @@ TEST_F(BufferHubMetadataTest, Import_Success) { // Check if the newly allocated buffer is initialized in released state (i.e. // state equals to 0U). - EXPECT_TRUE(mh2->buffer_state.load() == 0U); + EXPECT_TRUE(mh2->bufferState.load() == 0U); } TEST_F(BufferHubMetadataTest, MoveMetadataInvalidatesOldOne) { diff --git a/libs/vr/libbufferhub/buffer_hub_base.cpp b/libs/vr/libbufferhub/buffer_hub_base.cpp index b28d10147b..17930b4405 100644 --- a/libs/vr/libbufferhub/buffer_hub_base.cpp +++ b/libs/vr/libbufferhub/buffer_hub_base.cpp @@ -122,15 +122,15 @@ int BufferHubBase::ImportBuffer() { // are 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 will be preserved. - buffer_state_ = &metadata_header_->buffer_state; + buffer_state_ = &metadata_header_->bufferState; ALOGD_IF(TRACE, "BufferHubBase::ImportBuffer: id=%d, buffer_state=%" PRIx32 ".", id(), buffer_state_->load(std::memory_order_acquire)); - fence_state_ = &metadata_header_->fence_state; + fence_state_ = &metadata_header_->fenceState; ALOGD_IF(TRACE, "BufferHubBase::ImportBuffer: id=%d, fence_state=%" PRIx32 ".", id(), fence_state_->load(std::memory_order_acquire)); - active_clients_bit_mask_ = &metadata_header_->active_clients_bit_mask; + active_clients_bit_mask_ = &metadata_header_->activeClientsBitMask; ALOGD_IF( TRACE, "BufferHubBase::ImportBuffer: id=%d, active_clients_bit_mask=%" PRIx32 diff --git a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_base.h b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_base.h index fa39d081e1..8a490d9983 100644 --- a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_base.h +++ b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_base.h @@ -97,8 +97,8 @@ class BufferHubBase : public pdx::Client { uint32_t usage() const { return buffer_.usage(); } uint32_t layer_count() const { return buffer_.layer_count(); } - uint64_t GetQueueIndex() const { return metadata_header_->queue_index; } - void SetQueueIndex(uint64_t index) { metadata_header_->queue_index = index; } + uint64_t GetQueueIndex() const { return metadata_header_->queueIndex; } + void SetQueueIndex(uint64_t index) { metadata_header_->queueIndex = index; } protected: explicit BufferHubBase(LocalChannelHandle channel); diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index ade08e7d17..7a3472fa7e 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -241,8 +241,8 @@ Return BufferHubService::debug(const hidl_handle& fd, const hidl_vec(&node->metadata())->metadataHeader(); - const uint32_t state = metadataHeader->buffer_state.load(std::memory_order_acquire); - const uint64_t index = metadataHeader->queue_index; + const uint32_t state = metadataHeader->bufferState.load(std::memory_order_acquire); + const uint64_t index = metadataHeader->queueIndex; stream << std::right; stream << std::setw(6) << /*Id=*/node->id(); diff --git a/services/bufferhub/BufferNode.cpp b/services/bufferhub/BufferNode.cpp index 1efb27ee0a..04ca6493f7 100644 --- a/services/bufferhub/BufferNode.cpp +++ b/services/bufferhub/BufferNode.cpp @@ -15,9 +15,9 @@ void BufferNode::initializeMetadata() { // Using placement new here to reuse shared memory instead of new allocation // Initialize the atomic variables to zero. BufferHubDefs::MetadataHeader* metadataHeader = mMetadata.metadataHeader(); - mBufferState = new (&metadataHeader->buffer_state) std::atomic(0); - mFenceState = new (&metadataHeader->fence_state) std::atomic(0); - mActiveClientsBitMask = new (&metadataHeader->active_clients_bit_mask) std::atomic(0); + mBufferState = new (&metadataHeader->bufferState) std::atomic(0); + mFenceState = new (&metadataHeader->fenceState) std::atomic(0); + mActiveClientsBitMask = new (&metadataHeader->activeClientsBitMask) std::atomic(0); // The C++ standard recommends (but does not require) that lock-free atomic operations are // also address-free, that is, suitable for communication between processes using shared // memory. diff --git a/services/vr/bufferhubd/producer_channel.cpp b/services/vr/bufferhubd/producer_channel.cpp index f3e54a0ed2..b49d89470f 100644 --- a/services/vr/bufferhubd/producer_channel.cpp +++ b/services/vr/bufferhubd/producer_channel.cpp @@ -91,11 +91,10 @@ int ProducerChannel::InitializeBuffer() { // Using placement new here to reuse shared memory instead of new allocation // and also initialize the value to zero. - buffer_state_ = - new (&metadata_header_->buffer_state) std::atomic(0); - fence_state_ = new (&metadata_header_->fence_state) std::atomic(0); + buffer_state_ = new (&metadata_header_->bufferState) std::atomic(0); + fence_state_ = new (&metadata_header_->fenceState) std::atomic(0); active_clients_bit_mask_ = - new (&metadata_header_->active_clients_bit_mask) std::atomic(0); + new (&metadata_header_->activeClientsBitMask) std::atomic(0); // Producer channel is never created after consumer channel, and one buffer // only have one fixed producer for now. Thus, it is correct to assume @@ -183,7 +182,7 @@ BufferHubChannel::BufferInfo ProducerChannel::GetBufferInfo() const { buffer_.height(), buffer_.layer_count(), buffer_.format(), buffer_.usage(), buffer_state_->load(std::memory_order_acquire), - signaled_mask, metadata_header_->queue_index); + signaled_mask, metadata_header_->queueIndex); } void ProducerChannel::HandleImpulse(Message& message) { @@ -547,7 +546,7 @@ Status ProducerChannel::OnConsumerRelease(Message&, "%s: orphaned buffer detected during the this acquire/release cycle: " "id=%d orphaned=0x%" PRIx32 " queue_index=%" PRId64 ".", __FUNCTION__, buffer_id(), orphaned_consumer_bit_mask_, - metadata_header_->queue_index); + metadata_header_->queueIndex); orphaned_consumer_bit_mask_ = 0; } } @@ -581,7 +580,7 @@ void ProducerChannel::OnConsumerOrphaned(const uint32_t& consumer_state_mask) { "consumer_state_mask=%" PRIx32 " queue_index=%" PRId64 " buffer_state=%" PRIx32 " fence_state=%" PRIx32 ".", __FUNCTION__, buffer_id(), consumer_state_mask, - metadata_header_->queue_index, + metadata_header_->queueIndex, buffer_state_->load(std::memory_order_acquire), fence_state_->load(std::memory_order_acquire)); } -- cgit v1.2.3-59-g8ed1b