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

Commit a515fd50 authored by Dominik Laskowski's avatar Dominik Laskowski Committed by Android Build Coastguard Worker
Browse files

SF: Restore mStateLock mutex for modesetting 

In order to fix a deadlock during display hotplug, the modesetting mutex
was changed from mStateLock to a granular DisplayModeController lock, in
the following CLs:

    I30ec756f134d2d67a70ac8797008dc792eac035e
    Iaae02d95e175e55f52f65da1481b64f1f533a04c (reverted)

This exposed a data race leading to a crash, which, unlike the deadlock,
affects all devices with refresh rate switching. The race will be fixed
by:

    If6141d2928643e82b3251b321e18c300cd8c201c (WIP)

As a stopgap until then, restore mStateLock (on top of the DMC lock) on
the main thread to fix the race, but reintroduce the deadlock. The lock
was already restored on the idle timer thread by the reverted CL above.

Fixes: 348827779
Flag: com.android.graphics.surfaceflinger.flags.connected_display
Test: Manual (foldable, connected display)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:b2fc4235635f5160ed5ae35c1fbd1fcc3e45c59f)
Merged-In: Ie15de10d16eefeb65289574b120d2ef4cc09d6c0
Change-Id: Ie15de10d16eefeb65289574b120d2ef4cc09d6c0
parent f8483485

services/surfaceflinger/SurfaceFlinger.cpp

+19 −6
Original line number Diff line number Diff line
@@ -1414,6 +1414,8 @@ status_t SurfaceFlinger::setActiveModeFromBackdoor(const sp<display::DisplayToke 
    return future.get();

}



// TODO: b/241285876 - Restore thread safety analysis once mStateLock below is unconditional.

[[clang::no_thread_safety_analysis]]

void SurfaceFlinger::finalizeDisplayModeChange(PhysicalDisplayId displayId) {

    ATRACE_NAME(ftl::Concat(__func__, ' ', displayId.value).c_str());
@@ -1429,7 +1431,7 @@ void SurfaceFlinger::finalizeDisplayModeChange(PhysicalDisplayId displayId) { 
    if (const auto oldResolution =

                mDisplayModeController.getActiveMode(displayId).modePtr->getResolution();

        oldResolution != activeMode.modePtr->getResolution()) {

        Mutex::Autolock lock(mStateLock);

        ConditionalLock lock(mStateLock, !FlagManager::getInstance().connected_display());



        auto& state = mCurrentState.displays.editValueFor(getPhysicalDisplayTokenLocked(displayId));

        // We need to generate new sequenceId in order to recreate the display (and this
@@ -1483,7 +1485,7 @@ void SurfaceFlinger::initiateDisplayModeChanges() { 


    std::optional<PhysicalDisplayId> displayToUpdateImmediately;



    for (const auto& [displayId, physical] : FTL_FAKE_GUARD(mStateLock, mPhysicalDisplays)) {

    for (const auto& [displayId, physical] : mPhysicalDisplays) {

        auto desiredModeOpt = mDisplayModeController.getDesiredMode(displayId);

        if (!desiredModeOpt) {

            continue;
@@ -2618,11 +2620,15 @@ bool SurfaceFlinger::commit(PhysicalDisplayId pacesetterId, 
        return false;

    }



    {

        ConditionalLock lock(mStateLock, FlagManager::getInstance().connected_display());



        for (const auto [displayId, _] : frameTargets) {

            if (mDisplayModeController.isModeSetPending(displayId)) {

                finalizeDisplayModeChange(displayId);

            }

        }

    }



    if (pacesetterFrameTarget.isFramePending()) {

        if (mBackpressureGpuComposition || pacesetterFrameTarget.didMissHwcFrame()) {
@@ -2719,9 +2725,16 @@ bool SurfaceFlinger::commit(PhysicalDisplayId pacesetterId, 
                                                        ? &mLayerHierarchyBuilder.getHierarchy()

                                                        : nullptr,

                                                updateAttachedChoreographer);



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

            initiateDisplayModeChanges();

        }

    }



    if (!FlagManager::getInstance().connected_display()) {

        ftl::FakeGuard guard(mStateLock);

        initiateDisplayModeChanges();

    }



    updateCursorAsync();

    if (!mustComposite) {

services/surfaceflinger/SurfaceFlinger.h

+2 −2
Original line number Diff line number Diff line
@@ -738,9 +738,9 @@ private: 
    status_t setActiveModeFromBackdoor(const sp<display::DisplayToken>&, DisplayModeId, Fps minFps,

                                       Fps maxFps);



    void initiateDisplayModeChanges() REQUIRES(kMainThreadContext) EXCLUDES(mStateLock);

    void initiateDisplayModeChanges() REQUIRES(kMainThreadContext) REQUIRES(mStateLock);

    void finalizeDisplayModeChange(PhysicalDisplayId) REQUIRES(kMainThreadContext)

            EXCLUDES(mStateLock);

            REQUIRES(mStateLock);



    void dropModeRequest(PhysicalDisplayId) REQUIRES(kMainThreadContext);

    void applyActiveMode(PhysicalDisplayId) REQUIRES(kMainThreadContext);