From fbc80aef0ba1b11982cf4ca88d218b65b6eca0f3 Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Fri, 26 May 2017 16:23:54 -0700 Subject: Reduce number of Fence syscalls made. This patch saves 6 or more Fence syscalls per frame. * Timelines are updated just before adding a new fence since the newly added fence is unlikely to have signaled. * Layer::latch uses a FenceTime now, so the signal time is automatically shared with other owners of the FenceTime. * DispSync uses FenceTime now, only using cached values of the signal time that have been populated by a Timeline. Test: SurfaceFlinger boots and dumps still work. Change-Id: Ie0cfc1af2aca143dd8d5f08f08dbe1e597376f2f --- services/surfaceflinger/DispSync.cpp | 72 +++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 33 deletions(-) (limited to 'services/surfaceflinger/DispSync.cpp') diff --git a/services/surfaceflinger/DispSync.cpp b/services/surfaceflinger/DispSync.cpp index bd9b8aafcd..ac8aa04e9d 100644 --- a/services/surfaceflinger/DispSync.cpp +++ b/services/surfaceflinger/DispSync.cpp @@ -30,7 +30,7 @@ #include #include -#include +#include #include "DispSync.h" #include "SurfaceFlinger.h" @@ -419,25 +419,13 @@ void DispSync::reset() { resetErrorLocked(); } -bool DispSync::addPresentFence(const sp& fence) { +bool DispSync::addPresentFence(const std::shared_ptr& fenceTime) { Mutex::Autolock lock(mMutex); - mPresentFences[mPresentSampleOffset] = fence; - mPresentTimes[mPresentSampleOffset] = 0; + mPresentFences[mPresentSampleOffset] = fenceTime; mPresentSampleOffset = (mPresentSampleOffset + 1) % NUM_PRESENT_SAMPLES; mNumResyncSamplesSincePresent = 0; - for (size_t i = 0; i < NUM_PRESENT_SAMPLES; i++) { - const sp& f(mPresentFences[i]); - if (f != NULL) { - nsecs_t t = f->getSignalTime(); - if (t < INT64_MAX) { - mPresentFences[i].clear(); - mPresentTimes[i] = t + mPresentTimeOffset; - } - } - } - updateErrorLocked(); return !mModelUpdated || mError > kErrorThreshold; @@ -602,21 +590,39 @@ void DispSync::updateErrorLocked() { nsecs_t sqErrSum = 0; for (size_t i = 0; i < NUM_PRESENT_SAMPLES; i++) { - nsecs_t sample = mPresentTimes[i] - mReferenceTime; - if (sample > mPhase) { - nsecs_t sampleErr = (sample - mPhase) % period; - if (sampleErr > period / 2) { - sampleErr -= period; - } - sqErrSum += sampleErr * sampleErr; - numErrSamples++; + // Only check for the cached value of signal time to avoid unecessary + // syscalls. It is the responsibility of the DispSync owner to + // call getSignalTime() periodically so the cache is updated when the + // fence signals. + nsecs_t time = mPresentFences[i]->getCachedSignalTime(); + if (time == Fence::SIGNAL_TIME_PENDING || + time == Fence::SIGNAL_TIME_INVALID) { + continue; + } + + nsecs_t sample = time - mReferenceTime; + if (sample <= mPhase) { + continue; + } + + nsecs_t sampleErr = (sample - mPhase) % period; + if (sampleErr > period / 2) { + sampleErr -= period; } + sqErrSum += sampleErr * sampleErr; + numErrSamples++; } if (numErrSamples > 0) { mError = sqErrSum / numErrSamples; + mZeroErrSamplesCount = 0; } else { mError = 0; + // Use mod ACCEPTABLE_ZERO_ERR_SAMPLES_COUNT to avoid log spam. + mZeroErrSamplesCount++; + ALOGE_IF( + (mZeroErrSamplesCount % ACCEPTABLE_ZERO_ERR_SAMPLES_COUNT) == 0, + "No present times for model error."); } if (kTraceDetailedInfo) { @@ -627,9 +633,9 @@ void DispSync::updateErrorLocked() { void DispSync::resetErrorLocked() { mPresentSampleOffset = 0; mError = 0; + mZeroErrSamplesCount = 0; for (size_t i = 0; i < NUM_PRESENT_SAMPLES; i++) { - mPresentFences[i].clear(); - mPresentTimes[i] = 0; + mPresentFences[i] = FenceTime::NO_FENCE; } } @@ -668,19 +674,19 @@ void DispSync::dump(String8& result) const { previous = sampleTime; } - result.appendFormat("mPresentFences / mPresentTimes [%d]:\n", + result.appendFormat("mPresentFences [%d]:\n", NUM_PRESENT_SAMPLES); nsecs_t now = systemTime(SYSTEM_TIME_MONOTONIC); - previous = 0; + previous = Fence::SIGNAL_TIME_INVALID; for (size_t i = 0; i < NUM_PRESENT_SAMPLES; i++) { size_t idx = (i + mPresentSampleOffset) % NUM_PRESENT_SAMPLES; - bool signaled = mPresentFences[idx] == NULL; - nsecs_t presentTime = mPresentTimes[idx]; - if (!signaled) { + nsecs_t presentTime = mPresentFences[idx]->getSignalTime(); + if (presentTime == Fence::SIGNAL_TIME_PENDING) { result.appendFormat(" [unsignaled fence]\n"); - } else if (presentTime == 0) { - result.appendFormat(" 0\n"); - } else if (previous == 0) { + } else if(presentTime == Fence::SIGNAL_TIME_INVALID) { + result.appendFormat(" [invalid fence]\n"); + } else if (previous == Fence::SIGNAL_TIME_PENDING || + previous == Fence::SIGNAL_TIME_INVALID) { result.appendFormat(" %" PRId64 " (%.3f ms ago)\n", presentTime, (now - presentTime) / 1000000.0); } else { -- cgit v1.2.3-59-g8ed1b From f41745301d5ecfa680dcef3a1948a8a321f80509 Mon Sep 17 00:00:00 2001 From: Saurabh Shah Date: Thu, 13 Jul 2017 10:45:07 -0700 Subject: sf: Defer DispSync initialization Some DispSync members are initialized based on uninitialized static members of sf, that are in turn initialized in sf constructor. Fix the sequence by deferring DispSync initialization. Current sequence: sf constructor|-> DispSync constructor -> Access static sf members |-> Initialize sf static members New sequence: sf constructor|-> DispSync constructor |-> Initialize sf static members |-> DispSync init -> Access static sf members Bug: 63671437 Test: "present fences are ignored" not present in SF dumpsys Change-Id: I618d2bbbbd4e39fc382e67f85dd8d637dd82cf38 --- services/surfaceflinger/DispSync.cpp | 14 ++++++++------ services/surfaceflinger/DispSync.h | 2 ++ services/surfaceflinger/SurfaceFlinger.cpp | 2 ++ 3 files changed, 12 insertions(+), 6 deletions(-) (limited to 'services/surfaceflinger/DispSync.cpp') diff --git a/services/surfaceflinger/DispSync.cpp b/services/surfaceflinger/DispSync.cpp index ac8aa04e9d..bef12ea50f 100644 --- a/services/surfaceflinger/DispSync.cpp +++ b/services/surfaceflinger/DispSync.cpp @@ -377,11 +377,16 @@ private: DispSync::DispSync(const char* name) : mName(name), mRefreshSkipCount(0), - mThread(new DispSyncThread(name)), - mIgnorePresentFences(!SurfaceFlinger::hasSyncFramework){ + mThread(new DispSyncThread(name)) { +} + +DispSync::~DispSync() {} - mPresentTimeOffset = SurfaceFlinger::dispSyncPresentTimeOffset; +void DispSync::init(bool hasSyncFramework, int64_t dispSyncPresentTimeOffset) { + mIgnorePresentFences = !hasSyncFramework; + mPresentTimeOffset = dispSyncPresentTimeOffset; mThread->run("DispSync", PRIORITY_URGENT_DISPLAY + PRIORITY_MORE_FAVORABLE); + // set DispSync to SCHED_FIFO to minimize jitter struct sched_param param = {0}; param.sched_priority = 2; @@ -389,7 +394,6 @@ DispSync::DispSync(const char* name) : ALOGE("Couldn't set SCHED_FIFO for DispSyncThread"); } - reset(); beginResync(); @@ -405,8 +409,6 @@ DispSync::DispSync(const char* name) : } } -DispSync::~DispSync() {} - void DispSync::reset() { Mutex::Autolock lock(mMutex); diff --git a/services/surfaceflinger/DispSync.h b/services/surfaceflinger/DispSync.h index c9f3b0481e..880a24d6ad 100644 --- a/services/surfaceflinger/DispSync.h +++ b/services/surfaceflinger/DispSync.h @@ -59,6 +59,8 @@ public: explicit DispSync(const char* name); ~DispSync(); + void init(bool hasSyncFramework, int64_t dispSyncPresentTimeOffset); + // reset clears the resync samples and error value. void reset(); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 7fb315865f..fe0fa2b0ff 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -195,6 +195,8 @@ SurfaceFlinger::SurfaceFlinger() hasWideColorDisplay = getBool(false); + mPrimaryDispSync.init(hasSyncFramework, dispSyncPresentTimeOffset); + // debugging stuff... char value[PROPERTY_VALUE_MAX]; -- cgit v1.2.3-59-g8ed1b