From ec74596af82626ba358ec7c756f0248acab9c8f9 Mon Sep 17 00:00:00 2001 From: Chia-I Wu Date: Fri, 21 Apr 2017 15:02:22 -0700 Subject: [PATCH 1/2] graphics: clarify importBuffer and passthrough HALs A buffer handle recieved from a HAL is by definition raw and needs to be imported. But because of passthrough HALs, such a raw handle may have been imported already. Explicitly specify that an implementation must accept such a raw handle. Bug: 37540361 Test: boots on angler, ryu and marlin Change-Id: I5ecf526e59b27cc4a8f7f5d5ec27477da0946ece --- current.txt | 2 +- graphics/mapper/2.0/IMapper.hal | 15 +++++++++++---- graphics/mapper/2.0/default/GrallocMapper.cpp | 7 ++----- .../VtsHalGraphicsMapperV2_0TargetTest.cpp | 12 ------------ 4 files changed, 14 insertions(+), 22 deletions(-) diff --git a/current.txt b/current.txt index 575e8faccb..2b46fcb21f 100644 --- a/current.txt +++ b/current.txt @@ -103,7 +103,7 @@ b3aac6c3817f039964fcd62268274b3039e17bd7d0d5b40b4d1d1c7b19a1f866 android.hardwar b19d00eb8a8b3b0034a0321f22e8f32162bf4c2aebbce6da22c025f56e459ea2 android.hardware.graphics.composer@2.1::IComposerCallback e992684e690dfe67a8cbeab5005bfa3fa9c2bf3d4b0b75657fb1f0c2d5dd2bae android.hardware.graphics.composer@2.1::IComposerClient 1c98c2f5154345312ec054871792a2982ec5f3e2bc2abfb61a10c0b517978e20 android.hardware.graphics.composer@2.1::types -04fbe91f2a860d463fa6287fbcfa05a8cc4f17848cabcd2aa9eef0f2aa1fedc0 android.hardware.graphics.mapper@2.0::IMapper +a695898589e1ef15b2b2510f11edd6aafac9918d9cf8d74b4b6143b309dee542 android.hardware.graphics.mapper@2.0::IMapper 28507d385a3dd224bf3c32f1bfd9f96092c4701b9c1cc66caa578fc3efc97877 android.hardware.graphics.mapper@2.0::types 91e2ba3805c923f01fc1231ec9ff838942aee3346f2d7614ecc0caeadbe57ed4 android.hardware.health@1.0::IHealth 1275aa2e8732909101b26aec49ed2285489e89d97b8610a8908b7868e35a3cc5 android.hardware.health@1.0::types diff --git a/graphics/mapper/2.0/IMapper.hal b/graphics/mapper/2.0/IMapper.hal index 246be240f2..4ee206bcdc 100644 --- a/graphics/mapper/2.0/IMapper.hal +++ b/graphics/mapper/2.0/IMapper.hal @@ -85,10 +85,11 @@ interface IMapper { * Imports a raw buffer handle to create an imported buffer handle for use * with the rest of the mapper or with other in-process libraries. * - * A buffer handle is considered raw when it is cloned or when it is - * received from another HAL or another process. A raw buffer handle must - * not be used to access the underlying graphics buffer. It must be - * imported to create an imported handle first. + * A buffer handle is considered raw when it is cloned (e.g., with + * native_handle_clone) from another buffer handle locally, or when it is + * received from another HAL server/client or another process. A raw + * buffer handle must not be used to access the underlying graphics + * buffer. It must be imported to create an imported handle first. * * This function must at least validate the raw handle before creating the * imported handle. It must also support importing the same raw handle @@ -96,6 +97,12 @@ interface IMapper { * must be considered valid everywhere in the process, including in * another instance of the mapper. * + * Because of passthrough HALs, a raw buffer handle received from a HAL + * may actually have been imported in the process. importBuffer must treat + * such a handle as if it is raw and must not return BAD_BUFFER. The + * returned handle is independent from the input handle as usual, and + * freeBuffer must be called on it when it is no longer needed. + * * @param rawHandle is the raw buffer handle to import. * @return error is NONE upon success. Otherwise, * BAD_BUFFER when the raw handle is invalid. diff --git a/graphics/mapper/2.0/default/GrallocMapper.cpp b/graphics/mapper/2.0/default/GrallocMapper.cpp index 6441af6396..d16143da49 100644 --- a/graphics/mapper/2.0/default/GrallocMapper.cpp +++ b/graphics/mapper/2.0/default/GrallocMapper.cpp @@ -125,11 +125,8 @@ Return GrallocMapper::createDescriptor( Return GrallocMapper::importBuffer(const hidl_handle& rawHandle, importBuffer_cb hidl_cb) { - // importing an already imported handle rather than a raw handle - if (gRegisteredHandles->get(rawHandle.getNativeHandle())) { - hidl_cb(Error::BAD_BUFFER, nullptr); - return Void(); - } + // because of passthrough HALs, we must not generate an error when + // rawHandle has been imported if (!rawHandle.getNativeHandle()) { hidl_cb(Error::BAD_BUFFER, nullptr); diff --git a/graphics/mapper/2.0/vts/functional/VtsHalGraphicsMapperV2_0TargetTest.cpp b/graphics/mapper/2.0/vts/functional/VtsHalGraphicsMapperV2_0TargetTest.cpp index f066a1e2c5..c74013b336 100644 --- a/graphics/mapper/2.0/vts/functional/VtsHalGraphicsMapperV2_0TargetTest.cpp +++ b/graphics/mapper/2.0/vts/functional/VtsHalGraphicsMapperV2_0TargetTest.cpp @@ -216,18 +216,6 @@ TEST_F(GraphicsMapperHidlTest, ImportBufferNegative) { << "importBuffer with invalid handle did not fail with BAD_BUFFER"; }); native_handle_delete(invalidHandle); - - const native_handle_t* importedHandle; - ASSERT_NO_FATAL_FAILURE(importedHandle = - mGralloc->allocate(mDummyDescriptorInfo, true)); - mGralloc->getMapper()->importBuffer( - importedHandle, [&](const auto& tmpError, const auto&) { - EXPECT_EQ(Error::BAD_BUFFER, tmpError) - << "importBuffer with an " - "already imported handle did " - "not fail with BAD_BUFFER"; - }); - mGralloc->freeBuffer(importedHandle); } /** From 2ae85702f7b51386c2daf021940aed0d10759bc0 Mon Sep 17 00:00:00 2001 From: Chia-I Wu Date: Thu, 20 Apr 2017 11:01:18 -0700 Subject: [PATCH 2/2] graphics: use mapper from the composer We must use the mapper HAL instead of gralloc0/gralloc1 from the composer. Bug: 37540361 Test: boots on marlin, angler, and ryu Change-Id: I5a3ff6a025bf51a3507a4f33fa77e9506a6f1ec9 --- graphics/composer/2.1/default/Android.bp | 5 +- .../composer/2.1/default/ComposerClient.cpp | 128 +++--------------- 2 files changed, 22 insertions(+), 111 deletions(-) diff --git a/graphics/composer/2.1/default/Android.bp b/graphics/composer/2.1/default/Android.bp index fb75bebecc..037f81089e 100644 --- a/graphics/composer/2.1/default/Android.bp +++ b/graphics/composer/2.1/default/Android.bp @@ -5,8 +5,8 @@ cc_library_static { export_include_dirs: ["."], srcs: ["ComposerClient.cpp"], shared_libs: [ - "android.hardware.graphics.allocator@2.0", "android.hardware.graphics.composer@2.1", + "android.hardware.graphics.mapper@2.0", "libbase", "libcutils", "libfmq", @@ -27,8 +27,8 @@ cc_library_shared { srcs: ["Hwc.cpp"], static_libs: ["libhwcomposer-client"], shared_libs: [ - "android.hardware.graphics.allocator@2.0", "android.hardware.graphics.composer@2.1", + "android.hardware.graphics.mapper@2.0", "libbase", "libcutils", "libfmq", @@ -51,7 +51,6 @@ cc_binary { init_rc: ["android.hardware.graphics.composer@2.1-service.rc"], static_libs: ["libhwcomposer-client"], shared_libs: [ - "android.hardware.graphics.allocator@2.0", "android.hardware.graphics.composer@2.1", "libbase", "libbinder", diff --git a/graphics/composer/2.1/default/ComposerClient.cpp b/graphics/composer/2.1/default/ComposerClient.cpp index d599b44ce9..87e4d3b7a7 100644 --- a/graphics/composer/2.1/default/ComposerClient.cpp +++ b/graphics/composer/2.1/default/ComposerClient.cpp @@ -16,8 +16,7 @@ #define LOG_TAG "HwcPassthrough" -#include -#include +#include #include #include "ComposerClient.h" @@ -33,10 +32,11 @@ namespace implementation { namespace { +using MapperError = android::hardware::graphics::mapper::V2_0::Error; +using android::hardware::graphics::mapper::V2_0::IMapper; + class HandleImporter { public: - HandleImporter() : mInitialized(false) {} - bool initialize() { // allow only one client @@ -44,9 +44,7 @@ public: return false; } - if (!openGralloc()) { - return false; - } + mMapper = IMapper::getService(); mInitialized = true; return true; @@ -54,11 +52,7 @@ public: void cleanup() { - if (!mInitialized) { - return; - } - - closeGralloc(); + mMapper.clear(); mInitialized = false; } @@ -76,12 +70,20 @@ public: return true; } - buffer_handle_t clone = cloneBuffer(handle); - if (!clone) { + MapperError error; + buffer_handle_t importedHandle; + mMapper->importBuffer( + hidl_handle(handle), + [&](const auto& tmpError, const auto& tmpBufferHandle) { + error = tmpError; + importedHandle = static_cast(tmpBufferHandle); + }); + if (error != MapperError::NONE) { return false; } - handle = clone; + handle = importedHandle; + return true; } @@ -91,102 +93,12 @@ public: return; } - releaseBuffer(handle); + mMapper->freeBuffer(const_cast(handle)); } private: - bool mInitialized; - - // Some existing gralloc drivers do not support retaining more than once, - // when we are in passthrough mode. - bool openGralloc() - { - const hw_module_t* module = nullptr; - int err = hw_get_module(GRALLOC_HARDWARE_MODULE_ID, &module); - if (err) { - ALOGE("failed to get gralloc module"); - return false; - } - - uint8_t major = (module->module_api_version >> 8) & 0xff; - if (major > 1) { - ALOGE("unknown gralloc module major version %d", major); - return false; - } - - if (major == 1) { - err = gralloc1_open(module, &mDevice); - if (err) { - ALOGE("failed to open gralloc1 device"); - return false; - } - - mRetain = reinterpret_cast( - mDevice->getFunction(mDevice, GRALLOC1_FUNCTION_RETAIN)); - mRelease = reinterpret_cast( - mDevice->getFunction(mDevice, GRALLOC1_FUNCTION_RELEASE)); - if (!mRetain || !mRelease) { - ALOGE("invalid gralloc1 device"); - gralloc1_close(mDevice); - return false; - } - } else { - mModule = reinterpret_cast(module); - } - - return true; - } - - void closeGralloc() - { - if (mDevice) { - gralloc1_close(mDevice); - } - } - - buffer_handle_t cloneBuffer(buffer_handle_t handle) - { - native_handle_t* clone = native_handle_clone(handle); - if (!clone) { - ALOGE("failed to clone buffer %p", handle); - return nullptr; - } - - bool err; - if (mDevice) { - err = (mRetain(mDevice, clone) != GRALLOC1_ERROR_NONE); - } else { - err = (mModule->registerBuffer(mModule, clone) != 0); - } - - if (err) { - ALOGE("failed to retain/register buffer %p", clone); - native_handle_close(clone); - native_handle_delete(clone); - return nullptr; - } - - return clone; - } - - void releaseBuffer(buffer_handle_t handle) - { - if (mDevice) { - mRelease(mDevice, handle); - } else { - mModule->unregisterBuffer(mModule, handle); - } - native_handle_close(handle); - native_handle_delete(const_cast(handle)); - } - - // gralloc1 - gralloc1_device_t* mDevice; - GRALLOC1_PFN_RETAIN mRetain; - GRALLOC1_PFN_RELEASE mRelease; - - // gralloc0 - const gralloc_module_t* mModule; + bool mInitialized = false; + sp mMapper; }; HandleImporter sHandleImporter;