Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 1b8e52d5 authored by Gil Dekel's avatar Gil Dekel
Browse files

SF: Virtual display ID conflict resolution 

Currently, there are no safeguards against duplicate display IDs when
creating displays of any kind (physical or virtual). In rare cases where
duplicates occur, they cause critical failures in display management.

This patch introduces:
1. Logic around virtual display creation to keep generating virtual
   display IDs until there is no collision with existing displays.
2. Avoidance logic and checks against display ID duplicates in
   SurfaceFlinger around creation of VirtualDisplaySnapshot.
3. Robust testing of SurfaceFlinger::acquireVirtualDisplay.

Flag: com.android.graphics.surfaceflinger.flags.stable_edid_ids
Bug: 421463837
Test: DisplayTransactionCommitTest#acquire*VirtualDisplayId*
Change-Id: Ib666e4ddc5ce10cd7ee624ccd10c321d5c88d932
parent afa7afa4

libs/ui/include/ui/DisplayId.h

+2 −1
Original line number Diff line number Diff line
@@ -174,7 +174,8 @@ inline auto asPhysicalDisplayId(DisplayIdVariant variant) -> ftl::Optional<Physi 
    return asDisplayIdOfType<PhysicalDisplayId>(variant);

}



inline auto asVirtualDisplayId(DisplayIdVariant variant) -> ftl::Optional<VirtualDisplayId> {

template <typename Variant>

inline auto asVirtualDisplayId(Variant variant) -> ftl::Optional<VirtualDisplayId> {

    return ftl::match(

            variant,

            [](GpuVirtualDisplayId id) -> ftl::Optional<VirtualDisplayId> {

services/surfaceflinger/DisplayIdGenerator.h

+2 −1
Original line number Diff line number Diff line
@@ -33,10 +33,11 @@ class DisplayIdGenerator { 
public:

    explicit DisplayIdGenerator(size_t maxIdsCount = std::numeric_limits<size_t>::max())

          : mMaxIdsCount(maxIdsCount) {}

    virtual ~DisplayIdGenerator() = default;



    bool inUse() const { return !mUsedIds.empty(); }



    std::optional<Id> generateId() {

    virtual std::optional<Id> generateId() {

        if (mUsedIds.size() >= mMaxIdsCount) {

            return std::nullopt;

        }

services/surfaceflinger/SurfaceFlinger.cpp

+20 −17
Original line number Diff line number Diff line
@@ -657,7 +657,8 @@ void SurfaceFlinger::enableHalVirtualDisplays(bool enable) { 
    auto& generator = mVirtualDisplayIdGenerators.hal;

    if (!generator && enable) {

        ALOGI("Enabling HAL virtual displays");

        generator.emplace(getHwComposer().getMaxVirtualDisplayCount());

        generator = std::make_unique<DisplayIdGenerator<HalVirtualDisplayId>>(

                getHwComposer().getMaxVirtualDisplayCount());

    } else if (generator && !enable) {

        ALOGW_IF(generator->inUse(), "Disabling HAL virtual displays while in use");

        generator.reset();
@@ -668,26 +669,28 @@ std::optional<VirtualDisplayIdVariant> SurfaceFlinger::acquireVirtualDisplay( 
        ui::Size resolution, ui::PixelFormat format, const std::string& uniqueId,

        compositionengine::DisplayCreationArgsBuilder& builder) {

    if (auto& generator = mVirtualDisplayIdGenerators.hal) {

        if (const auto id = generator->generateId()) {

            if (getHwComposer().allocateVirtualDisplay(*id, resolution, &format)) {

                acquireVirtualDisplaySnapshot(*id, uniqueId);

                builder.setId(*id);

                return *id;

        if (const auto halIdOpt = generateVirtualDisplayId(*generator)) {

            if (getHwComposer().allocateVirtualDisplay(*halIdOpt, resolution, &format) &&

                acquireVirtualDisplaySnapshot(*halIdOpt, uniqueId)) {

                builder.setId(*halIdOpt);

                return *halIdOpt;

            }



            generator->releaseId(*id);

        } else {

            ALOGW("%s: Exhausted HAL virtual displays", __func__);

            generator->releaseId(*halIdOpt);

        }



        ALOGW("%s: Falling back to GPU virtual display", __func__);

    }



    const auto id = mVirtualDisplayIdGenerators.gpu.generateId();

    LOG_ALWAYS_FATAL_IF(!id, "Failed to generate ID for GPU virtual display");

    acquireVirtualDisplaySnapshot(*id, uniqueId);

    builder.setId(*id);

    return *id;

    auto& generator = mVirtualDisplayIdGenerators.gpu;

    if (const auto gpuIdOpt = generateVirtualDisplayId(*generator)) {

        if (acquireVirtualDisplaySnapshot(*gpuIdOpt, uniqueId)) {

            builder.setId(*gpuIdOpt);

            return *gpuIdOpt;

        }

    }



    ALOGE("Failed to generate ID for virtual display %s", uniqueId.c_str());

    return std::nullopt;

}



void SurfaceFlinger::releaseVirtualDisplay(VirtualDisplayIdVariant displayId) {
@@ -700,7 +703,7 @@ void SurfaceFlinger::releaseVirtualDisplay(VirtualDisplayIdVariant displayId) { 
                }

            },

            [this](GpuVirtualDisplayId gpuVirtualDisplayId) {

                mVirtualDisplayIdGenerators.gpu.releaseId(gpuVirtualDisplayId);

                mVirtualDisplayIdGenerators.gpu->releaseId(gpuVirtualDisplayId);

                releaseVirtualDisplaySnapshot(gpuVirtualDisplayId);

            });

}
@@ -4053,6 +4056,7 @@ void SurfaceFlinger::processDisplayAdded(const wp<IBinder>& displayToken, 
    } else {

        virtualDisplayIdVariantOpt =

                acquireVirtualDisplay(resolution, pixelFormat, state.uniqueId, builder);

        LOG_ALWAYS_FATAL_IF(!virtualDisplayIdVariantOpt);

    }



    builder.setPixels(resolution);
@@ -4072,7 +4076,6 @@ void SurfaceFlinger::processDisplayAdded(const wp<IBinder>& displayToken, 
    getFactory().createBufferQueue(&bqProducer, &bqConsumer, /*consumerIsSurfaceFlinger =*/false);



    if (state.isVirtual()) {

        LOG_FATAL_IF(!virtualDisplayIdVariantOpt);

        if (FlagManager::getInstance().wb_virtualdisplay2()) {

            auto surface =

                    sp<VirtualDisplaySurface2>::make(getHwComposer(), *virtualDisplayIdVariantOpt,

services/surfaceflinger/SurfaceFlinger.h

+35 −7
Original line number Diff line number Diff line
@@ -1207,13 +1207,40 @@ private: 
            compositionengine::DisplayCreationArgsBuilder&) REQUIRES(mStateLock);



    template <typename ID>

    void acquireVirtualDisplaySnapshot(ID displayId, const std::string& uniqueId) {

    bool acquireVirtualDisplaySnapshot(ID displayId, const std::string& uniqueId) {

        std::lock_guard lock(mVirtualDisplaysMutex);

        const bool emplace_success =

                mVirtualDisplays.try_emplace(displayId, displayId, uniqueId).second;

        if (!emplace_success) {

            ALOGW("%s: Virtual display snapshot with the same ID already exists", __func__);

        if (!mVirtualDisplays.try_emplace(displayId, displayId, uniqueId).second) {

            ALOGE("%s: Virtual display snapshot with the same ID already exists", __func__);

            return false;

        }



        return true;

    }



    template <typename ID>

    std::optional<ID> generateVirtualDisplayId(DisplayIdGenerator<ID>& generator)

            REQUIRES(mStateLock) {

        std::optional<ID> id;

        if (FlagManager::getInstance().stable_edid_ids()) {

            int attempts = 10;

            do {

                id = generator.generateId();

            } while (id.has_value() && hasDisplayWithId(*id) && --attempts > 0);

        } else {

            id = generator.generateId();

        }



        if (!id.has_value()) {

            ALOGW("%s: Exhausted virtual displays", __func__);

            return std::nullopt;

        }



        if (FlagManager::getInstance().stable_edid_ids() && hasDisplayWithId(*id)) {

            ALOGW("%s: Could not resolve virtual display ID conflicts", __func__);

            return std::nullopt;

        }



        return *id;

    }



    void releaseVirtualDisplay(VirtualDisplayIdVariant displayId);
@@ -1440,8 +1467,9 @@ private: 
    display::DisplayModeController mDisplayModeController;



    struct {

        DisplayIdGenerator<GpuVirtualDisplayId> gpu;

        std::optional<DisplayIdGenerator<HalVirtualDisplayId>> hal;

        std::unique_ptr<DisplayIdGenerator<GpuVirtualDisplayId>> gpu =

                std::make_unique<DisplayIdGenerator<GpuVirtualDisplayId>>();

        std::unique_ptr<DisplayIdGenerator<HalVirtualDisplayId>> hal;

    } mVirtualDisplayIdGenerators;



    std::atomic_uint mDebugFlashDelay = 0;

services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayTransactionCommitTest.cpp

+256 −0
Original line number Diff line number Diff line
@@ -17,6 +17,7 @@ 
#undef LOG_TAG

#define LOG_TAG "LibSurfaceFlingerUnittests"



#include <common/test/FlagUtils.h>

#include <ui/ScreenPartStatus.h>



#include "DisplayTransactionTestHelpers.h"
@@ -24,6 +25,12 @@ 
namespace android {

namespace {



template <typename Id>

class MockDisplayIdGenerator : public DisplayIdGenerator<Id> {

public:

    MOCK_METHOD(std::optional<Id>, generateId, (), (override));

};



struct DisplayTransactionCommitTest : DisplayTransactionTest {

    template <typename Case>

    void setupCommonPreconditions();
@@ -565,6 +572,255 @@ TEST_F(DisplayTransactionCommitTest, processesVirtualDisplayRemoval) { 
    verifyDisplayIsNotConnected(existing.token());

}



TEST_F(DisplayTransactionCommitTest, acquireHalVirtualDisplayId) {

    using Case = SimplePrimaryDisplayCase;



    // --------------------------------------------------------------------

    // Preconditions



    // Set up a primary physical display.

    processesHotplugConnectCommon<Case>();

    const uint64_t primaryDisplayId = asDisplayId(Case::Display::DISPLAY_ID::get()).value;



    // The HWC supports at least one virtual display

    injectMockComposer(1);



    // --------------------------------------------------------------------

    // Call Expectations

    static constexpr ui::Size kResolution{1920U, 1080U};

    static ui::PixelFormat format = static_cast<ui::PixelFormat>(PIXEL_FORMAT_RGBA_8888);

    EXPECT_CALL(*mComposer,

                createVirtualDisplay(static_cast<uint32_t>(kResolution.width),

                                     static_cast<uint32_t>(kResolution.height),

                                     testing::Pointee(format), _))

            .Times(1)

            .WillOnce(Return(Error::NONE));



    // --------------------------------------------------------------------

    // Invocation

    const std::string name("virtual.test");

    auto builder = compositionengine::DisplayCreationArgsBuilder();

    auto virtualDisplayIdVariantOpt =

            mFlinger.acquireVirtualDisplay(kResolution, format, name, builder);



    ASSERT_TRUE(virtualDisplayIdVariantOpt);

    ASSERT_TRUE(std::holds_alternative<HalVirtualDisplayId>(*virtualDisplayIdVariantOpt));

    ASSERT_NE(primaryDisplayId, asVirtualDisplayId(*virtualDisplayIdVariantOpt)->value);

}



TEST_F(DisplayTransactionCommitTest, acquireGpuVirtualDisplayId) {

    using Case = SimplePrimaryDisplayCase;



    // --------------------------------------------------------------------

    // Preconditions



    // Set up a primary physical display.

    processesHotplugConnectCommon<Case>();

    const uint64_t primaryDisplayId = asDisplayId(Case::Display::DISPLAY_ID::get()).value;



    // --------------------------------------------------------------------

    // Call Expectations



    // The HAL should not be involved in the creation of GPU virtual displays.

    EXPECT_CALL(*mComposer, createVirtualDisplay(_, _, _, _)).Times(0);



    // --------------------------------------------------------------------

    // Invocation

    constexpr ui::Size kResolution{1920U, 1080U};

    ui::PixelFormat format = static_cast<ui::PixelFormat>(PIXEL_FORMAT_RGBA_8888);

    const std::string name("virtual.test");

    auto builder = compositionengine::DisplayCreationArgsBuilder();

    auto virtualDisplayIdVariantOpt =

            mFlinger.acquireVirtualDisplay(kResolution, format, name, builder);



    ASSERT_TRUE(virtualDisplayIdVariantOpt);

    ASSERT_TRUE(std::holds_alternative<GpuVirtualDisplayId>(*virtualDisplayIdVariantOpt));

    ASSERT_NE(primaryDisplayId, asVirtualDisplayId(*virtualDisplayIdVariantOpt)->value);

}



TEST_F(DisplayTransactionCommitTest, acquireGpuVirtualDisplayIdFailure) {

    // --------------------------------------------------------------------

    // Preconditions



    // Setting the HAL generator to nullptr disables HWC composition for virtual displays.

    auto gpuDisplayIdGenerator = std::make_unique<MockDisplayIdGenerator<GpuVirtualDisplayId>>();

    auto* mockGpuDisplayIdGeneratorPtr = gpuDisplayIdGenerator.get();

    mFlinger.injectDisplayIdGenerators(std::move(gpuDisplayIdGenerator), nullptr);



    // --------------------------------------------------------------------

    // Call Expectations



    // The GPU display ID generator will fail to provide a valid virtual display ID.

    EXPECT_CALL(*mockGpuDisplayIdGeneratorPtr, generateId())

            .WillOnce(testing::Return(std::nullopt));

    // The HAL should not be involved in the creation of GPU virtual displays.

    EXPECT_CALL(*mComposer, createVirtualDisplay(_, _, _, _)).Times(0);



    // --------------------------------------------------------------------

    // Invocation

    constexpr ui::Size kResolution{1920U, 1080U};

    const ui::PixelFormat format = static_cast<ui::PixelFormat>(PIXEL_FORMAT_RGBA_8888);

    const std::string name("virtual.test");

    auto builder = compositionengine::DisplayCreationArgsBuilder();

    auto virtualDisplayIdVariantOpt =

            mFlinger.acquireVirtualDisplay(kResolution, format, name, builder);



    ASSERT_FALSE(virtualDisplayIdVariantOpt);

}



TEST_F(DisplayTransactionCommitTest, acquireHalVirtualDisplayIdWithConflictResolutionSuccess) {

    SET_FLAG_FOR_TEST(com::android::graphics::surfaceflinger::flags::stable_edid_ids, true);

    using Case = SimplePrimaryDisplayCase;



    // --------------------------------------------------------------------

    // Preconditions



    // Set up a primary physical display.

    processesHotplugConnectCommon<Case>();

    const uint64_t primaryDisplayId = asDisplayId(Case::Display::DISPLAY_ID::get()).value;



    // Injecting a mock Hal generator enables Hal composition.

    auto halDisplayIdGenerator = std::make_unique<MockDisplayIdGenerator<HalVirtualDisplayId>>();

    auto* mockHalDisplayIdGeneratorPtr = halDisplayIdGenerator.get();

    auto gpuDisplayIdGenerator = std::make_unique<MockDisplayIdGenerator<GpuVirtualDisplayId>>();

    auto* mockGpuDisplayIdGeneratorPtr = gpuDisplayIdGenerator.get();

    mFlinger.injectDisplayIdGenerators(std::move(gpuDisplayIdGenerator),

                                       std::move(halDisplayIdGenerator));



    // --------------------------------------------------------------------

    // Call Expectations



    // The HAL display ID generator will return the primary physical display's ID once

    // to produce conflict in the virtual display acquisition logic, then return a non-conflicting

    // ID.

    const uint64_t nonConflictingVirtualId =

            asDisplayId(HwcVirtualDisplayCase::Display::DISPLAY_ID::get()).value;

    EXPECT_CALL(*mockHalDisplayIdGeneratorPtr, generateId())

            .WillOnce(testing::Return(HalVirtualDisplayId::fromValue(primaryDisplayId)))

            .WillOnce(testing::Return(HalVirtualDisplayId::fromValue(nonConflictingVirtualId)));



    // There should be no effort to generate GPU display IDs.

    EXPECT_CALL(*mockGpuDisplayIdGeneratorPtr, generateId()).Times(0);



    static constexpr ui::Size kResolution{1920U, 1080U};

    static ui::PixelFormat format = static_cast<ui::PixelFormat>(PIXEL_FORMAT_RGBA_8888);

    EXPECT_CALL(*mComposer,

                createVirtualDisplay(static_cast<uint32_t>(kResolution.width),

                                     static_cast<uint32_t>(kResolution.height),

                                     testing::Pointee(format), _))

            .Times(1)

            .WillOnce(Return(Error::NONE));

    EXPECT_CALL(*mComposer, setClientTargetSlotCount(_)).WillOnce(Return(hal::Error::NONE));



    // --------------------------------------------------------------------

    // Invocation

    const std::string name("virtual.test");

    auto builder = compositionengine::DisplayCreationArgsBuilder();

    auto virtualDisplayIdVariantOpt =

            mFlinger.acquireVirtualDisplay(kResolution, format, name, builder);



    ASSERT_TRUE(virtualDisplayIdVariantOpt);

    ASSERT_TRUE(std::holds_alternative<HalVirtualDisplayId>(*virtualDisplayIdVariantOpt));



    const uint64_t halVirtualDisplayIdValue =

            asVirtualDisplayId(*virtualDisplayIdVariantOpt)->value;

    ASSERT_EQ(nonConflictingVirtualId, halVirtualDisplayIdValue);

    ASSERT_NE(primaryDisplayId, halVirtualDisplayIdValue);

}



TEST_F(DisplayTransactionCommitTest, acquireGpuVirtualDisplayIdWithConflictResolutionSuccess) {

    SET_FLAG_FOR_TEST(com::android::graphics::surfaceflinger::flags::stable_edid_ids, true);

    using Case = SimplePrimaryDisplayCase;



    // --------------------------------------------------------------------

    // Preconditions



    // Set up a primary physical display.

    processesHotplugConnectCommon<Case>();

    const uint64_t primaryDisplayId = asDisplayId(Case::Display::DISPLAY_ID::get()).value;



    // Setting the HAL generator to nullptr disables HWC composition for virtual displays.

    auto gpuDisplayIdGenerator = std::make_unique<MockDisplayIdGenerator<GpuVirtualDisplayId>>();

    auto* mockGpuDisplayIdGeneratorPtr = gpuDisplayIdGenerator.get();

    mFlinger.injectDisplayIdGenerators(std::move(gpuDisplayIdGenerator), nullptr);



    // --------------------------------------------------------------------

    // Call Expectations



    // The GPU display ID generator will return the primary physical display's ID once

    // to produce conflict in the virtual display acquisition logic, then return a non-conflicting

    // ID.

    const uint64_t nonConflictingVirtualId =

            asDisplayId(NonHwcVirtualDisplayCase::Display::DISPLAY_ID::get()).value;

    EXPECT_CALL(*mockGpuDisplayIdGeneratorPtr, generateId())

            .WillOnce(testing::Return(GpuVirtualDisplayId::fromValue(primaryDisplayId)))

            .WillOnce(testing::Return(GpuVirtualDisplayId::fromValue(nonConflictingVirtualId)));



    // The HAL should not be involved in the creation of GPU virtual displays.

    EXPECT_CALL(*mComposer, createVirtualDisplay(_, _, _, _)).Times(0);



    // --------------------------------------------------------------------

    // Invocation

    constexpr ui::Size kResolution{1920U, 1080U};

    const ui::PixelFormat format = static_cast<ui::PixelFormat>(PIXEL_FORMAT_RGBA_8888);

    const std::string name("virtual.test");

    auto builder = compositionengine::DisplayCreationArgsBuilder();

    auto virtualDisplayIdVariantOpt =

            mFlinger.acquireVirtualDisplay(kResolution, format, name, builder);



    ASSERT_TRUE(virtualDisplayIdVariantOpt);

    ASSERT_TRUE(std::holds_alternative<GpuVirtualDisplayId>(*virtualDisplayIdVariantOpt));



    const uint64_t gpuVirtualDisplayIdValue =

            asVirtualDisplayId(*virtualDisplayIdVariantOpt)->value;

    ASSERT_EQ(nonConflictingVirtualId, gpuVirtualDisplayIdValue);

    ASSERT_NE(primaryDisplayId, gpuVirtualDisplayIdValue);

}



TEST_F(DisplayTransactionCommitTest, acquireVirtualDisplayIdWithConflictResolutionCompleteFailure) {

    SET_FLAG_FOR_TEST(com::android::graphics::surfaceflinger::flags::stable_edid_ids, true);

    using Case = SimplePrimaryDisplayCase;



    // --------------------------------------------------------------------

    // Preconditions



    // Set up a primary physical display.

    processesHotplugConnectCommon<Case>();

    const uint64_t primaryDisplayId = asDisplayId(Case::Display::DISPLAY_ID::get()).value;



    // Injecting a mock Hal generator enables Hal composition.

    auto halDisplayIdGenerator = std::make_unique<MockDisplayIdGenerator<HalVirtualDisplayId>>();

    auto* mockHalDisplayIdGeneratorPtr = halDisplayIdGenerator.get();

    auto gpuDisplayIdGenerator = std::make_unique<MockDisplayIdGenerator<GpuVirtualDisplayId>>();

    auto* mockGpuDisplayIdGeneratorPtr = gpuDisplayIdGenerator.get();

    mFlinger.injectDisplayIdGenerators(std::move(gpuDisplayIdGenerator),

                                       std::move(halDisplayIdGenerator));



    // --------------------------------------------------------------------

    // Call Expectations



    // The generators will repeatedly return the primary physical display's ID

    // to produce conflict in the virtual display acquisition logic.

    EXPECT_CALL(*mockHalDisplayIdGeneratorPtr, generateId())

            .Times(10)

            .WillRepeatedly(testing::Return(HalVirtualDisplayId::fromValue(primaryDisplayId)));

    EXPECT_CALL(*mockGpuDisplayIdGeneratorPtr, generateId())

            .Times(10)

            .WillRepeatedly(testing::Return(GpuVirtualDisplayId::fromValue(primaryDisplayId)));

    EXPECT_CALL(*mComposer, createVirtualDisplay(_, _, _, _)).Times(0);



    // --------------------------------------------------------------------

    // Invocation

    static constexpr ui::Size kResolution{1920U, 1080U};

    static const ui::PixelFormat format = static_cast<ui::PixelFormat>(PIXEL_FORMAT_RGBA_8888);

    static const std::string name("virtual.test");

    auto builder = compositionengine::DisplayCreationArgsBuilder();

    auto virtualDisplayIdVariantOpt =

            mFlinger.acquireVirtualDisplay(kResolution, format, name, builder);



    ASSERT_FALSE(virtualDisplayIdVariantOpt);

}



TEST_F(DisplayTransactionCommitTest, processesDisplayLayerStackChanges) {

    using Case = NonHwcVirtualDisplayCase;