SF: Clean up HWC2::Layer ownership
Changes the HWC2::Display::createLayer() call to return a shared_ptr
instead of a bare pointer. Also if the HWC2::Display is disconnected,
instead of directly destroying the HWC2::Layers associated with the
display, a new call is made to clear the display information from the
layer. For safety, checks are added to take an early out if the display
information isn't set for any call where it is used.
The CompositionEngine code was already creating a shared_ptr for the
createLayer call, and expecting the pointer to be valid until it
released it via destroyLayer.
However a hotplug disconnect could leave the CompositionEngine code
holding an invalid pointer until the next display refresh, and it could
lead to bad memory accesses if the pointer was dereferenced.
CompositionEngine itself did not do so -- it released its ownership of
the layer (and the associated display) without accessing them. This
seems to have only affected "dumpsys SurfaceFlinger", if it happened to
be executed after the disconnect, and before another refresh, and only
because it tried to print out the HWC layer id.
Bug: 181061001
Test: libsurfaceflinger_unittest
Test: libcompositionengine_test
Test: dumpsys SurfaceFlinger # after hotplug event
Change-Id: I508d6aa8ef7a6af848dd54198408d5f311175070
Merged-In: I508d6aa8ef7a6af848dd54198408d5f311175070
diff --git a/services/surfaceflinger/DisplayHardware/HWC2.cpp b/services/surfaceflinger/DisplayHardware/HWC2.cpp
index d04b5f7..27146ab 100644
--- a/services/surfaceflinger/DisplayHardware/HWC2.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWC2.cpp
@@ -78,7 +78,19 @@
}
Display::~Display() {
- mLayers.clear();
+ // Note: The calls to onOwningDisplayDestroyed() are allowed (and expected)
+ // to call Display::onLayerDestroyed(). As that call removes entries from
+ // mLayers, we do not want to have a for loop directly over it here. Since
+ // the end goal is an empty mLayers anyway, we just go ahead and swap an
+ // initially empty local container with mLayers, and then enumerate
+ // the contents of the local container.
+ Layers destroyingLayers;
+ std::swap(mLayers, destroyingLayers);
+ for (const auto& [_, weakLayer] : destroyingLayers) {
+ if (std::shared_ptr layer = weakLayer.lock()) {
+ layer->onOwningDisplayDestroyed();
+ }
+ }
Error error = Error::NONE;
const char* msg;
@@ -110,29 +122,21 @@
return static_cast<Error>(intError);
}
-Error Display::createLayer(HWC2::Layer** outLayer) {
- if (!outLayer) {
- return Error::BAD_PARAMETER;
- }
+base::expected<std::shared_ptr<HWC2::Layer>, hal::Error> Display::createLayer() {
HWLayerId layerId = 0;
auto intError = mComposer.createLayer(mId, &layerId);
auto error = static_cast<Error>(intError);
if (error != Error::NONE) {
- return error;
+ return base::unexpected(error);
}
- auto layer = std::make_unique<impl::Layer>(mComposer, mCapabilities, mId, layerId);
- *outLayer = layer.get();
- mLayers.emplace(layerId, std::move(layer));
- return Error::NONE;
+ auto layer = std::make_shared<impl::Layer>(mComposer, mCapabilities, *this, layerId);
+ mLayers.emplace(layerId, layer);
+ return layer;
}
-Error Display::destroyLayer(HWC2::Layer* layer) {
- if (!layer) {
- return Error::BAD_PARAMETER;
- }
- mLayers.erase(layer->getId());
- return Error::NONE;
+void Display::onLayerDestroyed(hal::HWLayerId layerId) {
+ mLayers.erase(layerId);
}
bool Display::isVsyncPeriodSwitchSupported() const {
@@ -161,7 +165,7 @@
auto type = types[element];
ALOGV("getChangedCompositionTypes: adding %" PRIu64 " %s",
layer->getId(), to_string(type).c_str());
- outTypes->emplace(layer, type);
+ outTypes->emplace(layer.get(), type);
} else {
ALOGE("getChangedCompositionTypes: invalid layer %" PRIu64 " found"
" on display %" PRIu64, layerIds[element], mId);
@@ -254,7 +258,7 @@
if (layer) {
auto layerRequest =
static_cast<LayerRequest>(layerRequests[element]);
- outLayerRequests->emplace(layer, layerRequest);
+ outLayerRequests->emplace(layer.get(), layerRequest);
} else {
ALOGE("getRequests: invalid layer %" PRIu64 " found on display %"
PRIu64, layerIds[element], mId);
@@ -340,7 +344,7 @@
auto layer = getLayerById(layerIds[element]);
if (layer) {
sp<Fence> fence(new Fence(fenceFds[element]));
- releaseFences.emplace(layer, fence);
+ releaseFences.emplace(layer.get(), fence);
} else {
ALOGE("getReleaseFences: invalid layer %" PRIu64
" found on display %" PRIu64, layerIds[element], mId);
@@ -550,12 +554,9 @@
// Other Display methods
-HWC2::Layer* Display::getLayerById(HWLayerId id) const {
- if (mLayers.count(id) == 0) {
- return nullptr;
- }
-
- return mLayers.at(id).get();
+std::shared_ptr<HWC2::Layer> Display::getLayerById(HWLayerId id) const {
+ auto it = mLayers.find(id);
+ return it != mLayers.end() ? it->second.lock() : nullptr;
}
} // namespace impl
@@ -566,47 +567,78 @@
namespace impl {
Layer::Layer(android::Hwc2::Composer& composer, const std::unordered_set<Capability>& capabilities,
- HWDisplayId displayId, HWLayerId layerId)
+ HWC2::Display& display, HWLayerId layerId)
: mComposer(composer),
mCapabilities(capabilities),
- mDisplayId(displayId),
+ mDisplay(&display),
mId(layerId),
mColorMatrix(android::mat4()) {
- ALOGV("Created layer %" PRIu64 " on display %" PRIu64, layerId, displayId);
+ ALOGV("Created layer %" PRIu64 " on display %" PRIu64, layerId, display.getId());
}
Layer::~Layer()
{
- auto intError = mComposer.destroyLayer(mDisplayId, mId);
+ onOwningDisplayDestroyed();
+}
+
+void Layer::onOwningDisplayDestroyed() {
+ // Note: onOwningDisplayDestroyed() may be called to perform cleanup by
+ // either the Layer dtor or by the Display dtor and must be safe to call
+ // from either path. In particular, the call to Display::onLayerDestroyed()
+ // is expected to be safe to do,
+
+ if (CC_UNLIKELY(!mDisplay)) {
+ return;
+ }
+
+ mDisplay->onLayerDestroyed(mId);
+
+ // Note: If the HWC display was actually disconnected, these calls are will
+ // return an error. We always make them as there may be other reasons for
+ // the HWC2::Display to be destroyed.
+ auto intError = mComposer.destroyLayer(mDisplay->getId(), mId);
auto error = static_cast<Error>(intError);
ALOGE_IF(error != Error::NONE,
"destroyLayer(%" PRIu64 ", %" PRIu64 ")"
" failed: %s (%d)",
- mDisplayId, mId, to_string(error).c_str(), intError);
+ mDisplay->getId(), mId, to_string(error).c_str(), intError);
+
+ mDisplay = nullptr;
}
Error Layer::setCursorPosition(int32_t x, int32_t y)
{
- auto intError = mComposer.setCursorPosition(mDisplayId, mId, x, y);
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
+ auto intError = mComposer.setCursorPosition(mDisplay->getId(), mId, x, y);
return static_cast<Error>(intError);
}
Error Layer::setBuffer(uint32_t slot, const sp<GraphicBuffer>& buffer,
const sp<Fence>& acquireFence)
{
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
if (buffer == nullptr && mBufferSlot == slot) {
return Error::NONE;
}
mBufferSlot = slot;
int32_t fenceFd = acquireFence->dup();
- auto intError = mComposer.setLayerBuffer(mDisplayId, mId, slot, buffer,
- fenceFd);
+ auto intError = mComposer.setLayerBuffer(mDisplay->getId(), mId, slot, buffer, fenceFd);
return static_cast<Error>(intError);
}
Error Layer::setSurfaceDamage(const Region& damage)
{
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
if (damage.isRect() && mDamageRegion.isRect() &&
(damage.getBounds() == mDamageRegion.getBounds())) {
return Error::NONE;
@@ -617,8 +649,8 @@
// rects for HWC
Hwc2::Error intError = Hwc2::Error::NONE;
if (damage.isRect() && damage.getBounds() == Rect::INVALID_RECT) {
- intError = mComposer.setLayerSurfaceDamage(mDisplayId,
- mId, std::vector<Hwc2::IComposerClient::Rect>());
+ intError = mComposer.setLayerSurfaceDamage(mDisplay->getId(), mId,
+ std::vector<Hwc2::IComposerClient::Rect>());
} else {
size_t rectCount = 0;
auto rectArray = damage.getArray(&rectCount);
@@ -629,7 +661,7 @@
rectArray[rect].right, rectArray[rect].bottom});
}
- intError = mComposer.setLayerSurfaceDamage(mDisplayId, mId, hwcRects);
+ intError = mComposer.setLayerSurfaceDamage(mDisplay->getId(), mId, hwcRects);
}
return static_cast<Error>(intError);
@@ -637,34 +669,54 @@
Error Layer::setBlendMode(BlendMode mode)
{
- auto intError = mComposer.setLayerBlendMode(mDisplayId, mId, mode);
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
+ auto intError = mComposer.setLayerBlendMode(mDisplay->getId(), mId, mode);
return static_cast<Error>(intError);
}
Error Layer::setColor(Color color) {
- auto intError = mComposer.setLayerColor(mDisplayId, mId, color);
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
+ auto intError = mComposer.setLayerColor(mDisplay->getId(), mId, color);
return static_cast<Error>(intError);
}
Error Layer::setCompositionType(Composition type)
{
- auto intError = mComposer.setLayerCompositionType(mDisplayId, mId, type);
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
+ auto intError = mComposer.setLayerCompositionType(mDisplay->getId(), mId, type);
return static_cast<Error>(intError);
}
Error Layer::setDataspace(Dataspace dataspace)
{
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
if (dataspace == mDataSpace) {
return Error::NONE;
}
mDataSpace = dataspace;
- auto intError = mComposer.setLayerDataspace(mDisplayId, mId, mDataSpace);
+ auto intError = mComposer.setLayerDataspace(mDisplay->getId(), mId, mDataSpace);
return static_cast<Error>(intError);
}
Error Layer::setPerFrameMetadata(const int32_t supportedPerFrameMetadata,
const android::HdrMetadata& metadata)
{
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
if (metadata == mHdrMetadata) {
return Error::NONE;
}
@@ -705,7 +757,7 @@
}
Error error = static_cast<Error>(
- mComposer.setLayerPerFrameMetadata(mDisplayId, mId, perFrameMetadatas));
+ mComposer.setLayerPerFrameMetadata(mDisplay->getId(), mId, perFrameMetadatas));
if (validTypes & HdrMetadata::HDR10PLUS) {
if (CC_UNLIKELY(mHdrMetadata.hdr10plus.size() == 0)) {
@@ -715,8 +767,9 @@
std::vector<Hwc2::PerFrameMetadataBlob> perFrameMetadataBlobs;
perFrameMetadataBlobs.push_back(
{Hwc2::PerFrameMetadataKey::HDR10_PLUS_SEI, mHdrMetadata.hdr10plus});
- Error setMetadataBlobsError = static_cast<Error>(
- mComposer.setLayerPerFrameMetadataBlobs(mDisplayId, mId, perFrameMetadataBlobs));
+ Error setMetadataBlobsError =
+ static_cast<Error>(mComposer.setLayerPerFrameMetadataBlobs(mDisplay->getId(), mId,
+ perFrameMetadataBlobs));
if (error == Error::NONE) {
return setMetadataBlobsError;
}
@@ -726,46 +779,70 @@
Error Layer::setDisplayFrame(const Rect& frame)
{
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
Hwc2::IComposerClient::Rect hwcRect{frame.left, frame.top,
frame.right, frame.bottom};
- auto intError = mComposer.setLayerDisplayFrame(mDisplayId, mId, hwcRect);
+ auto intError = mComposer.setLayerDisplayFrame(mDisplay->getId(), mId, hwcRect);
return static_cast<Error>(intError);
}
Error Layer::setPlaneAlpha(float alpha)
{
- auto intError = mComposer.setLayerPlaneAlpha(mDisplayId, mId, alpha);
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
+ auto intError = mComposer.setLayerPlaneAlpha(mDisplay->getId(), mId, alpha);
return static_cast<Error>(intError);
}
Error Layer::setSidebandStream(const native_handle_t* stream)
{
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
if (mCapabilities.count(Capability::SIDEBAND_STREAM) == 0) {
ALOGE("Attempted to call setSidebandStream without checking that the "
"device supports sideband streams");
return Error::UNSUPPORTED;
}
- auto intError = mComposer.setLayerSidebandStream(mDisplayId, mId, stream);
+ auto intError = mComposer.setLayerSidebandStream(mDisplay->getId(), mId, stream);
return static_cast<Error>(intError);
}
Error Layer::setSourceCrop(const FloatRect& crop)
{
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
Hwc2::IComposerClient::FRect hwcRect{
crop.left, crop.top, crop.right, crop.bottom};
- auto intError = mComposer.setLayerSourceCrop(mDisplayId, mId, hwcRect);
+ auto intError = mComposer.setLayerSourceCrop(mDisplay->getId(), mId, hwcRect);
return static_cast<Error>(intError);
}
Error Layer::setTransform(Transform transform)
{
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
auto intTransform = static_cast<Hwc2::Transform>(transform);
- auto intError = mComposer.setLayerTransform(mDisplayId, mId, intTransform);
+ auto intError = mComposer.setLayerTransform(mDisplay->getId(), mId, intTransform);
return static_cast<Error>(intError);
}
Error Layer::setVisibleRegion(const Region& region)
{
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
if (region.isRect() && mVisibleRegion.isRect() &&
(region.getBounds() == mVisibleRegion.getBounds())) {
return Error::NONE;
@@ -781,22 +858,30 @@
rectArray[rect].right, rectArray[rect].bottom});
}
- auto intError = mComposer.setLayerVisibleRegion(mDisplayId, mId, hwcRects);
+ auto intError = mComposer.setLayerVisibleRegion(mDisplay->getId(), mId, hwcRects);
return static_cast<Error>(intError);
}
Error Layer::setZOrder(uint32_t z)
{
- auto intError = mComposer.setLayerZOrder(mDisplayId, mId, z);
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
+ auto intError = mComposer.setLayerZOrder(mDisplay->getId(), mId, z);
return static_cast<Error>(intError);
}
// Composer HAL 2.3
Error Layer::setColorTransform(const android::mat4& matrix) {
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
if (matrix == mColorMatrix) {
return Error::NONE;
}
- auto intError = mComposer.setLayerColorTransform(mDisplayId, mId, matrix.asArray());
+ auto intError = mComposer.setLayerColorTransform(mDisplay->getId(), mId, matrix.asArray());
Error error = static_cast<Error>(intError);
if (error != Error::NONE) {
return error;
@@ -808,7 +893,12 @@
// Composer HAL 2.4
Error Layer::setLayerGenericMetadata(const std::string& name, bool mandatory,
const std::vector<uint8_t>& value) {
- auto intError = mComposer.setLayerGenericMetadata(mDisplayId, mId, name, mandatory, value);
+ if (CC_UNLIKELY(!mDisplay)) {
+ return Error::BAD_DISPLAY;
+ }
+
+ auto intError =
+ mComposer.setLayerGenericMetadata(mDisplay->getId(), mId, name, mandatory, value);
return static_cast<Error>(intError);
}