From ac6cc1f97ee70717dacccf28faae8d7a9196b48e Mon Sep 17 00:00:00 2001 From: Marin Shalamanov Date: Fri, 23 Oct 2020 17:42:59 +0200 Subject: [PATCH] Clean ComposerClient cache on hotplug On subsequent hotplug connected event for a display SurfaceFlinger destroys the previous framebuffers and recreates them. When the new buffers are created ComposerClient still holds a handle to the old buffers and they are not destroyed. This way the new framebuffers may get allocated on non continuous memory causing garbled screens for the user. Bug: 160112047 Bug: 169255692 Test: 1. limit cma ion memory to 32 MB 2. flash device 3. plug hdmi out and in 4. verify that the display image is not garbled Change-Id: Idf7cdf7a070ffc83ecec34ac24c8a7d696f68aa6 --- .../include/composer-hal/2.1/ComposerClient.h | 88 ++++++++++++++++--- .../2.1/utils/resources/ComposerResources.cpp | 36 ++++++++ .../2.1/ComposerResources.h | 7 +- 3 files changed, 116 insertions(+), 15 deletions(-) diff --git a/graphics/composer/2.1/utils/hal/include/composer-hal/2.1/ComposerClient.h b/graphics/composer/2.1/utils/hal/include/composer-hal/2.1/ComposerClient.h index 47ead41fb0..3d74af4a41 100644 --- a/graphics/composer/2.1/utils/hal/include/composer-hal/2.1/ComposerClient.h +++ b/graphics/composer/2.1/utils/hal/include/composer-hal/2.1/ComposerClient.h @@ -87,19 +87,28 @@ class ComposerClientImpl : public Interface { class HalEventCallback : public Hal::EventCallback { public: - HalEventCallback(const sp callback, ComposerResources* resources) - : mCallback(callback), mResources(resources) {} + HalEventCallback(Hal* hal, const sp callback, + ComposerResources* resources) + : mHal(hal), mCallback(callback), mResources(resources) {} - void onHotplug(Display display, IComposerCallback::Connection connected) { - if (connected == IComposerCallback::Connection::CONNECTED) { - mResources->addPhysicalDisplay(display); - } else if (connected == IComposerCallback::Connection::DISCONNECTED) { - mResources->removeDisplay(display); - } + void onHotplug(Display display, IComposerCallback::Connection connected) { + if (connected == IComposerCallback::Connection::CONNECTED) { + if (mResources->hasDisplay(display)) { + // This is a subsequent hotplug "connected" for a display. This signals a + // display change and thus the framework may want to reallocate buffers. We + // need to free all cached handles, since they are holding a strong reference + // to the underlying buffers. + cleanDisplayResources(display); + mResources->removeDisplay(display); + } + mResources->addPhysicalDisplay(display); + } else if (connected == IComposerCallback::Connection::DISCONNECTED) { + mResources->removeDisplay(display); + } - auto ret = mCallback->onHotplug(display, connected); - ALOGE_IF(!ret.isOk(), "failed to send onHotplug: %s", ret.description().c_str()); - } + auto ret = mCallback->onHotplug(display, connected); + ALOGE_IF(!ret.isOk(), "failed to send onHotplug: %s", ret.description().c_str()); + } void onRefresh(Display display) { mResources->setDisplayMustValidateState(display, true); @@ -113,13 +122,64 @@ class ComposerClientImpl : public Interface { } protected: - const sp mCallback; - ComposerResources* const mResources; + Hal* const mHal; + const sp mCallback; + ComposerResources* const mResources; + + void cleanDisplayResources(Display display) { + size_t cacheSize; + Error err = mResources->getDisplayClientTargetCacheSize(display, &cacheSize); + if (err == Error::NONE) { + for (int slot = 0; slot < cacheSize; slot++) { + ComposerResources::ReplacedHandle replacedBuffer(/*isBuffer*/ true); + // Replace the buffer slots with NULLs. Keep the old handle until it is + // replaced in ComposerHal, otherwise we risk leaving a dangling pointer. + const native_handle_t* clientTarget = nullptr; + err = mResources->getDisplayClientTarget(display, slot, /*useCache*/ true, + /*rawHandle*/ nullptr, &clientTarget, + &replacedBuffer); + if (err != Error::NONE) { + continue; + } + const std::vector damage; + err = mHal->setClientTarget(display, clientTarget, /*fence*/ -1, 0, damage); + ALOGE_IF(err != Error::NONE, + "Can't clean slot %d of the client target buffer" + "cache for display %" PRIu64, + slot, display); + } + } else { + ALOGE("Can't clean client target cache for display %" PRIu64, display); + } + + err = mResources->getDisplayOutputBufferCacheSize(display, &cacheSize); + if (err == Error::NONE) { + for (int slot = 0; slot < cacheSize; slot++) { + // Replace the buffer slots with NULLs. Keep the old handle until it is + // replaced in ComposerHal, otherwise we risk leaving a dangling pointer. + ComposerResources::ReplacedHandle replacedBuffer(/*isBuffer*/ true); + const native_handle_t* outputBuffer = nullptr; + err = mResources->getDisplayOutputBuffer(display, slot, /*useCache*/ true, + /*rawHandle*/ nullptr, &outputBuffer, + &replacedBuffer); + if (err != Error::NONE) { + continue; + } + err = mHal->setOutputBuffer(display, outputBuffer, /*fence*/ -1); + ALOGE_IF(err != Error::NONE, + "Can't clean slot %d of the output buffer cache" + "for display %" PRIu64, + slot, display); + } + } else { + ALOGE("Can't clean output buffer cache for display %" PRIu64, display); + } + } }; Return registerCallback(const sp& callback) override { // no locking as we require this function to be called only once - mHalEventCallback = std::make_unique(callback, mResources.get()); + mHalEventCallback = std::make_unique(mHal, callback, mResources.get()); mHal->registerEventCallback(mHalEventCallback.get()); return Void(); } diff --git a/graphics/composer/2.1/utils/resources/ComposerResources.cpp b/graphics/composer/2.1/utils/resources/ComposerResources.cpp index 21f60355f8..e52bf7124a 100644 --- a/graphics/composer/2.1/utils/resources/ComposerResources.cpp +++ b/graphics/composer/2.1/utils/resources/ComposerResources.cpp @@ -144,6 +144,10 @@ ComposerHandleCache::~ComposerHandleCache() { } } +size_t ComposerHandleCache::getCacheSize() const { + return mHandles.size(); +} + bool ComposerHandleCache::initCache(HandleType type, uint32_t cacheSize) { // already initialized if (mHandleType != HandleType::INVALID) { @@ -220,6 +224,14 @@ bool ComposerDisplayResource::initClientTargetCache(uint32_t cacheSize) { return mClientTargetCache.initCache(ComposerHandleCache::HandleType::BUFFER, cacheSize); } +size_t ComposerDisplayResource::getClientTargetCacheSize() const { + return mClientTargetCache.getCacheSize(); +} + +size_t ComposerDisplayResource::getOutputBufferCacheSize() const { + return mOutputBufferCache.getCacheSize(); +} + bool ComposerDisplayResource::isVirtual() const { return mType == DisplayType::VIRTUAL; } @@ -293,6 +305,10 @@ void ComposerResources::clear(RemoveDisplay removeDisplay) { mDisplayResources.clear(); } +bool ComposerResources::hasDisplay(Display display) { + return mDisplayResources.count(display) > 0; +} + Error ComposerResources::addPhysicalDisplay(Display display) { auto displayResource = createDisplayResource(ComposerDisplayResource::DisplayType::PHYSICAL, 0); @@ -327,6 +343,26 @@ Error ComposerResources::setDisplayClientTargetCacheSize(Display display, : Error::BAD_PARAMETER; } +Error ComposerResources::getDisplayClientTargetCacheSize(Display display, size_t* outCacheSize) { + std::lock_guard lock(mDisplayResourcesMutex); + ComposerDisplayResource* displayResource = findDisplayResourceLocked(display); + if (!displayResource) { + return Error::BAD_DISPLAY; + } + *outCacheSize = displayResource->getClientTargetCacheSize(); + return Error::NONE; +} + +Error ComposerResources::getDisplayOutputBufferCacheSize(Display display, size_t* outCacheSize) { + std::lock_guard lock(mDisplayResourcesMutex); + ComposerDisplayResource* displayResource = findDisplayResourceLocked(display); + if (!displayResource) { + return Error::BAD_DISPLAY; + } + *outCacheSize = displayResource->getOutputBufferCacheSize(); + return Error::NONE; +} + Error ComposerResources::addLayer(Display display, Layer layer, uint32_t bufferCacheSize) { auto layerResource = createLayerResource(bufferCacheSize); diff --git a/graphics/composer/2.1/utils/resources/include/composer-resources/2.1/ComposerResources.h b/graphics/composer/2.1/utils/resources/include/composer-resources/2.1/ComposerResources.h index df5513ea0a..de78a59e91 100644 --- a/graphics/composer/2.1/utils/resources/include/composer-resources/2.1/ComposerResources.h +++ b/graphics/composer/2.1/utils/resources/include/composer-resources/2.1/ComposerResources.h @@ -74,6 +74,7 @@ class ComposerHandleCache { ComposerHandleCache& operator=(const ComposerHandleCache&) = delete; bool initCache(HandleType type, uint32_t cacheSize); + size_t getCacheSize() const; Error lookupCache(uint32_t slot, const native_handle_t** outHandle); Error updateCache(uint32_t slot, const native_handle_t* handle, const native_handle** outReplacedHandle); @@ -120,7 +121,8 @@ class ComposerDisplayResource { uint32_t outputBufferCacheSize); bool initClientTargetCache(uint32_t cacheSize); - + size_t getClientTargetCacheSize() const; + size_t getOutputBufferCacheSize() const; bool isVirtual() const; Error getClientTarget(uint32_t slot, bool fromCache, const native_handle_t* inHandle, @@ -162,12 +164,15 @@ class ComposerResources { std::function& layers)>; void clear(RemoveDisplay removeDisplay); + bool hasDisplay(Display display); Error addPhysicalDisplay(Display display); Error addVirtualDisplay(Display display, uint32_t outputBufferCacheSize); Error removeDisplay(Display display); Error setDisplayClientTargetCacheSize(Display display, uint32_t clientTargetCacheSize); + Error getDisplayClientTargetCacheSize(Display display, size_t* outCacheSize); + Error getDisplayOutputBufferCacheSize(Display display, size_t* outCacheSize); Error addLayer(Display display, Layer layer, uint32_t bufferCacheSize); Error removeLayer(Display display, Layer layer);