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
diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp
index 0d14267..3684260 100644
--- a/services/surfaceflinger/RegionSamplingThread.cpp
+++ b/services/surfaceflinger/RegionSamplingThread.cpp
@@ -168,11 +168,8 @@
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");
- }
+ mThread = std::thread([this]() { threadMain(); });
+ pthread_setname_np(mThread.native_handle(), "RegionSamplingThread");
mIdleTimer.start();
}
@@ -186,12 +183,11 @@
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 @@
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::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::binderDied(const wp<IBinder>& who) {
- std::lock_guard lock(mMutex);
+ std::lock_guard lock(mSamplingMutex);
mDescriptors.erase(who);
}
@@ -315,6 +311,7 @@
void RegionSamplingThread::captureSample() {
ATRACE_CALL();
+ std::lock_guard lock(mSamplingMutex);
if (mDescriptors.empty()) {
return;
@@ -387,19 +384,8 @@
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 @@
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;
+ });
}
}
diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h
index 72b2042..08134e6 100644
--- a/services/surfaceflinger/RegionSamplingThread.h
+++ b/services/surfaceflinger/RegionSamplingThread.h
@@ -100,7 +100,7 @@
void binderDied(const wp<IBinder>& who) override;
void checkForStaleLuma();
- void captureSample() REQUIRES(mMutex);
+ void captureSample();
void threadMain();
SurfaceFlinger& mFlinger;
@@ -110,19 +110,18 @@
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;
+ 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::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;
+ std::mutex mSamplingMutex;
+ std::unordered_map<wp<IBinder>, Descriptor, WpHash> mDescriptors GUARDED_BY(mSamplingMutex);
+ sp<GraphicBuffer> mCachedBuffer GUARDED_BY(mSamplingMutex) = nullptr;
};
} // namespace android