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

Commit 26afc788 authored by Kevin DuBois's avatar Kevin DuBois
Browse files

sf: avoid lock on main thread during luma calc 

The luma calculation operation was previously done while holding
a lock that was also used to protect the request to collect the
next sample. When the main thread would request the next sample,
it could stall waiting on the longer luma calculation to complete.

This patch refactors the sampling request locking mechanisms so
sampling request signalling is protected by one lock, and the members
needed during the sampling thread operations are protected by another
lock.

Fixes: http://b/132110951
Test: collect traces during during problematic animation (agsa
initial query for info) and verify problem went away
Test: visually inspect jank during agsa continued conversation
Test: libgui#RegionSamplingTest
Test: monkey 6000 event inject
Change-Id: I291d6bcb80d0588f2e1f3689bfdd4b3434132e90
parent c81a7996

services/surfaceflinger/RegionSamplingThread.cpp

+17 −27
Original line number Diff line number Diff line
@@ -168,11 +168,8 @@ RegionSamplingThread::RegionSamplingThread(SurfaceFlinger& flinger, Scheduler& s 
        mPhaseCallback(std::make_unique<SamplingOffsetCallback>(*this, mScheduler,

                                                                tunables.mSamplingOffset)),

        lastSampleTime(0ns) {

    {

        std::lock_guard threadLock(mThreadMutex);

    mThread = std::thread([this]() { threadMain(); });

    pthread_setname_np(mThread.native_handle(), "RegionSamplingThread");

    }

    mIdleTimer.start();

}
@@ -186,12 +183,11 @@ RegionSamplingThread::~RegionSamplingThread() { 
    mIdleTimer.stop();



    {

        std::lock_guard lock(mMutex);

        std::lock_guard lock(mThreadControlMutex);

        mRunning = false;

        mCondition.notify_one();

    }



    std::lock_guard threadLock(mThreadMutex);

    if (mThread.joinable()) {

        mThread.join();

    }
@@ -205,17 +201,17 @@ void RegionSamplingThread::addListener(const Rect& samplingArea, const sp<IBinde 


    sp<IBinder> asBinder = IInterface::asBinder(listener);

    asBinder->linkToDeath(this);

    std::lock_guard lock(mMutex);

    std::lock_guard lock(mSamplingMutex);

    mDescriptors.emplace(wp<IBinder>(asBinder), Descriptor{samplingArea, stopLayer, listener});

}



void RegionSamplingThread::removeListener(const sp<IRegionSamplingListener>& listener) {

    std::lock_guard lock(mMutex);

    std::lock_guard lock(mSamplingMutex);

    mDescriptors.erase(wp<IBinder>(IInterface::asBinder(listener)));

}



void RegionSamplingThread::checkForStaleLuma() {

    std::lock_guard lock(mMutex);

    std::lock_guard lock(mThreadControlMutex);



    if (mDiscardedFrames) {

        ATRACE_INT(lumaSamplingStepTag, static_cast<int>(samplingStep::waitForZeroPhase));
@@ -233,7 +229,7 @@ void RegionSamplingThread::notifySamplingOffset() { 
}



void RegionSamplingThread::doSample() {

    std::lock_guard lock(mMutex);

    std::lock_guard lock(mThreadControlMutex);

    auto now = std::chrono::nanoseconds(systemTime(SYSTEM_TIME_MONOTONIC));

    if (lastSampleTime + mTunables.mSamplingPeriod > now) {

        ATRACE_INT(lumaSamplingStepTag, static_cast<int>(samplingStep::idleTimerWaiting));
@@ -254,7 +250,7 @@ void RegionSamplingThread::doSample() { 
}



void RegionSamplingThread::binderDied(const wp<IBinder>& who) {

    std::lock_guard lock(mMutex);

    std::lock_guard lock(mSamplingMutex);

    mDescriptors.erase(who);

}
@@ -315,6 +311,7 @@ std::vector<float> RegionSamplingThread::sampleBuffer( 


void RegionSamplingThread::captureSample() {

    ATRACE_CALL();

    std::lock_guard lock(mSamplingMutex);



    if (mDescriptors.empty()) {

        return;
@@ -387,19 +384,8 @@ void RegionSamplingThread::captureSample() { 
                                   PIXEL_FORMAT_RGBA_8888, 1, usage, "RegionSamplingThread");

    }



    // When calling into SF, we post a message into the SF message queue (so the

    // screen capture runs on the main thread). This message blocks until the

    // screenshot is actually captured, but before the capture occurs, the main

    // thread may perform a normal refresh cycle. At the end of this cycle, it

    // can request another sample (because layers changed), which triggers a

    // call into sampleNow. When sampleNow attempts to grab the mutex, we can

    // deadlock.

    //

    // To avoid this, we drop the mutex while we call into SF.

    mMutex.unlock();

    bool ignored;

    mFlinger.captureScreenCommon(renderArea, traverseLayers, buffer, false, ignored);

    mMutex.lock();



    std::vector<Descriptor> activeDescriptors;

    for (const auto& descriptor : descriptors) {
@@ -429,15 +415,19 @@ void RegionSamplingThread::captureSample() { 
    ATRACE_INT(lumaSamplingStepTag, static_cast<int>(samplingStep::noWorkNeeded));

}



void RegionSamplingThread::threadMain() {

    std::lock_guard lock(mMutex);

// NO_THREAD_SAFETY_ANALYSIS is because std::unique_lock presently lacks thread safety annotations.

void RegionSamplingThread::threadMain() NO_THREAD_SAFETY_ANALYSIS {

    std::unique_lock<std::mutex> lock(mThreadControlMutex);

    while (mRunning) {

        if (mSampleRequested) {

            mSampleRequested = false;

            lock.unlock();

            captureSample();

            lock.lock();

        }

        mCondition.wait(mMutex,

                        [this]() REQUIRES(mMutex) { return mSampleRequested || !mRunning; });

        mCondition.wait(lock, [this]() REQUIRES(mThreadControlMutex) {

            return mSampleRequested || !mRunning;

        });

    }

}

services/surfaceflinger/RegionSamplingThread.h

+11 −12
Original line number Diff line number Diff line
@@ -100,7 +100,7 @@ private: 
    void binderDied(const wp<IBinder>& who) override;

    void checkForStaleLuma();



    void captureSample() REQUIRES(mMutex);

    void captureSample();

    void threadMain();



    SurfaceFlinger& mFlinger;
@@ -110,19 +110,18 @@ private: 


    std::unique_ptr<SamplingOffsetCallback> const mPhaseCallback;



    std::mutex mThreadMutex;

    std::thread mThread GUARDED_BY(mThreadMutex);

    std::thread mThread;



    std::mutex mMutex;

    std::mutex mThreadControlMutex;

    std::condition_variable_any mCondition;

    bool mRunning GUARDED_BY(mMutex) = true;

    bool mSampleRequested GUARDED_BY(mMutex) = false;



    std::unordered_map<wp<IBinder>, Descriptor, WpHash> mDescriptors GUARDED_BY(mMutex);

    std::chrono::nanoseconds lastSampleTime GUARDED_BY(mMutex);

    bool mDiscardedFrames GUARDED_BY(mMutex) = false;



    sp<GraphicBuffer> mCachedBuffer GUARDED_BY(mMutex) = nullptr;

    bool mRunning GUARDED_BY(mThreadControlMutex) = true;

    bool mSampleRequested GUARDED_BY(mThreadControlMutex) = false;

    bool mDiscardedFrames GUARDED_BY(mThreadControlMutex) = false;

    std::chrono::nanoseconds lastSampleTime GUARDED_BY(mThreadControlMutex);



    std::mutex mSamplingMutex;

    std::unordered_map<wp<IBinder>, Descriptor, WpHash> mDescriptors GUARDED_BY(mSamplingMutex);

    sp<GraphicBuffer> mCachedBuffer GUARDED_BY(mSamplingMutex) = nullptr;

};



} // namespace android