From 26afc788434bf0f120db2fbd980d557f07bd1283 Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Mon, 6 May 2019 16:46:45 -0700 Subject: 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 --- services/surfaceflinger/RegionSamplingThread.cpp | 44 +++++++++--------------- 1 file changed, 17 insertions(+), 27 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 0d142675db..368426018b 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -168,11 +168,8 @@ RegionSamplingThread::RegionSamplingThread(SurfaceFlinger& flinger, Scheduler& s mPhaseCallback(std::make_unique(*this, mScheduler, tunables.mSamplingOffset)), lastSampleTime(0ns) { - { - std::lock_guard threadLock(mThreadMutex); - mThread = std::thread([this]() { threadMain(); }); - pthread_setname_np(mThread.native_handle(), "RegionSamplingThread"); - } + 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 asBinder = IInterface::asBinder(listener); asBinder->linkToDeath(this); - std::lock_guard lock(mMutex); + std::lock_guard lock(mSamplingMutex); mDescriptors.emplace(wp(asBinder), Descriptor{samplingArea, stopLayer, listener}); } void RegionSamplingThread::removeListener(const sp& listener) { - std::lock_guard lock(mMutex); + std::lock_guard lock(mSamplingMutex); mDescriptors.erase(wp(IInterface::asBinder(listener))); } void RegionSamplingThread::checkForStaleLuma() { - std::lock_guard lock(mMutex); + std::lock_guard lock(mThreadControlMutex); if (mDiscardedFrames) { ATRACE_INT(lumaSamplingStepTag, static_cast(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(samplingStep::idleTimerWaiting)); @@ -254,7 +250,7 @@ void RegionSamplingThread::doSample() { } void RegionSamplingThread::binderDied(const wp& who) { - std::lock_guard lock(mMutex); + std::lock_guard lock(mSamplingMutex); mDescriptors.erase(who); } @@ -315,6 +311,7 @@ std::vector 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 activeDescriptors; for (const auto& descriptor : descriptors) { @@ -429,15 +415,19 @@ void RegionSamplingThread::captureSample() { ATRACE_INT(lumaSamplingStepTag, static_cast(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 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; + }); } } -- cgit v1.2.3-59-g8ed1b