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

Commit b2fc4235 authored by Dominik Laskowski's avatar Dominik Laskowski
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)
Change-Id: Ie15de10d16eefeb65289574b120d2ef4cc09d6c0
parent 687f9fea

services/surfaceflinger/SurfaceFlinger.cpp

+19 −6
Original line number Diff line number Diff line
@@ -1416,6 +1416,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) {

    SFTRACE_NAME(ftl::Concat(__func__, ' ', displayId.value).c_str());
@@ -1431,7 +1433,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::applyActiveMode(PhysicalDisplayId displayId) { 
void SurfaceFlinger::initiateDisplayModeChanges() {

    SFTRACE_CALL();



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

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

        auto desiredModeOpt = mDisplayModeController.getDesiredMode(displayId);

        if (!desiredModeOpt) {

            continue;
@@ -2611,11 +2613,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()) {
@@ -2712,9 +2718,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
@@ -739,9 +739,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);