From f8e689cfd25f27c4af36a27e145d4cac1d0bb9cb Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Mon, 20 May 2019 18:32:22 -0700 Subject: [SurfaceFlinger] fix permanently enabling early offsets. * Add notion of intended period in DispSync, and use that to detect whether the simulated period will be changed. * Propagate that signal to setDesiredActiveConfig, so that we don't fall into early offsets unnecessarily, which can cause early offsets to always be enabled. Bug: 132678707 Test: systrace Test: swappy test app Test: scrolling through google news Change-Id: I18df1b9d949cd534ecbf1c8891b6f88eab8be399 --- services/surfaceflinger/Scheduler/DispSync.cpp | 38 +++++++++++++++------- services/surfaceflinger/Scheduler/DispSync.h | 21 ++++++++---- services/surfaceflinger/Scheduler/Scheduler.cpp | 8 ++--- services/surfaceflinger/Scheduler/Scheduler.h | 11 +++++-- services/surfaceflinger/Scheduler/VSyncModulator.h | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 23 ++++++++++--- .../tests/unittests/mock/MockDispSync.h | 1 + 7 files changed, 72 insertions(+), 32 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/DispSync.cpp b/services/surfaceflinger/Scheduler/DispSync.cpp index cd6fa41940..f7a30af877 100644 --- a/services/surfaceflinger/Scheduler/DispSync.cpp +++ b/services/surfaceflinger/Scheduler/DispSync.cpp @@ -115,11 +115,13 @@ public: void lockModel() { Mutex::Autolock lock(mMutex); mModelLocked = true; + ATRACE_INT("DispSync:ModelLocked", mModelLocked); } void unlockModel() { Mutex::Autolock lock(mMutex); mModelLocked = false; + ATRACE_INT("DispSync:ModelLocked", mModelLocked); } virtual bool threadLoop() { @@ -493,7 +495,6 @@ void DispSync::init(bool hasSyncFramework, int64_t dispSyncPresentTimeOffset) { ALOGE("Couldn't set SCHED_FIFO for DispSyncThread"); } - reset(); beginResync(); if (mTraceDetailedInfo && kEnableZeroPhaseTracer) { @@ -545,17 +546,15 @@ bool DispSync::addPresentFence(const std::shared_ptr& fenceTime) { void DispSync::beginResync() { Mutex::Autolock lock(mMutex); ALOGV("[%s] beginResync", mName); - mThread->unlockModel(); - mModelUpdated = false; - mNumResyncSamples = 0; + resetLocked(); } -bool DispSync::addResyncSample(nsecs_t timestamp, bool* periodChanged) { +bool DispSync::addResyncSample(nsecs_t timestamp, bool* periodFlushed) { Mutex::Autolock lock(mMutex); ALOGV("[%s] addResyncSample(%" PRId64 ")", mName, ns2us(timestamp)); - *periodChanged = false; + *periodFlushed = false; const size_t idx = (mFirstResyncSample + mNumResyncSamples) % MAX_RESYNC_SAMPLES; mResyncSamples[idx] = timestamp; if (mNumResyncSamples == 0) { @@ -569,16 +568,20 @@ bool DispSync::addResyncSample(nsecs_t timestamp, bool* periodChanged) { const nsecs_t lastTimestamp = mResyncSamples[priorIdx]; const nsecs_t observedVsync = std::abs(timestamp - lastTimestamp); - if (std::abs(observedVsync - mPendingPeriod) < std::abs(observedVsync - mPeriod)) { - // Observed vsync is closer to the pending period, so reset the - // model and flush the pending period. + if (std::abs(observedVsync - mPendingPeriod) <= std::abs(observedVsync - mIntendedPeriod)) { + // Either the observed vsync is closer to the pending period, (and + // thus we detected a period change), or the period change will + // no-op. In either case, reset the model and flush the pending + // period. resetLocked(); + mIntendedPeriod = mPendingPeriod; mPeriod = mPendingPeriod; mPendingPeriod = 0; if (mTraceDetailedInfo) { ATRACE_INT("DispSync:PendingPeriod", mPendingPeriod); + ATRACE_INT("DispSync:IntendedPeriod", mIntendedPeriod); } - *periodChanged = true; + *periodFlushed = true; } } // Always update the reference time with the most recent timestamp. @@ -609,6 +612,7 @@ bool DispSync::addResyncSample(nsecs_t timestamp, bool* periodChanged) { bool modelLocked = mModelUpdated && mError < (kErrorThreshold / 2) && mPendingPeriod == 0; ALOGV("[%s] addResyncSample returning %s", mName, modelLocked ? "locked" : "unlocked"); if (modelLocked) { + *periodFlushed = true; mThread->lockModel(); } return !modelLocked; @@ -643,10 +647,17 @@ status_t DispSync::changePhaseOffset(Callback* callback, nsecs_t phase) { void DispSync::setPeriod(nsecs_t period) { Mutex::Autolock lock(mMutex); + + const bool pendingPeriodShouldChange = + period != mIntendedPeriod || (period == mIntendedPeriod && mPendingPeriod != 0); + + if (pendingPeriodShouldChange) { + mPendingPeriod = period; + } if (mTraceDetailedInfo) { - ATRACE_INT("DispSync:PendingPeriod", period); + ATRACE_INT("DispSync:IntendedPeriod", mIntendedPeriod); + ATRACE_INT("DispSync:PendingPeriod", mPendingPeriod); } - mPendingPeriod = period; } nsecs_t DispSync::getPeriod() { @@ -764,6 +775,9 @@ void DispSync::resetErrorLocked() { mPresentSampleOffset = 0; mError = 0; mZeroErrSamplesCount = 0; + if (mTraceDetailedInfo) { + ATRACE_INT64("DispSync:Error", mError); + } for (size_t i = 0; i < NUM_PRESENT_SAMPLES; i++) { mPresentFences[i] = FenceTime::NO_FENCE; } diff --git a/services/surfaceflinger/Scheduler/DispSync.h b/services/surfaceflinger/Scheduler/DispSync.h index 8f8b8e7a99..3e33c7edc0 100644 --- a/services/surfaceflinger/Scheduler/DispSync.h +++ b/services/surfaceflinger/Scheduler/DispSync.h @@ -49,7 +49,7 @@ public: virtual void reset() = 0; virtual bool addPresentFence(const std::shared_ptr&) = 0; virtual void beginResync() = 0; - virtual bool addResyncSample(nsecs_t timestamp, bool* periodChanged) = 0; + virtual bool addResyncSample(nsecs_t timestamp, bool* periodFlushed) = 0; virtual void endResync() = 0; virtual void setPeriod(nsecs_t period) = 0; virtual nsecs_t getPeriod() = 0; @@ -120,17 +120,19 @@ public: // from the hardware vsync events. void beginResync() override; // Adds a vsync sample to the dispsync model. The timestamp is the time - // of the vsync event that fired. periodChanged will return true if the + // of the vsync event that fired. periodFlushed will return true if the // vsync period was detected to have changed to mPendingPeriod. // // This method will return true if more vsync samples are needed to lock // down the DispSync model, and false otherwise. - bool addResyncSample(nsecs_t timestamp, bool* periodChanged) override; + // periodFlushed will be set to true if mPendingPeriod is flushed to + // mIntendedPeriod, and false otherwise. + bool addResyncSample(nsecs_t timestamp, bool* periodFlushed) override; void endResync() override; // The setPeriod method sets the vsync event model's period to a specific - // value. This should be used to prime the model when a display is first - // turned on. It should NOT be used after that. + // value. This should be used to prime the model when a display is first + // turned on, or when a refresh rate change is requested. void setPeriod(nsecs_t period) override; // The getPeriod method returns the current vsync period. @@ -205,6 +207,11 @@ private: // nanoseconds. nsecs_t mPeriod; + // mIntendedPeriod is the intended period of the modeled vsync events in + // nanoseconds. Under ideal conditions this should be similar if not the + // same as mPeriod, plus or minus an observed error. + nsecs_t mIntendedPeriod = 0; + // mPendingPeriod is the proposed period change in nanoseconds. // If mPendingPeriod differs from mPeriod and is nonzero, it will // be flushed to mPeriod when we detect that the hardware switched @@ -236,8 +243,8 @@ private: // process to store information about the hardware vsync event times used // to compute the model. nsecs_t mResyncSamples[MAX_RESYNC_SAMPLES] = {0}; - size_t mFirstResyncSample; - size_t mNumResyncSamples; + size_t mFirstResyncSample = 0; + size_t mNumResyncSamples = 0; int mNumResyncSamplesSincePresent; // These member variables store information about the present fences used diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index 513436a270..e2a348e146 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -280,13 +280,13 @@ void Scheduler::setVsyncPeriod(const nsecs_t period) { } } -void Scheduler::addResyncSample(const nsecs_t timestamp, bool* periodChanged) { +void Scheduler::addResyncSample(const nsecs_t timestamp, bool* periodFlushed) { bool needsHwVsync = false; - *periodChanged = false; + *periodFlushed = false; { // Scope for the lock std::lock_guard lock(mHWVsyncLock); if (mPrimaryHWVsyncEnabled) { - needsHwVsync = mPrimaryDispSync->addResyncSample(timestamp, periodChanged); + needsHwVsync = mPrimaryDispSync->addResyncSample(timestamp, periodFlushed); } } @@ -415,7 +415,7 @@ void Scheduler::resetKernelTimerCallback() { ATRACE_INT("ExpiredKernelIdleTimer", 0); std::lock_guard lock(mCallbackLock); if (mGetVsyncPeriod) { - resyncToHardwareVsync(false, mGetVsyncPeriod()); + resyncToHardwareVsync(true, mGetVsyncPeriod()); } } diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h index 96d4bd5214..f6cc87bb89 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.h +++ b/services/surfaceflinger/Scheduler/Scheduler.h @@ -135,13 +135,18 @@ public: void enableHardwareVsync(); void disableHardwareVsync(bool makeUnavailable); + // Resyncs the scheduler to hardware vsync. + // If makeAvailable is true, then hardware vsync will be turned on. + // Otherwise, if hardware vsync is not already enabled then this method will + // no-op. + // The period is the vsync period from the current display configuration. void resyncToHardwareVsync(bool makeAvailable, nsecs_t period); // Creates a callback for resyncing. ResyncCallback makeResyncCallback(GetVsyncPeriod&& getVsyncPeriod); void setRefreshSkipCount(int count); - // Passes a vsync sample to DispSync. periodChange will be true if DipSync - // detected that the vsync period changed, and false otherwise. - void addResyncSample(const nsecs_t timestamp, bool* periodChanged); + // Passes a vsync sample to DispSync. periodFlushed will be true if + // DispSync detected that the vsync period changed, and false otherwise. + void addResyncSample(const nsecs_t timestamp, bool* periodFlushed); void addPresentFence(const std::shared_ptr& fenceTime); void setIgnorePresentFences(bool ignore); nsecs_t expectedPresentTime(); diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.h b/services/surfaceflinger/Scheduler/VSyncModulator.h index 81a7864cdb..21dad12797 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.h +++ b/services/surfaceflinger/Scheduler/VSyncModulator.h @@ -116,7 +116,7 @@ public: // Called when we detect from vsync signals that the refresh rate changed. // This way we can move out of early offsets if no longer necessary. - void onRefreshRateChangeDetected() { + void onRefreshRateChangeCompleted() { if (!mRefreshRateChangePending) { return; } diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index dd75868443..e735e10cc4 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -935,9 +935,14 @@ void SurfaceFlinger::setDesiredActiveConfig(const ActiveConfigInfo& info) { // Start receiving vsync samples now, so that we can detect a period // switch. mScheduler->resyncToHardwareVsync(true, getVsyncPeriod()); + // We should only move to early offsets when we know that the refresh + // rate will change. Otherwise, we may be stuck in early offsets + // forever, as onRefreshRateChangeDetected will not be called. + if (mDesiredActiveConfig.event == Scheduler::ConfigEvent::Changed) { + mVsyncModulator.onRefreshRateChangeInitiated(); + } mPhaseOffsets->setRefreshRateType(info.type); const auto [early, gl, late] = mPhaseOffsets->getCurrentOffsets(); - mVsyncModulator.onRefreshRateChangeInitiated(); mVsyncModulator.setPhaseOffsets(early, gl, late); } mDesiredActiveConfigChanged = true; @@ -1014,6 +1019,10 @@ bool SurfaceFlinger::performSetActiveConfig() { std::lock_guard lock(mActiveConfigLock); mDesiredActiveConfig.event = Scheduler::ConfigEvent::None; mDesiredActiveConfigChanged = false; + // Update scheduler with the correct vsync period as a no-op. + // Otherwise, there exists a race condition where we get stuck in the + // incorrect vsync period. + mScheduler->resyncToHardwareVsync(false, getVsyncPeriod()); ATRACE_INT("DesiredActiveConfigChanged", mDesiredActiveConfigChanged); return false; } @@ -1026,6 +1035,10 @@ bool SurfaceFlinger::performSetActiveConfig() { mDesiredActiveConfig.event = Scheduler::ConfigEvent::None; mDesiredActiveConfig.configId = display->getActiveConfig(); mDesiredActiveConfigChanged = false; + // Update scheduler with the current vsync period as a no-op. + // Otherwise, there exists a race condition where we get stuck in the + // incorrect vsync period. + mScheduler->resyncToHardwareVsync(false, getVsyncPeriod()); ATRACE_INT("DesiredActiveConfigChanged", mDesiredActiveConfigChanged); return false; } @@ -1448,10 +1461,10 @@ void SurfaceFlinger::onVsyncReceived(int32_t sequenceId, hwc2_display_t hwcDispl return; } - bool periodChanged = false; - mScheduler->addResyncSample(timestamp, &periodChanged); - if (periodChanged) { - mVsyncModulator.onRefreshRateChangeDetected(); + bool periodFlushed = false; + mScheduler->addResyncSample(timestamp, &periodFlushed); + if (periodFlushed) { + mVsyncModulator.onRefreshRateChangeCompleted(); } } diff --git a/services/surfaceflinger/tests/unittests/mock/MockDispSync.h b/services/surfaceflinger/tests/unittests/mock/MockDispSync.h index 12a349dd76..9ca116d735 100644 --- a/services/surfaceflinger/tests/unittests/mock/MockDispSync.h +++ b/services/surfaceflinger/tests/unittests/mock/MockDispSync.h @@ -35,6 +35,7 @@ public: MOCK_METHOD0(endResync, void()); MOCK_METHOD1(setPeriod, void(nsecs_t)); MOCK_METHOD0(getPeriod, nsecs_t()); + MOCK_METHOD0(getIntendedPeriod, nsecs_t()); MOCK_METHOD1(setRefreshSkipCount, void(int)); MOCK_CONST_METHOD1(computeNextRefresh, nsecs_t(int)); MOCK_METHOD1(setIgnorePresentFences, void(bool)); -- cgit v1.2.3-59-g8ed1b From c3a482d7555905f8f65d693293175186aff19466 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Tue, 21 May 2019 00:51:01 -0700 Subject: [SurfaceFlinger] Some dispsync fixes for early event firing * Fix lastEventTime for listeners so that they don't fire early. * Properly set mHasFired for listeners so that if the dispsync model is currently being updated that mHasFired is always set to true if lastEventTime is after the most recent vsync reference. Bug: 132678707 Bug: 130684082 Test: systrace Change-Id: I5b860336f12b742cc67665776290939b61e7e3af --- services/surfaceflinger/Scheduler/DispSync.cpp | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/DispSync.cpp b/services/surfaceflinger/Scheduler/DispSync.cpp index f7a30af877..95ff9d0c73 100644 --- a/services/surfaceflinger/Scheduler/DispSync.cpp +++ b/services/surfaceflinger/Scheduler/DispSync.cpp @@ -79,11 +79,7 @@ public: Mutex::Autolock lock(mMutex); mPhase = phase; - if (mReferenceTime != referenceTime) { - for (auto& eventListener : mEventListeners) { - eventListener.mHasFired = false; - } - } + const bool referenceTimeChanged = mReferenceTime != referenceTime; mReferenceTime = referenceTime; if (mPeriod != 0 && mPeriod != period && mReferenceTime != 0) { // Inflate the reference time to be the most recent predicted @@ -94,6 +90,13 @@ public: mReferenceTime = mReferenceTime + (numOldPeriods)*mPeriod; } mPeriod = period; + if (!mModelLocked && referenceTimeChanged) { + for (auto& eventListener : mEventListeners) { + eventListener.mHasFired = false; + eventListener.mLastEventTime = + mReferenceTime - mPeriod + mPhase + eventListener.mPhase; + } + } if (mTraceDetailedInfo) { ATRACE_INT64("DispSync:Period", mPeriod); ATRACE_INT64("DispSync:Phase", mPhase + mPeriod / 2); @@ -120,6 +123,13 @@ public: void unlockModel() { Mutex::Autolock lock(mMutex); + if (mModelLocked) { + for (auto& eventListener : mEventListeners) { + if (eventListener.mLastEventTime > mReferenceTime) { + eventListener.mHasFired = true; + } + } + } mModelLocked = false; ATRACE_INT("DispSync:ModelLocked", mModelLocked); } @@ -249,6 +259,10 @@ public: listener.mLastCallbackTime = lastCallbackTime; } + if (!mModelLocked && listener.mLastEventTime > mReferenceTime) { + listener.mHasFired = true; + } + mEventListeners.push_back(listener); mCond.signal(); -- cgit v1.2.3-59-g8ed1b From 5a1021000dabfad591dc1afa6525b7165c046bf9 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 23 May 2019 07:14:20 -0700 Subject: [SurfaceFlinger] add minimum frame count for early gl offsets. This is to avoid edge case behavior by rapidly switching in and out of gl composition, causing poor vsync timelines. Bug: 132997413 Test: systrace Change-Id: I4665f34f7e027a7883cfb5e47e14f41d845d1298 --- services/surfaceflinger/Scheduler/VSyncModulator.h | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.h b/services/surfaceflinger/Scheduler/VSyncModulator.h index 81a7864cdb..72f90505c6 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.h +++ b/services/surfaceflinger/Scheduler/VSyncModulator.h @@ -35,6 +35,11 @@ private: // sending new transactions. const int MIN_EARLY_FRAME_COUNT_TRANSACTION = 2; + // Number of frames we'll keep the early gl phase offsets once they are activated. + // This acts as a low-pass filter to avoid scenarios where we rapidly + // switch in and out of gl composition. + const int MIN_EARLY_GL_FRAME_COUNT_TRANSACTION = 2; + public: struct Offsets { nsecs_t sf; @@ -130,10 +135,14 @@ public: mRemainingEarlyFrameCount--; updateOffsetsNeeded = true; } - if (usedRenderEngine != mLastFrameUsedRenderEngine) { - mLastFrameUsedRenderEngine = usedRenderEngine; + if (usedRenderEngine) { + mRemainingRenderEngineUsageCount = MIN_EARLY_GL_FRAME_COUNT_TRANSACTION; + updateOffsetsNeeded = true; + } else if (mRemainingRenderEngineUsageCount > 0) { + mRemainingRenderEngineUsageCount--; updateOffsetsNeeded = true; } + if (updateOffsetsNeeded) { updateOffsets(); } @@ -145,7 +154,7 @@ public: if (mTransactionStart == Scheduler::TransactionStart::EARLY || mRemainingEarlyFrameCount > 0 || mRefreshRateChangePending) { return mEarlyOffsets; - } else if (mLastFrameUsedRenderEngine) { + } else if (mRemainingRenderEngineUsageCount > 0) { return mEarlyGlOffsets; } else { return mLateOffsets; @@ -195,9 +204,9 @@ private: std::atomic mTransactionStart = Scheduler::TransactionStart::NORMAL; - std::atomic mLastFrameUsedRenderEngine = false; std::atomic mRefreshRateChangePending = false; std::atomic mRemainingEarlyFrameCount = 0; + std::atomic mRemainingRenderEngineUsageCount = 0; }; } // namespace android -- cgit v1.2.3-59-g8ed1b From 53852a5c891ea82b85391eec145c2ac9c2aaed23 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Tue, 28 May 2019 18:07:44 -0700 Subject: SurfaceFlinger: set refresh rate type when after an attempt to switch When a refresh rate change is starting we change the offsets to the new refresh rate to have a better alignment between buffers and the actual vsync. This change guarantees that we set the correct offset by updating PhaseOffset after desired config changed. Test: Let device go to 60Hz due to idle; Open an app; Get systrace; repeat Bug: 133241520 Change-Id: Id25f5e7d3d280813a106fd311906767d79e4e6ee --- services/surfaceflinger/SurfaceFlinger.cpp | 41 +++++++++++++----------------- services/surfaceflinger/SurfaceFlinger.h | 4 ++- 2 files changed, 20 insertions(+), 25 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index e735e10cc4..50fabe3c43 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -935,12 +935,9 @@ void SurfaceFlinger::setDesiredActiveConfig(const ActiveConfigInfo& info) { // Start receiving vsync samples now, so that we can detect a period // switch. mScheduler->resyncToHardwareVsync(true, getVsyncPeriod()); - // We should only move to early offsets when we know that the refresh - // rate will change. Otherwise, we may be stuck in early offsets - // forever, as onRefreshRateChangeDetected will not be called. - if (mDesiredActiveConfig.event == Scheduler::ConfigEvent::Changed) { - mVsyncModulator.onRefreshRateChangeInitiated(); - } + // As we called to set period, we will call to onRefreshRateChangeCompleted once + // DispSync model is locked. + mVsyncModulator.onRefreshRateChangeInitiated(); mPhaseOffsets->setRefreshRateType(info.type); const auto [early, gl, late] = mPhaseOffsets->getCurrentOffsets(); mVsyncModulator.setPhaseOffsets(early, gl, late); @@ -975,7 +972,6 @@ void SurfaceFlinger::setActiveConfigInternal() { display->setActiveConfig(mUpcomingActiveConfig.configId); - mScheduler->resyncToHardwareVsync(true, getVsyncPeriod()); mPhaseOffsets->setRefreshRateType(mUpcomingActiveConfig.type); const auto [early, gl, late] = mPhaseOffsets->getCurrentOffsets(); mVsyncModulator.setPhaseOffsets(early, gl, late); @@ -987,6 +983,18 @@ void SurfaceFlinger::setActiveConfigInternal() { } } +void SurfaceFlinger::desiredActiveConfigChangeDone() { + std::lock_guard lock(mActiveConfigLock); + mDesiredActiveConfig.event = Scheduler::ConfigEvent::None; + mDesiredActiveConfigChanged = false; + ATRACE_INT("DesiredActiveConfigChanged", mDesiredActiveConfigChanged); + + mScheduler->resyncToHardwareVsync(true, getVsyncPeriod()); + mPhaseOffsets->setRefreshRateType(mUpcomingActiveConfig.type); + const auto [early, gl, late] = mPhaseOffsets->getCurrentOffsets(); + mVsyncModulator.setPhaseOffsets(early, gl, late); +} + bool SurfaceFlinger::performSetActiveConfig() { ATRACE_CALL(); if (mCheckPendingFence) { @@ -1016,14 +1024,7 @@ bool SurfaceFlinger::performSetActiveConfig() { if (!display || display->getActiveConfig() == desiredActiveConfig.configId) { // display is not valid or we are already in the requested mode // on both cases there is nothing left to do - std::lock_guard lock(mActiveConfigLock); - mDesiredActiveConfig.event = Scheduler::ConfigEvent::None; - mDesiredActiveConfigChanged = false; - // Update scheduler with the correct vsync period as a no-op. - // Otherwise, there exists a race condition where we get stuck in the - // incorrect vsync period. - mScheduler->resyncToHardwareVsync(false, getVsyncPeriod()); - ATRACE_INT("DesiredActiveConfigChanged", mDesiredActiveConfigChanged); + desiredActiveConfigChangeDone(); return false; } @@ -1031,15 +1032,7 @@ bool SurfaceFlinger::performSetActiveConfig() { // allowed configs might have change by the time we process the refresh. // Make sure the desired config is still allowed if (!isDisplayConfigAllowed(desiredActiveConfig.configId)) { - std::lock_guard lock(mActiveConfigLock); - mDesiredActiveConfig.event = Scheduler::ConfigEvent::None; - mDesiredActiveConfig.configId = display->getActiveConfig(); - mDesiredActiveConfigChanged = false; - // Update scheduler with the current vsync period as a no-op. - // Otherwise, there exists a race condition where we get stuck in the - // incorrect vsync period. - mScheduler->resyncToHardwareVsync(false, getVsyncPeriod()); - ATRACE_INT("DesiredActiveConfigChanged", mDesiredActiveConfigChanged); + desiredActiveConfigChangeDone(); return false; } mUpcomingActiveConfig = desiredActiveConfig; diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index ddfe88c928..74fefa7a3f 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -508,13 +508,15 @@ private: // Sets the desired active config bit. It obtains the lock, and sets mDesiredActiveConfig. void setDesiredActiveConfig(const ActiveConfigInfo& info) REQUIRES(mStateLock); // Once HWC has returned the present fence, this sets the active config and a new refresh - // rate in SF. It also triggers HWC vsync. + // rate in SF. void setActiveConfigInternal() REQUIRES(mStateLock); // Active config is updated on INVALIDATE call in a state machine-like manner. When the // desired config was set, HWC needs to update the panel on the next refresh, and when // we receive the fence back, we know that the process was complete. It returns whether // we need to wait for the next invalidate bool performSetActiveConfig() REQUIRES(mStateLock); + // Called when active config is no longer is progress + void desiredActiveConfigChangeDone() REQUIRES(mStateLock); // called on the main thread in response to setPowerMode() void setPowerModeInternal(const sp& display, int mode) REQUIRES(mStateLock); -- cgit v1.2.3-59-g8ed1b From 8444c3642b469a3ec8074bf2fabd2d22abd3f9b3 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Thu, 30 May 2019 17:59:36 -0700 Subject: SurfaceFlinger: do not force HDR content to default refresh rate Now that HWC supports HDR content at 90Hz there is no need to override from the platform side. Test: Youtube with HDR video Bug: 129694529 Change-Id: If59bab3d40c783843f3c1d2312a5768a8af2b921 --- services/surfaceflinger/Scheduler/Scheduler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h index f6cc87bb89..d311f62fdb 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.h +++ b/services/surfaceflinger/Scheduler/Scheduler.h @@ -297,7 +297,7 @@ private: const scheduler::RefreshRateConfigs& mRefreshRateConfigs; // Global config to force HDR content to work on DEFAULT refreshRate - static constexpr bool mForceHDRContentToDefaultRefreshRate = true; + static constexpr bool mForceHDRContentToDefaultRefreshRate = false; }; } // namespace android -- cgit v1.2.3-59-g8ed1b From 9ba25125f9ced03fc3f96a111d2987494d074d57 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Mon, 3 Jun 2019 17:10:55 -0700 Subject: SurfaceFlinger: HWVsync when display is off HWC expects that HWVsync will not be turned on if display if off. To ensure that we move enabling/disabling of HWVsync to the main thread and caching the latest request in case the display is off. When the display is turned on again we apply the latest state of HWVsync. Bug: 134050982 Test: Toggle screen on/off in a loop Change-Id: Ib87f885f95195b8bb402d5a5b2c75f20e213d2aa --- services/surfaceflinger/SurfaceFlinger.cpp | 26 +++++++++++++++++++++++--- services/surfaceflinger/SurfaceFlinger.h | 6 ++++++ 2 files changed, 29 insertions(+), 3 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index e735e10cc4..a543bc7e24 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1537,10 +1537,25 @@ void SurfaceFlinger::onRefreshReceived(int sequenceId, hwc2_display_t /*hwcDispl void SurfaceFlinger::setPrimaryVsyncEnabled(bool enabled) { ATRACE_CALL(); - Mutex::Autolock lock(mStateLock); + + // Enable / Disable HWVsync from the main thread to avoid race conditions with + // display power state. + postMessageAsync(new LambdaMessage( + [=]() NO_THREAD_SAFETY_ANALYSIS { setPrimaryVsyncEnabledInternal(enabled); })); +} + +void SurfaceFlinger::setPrimaryVsyncEnabledInternal(bool enabled) { + ATRACE_CALL(); + if (const auto displayId = getInternalDisplayIdLocked()) { - getHwComposer().setVsyncEnabled(*displayId, - enabled ? HWC2::Vsync::Enable : HWC2::Vsync::Disable); + sp display = getDefaultDisplayDeviceLocked(); + if (display && display->isPoweredOn()) { + getHwComposer().setVsyncEnabled(*displayId, + enabled ? HWC2::Vsync::Enable : HWC2::Vsync::Disable); + } else { + // Cache the latest vsync state and apply it when screen is on again + mEnableHWVsyncScreenOn = enabled; + } } } @@ -4438,6 +4453,11 @@ void SurfaceFlinger::setPowerModeInternal(const sp& display, int // Turn on the display getHwComposer().setPowerMode(*displayId, mode); if (display->isPrimary() && mode != HWC_POWER_MODE_DOZE_SUSPEND) { + if (mEnableHWVsyncScreenOn) { + setPrimaryVsyncEnabledInternal(mEnableHWVsyncScreenOn); + mEnableHWVsyncScreenOn = false; + } + mScheduler->onScreenAcquired(mAppConnectionHandle); mScheduler->resyncToHardwareVsync(true, getVsyncPeriod()); } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index ddfe88c928..02ee46acb8 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -293,6 +293,9 @@ public: // TODO: this should be made accessible only to EventThread void setPrimaryVsyncEnabled(bool enabled); + // main thread function to enable/disable h/w composer event + void setPrimaryVsyncEnabledInternal(bool enabled); + // called on the main thread by MessageQueue when an internal message // is received // TODO: this should be made accessible only to MessageQueue @@ -1166,6 +1169,9 @@ private: // The Layer pointer is removed from the set when the destructor is called so there shouldn't // be any issues with a raw pointer referencing an invalid object. std::unordered_set mOffscreenLayers; + + // Flag to indicate whether to re-enable HWVsync when screen is on + bool mEnableHWVsyncScreenOn = false; }; } // namespace android -- cgit v1.2.3-59-g8ed1b From 0f4a1b19009a1e71ed2100b63b04aa015a111b44 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Tue, 4 Jun 2019 16:04:04 -0700 Subject: SurfaceFlinger: add explicit register for DISPLAY_EVENT_CONFIG_CHANGED When display refresh rate changes, SF fires DISPLAY_EVENT_CONFIG_CHANGED thru DisplayEventReceiver. If the other end of the pipe doesn't consume the events it may lead to pipe full and dropping of events. Furthermore, The only clients interested in this event in DisplayManager and hwui. To avoid spamming all clients with this event, this change is adding an explicit register for DISPLAY_EVENT_CONFIG_CHANGED events. Bug: 131688378 Test: adb shell /data/nativetest64/libsurfaceflinger_unittest/libsurfaceflinger_unittest Test: trigger config change and observe logcat Change-Id: I5973a1ecc1f3e3ff8d8a0cba19db0e49ef0d5341 --- libs/gui/DisplayEventReceiver.cpp | 5 ++- libs/gui/ISurfaceComposer.cpp | 12 ++++-- libs/gui/include/gui/DisplayEventReceiver.h | 5 ++- libs/gui/include/gui/ISurfaceComposer.h | 5 ++- libs/gui/tests/Surface_test.cpp | 4 +- services/surfaceflinger/Scheduler/EventThread.cpp | 18 +++++++-- services/surfaceflinger/Scheduler/EventThread.h | 10 +++-- services/surfaceflinger/Scheduler/MessageQueue.cpp | 3 +- services/surfaceflinger/Scheduler/Scheduler.cpp | 15 +++++--- services/surfaceflinger/Scheduler/Scheduler.h | 8 ++-- services/surfaceflinger/SurfaceFlinger.cpp | 5 ++- services/surfaceflinger/SurfaceFlinger.h | 4 +- .../tests/unittests/EventThreadTest.cpp | 44 +++++++++++++++++----- .../tests/unittests/SchedulerTest.cpp | 18 ++++++--- .../tests/unittests/TestableScheduler.h | 4 +- .../tests/unittests/mock/MockEventThread.h | 3 +- 16 files changed, 116 insertions(+), 47 deletions(-) (limited to 'services/surfaceflinger') diff --git a/libs/gui/DisplayEventReceiver.cpp b/libs/gui/DisplayEventReceiver.cpp index f5cf1c4d5a..b8faa2df4c 100644 --- a/libs/gui/DisplayEventReceiver.cpp +++ b/libs/gui/DisplayEventReceiver.cpp @@ -32,10 +32,11 @@ namespace android { // --------------------------------------------------------------------------- -DisplayEventReceiver::DisplayEventReceiver(ISurfaceComposer::VsyncSource vsyncSource) { +DisplayEventReceiver::DisplayEventReceiver(ISurfaceComposer::VsyncSource vsyncSource, + ISurfaceComposer::ConfigChanged configChanged) { sp sf(ComposerService::getComposerService()); if (sf != nullptr) { - mEventConnection = sf->createDisplayEventConnection(vsyncSource); + mEventConnection = sf->createDisplayEventConnection(vsyncSource, configChanged); if (mEventConnection != nullptr) { mDataChannel = std::make_unique(); mEventConnection->stealReceiveChannel(mDataChannel.get()); diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp index 6c9d81ab57..e487792c87 100644 --- a/libs/gui/ISurfaceComposer.cpp +++ b/libs/gui/ISurfaceComposer.cpp @@ -278,8 +278,8 @@ public: return NO_ERROR; } - virtual sp createDisplayEventConnection(VsyncSource vsyncSource) - { + virtual sp createDisplayEventConnection(VsyncSource vsyncSource, + ConfigChanged configChanged) { Parcel data, reply; sp result; int err = data.writeInterfaceToken( @@ -288,6 +288,7 @@ public: return result; } data.writeInt32(static_cast(vsyncSource)); + data.writeInt32(static_cast(configChanged)); err = remote()->transact( BnSurfaceComposer::CREATE_DISPLAY_EVENT_CONNECTION, data, &reply); @@ -1155,8 +1156,11 @@ status_t BnSurfaceComposer::onTransact( } case CREATE_DISPLAY_EVENT_CONNECTION: { CHECK_INTERFACE(ISurfaceComposer, data, reply); - sp connection(createDisplayEventConnection( - static_cast(data.readInt32()))); + auto vsyncSource = static_cast(data.readInt32()); + auto configChanged = static_cast(data.readInt32()); + + sp connection( + createDisplayEventConnection(vsyncSource, configChanged)); reply->writeStrongBinder(IInterface::asBinder(connection)); return NO_ERROR; } diff --git a/libs/gui/include/gui/DisplayEventReceiver.h b/libs/gui/include/gui/DisplayEventReceiver.h index 22de751498..a558cf9e18 100644 --- a/libs/gui/include/gui/DisplayEventReceiver.h +++ b/libs/gui/include/gui/DisplayEventReceiver.h @@ -88,10 +88,13 @@ public: * DisplayEventReceiver creates and registers an event connection with * SurfaceFlinger. VSync events are disabled by default. Call setVSyncRate * or requestNextVsync to receive them. + * To receive Config Changed events specify this in the constructor. * Other events start being delivered immediately. */ explicit DisplayEventReceiver( - ISurfaceComposer::VsyncSource vsyncSource = ISurfaceComposer::eVsyncSourceApp); + ISurfaceComposer::VsyncSource vsyncSource = ISurfaceComposer::eVsyncSourceApp, + ISurfaceComposer::ConfigChanged configChanged = + ISurfaceComposer::eConfigChangedSuppress); /* * ~DisplayEventReceiver severs the connection with SurfaceFlinger, new events diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index e2f77365b3..c84910b6ec 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -90,6 +90,8 @@ public: eVsyncSourceSurfaceFlinger = 1 }; + enum ConfigChanged { eConfigChangedSuppress = 0, eConfigChangedDispatch = 1 }; + /* * Create a connection with SurfaceFlinger. */ @@ -97,7 +99,8 @@ public: /* return an IDisplayEventConnection */ virtual sp createDisplayEventConnection( - VsyncSource vsyncSource = eVsyncSourceApp) = 0; + VsyncSource vsyncSource = eVsyncSourceApp, + ConfigChanged configChanged = eConfigChangedSuppress) = 0; /* create a virtual display * requires ACCESS_SURFACE_FLINGER permission. diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 960cf1846e..d3708586f5 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -548,8 +548,8 @@ public: } sp createConnection() override { return nullptr; } - sp createDisplayEventConnection(ISurfaceComposer::VsyncSource) - override { + sp createDisplayEventConnection( + ISurfaceComposer::VsyncSource, ISurfaceComposer::ConfigChanged) override { return nullptr; } sp createDisplay(const String8& /*displayName*/, diff --git a/services/surfaceflinger/Scheduler/EventThread.cpp b/services/surfaceflinger/Scheduler/EventThread.cpp index 05bad4ddd8..9d1f777859 100644 --- a/services/surfaceflinger/Scheduler/EventThread.cpp +++ b/services/surfaceflinger/Scheduler/EventThread.cpp @@ -76,6 +76,10 @@ std::string toString(const DisplayEventReceiver::Event& event) { return StringPrintf("VSync{displayId=%" ANDROID_PHYSICAL_DISPLAY_ID_FORMAT ", count=%u}", event.header.displayId, event.vsync.count); + case DisplayEventReceiver::DISPLAY_EVENT_CONFIG_CHANGED: + return StringPrintf("ConfigChanged{displayId=%" ANDROID_PHYSICAL_DISPLAY_ID_FORMAT + ", configId=%u}", + event.header.displayId, event.config.configId); default: return "Event{}"; } @@ -107,8 +111,10 @@ DisplayEventReceiver::Event makeConfigChanged(PhysicalDisplayId displayId, int32 } // namespace EventThreadConnection::EventThreadConnection(EventThread* eventThread, - ResyncCallback resyncCallback) + ResyncCallback resyncCallback, + ISurfaceComposer::ConfigChanged configChanged) : resyncCallback(std::move(resyncCallback)), + configChanged(configChanged), mEventThread(eventThread), mChannel(gui::BitTube::DefaultSize) {} @@ -203,8 +209,10 @@ void EventThread::setPhaseOffset(nsecs_t phaseOffset) { mVSyncSource->setPhaseOffset(phaseOffset); } -sp EventThread::createEventConnection(ResyncCallback resyncCallback) const { - return new EventThreadConnection(const_cast(this), std::move(resyncCallback)); +sp EventThread::createEventConnection( + ResyncCallback resyncCallback, ISurfaceComposer::ConfigChanged configChanged) const { + return new EventThreadConnection(const_cast(this), std::move(resyncCallback), + configChanged); } status_t EventThread::registerDisplayEventConnection(const sp& connection) { @@ -398,9 +406,11 @@ bool EventThread::shouldConsumeEvent(const DisplayEventReceiver::Event& event, const sp& connection) const { switch (event.header.type) { case DisplayEventReceiver::DISPLAY_EVENT_HOTPLUG: - case DisplayEventReceiver::DISPLAY_EVENT_CONFIG_CHANGED: return true; + case DisplayEventReceiver::DISPLAY_EVENT_CONFIG_CHANGED: + return connection->configChanged == ISurfaceComposer::eConfigChangedDispatch; + case DisplayEventReceiver::DISPLAY_EVENT_VSYNC: switch (connection->vsyncRequest) { case VSyncRequest::None: diff --git a/services/surfaceflinger/Scheduler/EventThread.h b/services/surfaceflinger/Scheduler/EventThread.h index 61530c62e5..dd23b88726 100644 --- a/services/surfaceflinger/Scheduler/EventThread.h +++ b/services/surfaceflinger/Scheduler/EventThread.h @@ -69,7 +69,8 @@ public: class EventThreadConnection : public BnDisplayEventConnection { public: - EventThreadConnection(EventThread*, ResyncCallback); + EventThreadConnection(EventThread*, ResyncCallback, + ISurfaceComposer::ConfigChanged configChanged); virtual ~EventThreadConnection(); virtual status_t postEvent(const DisplayEventReceiver::Event& event); @@ -82,6 +83,7 @@ public: const ResyncCallback resyncCallback; VSyncRequest vsyncRequest = VSyncRequest::None; + const ISurfaceComposer::ConfigChanged configChanged; private: virtual void onFirstRef(); @@ -93,7 +95,8 @@ class EventThread { public: virtual ~EventThread(); - virtual sp createEventConnection(ResyncCallback) const = 0; + virtual sp createEventConnection( + ResyncCallback, ISurfaceComposer::ConfigChanged configChanged) const = 0; // called before the screen is turned off from main thread virtual void onScreenReleased() = 0; @@ -128,7 +131,8 @@ public: EventThread(std::unique_ptr, InterceptVSyncsCallback, const char* threadName); ~EventThread(); - sp createEventConnection(ResyncCallback) const override; + sp createEventConnection( + ResyncCallback, ISurfaceComposer::ConfigChanged configChanged) const override; status_t registerDisplayEventConnection(const sp& connection) override; void setVsyncRate(uint32_t rate, const sp& connection) override; diff --git a/services/surfaceflinger/Scheduler/MessageQueue.cpp b/services/surfaceflinger/Scheduler/MessageQueue.cpp index baf900df52..fcb307f213 100644 --- a/services/surfaceflinger/Scheduler/MessageQueue.cpp +++ b/services/surfaceflinger/Scheduler/MessageQueue.cpp @@ -96,7 +96,8 @@ void MessageQueue::setEventThread(android::EventThread* eventThread, } mEventThread = eventThread; - mEvents = eventThread->createEventConnection(std::move(resyncCallback)); + mEvents = eventThread->createEventConnection(std::move(resyncCallback), + ISurfaceComposer::eConfigChangedSuppress); mEvents->stealReceiveChannel(&mEventTube); mLooper->addFd(mEventTube.getFd(), 0, Looper::EVENT_INPUT, MessageQueue::cb_eventReceiver, this); diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index e2a348e146..afcf3d45ba 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -129,7 +129,8 @@ sp Scheduler::createConnection( std::move(interceptCallback)); auto eventThreadConnection = - createConnectionInternal(eventThread.get(), std::move(resyncCallback)); + createConnectionInternal(eventThread.get(), std::move(resyncCallback), + ISurfaceComposer::eConfigChangedSuppress); mConnections.emplace(id, std::make_unique(new ConnectionHandle(id), eventThreadConnection, @@ -146,16 +147,18 @@ std::unique_ptr Scheduler::makeEventThread( std::move(interceptCallback), connectionName); } -sp Scheduler::createConnectionInternal(EventThread* eventThread, - ResyncCallback&& resyncCallback) { - return eventThread->createEventConnection(std::move(resyncCallback)); +sp Scheduler::createConnectionInternal( + EventThread* eventThread, ResyncCallback&& resyncCallback, + ISurfaceComposer::ConfigChanged configChanged) { + return eventThread->createEventConnection(std::move(resyncCallback), configChanged); } sp Scheduler::createDisplayEventConnection( - const sp& handle, ResyncCallback resyncCallback) { + const sp& handle, ResyncCallback resyncCallback, + ISurfaceComposer::ConfigChanged configChanged) { RETURN_VALUE_IF_INVALID(nullptr); return createConnectionInternal(mConnections[handle->id]->thread.get(), - std::move(resyncCallback)); + std::move(resyncCallback), configChanged); } EventThread* Scheduler::getEventThread(const sp& handle) { diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h index f6cc87bb89..6b5a8b715d 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.h +++ b/services/surfaceflinger/Scheduler/Scheduler.h @@ -100,8 +100,9 @@ public: ResyncCallback, impl::EventThread::InterceptVSyncsCallback); - sp createDisplayEventConnection(const sp& handle, - ResyncCallback); + sp createDisplayEventConnection( + const sp& handle, ResyncCallback, + ISurfaceComposer::ConfigChanged configChanged); // Getter methods. EventThread* getEventThread(const sp& handle); @@ -197,7 +198,8 @@ private: enum class TouchState { INACTIVE, ACTIVE }; // Creates a connection on the given EventThread and forwards the given callbacks. - sp createConnectionInternal(EventThread*, ResyncCallback&&); + sp createConnectionInternal(EventThread*, ResyncCallback&&, + ISurfaceComposer::ConfigChanged); nsecs_t calculateAverage() const; void updateFrameSkipping(const int64_t skipCount); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index e735e10cc4..114eca3123 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1379,7 +1379,7 @@ status_t SurfaceFlinger::notifyPowerHint(int32_t hintId) { // ---------------------------------------------------------------------------- sp SurfaceFlinger::createDisplayEventConnection( - ISurfaceComposer::VsyncSource vsyncSource) { + ISurfaceComposer::VsyncSource vsyncSource, ISurfaceComposer::ConfigChanged configChanged) { auto resyncCallback = mScheduler->makeResyncCallback([this] { Mutex::Autolock lock(mStateLock); return getVsyncPeriod(); @@ -1388,7 +1388,8 @@ sp SurfaceFlinger::createDisplayEventConnection( const auto& handle = vsyncSource == eVsyncSourceSurfaceFlinger ? mSfConnectionHandle : mAppConnectionHandle; - return mScheduler->createDisplayEventConnection(handle, std::move(resyncCallback)); + return mScheduler->createDisplayEventConnection(handle, std::move(resyncCallback), + configChanged); } // ---------------------------------------------------------------------------- diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index ddfe88c928..9577124253 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -399,7 +399,9 @@ private: const sp& bufferProducer) const override; status_t getSupportedFrameTimestamps(std::vector* outSupported) const override; sp createDisplayEventConnection( - ISurfaceComposer::VsyncSource vsyncSource = eVsyncSourceApp) override; + ISurfaceComposer::VsyncSource vsyncSource = eVsyncSourceApp, + ISurfaceComposer::ConfigChanged configChanged = + ISurfaceComposer::eConfigChangedSuppress) override; status_t captureScreen(const sp& displayToken, sp* outBuffer, bool& outCapturedSecureLayers, const ui::Dataspace reqDataspace, const ui::PixelFormat reqPixelFormat, Rect sourceCrop, diff --git a/services/surfaceflinger/tests/unittests/EventThreadTest.cpp b/services/surfaceflinger/tests/unittests/EventThreadTest.cpp index ea908a9018..dbd9b84039 100644 --- a/services/surfaceflinger/tests/unittests/EventThreadTest.cpp +++ b/services/surfaceflinger/tests/unittests/EventThreadTest.cpp @@ -55,8 +55,9 @@ protected: class MockEventThreadConnection : public EventThreadConnection { public: MockEventThreadConnection(android::impl::EventThread* eventThread, - ResyncCallback&& resyncCallback) - : EventThreadConnection(eventThread, std::move(resyncCallback)) {} + ResyncCallback&& resyncCallback, + ISurfaceComposer::ConfigChanged configChanged) + : EventThreadConnection(eventThread, std::move(resyncCallback), configChanged) {} MOCK_METHOD1(postEvent, status_t(const DisplayEventReceiver::Event& event)); }; @@ -67,7 +68,8 @@ protected: ~EventThreadTest() override; void createThread(); - sp createConnection(ConnectionEventRecorder& recorder); + sp createConnection(ConnectionEventRecorder& recorder, + ISurfaceComposer::ConfigChanged configChanged); void expectVSyncSetEnabledCallReceived(bool expectedState); void expectVSyncSetPhaseOffsetCallReceived(nsecs_t expectedPhaseOffset); @@ -110,7 +112,8 @@ EventThreadTest::EventThreadTest() { .WillRepeatedly(Invoke(mVSyncSetPhaseOffsetCallRecorder.getInvocable())); createThread(); - mConnection = createConnection(mConnectionEventCallRecorder); + mConnection = createConnection(mConnectionEventCallRecorder, + ISurfaceComposer::eConfigChangedDispatch); // A display must be connected for VSYNC events to be delivered. mThread->onHotplugReceived(INTERNAL_DISPLAY_ID, true); @@ -138,9 +141,10 @@ void EventThreadTest::createThread() { } sp EventThreadTest::createConnection( - ConnectionEventRecorder& recorder) { + ConnectionEventRecorder& recorder, ISurfaceComposer::ConfigChanged configChanged) { sp connection = - new MockEventThreadConnection(mThread.get(), mResyncCallRecorder.getInvocable()); + new MockEventThreadConnection(mThread.get(), mResyncCallRecorder.getInvocable(), + configChanged); EXPECT_CALL(*connection, postEvent(_)).WillRepeatedly(Invoke(recorder.getInvocable())); return connection; } @@ -267,7 +271,9 @@ TEST_F(EventThreadTest, requestNextVsyncPostsASingleVSyncEventToTheConnection) { TEST_F(EventThreadTest, setVsyncRateZeroPostsNoVSyncEventsToThatConnection) { // Create a first connection, register it, and request a vsync rate of zero. ConnectionEventRecorder firstConnectionEventRecorder{0}; - sp firstConnection = createConnection(firstConnectionEventRecorder); + sp firstConnection = + createConnection(firstConnectionEventRecorder, + ISurfaceComposer::eConfigChangedSuppress); mThread->setVsyncRate(0, firstConnection); // By itself, this should not enable vsync events @@ -277,7 +283,8 @@ TEST_F(EventThreadTest, setVsyncRateZeroPostsNoVSyncEventsToThatConnection) { // However if there is another connection which wants events at a nonzero rate..... ConnectionEventRecorder secondConnectionEventRecorder{0}; sp secondConnection = - createConnection(secondConnectionEventRecorder); + createConnection(secondConnectionEventRecorder, + ISurfaceComposer::eConfigChangedSuppress); mThread->setVsyncRate(1, secondConnection); // EventThread should enable vsync callbacks. @@ -363,7 +370,9 @@ TEST_F(EventThreadTest, connectionsRemovedIfInstanceDestroyed) { TEST_F(EventThreadTest, connectionsRemovedIfEventDeliveryError) { ConnectionEventRecorder errorConnectionEventRecorder{NO_MEMORY}; - sp errorConnection = createConnection(errorConnectionEventRecorder); + sp errorConnection = + createConnection(errorConnectionEventRecorder, + ISurfaceComposer::eConfigChangedSuppress); mThread->setVsyncRate(1, errorConnection); // EventThread should enable vsync callbacks. @@ -387,7 +396,9 @@ TEST_F(EventThreadTest, connectionsRemovedIfEventDeliveryError) { TEST_F(EventThreadTest, eventsDroppedIfNonfatalEventDeliveryError) { ConnectionEventRecorder errorConnectionEventRecorder{WOULD_BLOCK}; - sp errorConnection = createConnection(errorConnectionEventRecorder); + sp errorConnection = + createConnection(errorConnectionEventRecorder, + ISurfaceComposer::eConfigChangedSuppress); mThread->setVsyncRate(1, errorConnection); // EventThread should enable vsync callbacks. @@ -449,5 +460,18 @@ TEST_F(EventThreadTest, postConfigChangedPrimary64bit) { expectConfigChangedEventReceivedByConnection(DISPLAY_ID_64BIT, 7); } +TEST_F(EventThreadTest, suppressConfigChanged) { + ConnectionEventRecorder suppressConnectionEventRecorder{0}; + sp suppressConnection = + createConnection(suppressConnectionEventRecorder, + ISurfaceComposer::eConfigChangedSuppress); + + mThread->onConfigChanged(INTERNAL_DISPLAY_ID, 9); + expectConfigChangedEventReceivedByConnection(INTERNAL_DISPLAY_ID, 9); + + auto args = suppressConnectionEventRecorder.waitForCall(); + ASSERT_FALSE(args.has_value()); +} + } // namespace } // namespace android diff --git a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp index 1f8b11180b..af5ccbca21 100644 --- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp +++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp @@ -25,7 +25,8 @@ protected: class MockEventThreadConnection : public android::EventThreadConnection { public: explicit MockEventThreadConnection(EventThread* eventThread) - : EventThreadConnection(eventThread, ResyncCallback()) {} + : EventThreadConnection(eventThread, ResyncCallback(), + ISurfaceComposer::eConfigChangedSuppress) {} ~MockEventThreadConnection() = default; MOCK_METHOD1(stealReceiveChannel, status_t(gui::BitTube* outChannel)); @@ -81,7 +82,7 @@ SchedulerTest::SchedulerTest() { // createConnection call to scheduler makes a createEventConnection call to EventThread. Make // sure that call gets executed and returns an EventThread::Connection object. - EXPECT_CALL(*mEventThread, createEventConnection(_)) + EXPECT_CALL(*mEventThread, createEventConnection(_, _)) .WillRepeatedly(Return(mEventThreadConnection)); mConnectionHandle = mScheduler->createConnection("appConnection", 16, ResyncCallback(), @@ -105,7 +106,10 @@ TEST_F(SchedulerTest, testNullPtr) { // exceptions, just gracefully continues. sp returnedValue; ASSERT_NO_FATAL_FAILURE( - returnedValue = mScheduler->createDisplayEventConnection(nullptr, ResyncCallback())); + returnedValue = + mScheduler->createDisplayEventConnection(nullptr, ResyncCallback(), + ISurfaceComposer:: + eConfigChangedSuppress)); EXPECT_TRUE(returnedValue == nullptr); EXPECT_TRUE(mScheduler->getEventThread(nullptr) == nullptr); EXPECT_TRUE(mScheduler->getEventConnection(nullptr) == nullptr); @@ -126,7 +130,9 @@ TEST_F(SchedulerTest, invalidConnectionHandle) { sp returnedValue; ASSERT_NO_FATAL_FAILURE( returnedValue = - mScheduler->createDisplayEventConnection(connectionHandle, ResyncCallback())); + mScheduler->createDisplayEventConnection(connectionHandle, ResyncCallback(), + ISurfaceComposer:: + eConfigChangedSuppress)); EXPECT_TRUE(returnedValue == nullptr); EXPECT_TRUE(mScheduler->getEventThread(connectionHandle) == nullptr); EXPECT_TRUE(mScheduler->getEventConnection(connectionHandle) == nullptr); @@ -155,7 +161,9 @@ TEST_F(SchedulerTest, validConnectionHandle) { sp returnedValue; ASSERT_NO_FATAL_FAILURE( returnedValue = - mScheduler->createDisplayEventConnection(mConnectionHandle, ResyncCallback())); + mScheduler->createDisplayEventConnection(mConnectionHandle, ResyncCallback(), + ISurfaceComposer:: + eConfigChangedSuppress)); EXPECT_TRUE(returnedValue != nullptr); ASSERT_EQ(returnedValue, mEventThreadConnection); diff --git a/services/surfaceflinger/tests/unittests/TestableScheduler.h b/services/surfaceflinger/tests/unittests/TestableScheduler.h index c3d2b8de4f..cb6980ed1a 100644 --- a/services/surfaceflinger/tests/unittests/TestableScheduler.h +++ b/services/surfaceflinger/tests/unittests/TestableScheduler.h @@ -17,6 +17,7 @@ #pragma once #include +#include #include "Scheduler/EventThread.h" #include "Scheduler/RefreshRateConfigs.h" @@ -34,7 +35,8 @@ public: // Scheduler::Connection. This allows plugging in mock::EventThread. sp addConnection(std::unique_ptr eventThread) { sp eventThreadConnection = - new EventThreadConnection(eventThread.get(), ResyncCallback()); + new EventThreadConnection(eventThread.get(), ResyncCallback(), + ISurfaceComposer::eConfigChangedSuppress); const int64_t id = sNextId++; mConnections.emplace(id, std::make_unique(new ConnectionHandle(id), diff --git a/services/surfaceflinger/tests/unittests/mock/MockEventThread.h b/services/surfaceflinger/tests/unittests/mock/MockEventThread.h index 5b5f8e7fab..ed35ebf2b6 100644 --- a/services/surfaceflinger/tests/unittests/mock/MockEventThread.h +++ b/services/surfaceflinger/tests/unittests/mock/MockEventThread.h @@ -28,7 +28,8 @@ public: EventThread(); ~EventThread() override; - MOCK_CONST_METHOD1(createEventConnection, sp(ResyncCallback)); + MOCK_CONST_METHOD2(createEventConnection, + sp(ResyncCallback, ISurfaceComposer::ConfigChanged)); MOCK_METHOD0(onScreenReleased, void()); MOCK_METHOD0(onScreenAcquired, void()); MOCK_METHOD2(onHotplugReceived, void(PhysicalDisplayId, bool)); -- cgit v1.2.3-59-g8ed1b From aa61419b69daed4794c593f0718b3330ee2ec8dc Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 6 Jun 2019 13:28:34 -0700 Subject: [SurfaceFlinger] correct present time for negative phase offsets DispSync::expectedPresentTime returns the expected presentation time for the current frame, but when we're in negative offsets we are targetting the following frame instead. Bug: 133241520 Bug: 134589085 Test: systrace when flinging through news Change-Id: I7cc05a0b9e8e9b5c3e8d0c4b1d59b0a7dabd43d4 --- services/surfaceflinger/BufferQueueLayer.cpp | 2 +- services/surfaceflinger/BufferStateLayer.cpp | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 15 +++++++++++++-- services/surfaceflinger/SurfaceFlinger.h | 5 +++++ 4 files changed, 20 insertions(+), 4 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp index bd0b55f688..57f1008e85 100644 --- a/services/surfaceflinger/BufferQueueLayer.cpp +++ b/services/surfaceflinger/BufferQueueLayer.cpp @@ -143,7 +143,7 @@ bool BufferQueueLayer::framePresentTimeIsCurrent() const { } Mutex::Autolock lock(mQueueItemLock); - return mQueueItems[0].mTimestamp <= mFlinger->mScheduler->expectedPresentTime(); + return mQueueItems[0].mTimestamp <= mFlinger->getExpectedPresentTime(); } nsecs_t BufferQueueLayer::getDesiredPresentTime() { diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index 05c721f141..203bd72e6f 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -380,7 +380,7 @@ bool BufferStateLayer::framePresentTimeIsCurrent() const { return true; } - return mDesiredPresentTime <= mFlinger->mScheduler->expectedPresentTime(); + return mDesiredPresentTime <= mFlinger->getExpectedPresentTime(); } nsecs_t BufferStateLayer::getDesiredPresentTime() { diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 50fabe3c43..dadb3fed05 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1643,6 +1643,18 @@ bool SurfaceFlinger::previousFrameMissed() NO_THREAD_SAFETY_ANALYSIS { return fence != Fence::NO_FENCE && (fence->getStatus() == Fence::Status::Unsignaled); } +nsecs_t SurfaceFlinger::getExpectedPresentTime() NO_THREAD_SAFETY_ANALYSIS { + DisplayStatInfo stats; + mScheduler->getDisplayStatInfo(&stats); + const nsecs_t presentTime = mScheduler->expectedPresentTime(); + // Inflate the expected present time if we're targetting the next vsync. + const nsecs_t correctedTime = + mVsyncModulator.getOffsets().sf < mPhaseOffsets->getOffsetThresholdForNextVsync() + ? presentTime + : presentTime + stats.vsyncPeriod; + return correctedTime; +} + void SurfaceFlinger::onMessageReceived(int32_t what) NO_THREAD_SAFETY_ANALYSIS { ATRACE_CALL(); switch (what) { @@ -3244,8 +3256,7 @@ bool SurfaceFlinger::handlePageFlip() mDrawingState.traverseInZOrder([&](Layer* layer) { if (layer->hasReadyFrame()) { frameQueued = true; - nsecs_t expectedPresentTime; - expectedPresentTime = mScheduler->expectedPresentTime(); + const nsecs_t expectedPresentTime = getExpectedPresentTime(); if (layer->shouldPresentNow(expectedPresentTime)) { mLayersWithQueuedFrames.push_back(layer); } else { diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 74fefa7a3f..3d98ec19a5 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -298,6 +298,11 @@ public: // TODO: this should be made accessible only to MessageQueue void onMessageReceived(int32_t what); + // Returns the expected present time for this frame. + // When we are in negative offsets, we perform a correction so that the + // predicted vsync for the *next* frame is used instead. + nsecs_t getExpectedPresentTime(); + // for debugging only // TODO: this should be made accessible only to HWComposer const Vector>& getLayerSortedByZForHwcDisplay(DisplayId displayId); -- cgit v1.2.3-59-g8ed1b From 1c8d7209992582fcfef020d75c990565dee1c71d Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Sat, 1 Jun 2019 18:51:35 -0700 Subject: [SurfaceFlinger] support EGLImage management in BLAST This mirrors a performance optimization for BufferQueueLayers where EGLImages were allocated in onFrameAvailable. Here when buffers are passed over to SurfaceFlinger in a transaction, an EGLImage is also created for that buffer. This is critical for reducing jank when operating in higher refresh rates, as eglCreateImageKHR can take long enough for frames to miss. Bug: 133627730 Test: systrace of chrome in landscope orientation caches properly Change-Id: I2022564fbecace7cadd00c89abdcc358d6323315 --- services/surfaceflinger/BufferStateLayer.cpp | 15 ++++----------- services/surfaceflinger/BufferStateLayer.h | 4 +++- services/surfaceflinger/Layer.h | 16 +++++++++------- services/surfaceflinger/SurfaceFlinger.cpp | 3 +++ 4 files changed, 19 insertions(+), 19 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index 05c721f141..bcc9915099 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -50,12 +50,6 @@ BufferStateLayer::BufferStateLayer(const LayerCreationArgs& args) mOverrideScalingMode = NATIVE_WINDOW_SCALING_MODE_SCALE_TO_WINDOW; mCurrentState.dataspace = ui::Dataspace::V0_SRGB; } -BufferStateLayer::~BufferStateLayer() { - if (mActiveBuffer != nullptr) { - auto& engine(mFlinger->getRenderEngine()); - engine.unbindExternalTextureBuffer(mActiveBuffer->getId()); - } -} // ----------------------------------------------------------------------- // Interface implementation for Layer @@ -571,11 +565,6 @@ status_t BufferStateLayer::updateActiveBuffer() { return BAD_VALUE; } - if (mActiveBuffer != nullptr) { - // todo: get this to work with BufferStateLayerCache - auto& engine(mFlinger->getRenderEngine()); - engine.unbindExternalTextureBuffer(mActiveBuffer->getId()); - } mActiveBuffer = s.buffer; mActiveBufferFence = s.acquireFence; auto& layerCompositionState = getCompositionLayer()->editState().frontEnd; @@ -621,6 +610,10 @@ void BufferStateLayer::onFirstRef() { } } +void BufferStateLayer::bufferErased(const client_cache_t& clientCacheId) { + mFlinger->getRenderEngine().unbindExternalTextureBuffer(clientCacheId.id); +} + void BufferStateLayer::HwcSlotGenerator::bufferErased(const client_cache_t& clientCacheId) { std::lock_guard lock(mMutex); if (!clientCacheId.isValid()) { diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h index 4e2bc45287..db8ae0d337 100644 --- a/services/surfaceflinger/BufferStateLayer.h +++ b/services/surfaceflinger/BufferStateLayer.h @@ -34,7 +34,6 @@ class SlotGenerationTest; class BufferStateLayer : public BufferLayer { public: explicit BufferStateLayer(const LayerCreationArgs&); - ~BufferStateLayer() override; // ----------------------------------------------------------------------- // Interface implementation for Layer @@ -103,6 +102,9 @@ public: bool fenceHasSignaled() const override; bool framePresentTimeIsCurrent() const override; + // Inherit from ClientCache::ErasedRecipient + void bufferErased(const client_cache_t& clientCacheId) override; + private: nsecs_t getDesiredPresentTime() override; std::shared_ptr getCurrentFenceTime() const override; diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 3712b2aff9..7aecc90ea3 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -17,8 +17,6 @@ #ifndef ANDROID_LAYER_H #define ANDROID_LAYER_H -#include - #include #include #include @@ -28,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -44,16 +43,16 @@ #include #include "Client.h" +#include "ClientCache.h" +#include "DisplayHardware/ComposerHal.h" +#include "DisplayHardware/HWComposer.h" #include "FrameTracker.h" #include "LayerVector.h" #include "MonitoredProducer.h" +#include "RenderArea.h" #include "SurfaceFlinger.h" #include "TransactionCompletedThread.h" -#include "DisplayHardware/ComposerHal.h" -#include "DisplayHardware/HWComposer.h" -#include "RenderArea.h" - using namespace android::surfaceflinger; namespace android { @@ -94,7 +93,7 @@ struct LayerCreationArgs { LayerMetadata metadata; }; -class Layer : public virtual compositionengine::LayerFE { +class Layer : public virtual compositionengine::LayerFE, public ClientCache::ErasedRecipient { static std::atomic sSequence; public: @@ -690,6 +689,9 @@ public: compositionengine::OutputLayer* findOutputLayerForDisplay( const sp& display) const; + // Inherit from ClientCache::ErasedRecipient + void bufferErased(const client_cache_t& /*clientCacheId*/) override {} + protected: // constant sp mFlinger; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 50fabe3c43..278191079f 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -4122,6 +4122,9 @@ uint32_t SurfaceFlinger::setClientStateLocked( sp buffer; if (bufferChanged && cacheIdChanged) { ClientCache::getInstance().add(s.cachedBuffer, s.buffer); + ClientCache::getInstance().registerErasedRecipient(s.cachedBuffer, + wp(layer)); + getRenderEngine().cacheExternalTextureBuffer(s.buffer); buffer = s.buffer; } else if (cacheIdChanged) { buffer = ClientCache::getInstance().get(s.cachedBuffer); -- cgit v1.2.3-59-g8ed1b From ae211b448ade43c60500dcb2fbd0108ddd4ce315 Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Fri, 7 Jun 2019 17:22:49 -0700 Subject: Surfacefinger: Set HWC Callback to SCHED_FIFO IComposerCallback is on critical path to correct vsync, this CL makes the callback runs in SCHED_FIFO to minimize scheduling latency. Bug: 133241520 Test: Build Change-Id: Ifda73c4b33771763c2b58b25d32b805ed602369a --- services/surfaceflinger/DisplayHardware/ComposerHal.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/DisplayHardware/ComposerHal.cpp b/services/surfaceflinger/DisplayHardware/ComposerHal.cpp index cc5a5b5729..7f47a2ecd4 100644 --- a/services/surfaceflinger/DisplayHardware/ComposerHal.cpp +++ b/services/surfaceflinger/DisplayHardware/ComposerHal.cpp @@ -24,6 +24,7 @@ #include #include +#include #include namespace android { @@ -229,6 +230,7 @@ std::string Composer::dumpDebugInfo() void Composer::registerCallback(const sp& callback) { + android::hardware::setMinSchedulerPolicy(callback, SCHED_FIFO, 2); auto ret = mClient->registerCallback(callback); if (!ret.isOk()) { ALOGE("failed to register IComposerCallback"); -- cgit v1.2.3-59-g8ed1b From 45e4e3634e94171bca7e3df287bcbcb74df6ca4a Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Fri, 7 Jun 2019 18:20:51 -0700 Subject: SurfaceFlinger: Some fixes to DispSync Pass negative offsets to DispSync to fix the scheduling when SurfaceFlinger uses negative offsets. Change-Id: I1f9544b064305c87f973120cc1bc59a0268b78e5 Bug: 132284303 Test: UI-Bench --- services/surfaceflinger/Scheduler/DispSync.cpp | 28 +++++++--------------- .../surfaceflinger/Scheduler/DispSyncSource.cpp | 22 +++++++++-------- services/surfaceflinger/Scheduler/DispSyncSource.h | 4 +++- services/surfaceflinger/Scheduler/Scheduler.cpp | 11 +++++---- services/surfaceflinger/Scheduler/Scheduler.h | 7 +++--- services/surfaceflinger/Scheduler/VSyncModulator.h | 5 +++- services/surfaceflinger/SurfaceFlinger.cpp | 25 ++++++++++++------- .../tests/unittests/DispSyncSourceTest.cpp | 4 +++- .../tests/unittests/SchedulerTest.cpp | 4 ++-- 9 files changed, 59 insertions(+), 51 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/DispSync.cpp b/services/surfaceflinger/Scheduler/DispSync.cpp index 95ff9d0c73..e59d459242 100644 --- a/services/surfaceflinger/Scheduler/DispSync.cpp +++ b/services/surfaceflinger/Scheduler/DispSync.cpp @@ -92,7 +92,6 @@ public: mPeriod = period; if (!mModelLocked && referenceTimeChanged) { for (auto& eventListener : mEventListeners) { - eventListener.mHasFired = false; eventListener.mLastEventTime = mReferenceTime - mPeriod + mPhase + eventListener.mPhase; } @@ -123,13 +122,6 @@ public: void unlockModel() { Mutex::Autolock lock(mMutex); - if (mModelLocked) { - for (auto& eventListener : mEventListeners) { - if (eventListener.mLastEventTime > mReferenceTime) { - eventListener.mHasFired = true; - } - } - } mModelLocked = false; ATRACE_INT("DispSync:ModelLocked", mModelLocked); } @@ -259,10 +251,6 @@ public: listener.mLastCallbackTime = lastCallbackTime; } - if (!mModelLocked && listener.mLastEventTime > mReferenceTime) { - listener.mHasFired = true; - } - mEventListeners.push_back(listener); mCond.signal(); @@ -305,7 +293,14 @@ public: } else if (diff < -mPeriod / 2) { diff += mPeriod; } + + if (phase < 0 && oldPhase > 0) { + diff += mPeriod; + } else if (phase > 0 && oldPhase < 0) { + diff -= mPeriod; + } eventListener.mLastEventTime -= diff; + eventListener.mLastCallbackTime -= diff; mCond.signal(); return NO_ERROR; } @@ -320,7 +315,6 @@ private: nsecs_t mLastEventTime; nsecs_t mLastCallbackTime; DispSync::Callback* mCallback; - bool mHasFired = false; }; struct CallbackInvocation { @@ -368,12 +362,7 @@ private: eventListener.mName); continue; } - if (eventListener.mHasFired && !mModelLocked) { - eventListener.mLastEventTime = t; - ALOGV("[%s] [%s] Skipping event due to already firing", mName, - eventListener.mName); - continue; - } + CallbackInvocation ci; ci.mCallback = eventListener.mCallback; ci.mEventTime = t; @@ -382,7 +371,6 @@ private: callbackInvocations.push_back(ci); eventListener.mLastEventTime = t; eventListener.mLastCallbackTime = now; - eventListener.mHasFired = true; } } diff --git a/services/surfaceflinger/Scheduler/DispSyncSource.cpp b/services/surfaceflinger/Scheduler/DispSyncSource.cpp index 00948aedb4..265b8aa47f 100644 --- a/services/surfaceflinger/Scheduler/DispSyncSource.cpp +++ b/services/surfaceflinger/Scheduler/DispSyncSource.cpp @@ -27,14 +27,16 @@ namespace android { -DispSyncSource::DispSyncSource(DispSync* dispSync, nsecs_t phaseOffset, bool traceVsync, +DispSyncSource::DispSyncSource(DispSync* dispSync, nsecs_t phaseOffset, + nsecs_t offsetThresholdForNextVsync, bool traceVsync, const char* name) : mName(name), mTraceVsync(traceVsync), mVsyncOnLabel(base::StringPrintf("VsyncOn-%s", name)), mVsyncEventLabel(base::StringPrintf("VSYNC-%s", name)), mDispSync(dispSync), - mPhaseOffset(phaseOffset) {} + mPhaseOffset(phaseOffset), + mOffsetThresholdForNextVsync(offsetThresholdForNextVsync) {} void DispSyncSource::setVSyncEnabled(bool enable) { std::lock_guard lock(mVsyncMutex); @@ -64,15 +66,15 @@ void DispSyncSource::setCallback(VSyncSource::Callback* callback) { void DispSyncSource::setPhaseOffset(nsecs_t phaseOffset) { std::lock_guard lock(mVsyncMutex); - - // Normalize phaseOffset to [0, period) - auto period = mDispSync->getPeriod(); - phaseOffset %= period; - if (phaseOffset < 0) { - // If we're here, then phaseOffset is in (-period, 0). After this - // operation, it will be in (0, period) - phaseOffset += period; + const nsecs_t period = mDispSync->getPeriod(); + // Check if offset should be handled as negative + if (phaseOffset >= mOffsetThresholdForNextVsync) { + phaseOffset -= period; } + + // Normalize phaseOffset to [-period, period) + const int numPeriods = phaseOffset / period; + phaseOffset -= numPeriods * period; mPhaseOffset = phaseOffset; // If we're not enabled, we don't need to mess with the listeners diff --git a/services/surfaceflinger/Scheduler/DispSyncSource.h b/services/surfaceflinger/Scheduler/DispSyncSource.h index 4759699c5e..b6785c5f74 100644 --- a/services/surfaceflinger/Scheduler/DispSyncSource.h +++ b/services/surfaceflinger/Scheduler/DispSyncSource.h @@ -25,7 +25,8 @@ namespace android { class DispSyncSource final : public VSyncSource, private DispSync::Callback { public: - DispSyncSource(DispSync* dispSync, nsecs_t phaseOffset, bool traceVsync, const char* name); + DispSyncSource(DispSync* dispSync, nsecs_t phaseOffset, nsecs_t offsetThresholdForNextVsync, + bool traceVsync, const char* name); ~DispSyncSource() override = default; @@ -53,6 +54,7 @@ private: std::mutex mVsyncMutex; nsecs_t mPhaseOffset GUARDED_BY(mVsyncMutex); + const nsecs_t mOffsetThresholdForNextVsync; bool mEnabled GUARDED_BY(mVsyncMutex) = false; }; diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index e2a348e146..ef53933d2d 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -119,14 +119,15 @@ Scheduler::~Scheduler() { } sp Scheduler::createConnection( - const char* connectionName, int64_t phaseOffsetNs, ResyncCallback resyncCallback, + const char* connectionName, nsecs_t phaseOffsetNs, nsecs_t offsetThresholdForNextVsync, + ResyncCallback resyncCallback, impl::EventThread::InterceptVSyncsCallback interceptCallback) { const int64_t id = sNextId++; ALOGV("Creating a connection handle with ID: %" PRId64 "\n", id); std::unique_ptr eventThread = makeEventThread(connectionName, mPrimaryDispSync.get(), phaseOffsetNs, - std::move(interceptCallback)); + offsetThresholdForNextVsync, std::move(interceptCallback)); auto eventThreadConnection = createConnectionInternal(eventThread.get(), std::move(resyncCallback)); @@ -138,10 +139,12 @@ sp Scheduler::createConnection( } std::unique_ptr Scheduler::makeEventThread( - const char* connectionName, DispSync* dispSync, int64_t phaseOffsetNs, + const char* connectionName, DispSync* dispSync, nsecs_t phaseOffsetNs, + nsecs_t offsetThresholdForNextVsync, impl::EventThread::InterceptVSyncsCallback interceptCallback) { std::unique_ptr eventThreadSource = - std::make_unique(dispSync, phaseOffsetNs, true, connectionName); + std::make_unique(dispSync, phaseOffsetNs, offsetThresholdForNextVsync, + true, connectionName); return std::make_unique(std::move(eventThreadSource), std::move(interceptCallback), connectionName); } diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h index d311f62fdb..226cb18700 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.h +++ b/services/surfaceflinger/Scheduler/Scheduler.h @@ -96,8 +96,8 @@ public: virtual ~Scheduler(); /** Creates an EventThread connection. */ - sp createConnection(const char* connectionName, int64_t phaseOffsetNs, - ResyncCallback, + sp createConnection(const char* connectionName, nsecs_t phaseOffsetNs, + nsecs_t offsetThresholdForNextVsync, ResyncCallback, impl::EventThread::InterceptVSyncsCallback); sp createDisplayEventConnection(const sp& handle, @@ -184,7 +184,8 @@ public: protected: virtual std::unique_ptr makeEventThread( - const char* connectionName, DispSync* dispSync, int64_t phaseOffsetNs, + const char* connectionName, DispSync* dispSync, nsecs_t phaseOffsetNs, + nsecs_t offsetThresholdForNextVsync, impl::EventThread::InterceptVSyncsCallback interceptCallback); private: diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.h b/services/surfaceflinger/Scheduler/VSyncModulator.h index 41c3a3a605..73a1fb9816 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.h +++ b/services/surfaceflinger/Scheduler/VSyncModulator.h @@ -56,10 +56,12 @@ public: // appEarly: Like sfEarly, but for the app-vsync // appEarlyGl: Like sfEarlyGl, but for the app-vsync. // appLate: The regular app vsync phase offset. - void setPhaseOffsets(Offsets early, Offsets earlyGl, Offsets late) { + void setPhaseOffsets(Offsets early, Offsets earlyGl, Offsets late, + nsecs_t thresholdForNextVsync) { mEarlyOffsets = early; mEarlyGlOffsets = earlyGl; mLateOffsets = late; + mThresholdForNextVsync = thresholdForNextVsync; if (mSfConnectionHandle && late.sf != mOffsets.load().sf) { mScheduler->setPhaseOffset(mSfConnectionHandle, late.sf); @@ -192,6 +194,7 @@ private: Offsets mLateOffsets; Offsets mEarlyOffsets; Offsets mEarlyGlOffsets; + nsecs_t mThresholdForNextVsync; EventThread* mSfEventThread = nullptr; EventThread* mAppEventThread = nullptr; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 87c1bf9afa..8979ebf737 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -377,7 +377,8 @@ SurfaceFlinger::SurfaceFlinger(Factory& factory) : SurfaceFlinger(factory, SkipI mLumaSampling = atoi(value); const auto [early, gl, late] = mPhaseOffsets->getCurrentOffsets(); - mVsyncModulator.setPhaseOffsets(early, gl, late); + mVsyncModulator.setPhaseOffsets(early, gl, late, + mPhaseOffsets->getOffsetThresholdForNextVsync()); // We should be reading 'persist.sys.sf.color_saturation' here // but since /data may be encrypted, we need to wait until after vold @@ -616,13 +617,16 @@ void SurfaceFlinger::init() { mScheduler->makeResyncCallback(std::bind(&SurfaceFlinger::getVsyncPeriod, this)); mAppConnectionHandle = - mScheduler->createConnection("app", mPhaseOffsets->getCurrentAppOffset(), + mScheduler->createConnection("app", mVsyncModulator.getOffsets().app, + mPhaseOffsets->getOffsetThresholdForNextVsync(), resyncCallback, impl::EventThread::InterceptVSyncsCallback()); - mSfConnectionHandle = mScheduler->createConnection("sf", mPhaseOffsets->getCurrentSfOffset(), - resyncCallback, [this](nsecs_t timestamp) { - mInterceptor->saveVSyncEvent(timestamp); - }); + mSfConnectionHandle = + mScheduler->createConnection("sf", mVsyncModulator.getOffsets().sf, + mPhaseOffsets->getOffsetThresholdForNextVsync(), + resyncCallback, [this](nsecs_t timestamp) { + mInterceptor->saveVSyncEvent(timestamp); + }); mEventQueue->setEventConnection(mScheduler->getEventConnection(mSfConnectionHandle)); mVsyncModulator.setSchedulerAndHandles(mScheduler.get(), mAppConnectionHandle.get(), @@ -940,7 +944,8 @@ void SurfaceFlinger::setDesiredActiveConfig(const ActiveConfigInfo& info) { mVsyncModulator.onRefreshRateChangeInitiated(); mPhaseOffsets->setRefreshRateType(info.type); const auto [early, gl, late] = mPhaseOffsets->getCurrentOffsets(); - mVsyncModulator.setPhaseOffsets(early, gl, late); + mVsyncModulator.setPhaseOffsets(early, gl, late, + mPhaseOffsets->getOffsetThresholdForNextVsync()); } mDesiredActiveConfigChanged = true; ATRACE_INT("DesiredActiveConfigChanged", mDesiredActiveConfigChanged); @@ -974,7 +979,8 @@ void SurfaceFlinger::setActiveConfigInternal() { mPhaseOffsets->setRefreshRateType(mUpcomingActiveConfig.type); const auto [early, gl, late] = mPhaseOffsets->getCurrentOffsets(); - mVsyncModulator.setPhaseOffsets(early, gl, late); + mVsyncModulator.setPhaseOffsets(early, gl, late, + mPhaseOffsets->getOffsetThresholdForNextVsync()); ATRACE_INT("ActiveConfigMode", mUpcomingActiveConfig.configId); if (mUpcomingActiveConfig.event != Scheduler::ConfigEvent::None) { @@ -992,7 +998,8 @@ void SurfaceFlinger::desiredActiveConfigChangeDone() { mScheduler->resyncToHardwareVsync(true, getVsyncPeriod()); mPhaseOffsets->setRefreshRateType(mUpcomingActiveConfig.type); const auto [early, gl, late] = mPhaseOffsets->getCurrentOffsets(); - mVsyncModulator.setPhaseOffsets(early, gl, late); + mVsyncModulator.setPhaseOffsets(early, gl, late, + mPhaseOffsets->getOffsetThresholdForNextVsync()); } bool SurfaceFlinger::performSetActiveConfig() { diff --git a/services/surfaceflinger/tests/unittests/DispSyncSourceTest.cpp b/services/surfaceflinger/tests/unittests/DispSyncSourceTest.cpp index 2e705dad6d..0aa8cf565d 100644 --- a/services/surfaceflinger/tests/unittests/DispSyncSourceTest.cpp +++ b/services/surfaceflinger/tests/unittests/DispSyncSourceTest.cpp @@ -51,6 +51,7 @@ protected: AsyncCallRecorder mVSyncEventCallRecorder; static constexpr std::chrono::nanoseconds mPhaseOffset = 6ms; + static constexpr std::chrono::nanoseconds mOffsetThresholdForNextVsync = 16ms; static constexpr int mIterations = 100; }; @@ -78,7 +79,8 @@ void DispSyncSourceTest::createDispSync() { void DispSyncSourceTest::createDispSyncSource() { createDispSync(); - mDispSyncSource = std::make_unique(mDispSync.get(), mPhaseOffset.count(), true, + mDispSyncSource = std::make_unique(mDispSync.get(), mPhaseOffset.count(), + mOffsetThresholdForNextVsync.count(), true, "DispSyncSourceTest"); mDispSyncSource->setCallback(this); } diff --git a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp index 1f8b11180b..982e562428 100644 --- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp +++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp @@ -47,7 +47,7 @@ protected: std::unique_ptr makeEventThread( const char* /* connectionName */, DispSync* /* dispSync */, - nsecs_t /* phaseOffsetNs */, + nsecs_t /* phaseOffsetNs */, nsecs_t /* offsetThresholdForNextVsync */, impl::EventThread::InterceptVSyncsCallback /* interceptCallback */) override { return std::move(mEventThread); } @@ -84,7 +84,7 @@ SchedulerTest::SchedulerTest() { EXPECT_CALL(*mEventThread, createEventConnection(_)) .WillRepeatedly(Return(mEventThreadConnection)); - mConnectionHandle = mScheduler->createConnection("appConnection", 16, ResyncCallback(), + mConnectionHandle = mScheduler->createConnection("appConnection", 16, 16, ResyncCallback(), impl::EventThread::InterceptVSyncsCallback()); EXPECT_TRUE(mConnectionHandle != nullptr); } -- cgit v1.2.3-59-g8ed1b From 27c702123056398d0509a8a61b5999d6a222f973 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Tue, 11 Jun 2019 10:52:26 -0700 Subject: SurfaceFlinger: always turn HWVsync off when display is off Maintain a state in SF about what is the latest HWVsync state in HWC and what it should be. Apply the desired state when screen turns on and always disabled HWVsync when the display is off. Test: display power on/off Bug: 134965007 Change-Id: I9c23d09ea2e32efa0b1f30a3999a08d15d7f35a6 --- services/surfaceflinger/SurfaceFlinger.cpp | 24 ++++++++++++++---------- services/surfaceflinger/SurfaceFlinger.h | 6 ++++-- 2 files changed, 18 insertions(+), 12 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 87c1bf9afa..3cb0ce9f8e 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1540,14 +1540,12 @@ void SurfaceFlinger::setPrimaryVsyncEnabled(bool enabled) { void SurfaceFlinger::setPrimaryVsyncEnabledInternal(bool enabled) { ATRACE_CALL(); + mHWCVsyncPendingState = enabled ? HWC2::Vsync::Enable : HWC2::Vsync::Disable; + if (const auto displayId = getInternalDisplayIdLocked()) { sp display = getDefaultDisplayDeviceLocked(); if (display && display->isPoweredOn()) { - getHwComposer().setVsyncEnabled(*displayId, - enabled ? HWC2::Vsync::Enable : HWC2::Vsync::Disable); - } else { - // Cache the latest vsync state and apply it when screen is on again - mEnableHWVsyncScreenOn = enabled; + setVsyncEnabledInHWC(*displayId, mHWCVsyncPendingState); } } } @@ -4420,6 +4418,13 @@ void SurfaceFlinger::initializeDisplays() { new LambdaMessage([this]() NO_THREAD_SAFETY_ANALYSIS { onInitializeDisplays(); })); } +void SurfaceFlinger::setVsyncEnabledInHWC(DisplayId displayId, HWC2::Vsync enabled) { + if (mHWCVsyncState != enabled) { + getHwComposer().setVsyncEnabled(displayId, enabled); + mHWCVsyncState = enabled; + } +} + void SurfaceFlinger::setPowerModeInternal(const sp& display, int mode) { if (display->isVirtual()) { ALOGE("%s: Invalid operation on virtual display", __FUNCTION__); @@ -4446,11 +4451,7 @@ void SurfaceFlinger::setPowerModeInternal(const sp& display, int // Turn on the display getHwComposer().setPowerMode(*displayId, mode); if (display->isPrimary() && mode != HWC_POWER_MODE_DOZE_SUSPEND) { - if (mEnableHWVsyncScreenOn) { - setPrimaryVsyncEnabledInternal(mEnableHWVsyncScreenOn); - mEnableHWVsyncScreenOn = false; - } - + setVsyncEnabledInHWC(*displayId, mHWCVsyncPendingState); mScheduler->onScreenAcquired(mAppConnectionHandle); mScheduler->resyncToHardwareVsync(true, getVsyncPeriod()); } @@ -4476,6 +4477,9 @@ void SurfaceFlinger::setPowerModeInternal(const sp& display, int mScheduler->onScreenReleased(mAppConnectionHandle); } + // Make sure HWVsync is disabled before turning off the display + setVsyncEnabledInHWC(*displayId, HWC2::Vsync::Disable); + getHwComposer().setPowerMode(*displayId, mode); mVisibleRegionsDirty = true; // from this point on, SF will stop drawing on this display diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 46fc4d2cd1..8e835bf9aa 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -838,6 +838,7 @@ private: } bool previousFrameMissed(); + void setVsyncEnabledInHWC(DisplayId displayId, HWC2::Vsync enabled); /* * Debugging & dumpsys @@ -1172,8 +1173,9 @@ private: // be any issues with a raw pointer referencing an invalid object. std::unordered_set mOffscreenLayers; - // Flag to indicate whether to re-enable HWVsync when screen is on - bool mEnableHWVsyncScreenOn = false; + // Flags to capture the state of Vsync in HWC + HWC2::Vsync mHWCVsyncState = HWC2::Vsync::Disable; + HWC2::Vsync mHWCVsyncPendingState = HWC2::Vsync::Disable; }; } // namespace android -- cgit v1.2.3-59-g8ed1b From d0b44a52f815fb20cb470aca8ac17da498797c1e Mon Sep 17 00:00:00 2001 From: John Dias Date: Tue, 11 Jun 2019 18:16:08 -0700 Subject: sf: optimize luma sampling code Removing some math from the luma-sampling code reduces its running time by about half (~.5ms -> ~.25ms at 1.1MHz on a little core in some experiments). Bug: 127973145 Bug: 134922154 Test: trace and compare sampleArea times when scrolling in news app Change-Id: Ie53d9595bea6685cf45f53972b42daa5e32fcc8e --- services/surfaceflinger/RegionSamplingThread.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 7fa33f597c..fdc68aa8d2 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -258,7 +258,7 @@ void RegionSamplingThread::binderDied(const wp& who) { namespace { // Using Rec. 709 primaries -float getLuma(float r, float g, float b) { +inline float getLuma(float r, float g, float b) { constexpr auto rec709_red_primary = 0.2126f; constexpr auto rec709_green_primary = 0.7152f; constexpr auto rec709_blue_primary = 0.0722f; @@ -289,10 +289,10 @@ float sampleArea(const uint32_t* data, int32_t width, int32_t height, int32_t st const uint32_t* rowBase = data + row * stride; for (int32_t column = area.left; column < area.right; ++column) { uint32_t pixel = rowBase[column]; - const float r = (pixel & 0xFF) / 255.0f; - const float g = ((pixel >> 8) & 0xFF) / 255.0f; - const float b = ((pixel >> 16) & 0xFF) / 255.0f; - const uint8_t luma = std::round(getLuma(r, g, b) * 255.0f); + const float r = pixel & 0xFF; + const float g = (pixel >> 8) & 0xFF; + const float b = (pixel >> 16) & 0xFF; + const uint8_t luma = std::round(getLuma(r, g, b)); ++brightnessBuckets[luma]; if (brightnessBuckets[luma] > majoritySampleNum) return luma / 255.0f; } -- cgit v1.2.3-59-g8ed1b From a9bf4cadf763fe8337c231c44e7d39827c98a0b8 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Tue, 11 Jun 2019 19:08:58 -0700 Subject: SurfaceFlinger: clear LayerHistory on touch Clear LayerHistory and mark all layers as inactive when getting a touch event to prevent a quick refresh rate bounce as LayerHistory picks up with the FPS. Fixes: 134663749 Test: live wallpaper + Open notification shade + wait for 60Hz + Click on "USB debugging connected" Change-Id: Ifa095fbcdb95ffe413cc77c5e062f1174815158b --- services/surfaceflinger/Scheduler/LayerHistory.cpp | 13 +++++++++++++ services/surfaceflinger/Scheduler/LayerHistory.h | 3 +++ services/surfaceflinger/Scheduler/Scheduler.cpp | 4 ++++ 3 files changed, 20 insertions(+) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/LayerHistory.cpp b/services/surfaceflinger/Scheduler/LayerHistory.cpp index 1db43a32cd..e762af3ec5 100644 --- a/services/surfaceflinger/Scheduler/LayerHistory.cpp +++ b/services/surfaceflinger/Scheduler/LayerHistory.cpp @@ -173,5 +173,18 @@ void LayerHistory::removeIrrelevantLayers() { } } +void LayerHistory::clearHistory() { + std::lock_guard lock(mLock); + + auto it = mActiveLayerInfos.begin(); + while (it != mActiveLayerInfos.end()) { + auto id = it->first; + auto layerInfo = it->second; + layerInfo->clearHistory(); + mInactiveLayerInfos.insert({id, layerInfo}); + it = mActiveLayerInfos.erase(it); + } +} + } // namespace scheduler } // namespace android \ No newline at end of file diff --git a/services/surfaceflinger/Scheduler/LayerHistory.h b/services/surfaceflinger/Scheduler/LayerHistory.h index adc5ce5f64..2569b4638a 100644 --- a/services/surfaceflinger/Scheduler/LayerHistory.h +++ b/services/surfaceflinger/Scheduler/LayerHistory.h @@ -64,6 +64,9 @@ public: // layers. See go/content-fps-detection-in-scheduler for more information. std::pair getDesiredRefreshRateAndHDR(); + // Clears all layer history. + void clearHistory(); + // Removes the handle and the object from the map. void destroyLayer(const int64_t id); diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index e2a348e146..da9526a0cd 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -404,6 +404,10 @@ void Scheduler::notifyTouchEvent() { if (mSupportKernelTimer) { resetIdleTimer(); } + + // Touch event will boost the refresh rate to performance. + // Clear Layer History to get fresh FPS detection + mLayerHistory.clearHistory(); } void Scheduler::resetTimerCallback() { -- cgit v1.2.3-59-g8ed1b From 8fe1102f1feec1aa8b9a00a22a3cab13166fff0d Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Wed, 12 Jun 2019 17:11:12 -0700 Subject: SurfaceFlinger: get present time from SF and not from Scheduler SF hold the most accurate expected present time as it also knows whether we are operating at negative offset and which vsync we are targeting. Bug: 133241520 Bug: 134589085 Test: systrace when scrolling Change-Id: I934df3a8bf807b0e52555765a6861f252b69c0d1 --- services/surfaceflinger/BufferQueueLayer.cpp | 4 ++-- services/surfaceflinger/Scheduler/Scheduler.cpp | 2 +- services/surfaceflinger/Scheduler/Scheduler.h | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp index 57f1008e85..d6853661a5 100644 --- a/services/surfaceflinger/BufferQueueLayer.cpp +++ b/services/surfaceflinger/BufferQueueLayer.cpp @@ -201,7 +201,7 @@ uint64_t BufferQueueLayer::getFrameNumber() const { uint64_t frameNumber = mQueueItems[0].mFrameNumber; // The head of the queue will be dropped if there are signaled and timely frames behind it - nsecs_t expectedPresentTime = mFlinger->mScheduler->expectedPresentTime(); + nsecs_t expectedPresentTime = mFlinger->getExpectedPresentTime(); if (isRemovedFromCurrentState()) { expectedPresentTime = 0; @@ -279,7 +279,7 @@ status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t getProducerStickyTransform() != 0, mName.string(), mOverrideScalingMode, getTransformToDisplayInverse(), mFreezeGeometryUpdates); - nsecs_t expectedPresentTime = mFlinger->mScheduler->expectedPresentTime(); + nsecs_t expectedPresentTime = mFlinger->getExpectedPresentTime(); if (isRemovedFromCurrentState()) { expectedPresentTime = 0; diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index e2a348e146..1d899dfbad 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -309,7 +309,7 @@ void Scheduler::setIgnorePresentFences(bool ignore) { mPrimaryDispSync->setIgnorePresentFences(ignore); } -nsecs_t Scheduler::expectedPresentTime() { +nsecs_t Scheduler::getDispSyncExpectedPresentTime() { return mPrimaryDispSync->expectedPresentTime(); } diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h index f6cc87bb89..a32bd4142d 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.h +++ b/services/surfaceflinger/Scheduler/Scheduler.h @@ -149,7 +149,7 @@ public: void addResyncSample(const nsecs_t timestamp, bool* periodFlushed); void addPresentFence(const std::shared_ptr& fenceTime); void setIgnorePresentFences(bool ignore); - nsecs_t expectedPresentTime(); + nsecs_t getDispSyncExpectedPresentTime(); // Registers the layer in the scheduler, and returns the handle for future references. std::unique_ptr registerLayer(std::string const& name, int windowType); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index dadb3fed05..cd4f69a9db 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1646,7 +1646,7 @@ bool SurfaceFlinger::previousFrameMissed() NO_THREAD_SAFETY_ANALYSIS { nsecs_t SurfaceFlinger::getExpectedPresentTime() NO_THREAD_SAFETY_ANALYSIS { DisplayStatInfo stats; mScheduler->getDisplayStatInfo(&stats); - const nsecs_t presentTime = mScheduler->expectedPresentTime(); + const nsecs_t presentTime = mScheduler->getDispSyncExpectedPresentTime(); // Inflate the expected present time if we're targetting the next vsync. const nsecs_t correctedTime = mVsyncModulator.getOffsets().sf < mPhaseOffsets->getOffsetThresholdForNextVsync() @@ -3653,7 +3653,7 @@ bool SurfaceFlinger::containsAnyInvalidClientState(const Vector& bool SurfaceFlinger::transactionIsReadyToBeApplied(int64_t desiredPresentTime, const Vector& states) { - nsecs_t expectedPresentTime = mScheduler->expectedPresentTime(); + nsecs_t expectedPresentTime = getExpectedPresentTime(); // Do not present if the desiredPresentTime has not passed unless it is more than one second // in the future. We ignore timestamps more than 1 second in the future for stability reasons. if (desiredPresentTime >= 0 && desiredPresentTime >= expectedPresentTime && -- cgit v1.2.3-59-g8ed1b From 616b83295e42d8fb700067bb8edf1fea7b071bdf Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Wed, 12 Jun 2019 13:30:07 -0700 Subject: SurfaceFlinger: tune FPS detection logic Keep 200ms of history for layer instead of 100ms to accomodate scenarios of YouTube playing music and renders at ~6Hz. In addition, change the way layers are considered relevant for fps detection to have a time based mechanism as well. Going by frame number only makes slow layers to become relevant too late. Fixes: 135009095 Test: YouTube playing music Change-Id: Id38290e453111c5cfd4a23383b36b6862c2dd386 --- services/surfaceflinger/Scheduler/LayerInfo.cpp | 6 ++++++ services/surfaceflinger/Scheduler/LayerInfo.h | 24 +++++++++++++++------- services/surfaceflinger/Scheduler/SchedulerUtils.h | 8 ++++---- 3 files changed, 27 insertions(+), 11 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/LayerInfo.cpp b/services/surfaceflinger/Scheduler/LayerInfo.cpp index 95d7d31564..3104724041 100644 --- a/services/surfaceflinger/Scheduler/LayerInfo.cpp +++ b/services/surfaceflinger/Scheduler/LayerInfo.cpp @@ -38,6 +38,12 @@ void LayerInfo::setLastPresentTime(nsecs_t lastPresentTime) { mLastUpdatedTime = std::max(lastPresentTime, systemTime()); mPresentTimeHistory.insertPresentTime(mLastUpdatedTime); + if (mLastPresentTime == 0) { + // First frame + mLastPresentTime = lastPresentTime; + return; + } + const nsecs_t timeDiff = lastPresentTime - mLastPresentTime; mLastPresentTime = lastPresentTime; // Ignore time diff that are too high - those are stale values diff --git a/services/surfaceflinger/Scheduler/LayerInfo.h b/services/surfaceflinger/Scheduler/LayerInfo.h index 02b6aefa2b..90d426969c 100644 --- a/services/surfaceflinger/Scheduler/LayerInfo.h +++ b/services/surfaceflinger/Scheduler/LayerInfo.h @@ -55,7 +55,7 @@ class LayerInfo { float getRefreshRateAvg() const { nsecs_t refreshDuration = mMinRefreshDuration; - if (mElements.size() == HISTORY_SIZE) { + if (mElements.size() > 0) { refreshDuration = scheduler::calculate_mean(mElements); } @@ -86,14 +86,23 @@ class LayerInfo { // Checks whether the present time that was inserted HISTORY_SIZE ago is within a // certain threshold: TIME_EPSILON_NS. bool isRelevant() const { + if (mElements.size() < 2) { + return false; + } + + // The layer had to publish at least HISTORY_SIZE or HISTORY_TIME of updates + if (mElements.size() != HISTORY_SIZE && + mElements.at(mElements.size() - 1) - mElements.at(0) < HISTORY_TIME.count()) { + return false; + } + + // The last update should not be older than TIME_EPSILON_NS nanoseconds. const int64_t obsoleteEpsilon = systemTime() - scheduler::TIME_EPSILON_NS.count(); - // The layer had to publish at least HISTORY_SIZE of updates, and the first - // update should not be older than TIME_EPSILON_NS nanoseconds. - if (mElements.size() == HISTORY_SIZE && - mElements.at(HISTORY_SIZE - 1) > obsoleteEpsilon) { - return true; + if (mElements.at(mElements.size() - 1) < obsoleteEpsilon) { + return false; } - return false; + + return true; } void clearHistory() { mElements.clear(); } @@ -101,6 +110,7 @@ class LayerInfo { private: std::deque mElements; static constexpr size_t HISTORY_SIZE = 10; + static constexpr std::chrono::nanoseconds HISTORY_TIME = 500ms; }; public: diff --git a/services/surfaceflinger/Scheduler/SchedulerUtils.h b/services/surfaceflinger/Scheduler/SchedulerUtils.h index 3bf3922edd..d3b1bd07c4 100644 --- a/services/surfaceflinger/Scheduler/SchedulerUtils.h +++ b/services/surfaceflinger/Scheduler/SchedulerUtils.h @@ -37,12 +37,12 @@ static constexpr int SCREEN_OFF_CONFIG_ID = -1; static constexpr uint32_t HWC2_SCREEN_OFF_CONFIG_ID = 0xffffffff; // This number is used when we try to determine how long does a given layer stay relevant. -// Currently it is set to 100ms, because that would indicate 10Hz rendering. -static constexpr std::chrono::nanoseconds TIME_EPSILON_NS = 100ms; +// The value is set based on testing different scenarios. +static constexpr std::chrono::nanoseconds TIME_EPSILON_NS = 200ms; // This number is used when we try to determine how long do we keep layer information around -// before we remove it. Currently it is set to 100ms. -static constexpr std::chrono::nanoseconds OBSOLETE_TIME_EPSILON_NS = 100ms; +// before we remove it. +static constexpr std::chrono::nanoseconds OBSOLETE_TIME_EPSILON_NS = 200ms; // Calculates the statistical mean (average) in the data structure (array, vector). The // function does not modify the contents of the array. -- cgit v1.2.3-59-g8ed1b From 2242f72398dfd4a2edc77c0cd1e9987f3ad5f21b Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Thu, 13 Jun 2019 17:31:28 -0700 Subject: SurfaceFlinger: Dispsync changePhaseOffset When changing the phase offset of DispSync there is no need to adjust the last event time beyond the diff between the old offset and the new one. Test: default <-> early offset transition Test: default <-> earlyGL offset transition Bug: 135215384 Change-Id: I436c63914e53ddb53963c590493ee160efb62db8 --- services/surfaceflinger/Scheduler/DispSync.cpp | 11 ----------- 1 file changed, 11 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/DispSync.cpp b/services/surfaceflinger/Scheduler/DispSync.cpp index e59d459242..81be372091 100644 --- a/services/surfaceflinger/Scheduler/DispSync.cpp +++ b/services/surfaceflinger/Scheduler/DispSync.cpp @@ -288,17 +288,6 @@ public: // new offset to allow for a seamless offset change without double-firing or // skipping. nsecs_t diff = oldPhase - phase; - if (diff > mPeriod / 2) { - diff -= mPeriod; - } else if (diff < -mPeriod / 2) { - diff += mPeriod; - } - - if (phase < 0 && oldPhase > 0) { - diff += mPeriod; - } else if (phase > 0 && oldPhase < 0) { - diff -= mPeriod; - } eventListener.mLastEventTime -= diff; eventListener.mLastCallbackTime -= diff; mCond.signal(); -- cgit v1.2.3-59-g8ed1b From b488afaa2addf5b101ef24d0e8fc9812cb3c7f26 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 23 May 2019 10:22:24 -0700 Subject: [SurfaceFlinger] Split VSyncModulator into .cpp/.h files Future changes require this so that trace events can be added for this class. Bug: 133325345 Test: builds Change-Id: I7b70a9df500b0fca5ae14e2b414d44ca11edf3ce --- services/surfaceflinger/Android.bp | 3 +- .../surfaceflinger/Scheduler/VSyncModulator.cpp | 135 +++++++++++++++++++++ services/surfaceflinger/Scheduler/VSyncModulator.h | 127 ++++--------------- services/surfaceflinger/test.png | Bin 0 -> 3058273 bytes 4 files changed, 158 insertions(+), 107 deletions(-) create mode 100644 services/surfaceflinger/Scheduler/VSyncModulator.cpp create mode 100644 services/surfaceflinger/test.png (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Android.bp b/services/surfaceflinger/Android.bp index 4cd0a13861..501d176401 100644 --- a/services/surfaceflinger/Android.bp +++ b/services/surfaceflinger/Android.bp @@ -149,9 +149,10 @@ filegroup { "Scheduler/LayerHistory.cpp", "Scheduler/LayerInfo.cpp", "Scheduler/MessageQueue.cpp", + "Scheduler/PhaseOffsets.cpp", "Scheduler/Scheduler.cpp", "Scheduler/SchedulerUtils.cpp", - "Scheduler/PhaseOffsets.cpp", + "Scheduler/VSyncModulator.cpp", "StartPropertySetThread.cpp", "SurfaceFlinger.cpp", "SurfaceInterceptor.cpp", diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.cpp b/services/surfaceflinger/Scheduler/VSyncModulator.cpp new file mode 100644 index 0000000000..af8f445b30 --- /dev/null +++ b/services/surfaceflinger/Scheduler/VSyncModulator.cpp @@ -0,0 +1,135 @@ +/* + * Copyright 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "VSyncModulator.h" + +#include +#include + +namespace android { + +void VSyncModulator::setPhaseOffsets(Offsets early, Offsets earlyGl, Offsets late, + nsecs_t thresholdForNextVsync) { + mEarlyOffsets = early; + mEarlyGlOffsets = earlyGl; + mLateOffsets = late; + mThresholdForNextVsync = thresholdForNextVsync; + + if (mSfConnectionHandle && late.sf != mOffsets.load().sf) { + mScheduler->setPhaseOffset(mSfConnectionHandle, late.sf); + } + + if (mAppConnectionHandle && late.app != mOffsets.load().app) { + mScheduler->setPhaseOffset(mAppConnectionHandle, late.app); + } + mOffsets = late; +} + +void VSyncModulator::setTransactionStart(Scheduler::TransactionStart transactionStart) { + if (transactionStart == Scheduler::TransactionStart::EARLY) { + mRemainingEarlyFrameCount = MIN_EARLY_FRAME_COUNT_TRANSACTION; + } + + // An early transaction stays an early transaction. + if (transactionStart == mTransactionStart || + mTransactionStart == Scheduler::TransactionStart::EARLY) { + return; + } + mTransactionStart = transactionStart; + updateOffsets(); +} + +void VSyncModulator::onTransactionHandled() { + if (mTransactionStart == Scheduler::TransactionStart::NORMAL) return; + mTransactionStart = Scheduler::TransactionStart::NORMAL; + updateOffsets(); +} + +void VSyncModulator::onRefreshRateChangeInitiated() { + if (mRefreshRateChangePending) { + return; + } + mRefreshRateChangePending = true; + updateOffsets(); +} + +void VSyncModulator::onRefreshRateChangeCompleted() { + if (!mRefreshRateChangePending) { + return; + } + mRefreshRateChangePending = false; + updateOffsets(); +} + +void VSyncModulator::onRefreshed(bool usedRenderEngine) { + bool updateOffsetsNeeded = false; + if (mRemainingEarlyFrameCount > 0) { + mRemainingEarlyFrameCount--; + updateOffsetsNeeded = true; + } + if (usedRenderEngine) { + mRemainingRenderEngineUsageCount = MIN_EARLY_GL_FRAME_COUNT_TRANSACTION; + updateOffsetsNeeded = true; + } else if (mRemainingRenderEngineUsageCount > 0) { + mRemainingRenderEngineUsageCount--; + updateOffsetsNeeded = true; + } + if (updateOffsetsNeeded) { + updateOffsets(); + } +} + +VSyncModulator::Offsets VSyncModulator::getOffsets() { + // Early offsets are used if we're in the middle of a refresh rate + // change, or if we recently begin a transaction. + if (mTransactionStart == Scheduler::TransactionStart::EARLY || mRemainingEarlyFrameCount > 0 || + mRefreshRateChangePending) { + return mEarlyOffsets; + } else if (mRemainingRenderEngineUsageCount > 0) { + return mEarlyGlOffsets; + } else { + return mLateOffsets; + } +} + +void VSyncModulator::updateOffsets() { + const Offsets desired = getOffsets(); + const Offsets current = mOffsets; + + bool changed = false; + if (desired.sf != current.sf) { + if (mSfConnectionHandle != nullptr) { + mScheduler->setPhaseOffset(mSfConnectionHandle, desired.sf); + } else { + mSfEventThread->setPhaseOffset(desired.sf); + } + changed = true; + } + if (desired.app != current.app) { + if (mAppConnectionHandle != nullptr) { + mScheduler->setPhaseOffset(mAppConnectionHandle, desired.app); + } else { + mAppEventThread->setPhaseOffset(desired.app); + } + changed = true; + } + + if (changed) { + mOffsets = desired; + } +} + +} // namespace android diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.h b/services/surfaceflinger/Scheduler/VSyncModulator.h index 73a1fb9816..677862e4fe 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.h +++ b/services/surfaceflinger/Scheduler/VSyncModulator.h @@ -16,8 +16,6 @@ #pragma once -#include - #include #include @@ -41,6 +39,8 @@ private: const int MIN_EARLY_GL_FRAME_COUNT_TRANSACTION = 2; public: + // Wrapper for a collection of surfaceflinger/app offsets for a particular + // configuration . struct Offsets { nsecs_t sf; nsecs_t app; @@ -57,32 +57,21 @@ public: // appEarlyGl: Like sfEarlyGl, but for the app-vsync. // appLate: The regular app vsync phase offset. void setPhaseOffsets(Offsets early, Offsets earlyGl, Offsets late, - nsecs_t thresholdForNextVsync) { - mEarlyOffsets = early; - mEarlyGlOffsets = earlyGl; - mLateOffsets = late; - mThresholdForNextVsync = thresholdForNextVsync; - - if (mSfConnectionHandle && late.sf != mOffsets.load().sf) { - mScheduler->setPhaseOffset(mSfConnectionHandle, late.sf); - } - - if (mAppConnectionHandle && late.app != mOffsets.load().app) { - mScheduler->setPhaseOffset(mAppConnectionHandle, late.app); - } - - mOffsets = late; - } + nsecs_t thresholdForNextVsync); + // Returns the configured early offsets. Offsets getEarlyOffsets() const { return mEarlyOffsets; } + // Returns the configured early gl offsets. Offsets getEarlyGlOffsets() const { return mEarlyGlOffsets; } + // Sets handles to the SF and app event threads. void setEventThreads(EventThread* sfEventThread, EventThread* appEventThread) { mSfEventThread = sfEventThread; mAppEventThread = appEventThread; } + // Sets the scheduler and vsync connection handlers. void setSchedulerAndHandles(Scheduler* scheduler, Scheduler::ConnectionHandle* appConnectionHandle, Scheduler::ConnectionHandle* sfConnectionHandle) { @@ -91,105 +80,31 @@ public: mSfConnectionHandle = sfConnectionHandle; } - void setTransactionStart(Scheduler::TransactionStart transactionStart) { - if (transactionStart == Scheduler::TransactionStart::EARLY) { - mRemainingEarlyFrameCount = MIN_EARLY_FRAME_COUNT_TRANSACTION; - } - - // An early transaction stays an early transaction. - if (transactionStart == mTransactionStart || - mTransactionStart == Scheduler::TransactionStart::EARLY) { - return; - } - mTransactionStart = transactionStart; - updateOffsets(); - } + // Signals that a transaction has started, and changes offsets accordingly. + void setTransactionStart(Scheduler::TransactionStart transactionStart); - void onTransactionHandled() { - if (mTransactionStart == Scheduler::TransactionStart::NORMAL) return; - mTransactionStart = Scheduler::TransactionStart::NORMAL; - updateOffsets(); - } + // Signals that a transaction has been completed, so that we can finish + // special handling for a transaction. + void onTransactionHandled(); // Called when we send a refresh rate change to hardware composer, so that // we can move into early offsets. - void onRefreshRateChangeInitiated() { - if (mRefreshRateChangePending) { - return; - } - mRefreshRateChangePending = true; - updateOffsets(); - } + void onRefreshRateChangeInitiated(); // Called when we detect from vsync signals that the refresh rate changed. // This way we can move out of early offsets if no longer necessary. - void onRefreshRateChangeCompleted() { - if (!mRefreshRateChangePending) { - return; - } - mRefreshRateChangePending = false; - updateOffsets(); - } + void onRefreshRateChangeCompleted(); - void onRefreshed(bool usedRenderEngine) { - bool updateOffsetsNeeded = false; - if (mRemainingEarlyFrameCount > 0) { - mRemainingEarlyFrameCount--; - updateOffsetsNeeded = true; - } - if (usedRenderEngine) { - mRemainingRenderEngineUsageCount = MIN_EARLY_GL_FRAME_COUNT_TRANSACTION; - updateOffsetsNeeded = true; - } else if (mRemainingRenderEngineUsageCount > 0) { - mRemainingRenderEngineUsageCount--; - updateOffsetsNeeded = true; - } - - if (updateOffsetsNeeded) { - updateOffsets(); - } - } + // Called when the display is presenting a new frame. usedRenderEngine + // should be set to true if RenderEngine was involved with composing the new + // frame. + void onRefreshed(bool usedRenderEngine); - Offsets getOffsets() { - // Early offsets are used if we're in the middle of a refresh rate - // change, or if we recently begin a transaction. - if (mTransactionStart == Scheduler::TransactionStart::EARLY || - mRemainingEarlyFrameCount > 0 || mRefreshRateChangePending) { - return mEarlyOffsets; - } else if (mRemainingRenderEngineUsageCount > 0) { - return mEarlyGlOffsets; - } else { - return mLateOffsets; - } - } + // Returns the offsets that should be used. + Offsets getOffsets(); private: - void updateOffsets() { - const Offsets desired = getOffsets(); - const Offsets current = mOffsets; - - bool changed = false; - if (desired.sf != current.sf) { - if (mSfConnectionHandle != nullptr) { - mScheduler->setPhaseOffset(mSfConnectionHandle, desired.sf); - } else { - mSfEventThread->setPhaseOffset(desired.sf); - } - changed = true; - } - if (desired.app != current.app) { - if (mAppConnectionHandle != nullptr) { - mScheduler->setPhaseOffset(mAppConnectionHandle, desired.app); - } else { - mAppEventThread->setPhaseOffset(desired.app); - } - changed = true; - } - - if (changed) { - mOffsets = desired; - } - } + void updateOffsets(); Offsets mLateOffsets; Offsets mEarlyOffsets; diff --git a/services/surfaceflinger/test.png b/services/surfaceflinger/test.png new file mode 100644 index 0000000000..773dcc3e72 Binary files /dev/null and b/services/surfaceflinger/test.png differ -- cgit v1.2.3-59-g8ed1b From 84be783ff179fd6589bd01091f08f174f1f6a95d Mon Sep 17 00:00:00 2001 From: John Dias Date: Tue, 18 Jun 2019 17:05:26 -0700 Subject: SF: delay region sampling when short on time In a number of janky traces, particularly at high frame rates, we've seen the surfaceflinger thread overrunning its time slot. In some of those cases, the surfaceflinger thread is doing region-sampling. This change causes region-sampling to check how much time is left until the next vsync before deciding whether to sample this frame. If low on time, it will defer the sampling to a later frame. Bug: 133779857 Test: trace inspection from scrolling in various apps Change-Id: I92c2368e80033c1ba6e27f947a456d14db02064c --- services/surfaceflinger/RegionSamplingThread.cpp | 23 +++++++++++++++++++---- services/surfaceflinger/RegionSamplingThread.h | 2 +- 2 files changed, 20 insertions(+), 5 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 66906e950c..4fca63aa96 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -44,11 +44,14 @@ constexpr auto lumaSamplingStepTag = "LumaSamplingStep"; enum class samplingStep { noWorkNeeded, idleTimerWaiting, + waitForQuietFrame, waitForZeroPhase, waitForSamplePhase, sample }; +constexpr auto timeForRegionSampling = 5000000ns; +constexpr auto maxRegionSamplingSkips = 10; constexpr auto defaultRegionSamplingOffset = -3ms; constexpr auto defaultRegionSamplingPeriod = 100ms; constexpr auto defaultRegionSamplingTimerTimeout = 100ms; @@ -215,9 +218,9 @@ void RegionSamplingThread::removeListener(const sp& lis void RegionSamplingThread::checkForStaleLuma() { std::lock_guard lock(mThreadControlMutex); - if (mDiscardedFrames) { + if (mDiscardedFrames > 0) { ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::waitForZeroPhase)); - mDiscardedFrames = false; + mDiscardedFrames = 0; mPhaseCallback->startVsyncListener(); } } @@ -235,13 +238,25 @@ void RegionSamplingThread::doSample() { auto now = std::chrono::nanoseconds(systemTime(SYSTEM_TIME_MONOTONIC)); if (lastSampleTime + mTunables.mSamplingPeriod > now) { ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::idleTimerWaiting)); - mDiscardedFrames = true; + if (mDiscardedFrames == 0) mDiscardedFrames++; return; } + if (mDiscardedFrames < maxRegionSamplingSkips) { + // If there is relatively little time left for surfaceflinger + // until the next vsync deadline, defer this sampling work + // to a later frame, when hopefully there will be more time. + DisplayStatInfo stats; + mScheduler.getDisplayStatInfo(&stats); + if (std::chrono::nanoseconds(stats.vsyncTime) - now < timeForRegionSampling) { + ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::waitForQuietFrame)); + mDiscardedFrames++; + return; + } + } ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::sample)); - mDiscardedFrames = false; + mDiscardedFrames = 0; lastSampleTime = now; mIdleTimer.reset(); diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h index 3c6fcf3872..96ffe207e4 100644 --- a/services/surfaceflinger/RegionSamplingThread.h +++ b/services/surfaceflinger/RegionSamplingThread.h @@ -117,7 +117,7 @@ private: std::condition_variable_any mCondition; bool mRunning GUARDED_BY(mThreadControlMutex) = true; bool mSampleRequested GUARDED_BY(mThreadControlMutex) = false; - bool mDiscardedFrames GUARDED_BY(mThreadControlMutex) = false; + uint32_t mDiscardedFrames GUARDED_BY(mThreadControlMutex) = 0; std::chrono::nanoseconds lastSampleTime GUARDED_BY(mThreadControlMutex); std::mutex mSamplingMutex; -- cgit v1.2.3-59-g8ed1b From d7599d832c96cc3ba0c2ad19653f29bdfe084284 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Wed, 22 May 2019 19:58:00 -0700 Subject: [SurfaceFlinger] Add vsync offset information to systrace. * Trace offset values in DispSyncSource * Trace offset type in VSyncModulator * Refactor how offsets are stored so that refresh rate type can be encoded in VSyncModulator * Add locking to catch a potential race condition when updating offsets, as phase offsets can be accessed or updated on multiple threads. Bug: 133325345 Test: verified that correct offsets are reported in systrace Change-Id: I38d43b722cd54728a2e4de3df7dd472aceb1de15 --- .../surfaceflinger/Scheduler/DispSyncSource.cpp | 24 +++++-- services/surfaceflinger/Scheduler/DispSyncSource.h | 6 +- services/surfaceflinger/Scheduler/PhaseOffsets.cpp | 53 ++++++++------- services/surfaceflinger/Scheduler/PhaseOffsets.h | 7 +- .../surfaceflinger/Scheduler/VSyncModulator.cpp | 79 +++++++++++++++++----- services/surfaceflinger/Scheduler/VSyncModulator.h | 42 ++++++++---- .../tests/unittests/FakePhaseOffsets.h | 14 ++-- 7 files changed, 150 insertions(+), 75 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/DispSyncSource.cpp b/services/surfaceflinger/Scheduler/DispSyncSource.cpp index 265b8aa47f..026b55707c 100644 --- a/services/surfaceflinger/Scheduler/DispSyncSource.cpp +++ b/services/surfaceflinger/Scheduler/DispSyncSource.cpp @@ -34,6 +34,8 @@ DispSyncSource::DispSyncSource(DispSync* dispSync, nsecs_t phaseOffset, mTraceVsync(traceVsync), mVsyncOnLabel(base::StringPrintf("VsyncOn-%s", name)), mVsyncEventLabel(base::StringPrintf("VSYNC-%s", name)), + mVsyncOffsetLabel(base::StringPrintf("VsyncOffset-%s", name)), + mVsyncNegativeOffsetLabel(base::StringPrintf("VsyncNegativeOffset-%s", name)), mDispSync(dispSync), mPhaseOffset(phaseOffset), mOffsetThresholdForNextVsync(offsetThresholdForNextVsync) {} @@ -41,6 +43,7 @@ DispSyncSource::DispSyncSource(DispSync* dispSync, nsecs_t phaseOffset, void DispSyncSource::setVSyncEnabled(bool enable) { std::lock_guard lock(mVsyncMutex); if (enable) { + tracePhaseOffset(); status_t err = mDispSync->addEventListener(mName, mPhaseOffset, static_cast(this), mLastCallbackTime); @@ -76,6 +79,7 @@ void DispSyncSource::setPhaseOffset(nsecs_t phaseOffset) { const int numPeriods = phaseOffset / period; phaseOffset -= numPeriods * period; mPhaseOffset = phaseOffset; + tracePhaseOffset(); // If we're not enabled, we don't need to mess with the listeners if (!mEnabled) { @@ -94,11 +98,11 @@ void DispSyncSource::onDispSyncEvent(nsecs_t when) { { std::lock_guard lock(mCallbackMutex); callback = mCallback; + } - if (mTraceVsync) { - mValue = (mValue + 1) % 2; - ATRACE_INT(mVsyncEventLabel.c_str(), mValue); - } + if (mTraceVsync) { + mValue = (mValue + 1) % 2; + ATRACE_INT(mVsyncEventLabel.c_str(), mValue); } if (callback != nullptr) { @@ -106,4 +110,14 @@ void DispSyncSource::onDispSyncEvent(nsecs_t when) { } } -} // namespace android \ No newline at end of file +void DispSyncSource::tracePhaseOffset() { + if (mPhaseOffset > 0) { + ATRACE_INT(mVsyncOffsetLabel.c_str(), mPhaseOffset); + ATRACE_INT(mVsyncNegativeOffsetLabel.c_str(), 0); + } else { + ATRACE_INT(mVsyncOffsetLabel.c_str(), 0); + ATRACE_INT(mVsyncNegativeOffsetLabel.c_str(), -mPhaseOffset); + } +} + +} // namespace android diff --git a/services/surfaceflinger/Scheduler/DispSyncSource.h b/services/surfaceflinger/Scheduler/DispSyncSource.h index b6785c5f74..50560a5a2b 100644 --- a/services/surfaceflinger/Scheduler/DispSyncSource.h +++ b/services/surfaceflinger/Scheduler/DispSyncSource.h @@ -39,12 +39,16 @@ private: // The following method is the implementation of the DispSync::Callback. virtual void onDispSyncEvent(nsecs_t when); + void tracePhaseOffset() REQUIRES(mVsyncMutex); + const char* const mName; int mValue = 0; const bool mTraceVsync; const std::string mVsyncOnLabel; const std::string mVsyncEventLabel; + const std::string mVsyncOffsetLabel; + const std::string mVsyncNegativeOffsetLabel; nsecs_t mLastCallbackTime GUARDED_BY(mVsyncMutex) = 0; DispSync* mDispSync; @@ -58,4 +62,4 @@ private: bool mEnabled GUARDED_BY(mVsyncMutex) = false; }; -} // namespace android \ No newline at end of file +} // namespace android diff --git a/services/surfaceflinger/Scheduler/PhaseOffsets.cpp b/services/surfaceflinger/Scheduler/PhaseOffsets.cpp index 276bce1f89..8a2604f4a3 100644 --- a/services/surfaceflinger/Scheduler/PhaseOffsets.cpp +++ b/services/surfaceflinger/Scheduler/PhaseOffsets.cpp @@ -25,6 +25,7 @@ using namespace android::sysprop; namespace scheduler { +using RefreshRateType = RefreshRateConfigs::RefreshRateType; PhaseOffsets::~PhaseOffsets() = default; namespace impl { @@ -72,25 +73,32 @@ PhaseOffsets::PhaseOffsets() { property_get("debug.sf.phase_offset_threshold_for_next_vsync_ns", value, "-1"); const int phaseOffsetThresholdForNextVsyncNs = atoi(value); - mDefaultRefreshRateOffsets.early = {earlySfOffsetNs != -1 ? earlySfOffsetNs - : sfVsyncPhaseOffsetNs, - earlyAppOffsetNs != -1 ? earlyAppOffsetNs - : vsyncPhaseOffsetNs}; - mDefaultRefreshRateOffsets.earlyGl = {earlyGlSfOffsetNs != -1 ? earlyGlSfOffsetNs - : sfVsyncPhaseOffsetNs, - earlyGlAppOffsetNs != -1 ? earlyGlAppOffsetNs - : vsyncPhaseOffsetNs}; - mDefaultRefreshRateOffsets.late = {sfVsyncPhaseOffsetNs, vsyncPhaseOffsetNs}; - - mHighRefreshRateOffsets.early = {highFpsEarlySfOffsetNs != -1 ? highFpsEarlySfOffsetNs - : highFpsLateSfOffsetNs, - highFpsEarlyAppOffsetNs != -1 ? highFpsEarlyAppOffsetNs - : highFpsLateAppOffsetNs}; - mHighRefreshRateOffsets.earlyGl = {highFpsEarlyGlSfOffsetNs != -1 ? highFpsEarlyGlSfOffsetNs - : highFpsLateSfOffsetNs, - highFpsEarlyGlAppOffsetNs != -1 ? highFpsEarlyGlAppOffsetNs - : highFpsLateAppOffsetNs}; - mHighRefreshRateOffsets.late = {highFpsLateSfOffsetNs, highFpsLateAppOffsetNs}; + Offsets defaultOffsets; + Offsets highFpsOffsets; + defaultOffsets.early = {RefreshRateType::DEFAULT, + earlySfOffsetNs != -1 ? earlySfOffsetNs : sfVsyncPhaseOffsetNs, + earlyAppOffsetNs != -1 ? earlyAppOffsetNs : vsyncPhaseOffsetNs}; + defaultOffsets.earlyGl = {RefreshRateType::DEFAULT, + earlyGlSfOffsetNs != -1 ? earlyGlSfOffsetNs : sfVsyncPhaseOffsetNs, + earlyGlAppOffsetNs != -1 ? earlyGlAppOffsetNs : vsyncPhaseOffsetNs}; + defaultOffsets.late = {RefreshRateType::DEFAULT, sfVsyncPhaseOffsetNs, vsyncPhaseOffsetNs}; + + highFpsOffsets.early = {RefreshRateType::PERFORMANCE, + highFpsEarlySfOffsetNs != -1 ? highFpsEarlySfOffsetNs + : highFpsLateSfOffsetNs, + highFpsEarlyAppOffsetNs != -1 ? highFpsEarlyAppOffsetNs + : highFpsLateAppOffsetNs}; + highFpsOffsets.earlyGl = {RefreshRateType::PERFORMANCE, + highFpsEarlyGlSfOffsetNs != -1 ? highFpsEarlyGlSfOffsetNs + : highFpsLateSfOffsetNs, + highFpsEarlyGlAppOffsetNs != -1 ? highFpsEarlyGlAppOffsetNs + : highFpsLateAppOffsetNs}; + highFpsOffsets.late = {RefreshRateType::PERFORMANCE, highFpsLateSfOffsetNs, + highFpsLateAppOffsetNs}; + + mOffsets.insert({RefreshRateType::POWER_SAVING, defaultOffsets}); + mOffsets.insert({RefreshRateType::DEFAULT, defaultOffsets}); + mOffsets.insert({RefreshRateType::PERFORMANCE, highFpsOffsets}); mOffsetThresholdForNextVsync = phaseOffsetThresholdForNextVsyncNs != -1 ? phaseOffsetThresholdForNextVsyncNs @@ -99,12 +107,7 @@ PhaseOffsets::PhaseOffsets() { PhaseOffsets::Offsets PhaseOffsets::getOffsetsForRefreshRate( android::scheduler::RefreshRateConfigs::RefreshRateType refreshRateType) const { - switch (refreshRateType) { - case RefreshRateConfigs::RefreshRateType::PERFORMANCE: - return mHighRefreshRateOffsets; - default: - return mDefaultRefreshRateOffsets; - } + return mOffsets.at(refreshRateType); } void PhaseOffsets::dump(std::string& result) const { diff --git a/services/surfaceflinger/Scheduler/PhaseOffsets.h b/services/surfaceflinger/Scheduler/PhaseOffsets.h index dc71e6eb60..2b5c2f10f1 100644 --- a/services/surfaceflinger/Scheduler/PhaseOffsets.h +++ b/services/surfaceflinger/Scheduler/PhaseOffsets.h @@ -17,6 +17,7 @@ #pragma once #include +#include #include "RefreshRateConfigs.h" #include "VSyncModulator.h" @@ -79,14 +80,10 @@ public: void dump(std::string& result) const override; private: - Offsets getDefaultRefreshRateOffsets() { return mDefaultRefreshRateOffsets; } - Offsets getHighRefreshRateOffsets() { return mHighRefreshRateOffsets; } - std::atomic mRefreshRateType = RefreshRateConfigs::RefreshRateType::DEFAULT; - Offsets mDefaultRefreshRateOffsets; - Offsets mHighRefreshRateOffsets; + std::unordered_map mOffsets; nsecs_t mOffsetThresholdForNextVsync; }; } // namespace impl diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.cpp b/services/surfaceflinger/Scheduler/VSyncModulator.cpp index af8f445b30..381308ab34 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.cpp +++ b/services/surfaceflinger/Scheduler/VSyncModulator.cpp @@ -14,28 +14,36 @@ * limitations under the License. */ +#define ATRACE_TAG ATRACE_TAG_GRAPHICS + #include "VSyncModulator.h" +#include +#include + #include #include namespace android { +using RefreshRateType = scheduler::RefreshRateConfigs::RefreshRateType; +VSyncModulator::VSyncModulator() { + char value[PROPERTY_VALUE_MAX]; + property_get("debug.sf.vsync_trace_detailed_info", value, "0"); + mTraceDetailedInfo = atoi(value); + // Populate the offset map with some default offsets. + const Offsets defaultOffsets = {RefreshRateType::DEFAULT, 0, 0}; + setPhaseOffsets(defaultOffsets, defaultOffsets, defaultOffsets, 0); +} + void VSyncModulator::setPhaseOffsets(Offsets early, Offsets earlyGl, Offsets late, nsecs_t thresholdForNextVsync) { - mEarlyOffsets = early; - mEarlyGlOffsets = earlyGl; - mLateOffsets = late; + std::lock_guard lock(mMutex); + mOffsetMap.insert_or_assign(OffsetType::Early, early); + mOffsetMap.insert_or_assign(OffsetType::EarlyGl, earlyGl); + mOffsetMap.insert_or_assign(OffsetType::Late, late); mThresholdForNextVsync = thresholdForNextVsync; - - if (mSfConnectionHandle && late.sf != mOffsets.load().sf) { - mScheduler->setPhaseOffset(mSfConnectionHandle, late.sf); - } - - if (mAppConnectionHandle && late.app != mOffsets.load().app) { - mScheduler->setPhaseOffset(mAppConnectionHandle, late.app); - } - mOffsets = late; + updateOffsetsLocked(); } void VSyncModulator::setTransactionStart(Scheduler::TransactionStart transactionStart) { @@ -93,21 +101,35 @@ void VSyncModulator::onRefreshed(bool usedRenderEngine) { } VSyncModulator::Offsets VSyncModulator::getOffsets() { + std::lock_guard lock(mMutex); + return mOffsetMap.at(mOffsetType); +} + +VSyncModulator::Offsets VSyncModulator::getNextOffsets() { + return mOffsetMap.at(getNextOffsetType()); +} + +VSyncModulator::OffsetType VSyncModulator::getNextOffsetType() { // Early offsets are used if we're in the middle of a refresh rate // change, or if we recently begin a transaction. if (mTransactionStart == Scheduler::TransactionStart::EARLY || mRemainingEarlyFrameCount > 0 || mRefreshRateChangePending) { - return mEarlyOffsets; + return OffsetType::Early; } else if (mRemainingRenderEngineUsageCount > 0) { - return mEarlyGlOffsets; + return OffsetType::EarlyGl; } else { - return mLateOffsets; + return OffsetType::Late; } } void VSyncModulator::updateOffsets() { - const Offsets desired = getOffsets(); - const Offsets current = mOffsets; + std::lock_guard lock(mMutex); + updateOffsetsLocked(); +} + +void VSyncModulator::updateOffsetsLocked() { + const Offsets desired = getNextOffsets(); + const Offsets current = mOffsetMap.at(mOffsetType); bool changed = false; if (desired.sf != current.sf) { @@ -128,8 +150,29 @@ void VSyncModulator::updateOffsets() { } if (changed) { - mOffsets = desired; + updateOffsetType(); + } +} + +void VSyncModulator::updateOffsetType() { + mOffsetType = getNextOffsetType(); + if (!mTraceDetailedInfo) { + return; } + OffsetType type = mOffsetType; + Offsets offsets = mOffsetMap.at(type); + ATRACE_INT("Vsync-EarlyOffsetsOn", + offsets.fpsMode == RefreshRateType::DEFAULT && type == OffsetType::Early); + ATRACE_INT("Vsync-EarlyGLOffsetsOn", + offsets.fpsMode == RefreshRateType::DEFAULT && type == OffsetType::EarlyGl); + ATRACE_INT("Vsync-LateOffsetsOn", + offsets.fpsMode == RefreshRateType::DEFAULT && type == OffsetType::Late); + ATRACE_INT("Vsync-HighFpsEarlyOffsetsOn", + offsets.fpsMode == RefreshRateType::PERFORMANCE && type == OffsetType::Early); + ATRACE_INT("Vsync-HighFpsEarlyGLOffsetsOn", + offsets.fpsMode == RefreshRateType::PERFORMANCE && type == OffsetType::EarlyGl); + ATRACE_INT("Vsync-HighFpsLateOffsetsOn", + offsets.fpsMode == RefreshRateType::PERFORMANCE && type == OffsetType::Late); } } // namespace android diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.h b/services/surfaceflinger/Scheduler/VSyncModulator.h index 677862e4fe..c6374be83a 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.h +++ b/services/surfaceflinger/Scheduler/VSyncModulator.h @@ -39,13 +39,22 @@ private: const int MIN_EARLY_GL_FRAME_COUNT_TRANSACTION = 2; public: + VSyncModulator(); + // Wrapper for a collection of surfaceflinger/app offsets for a particular // configuration . struct Offsets { + scheduler::RefreshRateConfigs::RefreshRateType fpsMode; nsecs_t sf; nsecs_t app; }; + enum class OffsetType { + Early, + EarlyGl, + Late, + }; + // Sets the phase offsets // // sfEarly: The phase offset when waking up SF early, which happens when marking a transaction @@ -57,13 +66,7 @@ public: // appEarlyGl: Like sfEarlyGl, but for the app-vsync. // appLate: The regular app vsync phase offset. void setPhaseOffsets(Offsets early, Offsets earlyGl, Offsets late, - nsecs_t thresholdForNextVsync); - - // Returns the configured early offsets. - Offsets getEarlyOffsets() const { return mEarlyOffsets; } - - // Returns the configured early gl offsets. - Offsets getEarlyGlOffsets() const { return mEarlyGlOffsets; } + nsecs_t thresholdForNextVsync) EXCLUDES(mMutex); // Sets handles to the SF and app event threads. void setEventThreads(EventThread* sfEventThread, EventThread* appEventThread) { @@ -100,15 +103,22 @@ public: // frame. void onRefreshed(bool usedRenderEngine); - // Returns the offsets that should be used. - Offsets getOffsets(); + // Returns the offsets that we are currently using + Offsets getOffsets() EXCLUDES(mMutex); private: - void updateOffsets(); - - Offsets mLateOffsets; - Offsets mEarlyOffsets; - Offsets mEarlyGlOffsets; + // Returns the next offsets that we should be using + Offsets getNextOffsets() REQUIRES(mMutex); + // Returns the next offset type that we should use. + OffsetType getNextOffsetType(); + // Updates offsets and persists them into the scheduler framework. + void updateOffsets() EXCLUDES(mMutex); + void updateOffsetsLocked() REQUIRES(mMutex); + // Updates the internal offset type. + void updateOffsetType() REQUIRES(mMutex); + + mutable std::mutex mMutex; + std::unordered_map mOffsetMap GUARDED_BY(mMutex); nsecs_t mThresholdForNextVsync; EventThread* mSfEventThread = nullptr; @@ -118,13 +128,15 @@ private: Scheduler::ConnectionHandle* mAppConnectionHandle = nullptr; Scheduler::ConnectionHandle* mSfConnectionHandle = nullptr; - std::atomic mOffsets; + OffsetType mOffsetType GUARDED_BY(mMutex) = OffsetType::Late; std::atomic mTransactionStart = Scheduler::TransactionStart::NORMAL; std::atomic mRefreshRateChangePending = false; std::atomic mRemainingEarlyFrameCount = 0; std::atomic mRemainingRenderEngineUsageCount = 0; + + bool mTraceDetailedInfo = false; }; } // namespace android diff --git a/services/surfaceflinger/tests/unittests/FakePhaseOffsets.h b/services/surfaceflinger/tests/unittests/FakePhaseOffsets.h index 96121bb088..1d7501102e 100644 --- a/services/surfaceflinger/tests/unittests/FakePhaseOffsets.h +++ b/services/surfaceflinger/tests/unittests/FakePhaseOffsets.h @@ -23,6 +23,8 @@ namespace android { namespace scheduler { +using RefreshRateType = RefreshRateConfigs::RefreshRateType; + class FakePhaseOffsets : public android::scheduler::PhaseOffsets { nsecs_t FAKE_PHASE_OFFSET_NS = 0; @@ -34,20 +36,20 @@ public: nsecs_t getCurrentSfOffset() override { return FAKE_PHASE_OFFSET_NS; } PhaseOffsets::Offsets getOffsetsForRefreshRate( - RefreshRateConfigs::RefreshRateType /*refreshRateType*/) const override { + RefreshRateType /*refreshRateType*/) const override { return getCurrentOffsets(); } // Returns early, early GL, and late offsets for Apps and SF. PhaseOffsets::Offsets getCurrentOffsets() const override { - return Offsets{{FAKE_PHASE_OFFSET_NS, FAKE_PHASE_OFFSET_NS}, - {FAKE_PHASE_OFFSET_NS, FAKE_PHASE_OFFSET_NS}, - {FAKE_PHASE_OFFSET_NS, FAKE_PHASE_OFFSET_NS}}; + return Offsets{{RefreshRateType::DEFAULT, FAKE_PHASE_OFFSET_NS, FAKE_PHASE_OFFSET_NS}, + {RefreshRateType::DEFAULT, FAKE_PHASE_OFFSET_NS, FAKE_PHASE_OFFSET_NS}, + {RefreshRateType::DEFAULT, FAKE_PHASE_OFFSET_NS, FAKE_PHASE_OFFSET_NS}}; } // This function should be called when the device is switching between different // refresh rates, to properly update the offsets. - void setRefreshRateType(RefreshRateConfigs::RefreshRateType /*refreshRateType*/) override {} + void setRefreshRateType(RefreshRateType /*refreshRateType*/) override {} nsecs_t getOffsetThresholdForNextVsync() const override { return FAKE_PHASE_OFFSET_NS; } @@ -56,4 +58,4 @@ public: }; } // namespace scheduler -} // namespace android \ No newline at end of file +} // namespace android -- cgit v1.2.3-59-g8ed1b From 769ab6f949369468ee6355d00e490ad14a2f3721 Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Wed, 19 Jun 2019 08:13:28 -0700 Subject: SF: do not extend DisplayDevice lifetime DisplayDevice lifetime was extended, resulting in stack corruption due to lifetime issue Fixes: 135211720 Test: 1h45m continual rotation with PIP video playing without crashing Test: Functional testing to make sure b/132394665 is still fixed. Change-Id: I1c4b3946a6da7994ff442a2345c1c878aa899ee4 --- services/surfaceflinger/RegionSamplingThread.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 8b85d4cdbf..72bbf5dc26 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -342,9 +342,19 @@ void RegionSamplingThread::captureSample() { } const auto device = mFlinger.getDefaultDisplayDevice(); - const auto display = device->getCompositionDisplay(); - const auto state = display->getState(); - const auto orientation = static_cast(state.orientation); + const auto orientation = [](uint32_t orientation) { + switch (orientation) { + default: + case DisplayState::eOrientationDefault: + return ui::Transform::ROT_0; + case DisplayState::eOrientation90: + return ui::Transform::ROT_90; + case DisplayState::eOrientation180: + return ui::Transform::ROT_180; + case DisplayState::eOrientation270: + return ui::Transform::ROT_270; + } + }(device->getOrientation()); std::vector descriptors; Region sampleRegion; -- cgit v1.2.3-59-g8ed1b From 81ca00ff8cd05913fe0e97e0f97b3a9ae7ed3299 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Wed, 19 Jun 2019 17:01:57 -0700 Subject: SurfaceFlinger: DispSync: negative offsets when model is unlocked When getting a HWVsync timestamp in DispSync, we correct the last event time to reflect the actual HWVsync time by setting it based on HWVsync. This logic needs to account for both positive and negative offsets. Bug: 135631964 Test: sanity Change-Id: I77a1e13d739e558d6cdf43c298e6fcee49d517b5 --- services/surfaceflinger/Scheduler/DispSync.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/DispSync.cpp b/services/surfaceflinger/Scheduler/DispSync.cpp index 81be372091..83fd42b43f 100644 --- a/services/surfaceflinger/Scheduler/DispSync.cpp +++ b/services/surfaceflinger/Scheduler/DispSync.cpp @@ -92,8 +92,12 @@ public: mPeriod = period; if (!mModelLocked && referenceTimeChanged) { for (auto& eventListener : mEventListeners) { - eventListener.mLastEventTime = - mReferenceTime - mPeriod + mPhase + eventListener.mPhase; + eventListener.mLastEventTime = mReferenceTime + mPhase + eventListener.mPhase; + // If mLastEventTime is after mReferenceTime (can happen when positive phase offsets + // are used) we treat it as like it happened in previous period. + if (eventListener.mLastEventTime > mReferenceTime) { + eventListener.mLastEventTime -= mPeriod; + } } } if (mTraceDetailedInfo) { -- cgit v1.2.3-59-g8ed1b From b656aed06739e5a5ec7563970452237dca431edd Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Fri, 21 Jun 2019 16:27:05 -0700 Subject: [SurfaceFlinger] Store current offsets in VSyncModulator. This way updated offsets are properly persisted to DispSyncSource. Bug: 135770834 Test: systrace shows correct offsets Change-Id: I34ed81ebecfa8a4f9826d4a7713da0ec0a8b23de --- .../surfaceflinger/Scheduler/VSyncModulator.cpp | 29 +++++++++++----------- services/surfaceflinger/Scheduler/VSyncModulator.h | 6 ++--- 2 files changed, 17 insertions(+), 18 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.cpp b/services/surfaceflinger/Scheduler/VSyncModulator.cpp index 381308ab34..d452c19b68 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.cpp +++ b/services/surfaceflinger/Scheduler/VSyncModulator.cpp @@ -102,7 +102,7 @@ void VSyncModulator::onRefreshed(bool usedRenderEngine) { VSyncModulator::Offsets VSyncModulator::getOffsets() { std::lock_guard lock(mMutex); - return mOffsetMap.at(mOffsetType); + return mOffsets; } VSyncModulator::Offsets VSyncModulator::getNextOffsets() { @@ -129,13 +129,13 @@ void VSyncModulator::updateOffsets() { void VSyncModulator::updateOffsetsLocked() { const Offsets desired = getNextOffsets(); - const Offsets current = mOffsetMap.at(mOffsetType); + const Offsets current = mOffsets; bool changed = false; if (desired.sf != current.sf) { if (mSfConnectionHandle != nullptr) { mScheduler->setPhaseOffset(mSfConnectionHandle, desired.sf); - } else { + } else if (mSfEventThread != nullptr) { mSfEventThread->setPhaseOffset(desired.sf); } changed = true; @@ -143,36 +143,35 @@ void VSyncModulator::updateOffsetsLocked() { if (desired.app != current.app) { if (mAppConnectionHandle != nullptr) { mScheduler->setPhaseOffset(mAppConnectionHandle, desired.app); - } else { + } else if (mAppEventThread != nullptr) { mAppEventThread->setPhaseOffset(desired.app); } changed = true; } if (changed) { - updateOffsetType(); + flushOffsets(); } } -void VSyncModulator::updateOffsetType() { - mOffsetType = getNextOffsetType(); +void VSyncModulator::flushOffsets() { + OffsetType type = getNextOffsetType(); + mOffsets = mOffsetMap.at(type); if (!mTraceDetailedInfo) { return; } - OffsetType type = mOffsetType; - Offsets offsets = mOffsetMap.at(type); ATRACE_INT("Vsync-EarlyOffsetsOn", - offsets.fpsMode == RefreshRateType::DEFAULT && type == OffsetType::Early); + mOffsets.fpsMode == RefreshRateType::DEFAULT && type == OffsetType::Early); ATRACE_INT("Vsync-EarlyGLOffsetsOn", - offsets.fpsMode == RefreshRateType::DEFAULT && type == OffsetType::EarlyGl); + mOffsets.fpsMode == RefreshRateType::DEFAULT && type == OffsetType::EarlyGl); ATRACE_INT("Vsync-LateOffsetsOn", - offsets.fpsMode == RefreshRateType::DEFAULT && type == OffsetType::Late); + mOffsets.fpsMode == RefreshRateType::DEFAULT && type == OffsetType::Late); ATRACE_INT("Vsync-HighFpsEarlyOffsetsOn", - offsets.fpsMode == RefreshRateType::PERFORMANCE && type == OffsetType::Early); + mOffsets.fpsMode == RefreshRateType::PERFORMANCE && type == OffsetType::Early); ATRACE_INT("Vsync-HighFpsEarlyGLOffsetsOn", - offsets.fpsMode == RefreshRateType::PERFORMANCE && type == OffsetType::EarlyGl); + mOffsets.fpsMode == RefreshRateType::PERFORMANCE && type == OffsetType::EarlyGl); ATRACE_INT("Vsync-HighFpsLateOffsetsOn", - offsets.fpsMode == RefreshRateType::PERFORMANCE && type == OffsetType::Late); + mOffsets.fpsMode == RefreshRateType::PERFORMANCE && type == OffsetType::Late); } } // namespace android diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.h b/services/surfaceflinger/Scheduler/VSyncModulator.h index c6374be83a..10cf8e6cc9 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.h +++ b/services/surfaceflinger/Scheduler/VSyncModulator.h @@ -114,8 +114,8 @@ private: // Updates offsets and persists them into the scheduler framework. void updateOffsets() EXCLUDES(mMutex); void updateOffsetsLocked() REQUIRES(mMutex); - // Updates the internal offset type. - void updateOffsetType() REQUIRES(mMutex); + // Updates the internal offsets and offset type. + void flushOffsets() REQUIRES(mMutex); mutable std::mutex mMutex; std::unordered_map mOffsetMap GUARDED_BY(mMutex); @@ -128,7 +128,7 @@ private: Scheduler::ConnectionHandle* mAppConnectionHandle = nullptr; Scheduler::ConnectionHandle* mSfConnectionHandle = nullptr; - OffsetType mOffsetType GUARDED_BY(mMutex) = OffsetType::Late; + Offsets mOffsets GUARDED_BY(mMutex) = {Scheduler::RefreshRateType::DEFAULT, 0, 0}; std::atomic mTransactionStart = Scheduler::TransactionStart::NORMAL; -- cgit v1.2.3-59-g8ed1b From ad083c40b7593e0cc9283588ac710ddf2124337a Mon Sep 17 00:00:00 2001 From: Ana Krulec Date: Wed, 26 Jun 2019 16:28:08 -0700 Subject: SF: Don't bump to PERFORMANCE refresh rate with infrequent updates Testing scenario: 1. Set brightness around 50%-55%, set dark theme. 2. Open Chrome. 3. Click search bar. 4. Can see the cursor show up. Notice the flickering of the screen. Explanation: Kernel idle timer detects inactivity after 100ms, and turns the refresh rate to 60Hz. When a cursor update happens (every 500ms), SF receives a new frame, notifies kernel, the refresh rate bumps to 90Hz. After 100ms the kernel again decreases the refresh rate to 60Hz. Desired goals: Stop the flickering (eg. changing between 60-90Hz too often). Continue having low battery impact. Solution in this AG: Add logic to SF to detect infrequent updates (for all layers). Description of the algorithm: 1) Store the timestamp of the last two buffers. 2) If the first buffer is older than 250 ms, detect inactivity, go into DEFAULT refresh rate. 3) EXIT: on touch event, layer requests 2 or more frames in less than 250ms. NOTE: if the application is explicitly requesting 90Hz, SF does not override that. Idle kernel still kicks in, and the flickering happens. tested on Chrome v74 Beta, and messaging app. Test: manual, b/135009095 Bug: 135718869 Change-Id: I72d8cd48b3ec900989afcf0fab1cdc3046b87274 --- services/surfaceflinger/Scheduler/LayerHistory.cpp | 4 ++- services/surfaceflinger/Scheduler/LayerHistory.h | 3 ++- services/surfaceflinger/Scheduler/LayerInfo.cpp | 5 ++-- services/surfaceflinger/Scheduler/LayerInfo.h | 31 +++++++++++++++++++--- services/surfaceflinger/Scheduler/Scheduler.cpp | 7 +++-- services/surfaceflinger/Scheduler/SchedulerUtils.h | 15 ++++++----- .../tests/unittests/LayerHistoryTest.cpp | 17 ++++++------ 7 files changed, 59 insertions(+), 23 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/LayerHistory.cpp b/services/surfaceflinger/Scheduler/LayerHistory.cpp index e762af3ec5..f80c2336f7 100644 --- a/services/surfaceflinger/Scheduler/LayerHistory.cpp +++ b/services/surfaceflinger/Scheduler/LayerHistory.cpp @@ -46,11 +46,13 @@ LayerHistory::LayerHistory() { LayerHistory::~LayerHistory() = default; std::unique_ptr LayerHistory::createLayer(const std::string name, + float minRefreshRate, float maxRefreshRate) { const int64_t id = sNextId++; std::lock_guard lock(mLock); - mInactiveLayerInfos.emplace(id, std::make_shared(name, maxRefreshRate)); + mInactiveLayerInfos.emplace(id, + std::make_shared(name, minRefreshRate, maxRefreshRate)); return std::make_unique(*this, id); } diff --git a/services/surfaceflinger/Scheduler/LayerHistory.h b/services/surfaceflinger/Scheduler/LayerHistory.h index 2569b4638a..5598cc1cf5 100644 --- a/services/surfaceflinger/Scheduler/LayerHistory.h +++ b/services/surfaceflinger/Scheduler/LayerHistory.h @@ -53,7 +53,8 @@ public: ~LayerHistory(); // When the layer is first created, register it. - std::unique_ptr createLayer(const std::string name, float maxRefreshRate); + std::unique_ptr createLayer(const std::string name, float minRefreshRate, + float maxRefreshRate); // Method for inserting layers and their requested present time into the unordered map. void insert(const std::unique_ptr& layerHandle, nsecs_t presentTime, bool isHdr); diff --git a/services/surfaceflinger/Scheduler/LayerInfo.cpp b/services/surfaceflinger/Scheduler/LayerInfo.cpp index 3104724041..beddb9be61 100644 --- a/services/surfaceflinger/Scheduler/LayerInfo.cpp +++ b/services/surfaceflinger/Scheduler/LayerInfo.cpp @@ -24,9 +24,10 @@ namespace android { namespace scheduler { -LayerInfo::LayerInfo(const std::string name, float maxRefreshRate) +LayerInfo::LayerInfo(const std::string name, float minRefreshRate, float maxRefreshRate) : mName(name), mMinRefreshDuration(1e9f / maxRefreshRate), + mLowActivityRefreshDuration(1e9f / minRefreshRate), mRefreshRateHistory(mMinRefreshDuration) {} LayerInfo::~LayerInfo() = default; @@ -47,7 +48,7 @@ void LayerInfo::setLastPresentTime(nsecs_t lastPresentTime) { const nsecs_t timeDiff = lastPresentTime - mLastPresentTime; mLastPresentTime = lastPresentTime; // Ignore time diff that are too high - those are stale values - if (timeDiff > TIME_EPSILON_NS.count()) return; + if (timeDiff > OBSOLETE_TIME_EPSILON_NS.count()) return; const nsecs_t refreshDuration = (timeDiff > 0) ? timeDiff : mMinRefreshDuration; mRefreshRateHistory.insertRefreshRate(refreshDuration); } diff --git a/services/surfaceflinger/Scheduler/LayerInfo.h b/services/surfaceflinger/Scheduler/LayerInfo.h index 90d426969c..2c50053ebd 100644 --- a/services/surfaceflinger/Scheduler/LayerInfo.h +++ b/services/surfaceflinger/Scheduler/LayerInfo.h @@ -96,8 +96,9 @@ class LayerInfo { return false; } - // The last update should not be older than TIME_EPSILON_NS nanoseconds. - const int64_t obsoleteEpsilon = systemTime() - scheduler::TIME_EPSILON_NS.count(); + // The last update should not be older than OBSOLETE_TIME_EPSILON_NS nanoseconds. + const int64_t obsoleteEpsilon = + systemTime() - scheduler::OBSOLETE_TIME_EPSILON_NS.count(); if (mElements.at(mElements.size() - 1) < obsoleteEpsilon) { return false; } @@ -105,6 +106,25 @@ class LayerInfo { return true; } + bool isLowActivityLayer() const { + // We want to make sure that we received more than two frames from the layer + // in order to check low activity. + if (mElements.size() < 2) { + return false; + } + + const int64_t obsoleteEpsilon = + systemTime() - scheduler::LOW_ACTIVITY_EPSILON_NS.count(); + // Check the frame before last to determine whether there is low activity. + // If that frame is older than LOW_ACTIVITY_EPSILON_NS, the layer is sending + // infrequent updates. + if (mElements.at(mElements.size() - 2) < obsoleteEpsilon) { + return true; + } + + return false; + } + void clearHistory() { mElements.clear(); } private: @@ -114,7 +134,7 @@ class LayerInfo { }; public: - LayerInfo(const std::string name, float maxRefreshRate); + LayerInfo(const std::string name, float minRefreshRate, float maxRefreshRate); ~LayerInfo(); LayerInfo(const LayerInfo&) = delete; @@ -144,6 +164,10 @@ public: // Calculate the average refresh rate. float getDesiredRefreshRate() const { std::lock_guard lock(mLock); + + if (mPresentTimeHistory.isLowActivityLayer()) { + return 1e9f / mLowActivityRefreshDuration; + } return mRefreshRateHistory.getRefreshRateAvg(); } @@ -175,6 +199,7 @@ public: private: const std::string mName; const nsecs_t mMinRefreshDuration; + const nsecs_t mLowActivityRefreshDuration; mutable std::mutex mLock; nsecs_t mLastUpdatedTime GUARDED_BY(mLock) = 0; nsecs_t mLastPresentTime GUARDED_BY(mLock) = 0; diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index bb24f73834..99d6faee1c 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -330,8 +330,11 @@ std::unique_ptr Scheduler::registerLayer( : RefreshRateType::PERFORMANCE; const auto refreshRate = mRefreshRateConfigs.getRefreshRate(refreshRateType); - const uint32_t fps = (refreshRate) ? refreshRate->fps : 0; - return mLayerHistory.createLayer(name, fps); + const uint32_t performanceFps = (refreshRate) ? refreshRate->fps : 0; + + const auto defaultRefreshRate = mRefreshRateConfigs.getRefreshRate(RefreshRateType::DEFAULT); + const uint32_t defaultFps = (defaultRefreshRate) ? defaultRefreshRate->fps : 0; + return mLayerHistory.createLayer(name, defaultFps, performanceFps); } void Scheduler::addLayerPresentTimeAndHDR( diff --git a/services/surfaceflinger/Scheduler/SchedulerUtils.h b/services/surfaceflinger/Scheduler/SchedulerUtils.h index d3b1bd07c4..ced1899109 100644 --- a/services/surfaceflinger/Scheduler/SchedulerUtils.h +++ b/services/surfaceflinger/Scheduler/SchedulerUtils.h @@ -36,13 +36,16 @@ static constexpr size_t ARRAY_SIZE = 30; static constexpr int SCREEN_OFF_CONFIG_ID = -1; static constexpr uint32_t HWC2_SCREEN_OFF_CONFIG_ID = 0xffffffff; -// This number is used when we try to determine how long does a given layer stay relevant. -// The value is set based on testing different scenarios. -static constexpr std::chrono::nanoseconds TIME_EPSILON_NS = 200ms; - // This number is used when we try to determine how long do we keep layer information around -// before we remove it. -static constexpr std::chrono::nanoseconds OBSOLETE_TIME_EPSILON_NS = 200ms; +// before we remove it. It is also used to determine how long the layer stays relevant. +// This time period captures infrequent updates when playing YouTube video with static image, +// or waiting idle in messaging app, when cursor is blinking. +static constexpr std::chrono::nanoseconds OBSOLETE_TIME_EPSILON_NS = 1200ms; + +// Layer is considered low activity if the buffers come more than LOW_ACTIVITY_EPSILON_NS +// apart. This is helping SF to vote for lower refresh rates when there is not activity +// in screen. +static constexpr std::chrono::nanoseconds LOW_ACTIVITY_EPSILON_NS = 250ms; // Calculates the statistical mean (average) in the data structure (array, vector). The // function does not modify the contents of the array. diff --git a/services/surfaceflinger/tests/unittests/LayerHistoryTest.cpp b/services/surfaceflinger/tests/unittests/LayerHistoryTest.cpp index 2b1dfa8f38..7ec90660ce 100644 --- a/services/surfaceflinger/tests/unittests/LayerHistoryTest.cpp +++ b/services/surfaceflinger/tests/unittests/LayerHistoryTest.cpp @@ -25,6 +25,7 @@ public: protected: std::unique_ptr mLayerHistory; + static constexpr float MIN_REFRESH_RATE = 30.f; static constexpr float MAX_REFRESH_RATE = 90.f; }; @@ -36,7 +37,7 @@ LayerHistoryTest::~LayerHistoryTest() {} namespace { TEST_F(LayerHistoryTest, oneLayer) { std::unique_ptr testLayer = - mLayerHistory->createLayer("TestLayer", MAX_REFRESH_RATE); + mLayerHistory->createLayer("TestLayer", MIN_REFRESH_RATE, MAX_REFRESH_RATE); mLayerHistory->setVisibility(testLayer, true); mLayerHistory->insert(testLayer, 0, false /*isHDR*/); @@ -60,7 +61,7 @@ TEST_F(LayerHistoryTest, oneLayer) { TEST_F(LayerHistoryTest, oneHDRLayer) { std::unique_ptr testLayer = - mLayerHistory->createLayer("TestHDRLayer", MAX_REFRESH_RATE); + mLayerHistory->createLayer("TestHDRLayer", MIN_REFRESH_RATE, MAX_REFRESH_RATE); mLayerHistory->setVisibility(testLayer, true); mLayerHistory->insert(testLayer, 0, true /*isHDR*/); @@ -74,7 +75,7 @@ TEST_F(LayerHistoryTest, oneHDRLayer) { TEST_F(LayerHistoryTest, explicitTimestamp) { std::unique_ptr test30FpsLayer = - mLayerHistory->createLayer("30FpsLayer", MAX_REFRESH_RATE); + mLayerHistory->createLayer("30FpsLayer", MIN_REFRESH_RATE, MAX_REFRESH_RATE); mLayerHistory->setVisibility(test30FpsLayer, true); nsecs_t startTime = systemTime(); @@ -87,13 +88,13 @@ TEST_F(LayerHistoryTest, explicitTimestamp) { TEST_F(LayerHistoryTest, multipleLayers) { std::unique_ptr testLayer = - mLayerHistory->createLayer("TestLayer", MAX_REFRESH_RATE); + mLayerHistory->createLayer("TestLayer", MIN_REFRESH_RATE, MAX_REFRESH_RATE); mLayerHistory->setVisibility(testLayer, true); std::unique_ptr test30FpsLayer = - mLayerHistory->createLayer("30FpsLayer", MAX_REFRESH_RATE); + mLayerHistory->createLayer("30FpsLayer", MIN_REFRESH_RATE, MAX_REFRESH_RATE); mLayerHistory->setVisibility(test30FpsLayer, true); std::unique_ptr testLayer2 = - mLayerHistory->createLayer("TestLayer2", MAX_REFRESH_RATE); + mLayerHistory->createLayer("TestLayer2", MIN_REFRESH_RATE, MAX_REFRESH_RATE); mLayerHistory->setVisibility(testLayer2, true); nsecs_t startTime = systemTime(); @@ -119,8 +120,8 @@ TEST_F(LayerHistoryTest, multipleLayers) { mLayerHistory->insert(testLayer2, 0, false /*isHDR*/); } EXPECT_FLOAT_EQ(MAX_REFRESH_RATE, mLayerHistory->getDesiredRefreshRateAndHDR().first); - // After 100 ms frames become obsolete. - std::this_thread::sleep_for(std::chrono::milliseconds(500)); + // After 1200 ms frames become obsolete. + std::this_thread::sleep_for(std::chrono::milliseconds(1500)); // Insert the 31st frame. mLayerHistory->insert(test30FpsLayer, startTime + (30 * 33333333), false /*isHDR*/); EXPECT_FLOAT_EQ(30.f, mLayerHistory->getDesiredRefreshRateAndHDR().first); -- cgit v1.2.3-59-g8ed1b From 09be73f64603ef71074d9fbf553fec5c7652d283 Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Tue, 2 Jul 2019 14:29:18 -0700 Subject: Surfaceflinger: adjust content detection fps selection Select the FPS with minimal error in the first pass and then adjust based on ratio/margin. The CL changes content driven FPS selection logic, before the change, the code switch to 90hz configs when mContentRefreshRate > 64 and with the CL it will switch to 90hz when mContentRefreshRate > 75. The second pass to select 90hz when mContentRefreshRate is 45fps is kept as is. It also simplified and removed 5% margin in the first pass. Test: boot and take SF trace Bug: 136472613 Change-Id: I4445386a301b7fcdd22d71bd49f45540cc414174 --- services/surfaceflinger/Scheduler/Scheduler.cpp | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index 99d6faee1c..cfdbd91e35 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -523,22 +523,19 @@ Scheduler::RefreshRateType Scheduler::calculateRefreshRateType() { return RefreshRateType::PERFORMANCE; } - // Content detection is on, find the appropriate refresh rate - // Start with the smallest refresh rate which is within a margin of the content - RefreshRateType currRefreshRateType = RefreshRateType::PERFORMANCE; - constexpr float MARGIN = 0.05f; - auto iter = mRefreshRateConfigs.getRefreshRates().cbegin(); - while (iter != mRefreshRateConfigs.getRefreshRates().cend()) { - if (iter->second->fps >= mContentRefreshRate * (1 - MARGIN)) { - currRefreshRateType = iter->first; - break; - } - ++iter; - } + // Content detection is on, find the appropriate refresh rate with minimal error + auto iter = min_element(mRefreshRateConfigs.getRefreshRates().cbegin(), + mRefreshRateConfigs.getRefreshRates().cend(), + [rate = mContentRefreshRate](const auto& l, const auto& r) -> bool { + return std::abs(l.second->fps - static_cast(rate)) < + std::abs(r.second->fps - static_cast(rate)); + }); + RefreshRateType currRefreshRateType = iter->first; // Some content aligns better on higher refresh rate. For example for 45fps we should choose // 90Hz config. However we should still prefer a lower refresh rate if the content doesn't // align well with both + constexpr float MARGIN = 0.05f; float ratio = mRefreshRateConfigs.getRefreshRate(currRefreshRateType)->fps / float(mContentRefreshRate); if (std::abs(std::round(ratio) - ratio) > MARGIN) { -- cgit v1.2.3-59-g8ed1b From da901bfc4f15723f0a7567ecd09742fd166df2f0 Mon Sep 17 00:00:00 2001 From: Yichi Chen Date: Fri, 28 Jun 2019 14:58:27 +0800 Subject: SF: enable device-specific dataspace for color space agnostic surfaces To reduce the DPU loading in color conversion, we enable device-specific dataspace for color space agnostic surfaces. Since the type of surfaces usually provide gray-level surfaces to users, it can be acceptable to ignore the color conversion on them. Bug: 134783740 Bug: 135140940 Test: Check ScreenDecorOverlays in expected dataspace Test: Play HDR video on B1 and C2 and check dataspace Change-Id: I7b11e11d2015eb5c8dfdc372e2c7ffcb40a2ac1d --- services/surfaceflinger/SurfaceFlinger.cpp | 14 +++++++++++--- services/surfaceflinger/SurfaceFlinger.h | 1 + services/surfaceflinger/SurfaceFlingerProperties.cpp | 8 ++++++++ services/surfaceflinger/SurfaceFlingerProperties.h | 3 +++ .../sysprop/SurfaceFlingerProperties.sysprop | 14 ++++++++++++++ services/surfaceflinger/sysprop/api/system-current.txt | 1 + 6 files changed, 38 insertions(+), 3 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 459cd0af89..6d9dc97c5a 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -311,6 +311,9 @@ SurfaceFlinger::SurfaceFlinger(Factory& factory) : SurfaceFlinger(factory, SkipI wideColorGamutCompositionPixelFormat = static_cast(wcg_composition_pixel_format(ui::PixelFormat::RGBA_8888)); + mColorSpaceAgnosticDataspace = + static_cast(color_space_agnostic_dataspace(Dataspace::UNKNOWN)); + useContextPriority = use_context_priority(true); auto tmpPrimaryDisplayOrientation = primary_display_orientation( @@ -1879,7 +1882,14 @@ void SurfaceFlinger::calculateWorkingSet() { RenderIntent renderIntent; pickColorMode(displayDevice, &colorMode, &targetDataspace, &renderIntent); display->setColorMode(colorMode, targetDataspace, renderIntent); + + if (isHdrColorMode(colorMode)) { + targetDataspace = Dataspace::UNKNOWN; + } else if (mColorSpaceAgnosticDataspace != Dataspace::UNKNOWN) { + targetDataspace = mColorSpaceAgnosticDataspace; + } } + for (auto& layer : displayDevice->getVisibleLayersSortedByZ()) { if (layer->isHdrY410()) { layer->forceClientComposition(displayDevice); @@ -1906,9 +1916,7 @@ void SurfaceFlinger::calculateWorkingSet() { const auto& displayState = display->getState(); layer->setPerFrameData(displayDevice, displayState.transform, displayState.viewport, - displayDevice->getSupportedPerFrameMetadata(), - isHdrColorMode(displayState.colorMode) ? Dataspace::UNKNOWN - : targetDataspace); + displayDevice->getSupportedPerFrameMetadata(), targetDataspace); } } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 6d4b2d755c..556a4b9be5 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -1115,6 +1115,7 @@ private: ui::Dataspace mDefaultCompositionDataspace; ui::Dataspace mWideColorGamutCompositionDataspace; + ui::Dataspace mColorSpaceAgnosticDataspace; SurfaceFlingerBE mBE; std::unique_ptr mCompositionEngine; diff --git a/services/surfaceflinger/SurfaceFlingerProperties.cpp b/services/surfaceflinger/SurfaceFlingerProperties.cpp index 2b33ba1746..237208d39f 100644 --- a/services/surfaceflinger/SurfaceFlingerProperties.cpp +++ b/services/surfaceflinger/SurfaceFlingerProperties.cpp @@ -218,6 +218,14 @@ int32_t wcg_composition_pixel_format(PixelFormat defaultValue) { return static_cast(defaultValue); } +int64_t color_space_agnostic_dataspace(Dataspace defaultValue) { + auto temp = SurfaceFlingerProperties::color_space_agnostic_dataspace(); + if (temp.has_value()) { + return *temp; + } + return static_cast(defaultValue); +} + int32_t set_idle_timer_ms(int32_t defaultValue) { auto temp = SurfaceFlingerProperties::set_idle_timer_ms(); if (temp.has_value()) { diff --git a/services/surfaceflinger/SurfaceFlingerProperties.h b/services/surfaceflinger/SurfaceFlingerProperties.h index 1964ccd582..b5418d6538 100644 --- a/services/surfaceflinger/SurfaceFlingerProperties.h +++ b/services/surfaceflinger/SurfaceFlingerProperties.h @@ -70,6 +70,9 @@ int64_t wcg_composition_dataspace( int32_t wcg_composition_pixel_format( android::hardware::graphics::common::V1_2::PixelFormat defaultValue); +int64_t color_space_agnostic_dataspace( + android::hardware::graphics::common::V1_2::Dataspace defaultValue); + int32_t set_idle_timer_ms(int32_t defaultValue); int32_t set_touch_timer_ms(int32_t defaultValue); diff --git a/services/surfaceflinger/sysprop/SurfaceFlingerProperties.sysprop b/services/surfaceflinger/sysprop/SurfaceFlingerProperties.sysprop index decabd5e88..f18f33c727 100644 --- a/services/surfaceflinger/sysprop/SurfaceFlingerProperties.sysprop +++ b/services/surfaceflinger/sysprop/SurfaceFlingerProperties.sysprop @@ -251,6 +251,20 @@ prop { prop_name: "ro.surface_flinger.wcg_composition_pixel_format" } +# colorSpaceAgnosticDataspace specifies the data space that +# SurfaceFlinger expects for surfaces which are color space agnostic. +# The variable works only when useColorManagement is specified. If +# unspecified, the data space follows what SurfaceFlinger expects for +# surfaces when useColorManagement is specified. + +prop { + api_name: "color_space_agnostic_dataspace" + type: Long + scope: System + access: Readonly + prop_name: "ro.surface_flinger.color_space_agnostic_dataspace" +} + # Return the native panel primary data. The data includes red, green, # blue and white. The primary format is CIE 1931 XYZ color space. # If unspecified, the primaries is sRGB gamut by default. diff --git a/services/surfaceflinger/sysprop/api/system-current.txt b/services/surfaceflinger/sysprop/api/system-current.txt index 6ae3ac15ee..89323c2ad0 100644 --- a/services/surfaceflinger/sysprop/api/system-current.txt +++ b/services/surfaceflinger/sysprop/api/system-current.txt @@ -2,6 +2,7 @@ package android.sysprop { public final class SurfaceFlingerProperties { + method public static java.util.Optional color_space_agnostic_dataspace(); method public static java.util.Optional default_composition_dataspace(); method public static java.util.Optional default_composition_pixel_format(); method public static java.util.List display_primary_blue(); -- cgit v1.2.3-59-g8ed1b From 42b3beb51f7f3e97762ed1db7ebc0decbfa56bb4 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Tue, 9 Jul 2019 18:09:52 -0700 Subject: SurfaceFlinger: add allowed display configs to dumpsys Dump the allowed display configs from SurfaceFlinger to help with debugging issues where the refresh rate does not match the policy. Test: adb shell dumpsys SurfaceFlinger Bug: 136503733 Change-Id: I74ec7d08901ad9edb1a3706249e8809574bf1e5f --- services/surfaceflinger/SurfaceFlinger.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 6d9dc97c5a..94eeef5eef 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -4707,6 +4707,16 @@ void SurfaceFlinger::dumpVSync(std::string& result) const { StringAppendF(&result, "Scheduler enabled."); StringAppendF(&result, "+ Smart 90 for video detection: %s\n\n", mUseSmart90ForVideo ? "on" : "off"); + StringAppendF(&result, "Allowed Display Configs: "); + for (int32_t configId : mAllowedDisplayConfigs) { + for (auto refresh : mRefreshRateConfigs.getRefreshRates()) { + if (refresh.second && refresh.second->configId == configId) { + StringAppendF(&result, "%dHz, ", refresh.second->fps); + } + } + } + StringAppendF(&result, "(config override by backdoor: %s)\n\n", + mDebugDisplayConfigSetByBackdoor ? "yes" : "no"); mScheduler->dump(mAppConnectionHandle, result); } -- cgit v1.2.3-59-g8ed1b From 11b6a70c6f6780f2f878ed3e103b5ffbb0b8fc26 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Thu, 27 Jun 2019 11:24:19 -0700 Subject: SurfaceFlinger: correct negative offset when refresh rate changes VsyncModulator sets the phase offset on DispSync source only when it changes. However, negative offsets depends on the vsync period so setting the same negative offset might result in a different wake up time i.e. -5ms on 60Hz is 11ms after the previous vsync where on 90Hz is 6ms after the previous vsync. Test: UI-Bench Bug: 135283780 Bug: 135297302 Change-Id: I6a05cd48d563a51d2ee38927c23d4946dd142f4b --- .../surfaceflinger/Scheduler/DispSyncSource.cpp | 4 ++++ .../surfaceflinger/Scheduler/VSyncModulator.cpp | 27 ++++++---------------- services/surfaceflinger/Scheduler/VSyncModulator.h | 9 -------- 3 files changed, 11 insertions(+), 29 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/DispSyncSource.cpp b/services/surfaceflinger/Scheduler/DispSyncSource.cpp index 026b55707c..5faf46e31e 100644 --- a/services/surfaceflinger/Scheduler/DispSyncSource.cpp +++ b/services/surfaceflinger/Scheduler/DispSyncSource.cpp @@ -78,6 +78,10 @@ void DispSyncSource::setPhaseOffset(nsecs_t phaseOffset) { // Normalize phaseOffset to [-period, period) const int numPeriods = phaseOffset / period; phaseOffset -= numPeriods * period; + if (mPhaseOffset == phaseOffset) { + return; + } + mPhaseOffset = phaseOffset; tracePhaseOffset(); diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.cpp b/services/surfaceflinger/Scheduler/VSyncModulator.cpp index d452c19b68..7a3bf8edaf 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.cpp +++ b/services/surfaceflinger/Scheduler/VSyncModulator.cpp @@ -129,29 +129,16 @@ void VSyncModulator::updateOffsets() { void VSyncModulator::updateOffsetsLocked() { const Offsets desired = getNextOffsets(); - const Offsets current = mOffsets; - - bool changed = false; - if (desired.sf != current.sf) { - if (mSfConnectionHandle != nullptr) { - mScheduler->setPhaseOffset(mSfConnectionHandle, desired.sf); - } else if (mSfEventThread != nullptr) { - mSfEventThread->setPhaseOffset(desired.sf); - } - changed = true; - } - if (desired.app != current.app) { - if (mAppConnectionHandle != nullptr) { - mScheduler->setPhaseOffset(mAppConnectionHandle, desired.app); - } else if (mAppEventThread != nullptr) { - mAppEventThread->setPhaseOffset(desired.app); - } - changed = true; + + if (mSfConnectionHandle != nullptr) { + mScheduler->setPhaseOffset(mSfConnectionHandle, desired.sf); } - if (changed) { - flushOffsets(); + if (mAppConnectionHandle != nullptr) { + mScheduler->setPhaseOffset(mAppConnectionHandle, desired.app); } + + flushOffsets(); } void VSyncModulator::flushOffsets() { diff --git a/services/surfaceflinger/Scheduler/VSyncModulator.h b/services/surfaceflinger/Scheduler/VSyncModulator.h index 10cf8e6cc9..ddbd221ef1 100644 --- a/services/surfaceflinger/Scheduler/VSyncModulator.h +++ b/services/surfaceflinger/Scheduler/VSyncModulator.h @@ -68,12 +68,6 @@ public: void setPhaseOffsets(Offsets early, Offsets earlyGl, Offsets late, nsecs_t thresholdForNextVsync) EXCLUDES(mMutex); - // Sets handles to the SF and app event threads. - void setEventThreads(EventThread* sfEventThread, EventThread* appEventThread) { - mSfEventThread = sfEventThread; - mAppEventThread = appEventThread; - } - // Sets the scheduler and vsync connection handlers. void setSchedulerAndHandles(Scheduler* scheduler, Scheduler::ConnectionHandle* appConnectionHandle, @@ -121,9 +115,6 @@ private: std::unordered_map mOffsetMap GUARDED_BY(mMutex); nsecs_t mThresholdForNextVsync; - EventThread* mSfEventThread = nullptr; - EventThread* mAppEventThread = nullptr; - Scheduler* mScheduler = nullptr; Scheduler::ConnectionHandle* mAppConnectionHandle = nullptr; Scheduler::ConnectionHandle* mSfConnectionHandle = nullptr; -- cgit v1.2.3-59-g8ed1b From 0e24a8385a63be6a799da902e1d5ffcbb7519c2a Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Wed, 10 Jul 2019 15:32:50 -0700 Subject: blast: fix leak on BufferStateLayer death SurfaceFlinger can occasionally leak graphic buffers. The leak happens when: 1) a transaction comes in and is placed in a queue 2) Chrome crashes 3) the parent layer is cleaned up 4) the child layer is told to release its buffer because it is no longer on screen 5) the transaction is applied with sets a callback handle on the layer which has a sp<> to the layer To fix this, the callback handle should not have a sp<> to layer. It is safe for the callback handle can have wp<> to the layer. The client side has a sp<> so during normal operation, SurfaceFlinger can promote the wp<>. The only time the promote will fail is if the client side is dead. If the client side is dead, there is no one to send a callback to so it doesn't matter if the promote fails. Bug: 135951943 Test: https://buganizer.corp.google.com/issues/135951943#comment34 Change-Id: I756ace14c90b03a6499a3187d235b42d91cdd05a --- services/surfaceflinger/TransactionCompletedThread.cpp | 10 ++++++++-- services/surfaceflinger/TransactionCompletedThread.h | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index 5cf8eb1a1d..fd466dedff 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -197,8 +197,14 @@ status_t TransactionCompletedThread::addCallbackHandle(const sp& } transactionStats->latchTime = handle->latchTime; - transactionStats->surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime, - handle->previousReleaseFence); + // If the layer has already been destroyed, don't add the SurfaceControl to the callback. + // The client side keeps a sp<> to the SurfaceControl so if the SurfaceControl has been + // destroyed the client side is dead and there won't be anyone to send the callback to. + sp surfaceControl = handle->surfaceControl.promote(); + if (surfaceControl) { + transactionStats->surfaceStats.emplace_back(surfaceControl, handle->acquireTime, + handle->previousReleaseFence); + } return NO_ERROR; } diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h index 21e2678701..e849f714d0 100644 --- a/services/surfaceflinger/TransactionCompletedThread.h +++ b/services/surfaceflinger/TransactionCompletedThread.h @@ -49,7 +49,7 @@ public: sp listener; std::vector callbackIds; - sp surfaceControl; + wp surfaceControl; bool releasePreviousBuffer = false; sp previousReleaseFence; -- cgit v1.2.3-59-g8ed1b From 187d2d812711188df2fc649e276f7cb6d0fb81e4 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Wed, 10 Jul 2019 16:18:48 -0700 Subject: SurfaceFlinger: store fps instead of duration in LayerInfo To get a better average of the content fps, we switch to store the momentarily fps rate instead of the refresh duration. For example: Frame#0 at 0ms Frame#1 at 100ms Frame#2 at 111ms Frame#3 at 122ms Average based on duration is AVERAGE(100, 11, 11) = 40.6ms (25fps) Average based on fps is AVERAGE(10, 90, 90) = 63fps Test: app launch Bug: 136558136 Change-Id: Icab848dd1f312498590f9735b8881ecdf0d24113 --- services/surfaceflinger/Scheduler/LayerInfo.cpp | 3 ++- services/surfaceflinger/Scheduler/LayerInfo.h | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/LayerInfo.cpp b/services/surfaceflinger/Scheduler/LayerInfo.cpp index beddb9be61..e782dd5361 100644 --- a/services/surfaceflinger/Scheduler/LayerInfo.cpp +++ b/services/surfaceflinger/Scheduler/LayerInfo.cpp @@ -50,7 +50,8 @@ void LayerInfo::setLastPresentTime(nsecs_t lastPresentTime) { // Ignore time diff that are too high - those are stale values if (timeDiff > OBSOLETE_TIME_EPSILON_NS.count()) return; const nsecs_t refreshDuration = (timeDiff > 0) ? timeDiff : mMinRefreshDuration; - mRefreshRateHistory.insertRefreshRate(refreshDuration); + const int fps = 1e9f / refreshDuration; + mRefreshRateHistory.insertRefreshRate(fps); } } // namespace scheduler diff --git a/services/surfaceflinger/Scheduler/LayerInfo.h b/services/surfaceflinger/Scheduler/LayerInfo.h index 2c50053ebd..66df9dca20 100644 --- a/services/surfaceflinger/Scheduler/LayerInfo.h +++ b/services/surfaceflinger/Scheduler/LayerInfo.h @@ -46,7 +46,7 @@ class LayerInfo { public: explicit RefreshRateHistory(nsecs_t minRefreshDuration) : mMinRefreshDuration(minRefreshDuration) {} - void insertRefreshRate(nsecs_t refreshRate) { + void insertRefreshRate(int refreshRate) { mElements.push_back(refreshRate); if (mElements.size() > HISTORY_SIZE) { mElements.pop_front(); @@ -54,13 +54,13 @@ class LayerInfo { } float getRefreshRateAvg() const { - nsecs_t refreshDuration = mMinRefreshDuration; - if (mElements.size() > 0) { - refreshDuration = scheduler::calculate_mean(mElements); + if (mElements.empty()) { + return 1e9f / mMinRefreshDuration; } - return 1e9f / refreshDuration; + return scheduler::calculate_mean(mElements); } + void clearHistory() { mElements.clear(); } private: -- cgit v1.2.3-59-g8ed1b From 7c7ede2f3f23d509b0e15ceba4f889ca1beac28e Mon Sep 17 00:00:00 2001 From: Daniel Solomon Date: Thu, 11 Jul 2019 16:35:40 -0700 Subject: SF: Update forced composition color space along with color mode Currently, a forced composition color space (ColorMode) can be set by the device via a persistent property. However, this property is read only during init, while some DisplayColorSetting values, which can change during runtime, require a specific ColorMode to be set simultaneously. This change adds the ability to specify a ColorMode in the same binder transaction that is used to set DisplayColorSetting. If a ColorMode is unspecified during the transaction, the current forced ColorMode is unchanged. Bug: 137140317 Bug: 137053654 Change-Id: I35bb292fbe70f944f509f7d34ef965adb66ca059 --- services/surfaceflinger/SurfaceFlinger.cpp | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 6d9dc97c5a..fe7a512745 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5405,7 +5405,12 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r return NO_ERROR; } case 1023: { // Set native mode + int32_t colorMode; + mDisplayColorSetting = static_cast(data.readInt32()); + if (data.readInt32(&colorMode) == NO_ERROR) { + mForceColorMode = static_cast(colorMode); + } invalidateHwcGeometry(); repaintEverything(); return NO_ERROR; -- cgit v1.2.3-59-g8ed1b From 7f01518eae5ce54d1b4bfc682c1c7b68bac89cb5 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 11 Jul 2019 13:56:22 -0700 Subject: [SurfaceFlinger] Don't touch hw vsync in DEFAULT RR with kernel timeout As a power optimization, hw vsync shouldn't be enabled when the kernel timer resets if the display is going to run at 60hz anyways. Bug: 136197211 Test: systrace when scrolling at peak refresh rate of 60hz. Change-Id: Ic7df3e7b735f79b183cdd91f770de83082e491b4 --- services/surfaceflinger/Scheduler/Scheduler.cpp | 29 ++++++++++++++++++++----- services/surfaceflinger/Scheduler/Scheduler.h | 6 ++++- services/surfaceflinger/SurfaceFlinger.cpp | 19 ++++++++++++++++ 3 files changed, 47 insertions(+), 7 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index cfdbd91e35..d2270d34b8 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -380,11 +380,17 @@ void Scheduler::updateFpsBasedOnContent() { } void Scheduler::setChangeRefreshRateCallback( - const ChangeRefreshRateCallback& changeRefreshRateCallback) { + const ChangeRefreshRateCallback&& changeRefreshRateCallback) { std::lock_guard lock(mCallbackLock); mChangeRefreshRateCallback = changeRefreshRateCallback; } +void Scheduler::setGetCurrentRefreshRateTypeCallback( + const GetCurrentRefreshRateTypeCallback&& getCurrentRefreshRateTypeCallback) { + std::lock_guard lock(mCallbackLock); + mGetCurrentRefreshRateTypeCallback = getCurrentRefreshRateTypeCallback; +} + void Scheduler::setGetVsyncPeriodCallback(const GetVsyncPeriod&& getVsyncPeriod) { std::lock_guard lock(mCallbackLock); mGetVsyncPeriod = getVsyncPeriod; @@ -427,8 +433,13 @@ void Scheduler::resetTimerCallback() { void Scheduler::resetKernelTimerCallback() { ATRACE_INT("ExpiredKernelIdleTimer", 0); std::lock_guard lock(mCallbackLock); - if (mGetVsyncPeriod) { - resyncToHardwareVsync(true, mGetVsyncPeriod()); + if (mGetVsyncPeriod && mGetCurrentRefreshRateTypeCallback) { + // If we're not in performance mode then the kernel timer shouldn't do + // anything, as the refresh rate during DPU power collapse will be the + // same. + if (mGetCurrentRefreshRateTypeCallback() == Scheduler::RefreshRateType::PERFORMANCE) { + resyncToHardwareVsync(true, mGetVsyncPeriod()); + } } } @@ -450,10 +461,16 @@ void Scheduler::expiredTouchTimerCallback() { } void Scheduler::expiredKernelTimerCallback() { + std::lock_guard lock(mCallbackLock); ATRACE_INT("ExpiredKernelIdleTimer", 1); - // Disable HW Vsync if the timer expired, as we don't need it - // enabled if we're not pushing frames. - disableHardwareVsync(false); + if (mGetCurrentRefreshRateTypeCallback) { + if (mGetCurrentRefreshRateTypeCallback() != Scheduler::RefreshRateType::PERFORMANCE) { + // Disable HW Vsync if the timer expired, as we don't need it + // enabled if we're not pushing frames, and if we're in PERFORMANCE + // mode then we'll need to re-update the DispSync model anyways. + disableHardwareVsync(false); + } + } } std::string Scheduler::doDump() { diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h index eaad37c3ee..c97664f164 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.h +++ b/services/surfaceflinger/Scheduler/Scheduler.h @@ -49,6 +49,7 @@ public: } using RefreshRateType = scheduler::RefreshRateConfigs::RefreshRateType; + using GetCurrentRefreshRateTypeCallback = std::function; using ChangeRefreshRateCallback = std::function; using GetVsyncPeriod = std::function; @@ -165,7 +166,9 @@ public: // Updates FPS based on the most content presented. void updateFpsBasedOnContent(); // Callback that gets invoked when Scheduler wants to change the refresh rate. - void setChangeRefreshRateCallback(const ChangeRefreshRateCallback& changeRefreshRateCallback); + void setChangeRefreshRateCallback(const ChangeRefreshRateCallback&& changeRefreshRateCallback); + void setGetCurrentRefreshRateTypeCallback( + const GetCurrentRefreshRateTypeCallback&& getCurrentRefreshRateType); void setGetVsyncPeriodCallback(const GetVsyncPeriod&& getVsyncPeriod); // Returns whether idle timer is enabled or not @@ -283,6 +286,7 @@ private: std::unique_ptr mTouchTimer; std::mutex mCallbackLock; + GetCurrentRefreshRateTypeCallback mGetCurrentRefreshRateTypeCallback GUARDED_BY(mCallbackLock); ChangeRefreshRateCallback mChangeRefreshRateCallback GUARDED_BY(mCallbackLock); GetVsyncPeriod mGetVsyncPeriod GUARDED_BY(mCallbackLock); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 94eeef5eef..577deff60a 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -713,6 +713,24 @@ void SurfaceFlinger::init() { Mutex::Autolock lock(mStateLock); setRefreshRateTo(type, event); }); + mScheduler->setGetCurrentRefreshRateTypeCallback([this] { + Mutex::Autolock lock(mStateLock); + const auto display = getDefaultDisplayDeviceLocked(); + if (!display) { + // If we don't have a default display the fallback to the default + // refresh rate type + return RefreshRateType::DEFAULT; + } + + const int configId = display->getActiveConfig(); + for (const auto& [type, refresh] : mRefreshRateConfigs.getRefreshRates()) { + if (refresh && refresh->configId == configId) { + return type; + } + } + // This should never happen, but just gracefully fallback to default. + return RefreshRateType::DEFAULT; + }); mScheduler->setGetVsyncPeriodCallback([this] { Mutex::Autolock lock(mStateLock); return getVsyncPeriod(); @@ -1045,6 +1063,7 @@ bool SurfaceFlinger::performSetActiveConfig() { desiredActiveConfigChangeDone(); return false; } + mUpcomingActiveConfig = desiredActiveConfig; const auto displayId = display->getId(); LOG_ALWAYS_FATAL_IF(!displayId); -- cgit v1.2.3-59-g8ed1b From 24363178b53efbc7a794bc72fe3b6e5fa4078ce6 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Fri, 12 Jul 2019 12:37:57 -0700 Subject: SurfaceFlinger: add display power state timer Coming out of DOZE mode might result in unstable HWVsync which may confuse FPS detection logic to set the desired refresh rate. To mitigate that, this change introduces a new timer that will provide a grace period when coming out of DOZE while in this grace period refresh rate will stay in PERFORMANCE. Test: Toggle DOZE by hitting the power button and collect systrace Bug: 135550670 Change-Id: Ib8ec3c9550336d691dd3868405d20b98aa983302 Merged-In: Ib8ec3c9550336d691dd3868405d20b98aa983302 --- services/surfaceflinger/Scheduler/Scheduler.cpp | 82 ++++++++++++++-------- services/surfaceflinger/Scheduler/Scheduler.h | 22 ++++-- services/surfaceflinger/SurfaceFlinger.cpp | 1 + .../surfaceflinger/SurfaceFlingerProperties.cpp | 8 +++ services/surfaceflinger/SurfaceFlingerProperties.h | 2 + .../sysprop/SurfaceFlingerProperties.sysprop | 12 ++++ .../surfaceflinger/sysprop/api/system-current.txt | 1 + 7 files changed, 96 insertions(+), 32 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index cfdbd91e35..a7784e9315 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -76,6 +76,7 @@ Scheduler::Scheduler(impl::EventControlThread::SetVSyncEnabledFunction function, mSupportKernelTimer = support_kernel_idle_timer(false); mSetTouchTimerMs = set_touch_timer_ms(0); + mSetDisplayPowerTimerMs = set_display_power_timer_ms(0); char value[PROPERTY_VALUE_MAX]; property_get("debug.sf.set_idle_timer_ms", value, "0"); @@ -110,10 +111,22 @@ Scheduler::Scheduler(impl::EventControlThread::SetVSyncEnabledFunction function, [this] { expiredTouchTimerCallback(); }); mTouchTimer->start(); } + + if (mSetDisplayPowerTimerMs > 0) { + mDisplayPowerTimer = + std::make_unique(std::chrono::milliseconds( + mSetDisplayPowerTimerMs), + [this] { resetDisplayPowerTimerCallback(); }, + [this] { + expiredDisplayPowerTimerCallback(); + }); + mDisplayPowerTimer->start(); + } } Scheduler::~Scheduler() { // Ensure the IdleTimer thread is joined before we start destroying state. + mDisplayPowerTimer.reset(); mTouchTimer.reset(); mIdleTimer.reset(); } @@ -419,8 +432,23 @@ void Scheduler::notifyTouchEvent() { mLayerHistory.clearHistory(); } +void Scheduler::setDisplayPowerState(bool normal) { + { + std::lock_guard lock(mFeatureStateLock); + mIsDisplayPowerStateNormal = normal; + } + + if (mDisplayPowerTimer) { + mDisplayPowerTimer->reset(); + } + + // Display Power event will boost the refresh rate to performance. + // Clear Layer History to get fresh FPS detection + mLayerHistory.clearHistory(); +} + void Scheduler::resetTimerCallback() { - timerChangeRefreshRate(IdleTimerState::RESET); + handleTimerStateChanged(&mCurrentIdleTimerState, IdleTimerState::RESET, false); ATRACE_INT("ExpiredIdleTimer", 0); } @@ -433,22 +461,30 @@ void Scheduler::resetKernelTimerCallback() { } void Scheduler::expiredTimerCallback() { - timerChangeRefreshRate(IdleTimerState::EXPIRED); + handleTimerStateChanged(&mCurrentIdleTimerState, IdleTimerState::EXPIRED, false); ATRACE_INT("ExpiredIdleTimer", 1); } void Scheduler::resetTouchTimerCallback() { - // We do not notify the applications about config changes when idle timer is reset. - touchChangeRefreshRate(TouchState::ACTIVE); + handleTimerStateChanged(&mCurrentTouchState, TouchState::ACTIVE, true); ATRACE_INT("TouchState", 1); } void Scheduler::expiredTouchTimerCallback() { - // We do not notify the applications about config changes when idle timer expires. - touchChangeRefreshRate(TouchState::INACTIVE); + handleTimerStateChanged(&mCurrentTouchState, TouchState::INACTIVE, true); ATRACE_INT("TouchState", 0); } +void Scheduler::resetDisplayPowerTimerCallback() { + handleTimerStateChanged(&mDisplayPowerTimerState, DisplayPowerTimerState::RESET, true); + ATRACE_INT("ExpiredDisplayPowerTimer", 0); +} + +void Scheduler::expiredDisplayPowerTimerCallback() { + handleTimerStateChanged(&mDisplayPowerTimerState, DisplayPowerTimerState::EXPIRED, true); + ATRACE_INT("ExpiredDisplayPowerTimer", 1); +} + void Scheduler::expiredKernelTimerCallback() { ATRACE_INT("ExpiredKernelIdleTimer", 1); // Disable HW Vsync if the timer expired, as we don't need it @@ -463,39 +499,23 @@ std::string Scheduler::doDump() { return stream.str(); } -void Scheduler::timerChangeRefreshRate(IdleTimerState idleTimerState) { - RefreshRateType newRefreshRateType; - { - std::lock_guard lock(mFeatureStateLock); - if (mCurrentIdleTimerState == idleTimerState) { - return; - } - mCurrentIdleTimerState = idleTimerState; - newRefreshRateType = calculateRefreshRateType(); - if (mRefreshRateType == newRefreshRateType) { - return; - } - mRefreshRateType = newRefreshRateType; - } - changeRefreshRate(newRefreshRateType, ConfigEvent::None); -} - -void Scheduler::touchChangeRefreshRate(TouchState touchState) { +template +void Scheduler::handleTimerStateChanged(T* currentState, T newState, bool eventOnContentDetection) { ConfigEvent event = ConfigEvent::None; RefreshRateType newRefreshRateType; { std::lock_guard lock(mFeatureStateLock); - if (mCurrentTouchState == touchState) { + if (*currentState == newState) { return; } - mCurrentTouchState = touchState; + *currentState = newState; newRefreshRateType = calculateRefreshRateType(); if (mRefreshRateType == newRefreshRateType) { return; } mRefreshRateType = newRefreshRateType; - // Send an event in case that content detection is on as touch has a higher priority - if (mCurrentContentFeatureState == ContentFeatureState::CONTENT_DETECTION_ON) { + if (eventOnContentDetection && + mCurrentContentFeatureState == ContentFeatureState::CONTENT_DETECTION_ON) { event = ConfigEvent::Changed; } } @@ -508,6 +528,12 @@ Scheduler::RefreshRateType Scheduler::calculateRefreshRateType() { return RefreshRateType::DEFAULT; } + // If Display Power is not in normal operation we want to be in performance mode. + // When coming back to normal mode, a grace period is given with DisplayPowerTimer + if (!mIsDisplayPowerStateNormal || mDisplayPowerTimerState == DisplayPowerTimerState::RESET) { + return RefreshRateType::PERFORMANCE; + } + // As long as touch is active we want to be in performance mode if (mCurrentTouchState == TouchState::ACTIVE) { return RefreshRateType::PERFORMANCE; diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h index eaad37c3ee..39c39d7639 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.h +++ b/services/surfaceflinger/Scheduler/Scheduler.h @@ -177,6 +177,9 @@ public: // Function that resets the touch timer. void notifyTouchEvent(); + // Function that sets whether display power mode is normal or not. + void setDisplayPowerState(bool normal); + // Returns relevant information about Scheduler for dumpsys purposes. std::string doDump(); @@ -197,6 +200,7 @@ private: enum class ContentFeatureState { CONTENT_DETECTION_ON, CONTENT_DETECTION_OFF }; enum class IdleTimerState { EXPIRED, RESET }; enum class TouchState { INACTIVE, ACTIVE }; + enum class DisplayPowerTimerState { EXPIRED, RESET }; // Creates a connection on the given EventThread and forwards the given callbacks. sp createConnectionInternal(EventThread*, ResyncCallback&&, @@ -221,12 +225,15 @@ private: void resetTouchTimerCallback(); // Function that is called when the touch timer expires. void expiredTouchTimerCallback(); + // Function that is called when the display power timer resets. + void resetDisplayPowerTimerCallback(); + // Function that is called when the display power timer expires. + void expiredDisplayPowerTimerCallback(); // Sets vsync period. void setVsyncPeriod(const nsecs_t period); - // Idle timer feature's function to change the refresh rate. - void timerChangeRefreshRate(IdleTimerState idleTimerState); - // Touch timer feature's function to change the refresh rate. - void touchChangeRefreshRate(TouchState touchState); + // handles various timer features to change the refresh rate. + template + void handleTimerStateChanged(T* currentState, T newState, bool eventOnContentDetection); // Calculate the new refresh rate type RefreshRateType calculateRefreshRateType() REQUIRES(mFeatureStateLock); // Acquires a lock and calls the ChangeRefreshRateCallback() with given parameters. @@ -282,6 +289,10 @@ private: int64_t mSetTouchTimerMs = 0; std::unique_ptr mTouchTimer; + // Timer used to monitor display power mode. + int64_t mSetDisplayPowerTimerMs = 0; + std::unique_ptr mDisplayPowerTimer; + std::mutex mCallbackLock; ChangeRefreshRateCallback mChangeRefreshRateCallback GUARDED_BY(mCallbackLock); GetVsyncPeriod mGetVsyncPeriod GUARDED_BY(mCallbackLock); @@ -293,9 +304,12 @@ private: ContentFeatureState::CONTENT_DETECTION_OFF; IdleTimerState mCurrentIdleTimerState GUARDED_BY(mFeatureStateLock) = IdleTimerState::RESET; TouchState mCurrentTouchState GUARDED_BY(mFeatureStateLock) = TouchState::INACTIVE; + DisplayPowerTimerState mDisplayPowerTimerState GUARDED_BY(mFeatureStateLock) = + DisplayPowerTimerState::EXPIRED; uint32_t mContentRefreshRate GUARDED_BY(mFeatureStateLock); RefreshRateType mRefreshRateType GUARDED_BY(mFeatureStateLock); bool mIsHDRContent GUARDED_BY(mFeatureStateLock) = false; + bool mIsDisplayPowerStateNormal GUARDED_BY(mFeatureStateLock) = true; const scheduler::RefreshRateConfigs& mRefreshRateConfigs; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 94eeef5eef..c9c6e3dee6 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -4543,6 +4543,7 @@ void SurfaceFlinger::setPowerModeInternal(const sp& display, int if (display->isPrimary()) { mTimeStats->setPowerMode(mode); mRefreshRateStats.setPowerMode(mode); + mScheduler->setDisplayPowerState(mode == HWC_POWER_MODE_NORMAL); } ALOGD("Finished setting power mode %d on display %s", mode, to_string(*displayId).c_str()); diff --git a/services/surfaceflinger/SurfaceFlingerProperties.cpp b/services/surfaceflinger/SurfaceFlingerProperties.cpp index 237208d39f..768074a6cd 100644 --- a/services/surfaceflinger/SurfaceFlingerProperties.cpp +++ b/services/surfaceflinger/SurfaceFlingerProperties.cpp @@ -242,6 +242,14 @@ int32_t set_touch_timer_ms(int32_t defaultValue) { return defaultValue; } +int32_t set_display_power_timer_ms(int32_t defaultValue) { + auto temp = SurfaceFlingerProperties::set_display_power_timer_ms(); + if (temp.has_value()) { + return *temp; + } + return defaultValue; +} + bool use_smart_90_for_video(bool defaultValue) { auto temp = SurfaceFlingerProperties::use_smart_90_for_video(); if (temp.has_value()) { diff --git a/services/surfaceflinger/SurfaceFlingerProperties.h b/services/surfaceflinger/SurfaceFlingerProperties.h index b5418d6538..5f88322f71 100644 --- a/services/surfaceflinger/SurfaceFlingerProperties.h +++ b/services/surfaceflinger/SurfaceFlingerProperties.h @@ -77,6 +77,8 @@ int32_t set_idle_timer_ms(int32_t defaultValue); int32_t set_touch_timer_ms(int32_t defaultValue); +int32_t set_display_power_timer_ms(int32_t defaultValue); + bool use_smart_90_for_video(bool defaultValue); bool enable_protected_contents(bool defaultValue); diff --git a/services/surfaceflinger/sysprop/SurfaceFlingerProperties.sysprop b/services/surfaceflinger/sysprop/SurfaceFlingerProperties.sysprop index f18f33c727..74baf37731 100644 --- a/services/surfaceflinger/sysprop/SurfaceFlingerProperties.sysprop +++ b/services/surfaceflinger/sysprop/SurfaceFlingerProperties.sysprop @@ -323,6 +323,18 @@ prop { prop_name: "ro.surface_flinger.set_touch_timer_ms" } +# setDisplayPowerTimerMs indicates what is considered a timeout in milliseconds for Scheduler. +# This value is used by the Scheduler to trigger display power inactivity callbacks that will +# keep the display in peak refresh rate as long as display power is not in normal mode. +# Setting this property to 0 means there is no timer. +prop { + api_name: "set_display_power_timer_ms" + type: Integer + scope: System + access: Readonly + prop_name: "ro.surface_flinger.set_display_power_timer_ms" +} + # useSmart90ForVideo indicates whether Scheduler should detect content FPS, and try to adjust the # screen refresh rate based on that. prop { diff --git a/services/surfaceflinger/sysprop/api/system-current.txt b/services/surfaceflinger/sysprop/api/system-current.txt index 89323c2ad0..79854b36a2 100644 --- a/services/surfaceflinger/sysprop/api/system-current.txt +++ b/services/surfaceflinger/sysprop/api/system-current.txt @@ -18,6 +18,7 @@ package android.sysprop { method public static java.util.Optional present_time_offset_from_vsync_ns(); method public static java.util.Optional primary_display_orientation(); method public static java.util.Optional running_without_sync_framework(); + method public static java.util.Optional set_display_power_timer_ms(); method public static java.util.Optional set_idle_timer_ms(); method public static java.util.Optional set_touch_timer_ms(); method public static java.util.Optional start_graphics_allocator_service(); -- cgit v1.2.3-59-g8ed1b From 38172ad8cf4a3dc3ee801cb295de891050993438 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Tue, 16 Jul 2019 16:10:28 -0700 Subject: SurfaceFlinger: debug layer bounds inset This is a debug change to make sure layer bounds will not overflow after an inset operation. Bug: 137560795 Test: manual tests Change-Id: Ifd9a8d84877e7f4c1f62c0419b0f86294ab576af --- services/surfaceflinger/Layer.cpp | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 1318bc0b2a..d6e86eb0e5 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -2037,6 +2037,13 @@ bool Layer::isRemovedFromCurrentState() const { return mRemovedFromCurrentState; } +// Debug helper for b/137560795 +#define INT32_MIGHT_OVERFLOW(n) (((n) >= INT32_MAX / 2) || ((n) <= INT32_MIN / 2)) + +#define RECT_BOUNDS_INVALID(rect) \ + (INT32_MIGHT_OVERFLOW((rect).left) || INT32_MIGHT_OVERFLOW((rect).right) || \ + INT32_MIGHT_OVERFLOW((rect).bottom) || INT32_MIGHT_OVERFLOW((rect).top)) + InputWindowInfo Layer::fillInputInfo() { InputWindowInfo info = mDrawingState.inputInfo; @@ -2066,6 +2073,26 @@ InputWindowInfo Layer::fillInputInfo() { layerBounds = getCroppedBufferSize(getDrawingState()); } layerBounds = t.transform(layerBounds); + + // debug check for b/137560795 + { + if (RECT_BOUNDS_INVALID(layerBounds)) { + ALOGE("layer %s bounds are invalid (%" PRIi32 ", %" PRIi32 ", %" PRIi32 ", %" PRIi32 + ")", + mName.c_str(), layerBounds.left, layerBounds.top, layerBounds.right, + layerBounds.bottom); + std::string out; + getTransform().dump(out, "Transform"); + ALOGE("%s", out.c_str()); + layerBounds.left = layerBounds.top = layerBounds.right = layerBounds.bottom = 0; + } + + if (INT32_MIGHT_OVERFLOW(xSurfaceInset) || INT32_MIGHT_OVERFLOW(ySurfaceInset)) { + ALOGE("layer %s surface inset are invalid (%" PRIi32 ", %" PRIi32 ")", mName.c_str(), + int32_t(xSurfaceInset), int32_t(ySurfaceInset)); + xSurfaceInset = ySurfaceInset = 0; + } + } layerBounds.inset(xSurfaceInset, ySurfaceInset, xSurfaceInset, ySurfaceInset); // Input coordinate should match the layer bounds. -- cgit v1.2.3-59-g8ed1b From 94390a32e24ac0f9286e11efd9ae5232ac3b9ac3 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Tue, 16 Jul 2019 13:20:19 -0700 Subject: [SurfaceFlinger] Exclude first vsync duration from period calculation Some displays don't exit from idle state immediately when vsync is enabled through the kernel idle timer, so exclude the first vsync period instead of just the first sample. Bug: 136197211 Test: Scrolling through settings Change-Id: I899fec289c8b08cfaa3264f7f6291a6fac489ab8 --- services/surfaceflinger/Scheduler/DispSync.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/DispSync.cpp b/services/surfaceflinger/Scheduler/DispSync.cpp index 83fd42b43f..0c94052979 100644 --- a/services/surfaceflinger/Scheduler/DispSync.cpp +++ b/services/surfaceflinger/Scheduler/DispSync.cpp @@ -668,7 +668,13 @@ void DispSync::updateModelLocked() { nsecs_t durationSum = 0; nsecs_t minDuration = INT64_MAX; nsecs_t maxDuration = 0; - for (size_t i = 1; i < mNumResyncSamples; i++) { + // We skip the first 2 samples because the first vsync duration on some + // devices may be much more inaccurate than on other devices, e.g. due + // to delays in ramping up from a power collapse. By doing so this + // actually increases the accuracy of the DispSync model even though + // we're effectively relying on fewer sample points. + static constexpr size_t numSamplesSkipped = 2; + for (size_t i = numSamplesSkipped; i < mNumResyncSamples; i++) { size_t idx = (mFirstResyncSample + i) % MAX_RESYNC_SAMPLES; size_t prev = (idx + MAX_RESYNC_SAMPLES - 1) % MAX_RESYNC_SAMPLES; nsecs_t duration = mResyncSamples[idx] - mResyncSamples[prev]; @@ -679,15 +685,14 @@ void DispSync::updateModelLocked() { // Exclude the min and max from the average durationSum -= minDuration + maxDuration; - mPeriod = durationSum / (mNumResyncSamples - 3); + mPeriod = durationSum / (mNumResyncSamples - numSamplesSkipped - 2); ALOGV("[%s] mPeriod = %" PRId64, mName, ns2us(mPeriod)); double sampleAvgX = 0; double sampleAvgY = 0; double scale = 2.0 * M_PI / double(mPeriod); - // Intentionally skip the first sample - for (size_t i = 1; i < mNumResyncSamples; i++) { + for (size_t i = numSamplesSkipped; i < mNumResyncSamples; i++) { size_t idx = (mFirstResyncSample + i) % MAX_RESYNC_SAMPLES; nsecs_t sample = mResyncSamples[idx] - mReferenceTime; double samplePhase = double(sample % mPeriod) * scale; @@ -695,8 +700,8 @@ void DispSync::updateModelLocked() { sampleAvgY += sin(samplePhase); } - sampleAvgX /= double(mNumResyncSamples - 1); - sampleAvgY /= double(mNumResyncSamples - 1); + sampleAvgX /= double(mNumResyncSamples - numSamplesSkipped); + sampleAvgY /= double(mNumResyncSamples - numSamplesSkipped); mPhase = nsecs_t(atan2(sampleAvgY, sampleAvgX) / scale); -- cgit v1.2.3-59-g8ed1b From a3b08efc43acebe8c15d31175ff04eeba6270913 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Mon, 15 Jul 2019 18:43:10 -0700 Subject: libgui: add EGL Image Tracking for debug Track EGL images allocated by libgui to help debug memory leaks Test: monkey Bug: 137514000 Change-Id: I0b193c0fdb7a4c07d7c2e5d06063e3dc01b5a57b --- libs/gui/Android.bp | 1 + libs/gui/DebugEGLImageTracker.cpp | 98 +++++++++++++++++++++++++++++ libs/gui/GLConsumer.cpp | 6 ++ libs/gui/include/gui/DebugEGLImageTracker.h | 44 +++++++++++++ libs/renderengine/gl/GLESRenderEngine.cpp | 7 +++ libs/renderengine/gl/GLFramebuffer.cpp | 2 + libs/renderengine/gl/GLImage.cpp | 3 + services/surfaceflinger/SurfaceFlinger.cpp | 4 ++ 8 files changed, 165 insertions(+) create mode 100644 libs/gui/DebugEGLImageTracker.cpp create mode 100644 libs/gui/include/gui/DebugEGLImageTracker.h (limited to 'services/surfaceflinger') diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index 34575f5d47..e3e63ee54e 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -34,6 +34,7 @@ cc_library_shared { "BufferItemConsumer.cpp", "ConsumerBase.cpp", "CpuConsumer.cpp", + "DebugEGLImageTracker.cpp", "DisplayEventReceiver.cpp", "GLConsumer.cpp", "GuiConfig.cpp", diff --git a/libs/gui/DebugEGLImageTracker.cpp b/libs/gui/DebugEGLImageTracker.cpp new file mode 100644 index 0000000000..ab6f36444a --- /dev/null +++ b/libs/gui/DebugEGLImageTracker.cpp @@ -0,0 +1,98 @@ +/* + * Copyright 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include + +#include +#include + +using android::base::StringAppendF; + +std::mutex DebugEGLImageTracker::mInstanceLock; +std::atomic DebugEGLImageTracker::mInstance; + +class DebugEGLImageTrackerNoOp : public DebugEGLImageTracker { +public: + DebugEGLImageTrackerNoOp() = default; + ~DebugEGLImageTrackerNoOp() override = default; + void create(const char * /*from*/) override {} + void destroy(const char * /*from*/) override {} + + void dump(std::string & /*result*/) override {} +}; + +class DebugEGLImageTrackerImpl : public DebugEGLImageTracker { +public: + DebugEGLImageTrackerImpl() = default; + ~DebugEGLImageTrackerImpl() override = default; + void create(const char * /*from*/) override; + void destroy(const char * /*from*/) override; + + void dump(std::string & /*result*/) override; + +private: + std::mutex mLock; + std::unordered_map mCreateTracker; + std::unordered_map mDestroyTracker; + + int64_t mTotalCreated = 0; + int64_t mTotalDestroyed = 0; +}; + +DebugEGLImageTracker *DebugEGLImageTracker::getInstance() { + std::lock_guard lock(mInstanceLock); + if (mInstance == nullptr) { + char value[PROPERTY_VALUE_MAX]; + property_get("debug.sf.enable_egl_image_tracker", value, "0"); + const bool enabled = static_cast(atoi(value)); + + if (enabled) { + mInstance = new DebugEGLImageTrackerImpl(); + } else { + mInstance = new DebugEGLImageTrackerNoOp(); + } + } + + return mInstance; +} + +void DebugEGLImageTrackerImpl::create(const char *from) { + std::lock_guard lock(mLock); + mCreateTracker[from]++; + mTotalCreated++; +} + +void DebugEGLImageTrackerImpl::destroy(const char *from) { + std::lock_guard lock(mLock); + mDestroyTracker[from]++; + mTotalDestroyed++; +} + +void DebugEGLImageTrackerImpl::dump(std::string &result) { + std::lock_guard lock(mLock); + StringAppendF(&result, "Live EGL Image objects: %" PRIi64 "\n", + mTotalCreated - mTotalDestroyed); + StringAppendF(&result, "Total EGL Image created: %" PRIi64 "\n", mTotalCreated); + for (const auto &[from, count] : mCreateTracker) { + StringAppendF(&result, "\t%s: %" PRIi64 "\n", from.c_str(), count); + } + StringAppendF(&result, "Total EGL Image destroyed: %" PRIi64 "\n", mTotalDestroyed); + for (const auto &[from, count] : mDestroyTracker) { + StringAppendF(&result, "\t%s: %" PRIi64 "\n", from.c_str(), count); + } +} diff --git a/libs/gui/GLConsumer.cpp b/libs/gui/GLConsumer.cpp index 8d66154bdd..8199c98582 100644 --- a/libs/gui/GLConsumer.cpp +++ b/libs/gui/GLConsumer.cpp @@ -34,6 +34,7 @@ #include #include +#include #include #include #include @@ -944,6 +945,7 @@ GLConsumer::EglImage::~EglImage() { if (!eglDestroyImageKHR(mEglDisplay, mEglImage)) { ALOGE("~EglImage: eglDestroyImageKHR failed"); } + DEBUG_EGL_IMAGE_TRACKER_DESTROY(); eglTerminate(mEglDisplay); } } @@ -957,6 +959,7 @@ status_t GLConsumer::EglImage::createIfNeeded(EGLDisplay eglDisplay, if (!eglDestroyImageKHR(mEglDisplay, mEglImage)) { ALOGE("createIfNeeded: eglDestroyImageKHR failed"); } + DEBUG_EGL_IMAGE_TRACKER_DESTROY(); eglTerminate(mEglDisplay); mEglImage = EGL_NO_IMAGE_KHR; mEglDisplay = EGL_NO_DISPLAY; @@ -1006,7 +1009,10 @@ EGLImageKHR GLConsumer::EglImage::createImage(EGLDisplay dpy, EGLint error = eglGetError(); ALOGE("error creating EGLImage: %#x", error); eglTerminate(dpy); + } else { + DEBUG_EGL_IMAGE_TRACKER_CREATE(); } + return image; } diff --git a/libs/gui/include/gui/DebugEGLImageTracker.h b/libs/gui/include/gui/DebugEGLImageTracker.h new file mode 100644 index 0000000000..5d369c9a35 --- /dev/null +++ b/libs/gui/include/gui/DebugEGLImageTracker.h @@ -0,0 +1,44 @@ +/* + * Copyright 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include +#include + +class DebugEGLImageTracker { +public: + static DebugEGLImageTracker *getInstance(); + + virtual void create(const char *from) = 0; + virtual void destroy(const char *from) = 0; + + virtual void dump(std::string &result) = 0; + +protected: + DebugEGLImageTracker() = default; + virtual ~DebugEGLImageTracker() = default; + DebugEGLImageTracker(const DebugEGLImageTracker &) = delete; + + static std::mutex mInstanceLock; + static std::atomic mInstance; +}; + +#define DEBUG_EGL_IMAGE_TRACKER_CREATE() \ + (DebugEGLImageTracker::getInstance()->create(__PRETTY_FUNCTION__)) +#define DEBUG_EGL_IMAGE_TRACKER_DESTROY() \ + (DebugEGLImageTracker::getInstance()->destroy(__PRETTY_FUNCTION__)) \ No newline at end of file diff --git a/libs/renderengine/gl/GLESRenderEngine.cpp b/libs/renderengine/gl/GLESRenderEngine.cpp index dd12b5532e..3f765be861 100644 --- a/libs/renderengine/gl/GLESRenderEngine.cpp +++ b/libs/renderengine/gl/GLESRenderEngine.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -433,6 +434,7 @@ GLESRenderEngine::~GLESRenderEngine() { EGLImageKHR expired = mFramebufferImageCache.front().second; mFramebufferImageCache.pop_front(); eglDestroyImageKHR(mEGLDisplay, expired); + DEBUG_EGL_IMAGE_TRACKER_DESTROY(); } mImageCache.clear(); eglMakeCurrent(mEGLDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); @@ -865,10 +867,15 @@ EGLImageKHR GLESRenderEngine::createFramebufferImageIfNeeded(ANativeWindowBuffer EGLImageKHR expired = mFramebufferImageCache.front().second; mFramebufferImageCache.pop_front(); eglDestroyImageKHR(mEGLDisplay, expired); + DEBUG_EGL_IMAGE_TRACKER_DESTROY(); } mFramebufferImageCache.push_back({graphicBuffer->getId(), image}); } } + + if (image != EGL_NO_IMAGE_KHR) { + DEBUG_EGL_IMAGE_TRACKER_CREATE(); + } return image; } diff --git a/libs/renderengine/gl/GLFramebuffer.cpp b/libs/renderengine/gl/GLFramebuffer.cpp index dacf8d3d82..5fbb5ba7d7 100644 --- a/libs/renderengine/gl/GLFramebuffer.cpp +++ b/libs/renderengine/gl/GLFramebuffer.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include "GLESRenderEngine.h" @@ -47,6 +48,7 @@ bool GLFramebuffer::setNativeWindowBuffer(ANativeWindowBuffer* nativeBuffer, boo if (mEGLImage != EGL_NO_IMAGE_KHR) { if (!usingFramebufferCache) { eglDestroyImageKHR(mEGLDisplay, mEGLImage); + DEBUG_EGL_IMAGE_TRACKER_DESTROY(); } mEGLImage = EGL_NO_IMAGE_KHR; mBufferWidth = 0; diff --git a/libs/renderengine/gl/GLImage.cpp b/libs/renderengine/gl/GLImage.cpp index 77e648e70f..8497721956 100644 --- a/libs/renderengine/gl/GLImage.cpp +++ b/libs/renderengine/gl/GLImage.cpp @@ -20,6 +20,7 @@ #include +#include #include #include #include "GLESRenderEngine.h" @@ -58,6 +59,7 @@ bool GLImage::setNativeWindowBuffer(ANativeWindowBuffer* buffer, bool isProtecte if (!eglDestroyImageKHR(mEGLDisplay, mEGLImage)) { ALOGE("failed to destroy image: %#x", eglGetError()); } + DEBUG_EGL_IMAGE_TRACKER_DESTROY(); mEGLImage = EGL_NO_IMAGE_KHR; } @@ -69,6 +71,7 @@ bool GLImage::setNativeWindowBuffer(ANativeWindowBuffer* buffer, bool isProtecte ALOGE("failed to create EGLImage: %#x", eglGetError()); return false; } + DEBUG_EGL_IMAGE_TRACKER_CREATE(); mProtected = isProtected; } diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 0c9263b408..e58e09209c 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -48,6 +48,8 @@ #include #include #include +#include + #include #include #include @@ -5022,6 +5024,8 @@ void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, std::string& result) co getRenderEngine().dump(result); + DebugEGLImageTracker::getInstance()->dump(result); + if (const auto display = getDefaultDisplayDeviceLocked()) { display->getCompositionDisplay()->getState().undefinedRegion.dump(result, "undefinedRegion"); -- cgit v1.2.3-59-g8ed1b From cab4c2f2673034356de895fa661aa3dc979ff20e Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Fri, 12 Jul 2019 16:59:39 -0700 Subject: SurfaceFlinger: tune LayerHistory to be less aggressive Increase the thresholds to mark layer as "relevant" to be less agressive when changing refresh rate based on content. Bug: 136558136 Test: 1) put icons in drawer/folder on homescreen 2) tap drawer to expand 3) tap homescreen to contract drawer 4) goto 2), repeating quickly. Test: libsurfaceflinger_unnittest Change-Id: I2a9b696f267d93720408a41dceb4acb7dc80bd69 --- services/surfaceflinger/Scheduler/LayerInfo.h | 4 +- .../tests/unittests/LayerHistoryTest.cpp | 60 ++++++++++++---------- 2 files changed, 36 insertions(+), 28 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/LayerInfo.h b/services/surfaceflinger/Scheduler/LayerInfo.h index 66df9dca20..a7337817e3 100644 --- a/services/surfaceflinger/Scheduler/LayerInfo.h +++ b/services/surfaceflinger/Scheduler/LayerInfo.h @@ -129,8 +129,8 @@ class LayerInfo { private: std::deque mElements; - static constexpr size_t HISTORY_SIZE = 10; - static constexpr std::chrono::nanoseconds HISTORY_TIME = 500ms; + static constexpr size_t HISTORY_SIZE = 90; + static constexpr std::chrono::nanoseconds HISTORY_TIME = 1s; }; public: diff --git a/services/surfaceflinger/tests/unittests/LayerHistoryTest.cpp b/services/surfaceflinger/tests/unittests/LayerHistoryTest.cpp index 7ec90660ce..8e7440c2e3 100644 --- a/services/surfaceflinger/tests/unittests/LayerHistoryTest.cpp +++ b/services/surfaceflinger/tests/unittests/LayerHistoryTest.cpp @@ -27,6 +27,15 @@ protected: static constexpr float MIN_REFRESH_RATE = 30.f; static constexpr float MAX_REFRESH_RATE = 90.f; + static constexpr auto RELEVANT_FRAME_THRESHOLD = 90u; + static constexpr uint64_t THIRTY_FPS_INTERVAL = 33'333'333; + + void forceRelevancy(const std::unique_ptr& testLayer) { + mLayerHistory->setVisibility(testLayer, true); + for (auto i = 0u; i < RELEVANT_FRAME_THRESHOLD; i++) { + mLayerHistory->insert(testLayer, 0, false /*isHDR*/); + } + }; }; LayerHistoryTest::LayerHistoryTest() { @@ -39,24 +48,18 @@ TEST_F(LayerHistoryTest, oneLayer) { std::unique_ptr testLayer = mLayerHistory->createLayer("TestLayer", MIN_REFRESH_RATE, MAX_REFRESH_RATE); mLayerHistory->setVisibility(testLayer, true); + for (auto i = 0u; i < RELEVANT_FRAME_THRESHOLD; i++) { + EXPECT_FLOAT_EQ(0.f, mLayerHistory->getDesiredRefreshRateAndHDR().first); + mLayerHistory->insert(testLayer, 0, false /*isHDR*/); + } - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - EXPECT_FLOAT_EQ(0.f, mLayerHistory->getDesiredRefreshRateAndHDR().first); - - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - // This is still 0, because the layer is not considered recently active if it - // has been present in less than 10 frames. - EXPECT_FLOAT_EQ(0.f, mLayerHistory->getDesiredRefreshRateAndHDR().first); - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - mLayerHistory->insert(testLayer, 0, false /*isHDR*/); - // This should be MAX_REFRESH_RATE as we have more than 10 samples - EXPECT_FLOAT_EQ(MAX_REFRESH_RATE, mLayerHistory->getDesiredRefreshRateAndHDR().first); + // Add a few more. This time we should get MAX refresh rate as the layer + // becomes relevant + static constexpr auto A_FEW = 10; + for (auto i = 0u; i < A_FEW; i++) { + EXPECT_FLOAT_EQ(MAX_REFRESH_RATE, mLayerHistory->getDesiredRefreshRateAndHDR().first); + mLayerHistory->insert(testLayer, 0, false /*isHDR*/); + } } TEST_F(LayerHistoryTest, oneHDRLayer) { @@ -79,8 +82,9 @@ TEST_F(LayerHistoryTest, explicitTimestamp) { mLayerHistory->setVisibility(test30FpsLayer, true); nsecs_t startTime = systemTime(); - for (int i = 0; i < 31; i++) { - mLayerHistory->insert(test30FpsLayer, startTime + (i * 33333333), false /*isHDR*/); + for (int i = 0; i < RELEVANT_FRAME_THRESHOLD; i++) { + mLayerHistory->insert(test30FpsLayer, startTime + (i * THIRTY_FPS_INTERVAL), + false /*isHDR*/); } EXPECT_FLOAT_EQ(30.f, mLayerHistory->getDesiredRefreshRateAndHDR().first); @@ -98,19 +102,21 @@ TEST_F(LayerHistoryTest, multipleLayers) { mLayerHistory->setVisibility(testLayer2, true); nsecs_t startTime = systemTime(); - for (int i = 0; i < 10; i++) { + for (int i = 0; i < RELEVANT_FRAME_THRESHOLD; i++) { mLayerHistory->insert(testLayer, 0, false /*isHDR*/); } EXPECT_FLOAT_EQ(MAX_REFRESH_RATE, mLayerHistory->getDesiredRefreshRateAndHDR().first); startTime = systemTime(); - for (int i = 0; i < 10; i++) { - mLayerHistory->insert(test30FpsLayer, startTime + (i * 33333333), false /*isHDR*/); + for (int i = 0; i < RELEVANT_FRAME_THRESHOLD; i++) { + mLayerHistory->insert(test30FpsLayer, startTime + (i * THIRTY_FPS_INTERVAL), + false /*isHDR*/); } EXPECT_FLOAT_EQ(MAX_REFRESH_RATE, mLayerHistory->getDesiredRefreshRateAndHDR().first); - for (int i = 10; i < 30; i++) { - mLayerHistory->insert(test30FpsLayer, startTime + (i * 33333333), false /*isHDR*/); + for (int i = 10; i < RELEVANT_FRAME_THRESHOLD; i++) { + mLayerHistory->insert(test30FpsLayer, startTime + (i * THIRTY_FPS_INTERVAL), + false /*isHDR*/); } EXPECT_FLOAT_EQ(MAX_REFRESH_RATE, mLayerHistory->getDesiredRefreshRateAndHDR().first); @@ -122,8 +128,10 @@ TEST_F(LayerHistoryTest, multipleLayers) { EXPECT_FLOAT_EQ(MAX_REFRESH_RATE, mLayerHistory->getDesiredRefreshRateAndHDR().first); // After 1200 ms frames become obsolete. std::this_thread::sleep_for(std::chrono::milliseconds(1500)); - // Insert the 31st frame. - mLayerHistory->insert(test30FpsLayer, startTime + (30 * 33333333), false /*isHDR*/); + + mLayerHistory->insert(test30FpsLayer, + startTime + (RELEVANT_FRAME_THRESHOLD * THIRTY_FPS_INTERVAL), + false /*isHDR*/); EXPECT_FLOAT_EQ(30.f, mLayerHistory->getDesiredRefreshRateAndHDR().first); } -- cgit v1.2.3-59-g8ed1b From 4658e11cbe1abae691454ddcf93a06763d1bf828 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Mon, 22 Jul 2019 13:07:26 -0700 Subject: SurfaceFlinger: add a sysprop for GL backpressure In some devices where SF misses a frame in GL comp, HWC queue may get stuffed. This causes application to block longer on dequeueBuffer which leads to jank. Test: 1. Open Camera 2. Swipe horizontally to go back to previous app. Bug: 138083790 Change-Id: I4c5963cb7ba7f9b10254c77dc7117ca3acac81cf --- services/surfaceflinger/SurfaceFlinger.cpp | 11 ++++++++--- services/surfaceflinger/SurfaceFlinger.h | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 2d319101b3..6166789fc4 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -353,6 +353,11 @@ SurfaceFlinger::SurfaceFlinger(Factory& factory) : SurfaceFlinger(factory, SkipI mPropagateBackpressure = !atoi(value); ALOGI_IF(!mPropagateBackpressure, "Disabling backpressure propagation"); + property_get("debug.sf.enable_gl_backpressure", value, "0"); + mPropagateBackpressureClientComposition = atoi(value); + ALOGI_IF(mPropagateBackpressureClientComposition, + "Enabling backpressure propagation for Client Composition"); + property_get("debug.sf.enable_hwc_vds", value, "0"); mUseHwcVirtualDisplays = atoi(value); ALOGI_IF(mUseHwcVirtualDisplays, "Enabling HWC virtual displays"); @@ -1670,9 +1675,9 @@ void SurfaceFlinger::onMessageReceived(int32_t what) NO_THREAD_SAFETY_ANALYSIS { break; } - // For now, only propagate backpressure when missing a hwc frame. - if (hwcFrameMissed && !gpuFrameMissed) { - if (mPropagateBackpressure) { + if (frameMissed && mPropagateBackpressure) { + if ((hwcFrameMissed && !gpuFrameMissed) || + mPropagateBackpressureClientComposition) { signalLayerUpdate(); break; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index ddfe88c928..52655944de 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -1003,6 +1003,7 @@ private: volatile nsecs_t mDebugInTransaction = 0; bool mForceFullDamage = false; bool mPropagateBackpressure = true; + bool mPropagateBackpressureClientComposition = false; std::unique_ptr mInterceptor; SurfaceTracing mTracing{*this}; bool mTracingEnabled = false; -- cgit v1.2.3-59-g8ed1b From 7c03ce6508a7887e732abe03dc2dd53e146c008e Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Fri, 19 Jul 2019 10:59:45 -0700 Subject: SurfaceFlinger: calculate expected present once Calculate the expected present time in the beginning of the frame and use the same value across the frame composition. Bug: 137873466 Test: App transitions Change-Id: I0b41c62796f6150702e8d50ec7d28838d3a5aa87 --- services/surfaceflinger/SurfaceFlinger.cpp | 10 +++++++--- services/surfaceflinger/SurfaceFlinger.h | 7 +++++-- 2 files changed, 12 insertions(+), 5 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 0c9263b408..56cccfbce5 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1686,22 +1686,26 @@ bool SurfaceFlinger::previousFrameMissed() NO_THREAD_SAFETY_ANALYSIS { return fence != Fence::NO_FENCE && (fence->getStatus() == Fence::Status::Unsignaled); } -nsecs_t SurfaceFlinger::getExpectedPresentTime() NO_THREAD_SAFETY_ANALYSIS { +void SurfaceFlinger::populateExpectedPresentTime() NO_THREAD_SAFETY_ANALYSIS { DisplayStatInfo stats; mScheduler->getDisplayStatInfo(&stats); const nsecs_t presentTime = mScheduler->getDispSyncExpectedPresentTime(); // Inflate the expected present time if we're targetting the next vsync. - const nsecs_t correctedTime = + mExpectedPresentTime = mVsyncModulator.getOffsets().sf < mPhaseOffsets->getOffsetThresholdForNextVsync() ? presentTime : presentTime + stats.vsyncPeriod; - return correctedTime; } void SurfaceFlinger::onMessageReceived(int32_t what) NO_THREAD_SAFETY_ANALYSIS { ATRACE_CALL(); switch (what) { case MessageQueue::INVALIDATE: { + // calculate the expected present time once and use the cached + // value throughout this frame to make sure all layers are + // seeing this same value. + populateExpectedPresentTime(); + bool frameMissed = previousFrameMissed(); bool hwcFrameMissed = mHadDeviceComposition && frameMissed; bool gpuFrameMissed = mHadClientComposition && frameMissed; diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 556a4b9be5..d836795e99 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -301,10 +301,11 @@ public: // TODO: this should be made accessible only to MessageQueue void onMessageReceived(int32_t what); - // Returns the expected present time for this frame. + // populates the expected present time for this frame. // When we are in negative offsets, we perform a correction so that the // predicted vsync for the *next* frame is used instead. - nsecs_t getExpectedPresentTime(); + void populateExpectedPresentTime(); + nsecs_t getExpectedPresentTime() const { return mExpectedPresentTime; } // for debugging only // TODO: this should be made accessible only to HWComposer @@ -1184,6 +1185,8 @@ private: // Flags to capture the state of Vsync in HWC HWC2::Vsync mHWCVsyncState = HWC2::Vsync::Disable; HWC2::Vsync mHWCVsyncPendingState = HWC2::Vsync::Disable; + + nsecs_t mExpectedPresentTime; }; } // namespace android -- cgit v1.2.3-59-g8ed1b From 44685cb3f0e53c0f0b5f260891b9dd78d25410d4 Mon Sep 17 00:00:00 2001 From: Steven Thomas Date: Tue, 23 Jul 2019 16:19:31 -0700 Subject: Merge damage region when dropping an app buffer When we decide to drop a buffer from a buffer queue, make sure to merge the damage region from the dropped buffer into the current damage region. Otherwise we'll pass the wrong damage region to hardware composer and get broken rendering. Bug: 136158117 Test: Wrote a new test in Transaction_test.cpp to repro the problem and confirm the fix works. Change-Id: Icdc61e1be3297450f2869c496dad1453fb6dca6d --- services/surfaceflinger/BufferLayerConsumer.cpp | 9 +++ services/surfaceflinger/BufferLayerConsumer.h | 3 + services/surfaceflinger/BufferQueueLayer.cpp | 2 + services/surfaceflinger/tests/Transaction_test.cpp | 94 ++++++++++++++++++++++ 4 files changed, 108 insertions(+) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/BufferLayerConsumer.cpp b/services/surfaceflinger/BufferLayerConsumer.cpp index 6709fb4b48..bd9bd81e2a 100644 --- a/services/surfaceflinger/BufferLayerConsumer.cpp +++ b/services/surfaceflinger/BufferLayerConsumer.cpp @@ -369,6 +369,15 @@ const Region& BufferLayerConsumer::getSurfaceDamage() const { return mCurrentSurfaceDamage; } +void BufferLayerConsumer::mergeSurfaceDamage(const Region& damage) { + if (damage.bounds() == Rect::INVALID_RECT || + mCurrentSurfaceDamage.bounds() == Rect::INVALID_RECT) { + mCurrentSurfaceDamage = Region::INVALID_REGION; + } else { + mCurrentSurfaceDamage |= damage; + } +} + int BufferLayerConsumer::getCurrentApi() const { Mutex::Autolock lock(mMutex); return mCurrentApi; diff --git a/services/surfaceflinger/BufferLayerConsumer.h b/services/surfaceflinger/BufferLayerConsumer.h index e3f6100c35..901556a53f 100644 --- a/services/surfaceflinger/BufferLayerConsumer.h +++ b/services/surfaceflinger/BufferLayerConsumer.h @@ -140,6 +140,9 @@ public: // must be called from SF main thread const Region& getSurfaceDamage() const; + // Merge the given damage region into the current damage region value. + void mergeSurfaceDamage(const Region& damage); + // getCurrentApi retrieves the API which queues the current buffer. int getCurrentApi() const; diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp index d6853661a5..f35a4fd497 100644 --- a/services/surfaceflinger/BufferQueueLayer.cpp +++ b/services/surfaceflinger/BufferQueueLayer.cpp @@ -316,6 +316,7 @@ status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t // and return early if (queuedBuffer) { Mutex::Autolock lock(mQueueItemLock); + mConsumer->mergeSurfaceDamage(mQueueItems[0].mSurfaceDamage); mFlinger->mTimeStats->removeTimeRecord(layerID, mQueueItems[0].mFrameNumber); mQueueItems.removeAt(0); mQueuedFrames--; @@ -351,6 +352,7 @@ status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t // Remove any stale buffers that have been dropped during // updateTexImage while (mQueueItems[0].mFrameNumber != currentFrameNumber) { + mConsumer->mergeSurfaceDamage(mQueueItems[0].mSurfaceDamage); mFlinger->mTimeStats->removeTimeRecord(layerID, mQueueItems[0].mFrameNumber); mQueueItems.removeAt(0); mQueuedFrames--; diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index d5f65348d8..c93e15ef96 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -28,6 +28,7 @@ #include #include +#include #include #include #include @@ -6059,4 +6060,97 @@ TEST_F(RelativeZTest, LayerRemovedOffscreenRelativeParent) { } } +// This test ensures that when we drop an app buffer in SurfaceFlinger, we merge +// the dropped buffer's damage region into the next buffer's damage region. If +// we don't do this, we'll report an incorrect damage region to hardware +// composer, resulting in broken rendering. This test checks the BufferQueue +// case. +// +// Unfortunately, we don't currently have a way to inspect the damage region +// SurfaceFlinger sends to hardware composer from a test, so this test requires +// the dev to manually watch the device's screen during the test to spot broken +// rendering. Because the results can't be automatically verified, this test is +// marked disabled. +TEST_F(LayerTransactionTest, DISABLED_BufferQueueLayerMergeDamageRegionWhenDroppingBuffers) { + const int width = mDisplayWidth; + const int height = mDisplayHeight; + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createLayer("test", width, height)); + const auto producer = layer->getIGraphicBufferProducer(); + const sp dummyListener(new DummyProducerListener); + IGraphicBufferProducer::QueueBufferOutput queueBufferOutput; + ASSERT_EQ(OK, + producer->connect(dummyListener, NATIVE_WINDOW_API_CPU, true, &queueBufferOutput)); + + std::map> slotMap; + auto slotToBuffer = [&](int slot, sp* buf) { + ASSERT_NE(nullptr, buf); + const auto iter = slotMap.find(slot); + ASSERT_NE(slotMap.end(), iter); + *buf = iter->second; + }; + + auto dequeue = [&](int* outSlot) { + ASSERT_NE(nullptr, outSlot); + *outSlot = -1; + int slot; + sp fence; + uint64_t age; + FrameEventHistoryDelta timestamps; + const status_t dequeueResult = + producer->dequeueBuffer(&slot, &fence, width, height, PIXEL_FORMAT_RGBA_8888, + GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN, + &age, ×tamps); + if (dequeueResult == IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION) { + sp newBuf; + ASSERT_EQ(OK, producer->requestBuffer(slot, &newBuf)); + ASSERT_NE(nullptr, newBuf.get()); + slotMap[slot] = newBuf; + } else { + ASSERT_EQ(OK, dequeueResult); + } + *outSlot = slot; + }; + + auto queue = [&](int slot, const Region& damage, nsecs_t displayTime) { + IGraphicBufferProducer::QueueBufferInput input( + /*timestamp=*/displayTime, /*isAutoTimestamp=*/false, HAL_DATASPACE_UNKNOWN, + /*crop=*/Rect::EMPTY_RECT, NATIVE_WINDOW_SCALING_MODE_SCALE_TO_WINDOW, + /*transform=*/0, Fence::NO_FENCE); + input.setSurfaceDamage(damage); + IGraphicBufferProducer::QueueBufferOutput output; + ASSERT_EQ(OK, producer->queueBuffer(slot, input, &output)); + }; + + auto fillAndPostBuffers = [&](const Color& color) { + int slot1; + ASSERT_NO_FATAL_FAILURE(dequeue(&slot1)); + int slot2; + ASSERT_NO_FATAL_FAILURE(dequeue(&slot2)); + + sp buf1; + ASSERT_NO_FATAL_FAILURE(slotToBuffer(slot1, &buf1)); + sp buf2; + ASSERT_NO_FATAL_FAILURE(slotToBuffer(slot2, &buf2)); + fillGraphicBufferColor(buf1, Rect(width, height), color); + fillGraphicBufferColor(buf2, Rect(width, height), color); + + const auto displayTime = systemTime() + milliseconds_to_nanoseconds(100); + ASSERT_NO_FATAL_FAILURE(queue(slot1, Region::INVALID_REGION, displayTime)); + ASSERT_NO_FATAL_FAILURE( + queue(slot2, Region(Rect(width / 3, height / 3, 2 * width / 3, 2 * height / 3)), + displayTime)); + }; + + const auto startTime = systemTime(); + const std::array colors = {Color::RED, Color::GREEN, Color::BLUE}; + int colorIndex = 0; + while (nanoseconds_to_seconds(systemTime() - startTime) < 10) { + ASSERT_NO_FATAL_FAILURE(fillAndPostBuffers(colors[colorIndex++ % colors.size()])); + std::this_thread::sleep_for(1s); + } + + ASSERT_EQ(OK, producer->disconnect(NATIVE_WINDOW_API_CPU)); +} + } // namespace android -- cgit v1.2.3-59-g8ed1b From 16a9940abee8752d8566e6c7b52b22d10693512b Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Mon, 29 Jul 2019 16:37:30 -0700 Subject: [RenderEngine] add an ImageManager thread Prior to this change, EGLImage management would be performed by both binder threads and the main rendering thread in a synchronized manner. But because of BufferQueue implementation details, it's possible for an app's Surface to be disconnected (destroying all backing EGLImages per the disconnect api spec), while still latching and presenting the most recently queued image (which requires recreating the EGLImage on the main thread), which can cause jank in some scenarios such as app-launch. To mitigate this, defer EGLImage creation and destruction to a separate thread behind RenderEngine so that creating the EGLImage should never be done on the main thread. So scenarios such as app-launch avoid jank by performing the EGLImage creation asynchronously with the main thread, so that the creation is done by the time RenderEngine needs to start rendering. Bug: 136806342 Bug: 137191934 Test: Launching photos app shows no bindExternalImage* calls during the rendering path Test: librenderengine_test Change-Id: Ib809e36267b5604bbbedd4089d15666b22cabc95 --- libs/renderengine/Android.bp | 2 + libs/renderengine/gl/GLESRenderEngine.cpp | 80 ++++++++++-- libs/renderengine/gl/GLESRenderEngine.h | 17 ++- libs/renderengine/gl/ImageManager.cpp | 140 +++++++++++++++++++++ libs/renderengine/gl/ImageManager.h | 70 +++++++++++ .../include/renderengine/RenderEngine.h | 12 +- .../include/renderengine/mock/RenderEngine.h | 2 +- libs/renderengine/tests/Android.bp | 1 + libs/renderengine/tests/RenderEngineTest.cpp | 54 ++++++-- services/surfaceflinger/BufferLayerConsumer.cpp | 7 +- services/surfaceflinger/BufferLayerConsumer.h | 3 +- 11 files changed, 360 insertions(+), 28 deletions(-) create mode 100644 libs/renderengine/gl/ImageManager.cpp create mode 100644 libs/renderengine/gl/ImageManager.h (limited to 'services/surfaceflinger') diff --git a/libs/renderengine/Android.bp b/libs/renderengine/Android.bp index 36211ca733..cc252d67ae 100644 --- a/libs/renderengine/Android.bp +++ b/libs/renderengine/Android.bp @@ -26,6 +26,7 @@ cc_defaults { "libgui", "liblog", "libnativewindow", + "libprocessgroup", "libsync", "libui", "libutils", @@ -51,6 +52,7 @@ filegroup { "gl/GLExtensions.cpp", "gl/GLFramebuffer.cpp", "gl/GLImage.cpp", + "gl/ImageManager.cpp", "gl/Program.cpp", "gl/ProgramCache.cpp", ], diff --git a/libs/renderengine/gl/GLESRenderEngine.cpp b/libs/renderengine/gl/GLESRenderEngine.cpp index 7caaef35be..d2a7525113 100644 --- a/libs/renderengine/gl/GLESRenderEngine.cpp +++ b/libs/renderengine/gl/GLESRenderEngine.cpp @@ -19,9 +19,8 @@ #define LOG_TAG "RenderEngine" #define ATRACE_TAG ATRACE_TAG_GRAPHICS -#include "GLESRenderEngine.h" - -#include +#include +#include #include #include #include @@ -43,6 +42,7 @@ #include #include #include +#include "GLESRenderEngine.h" #include "GLExtensions.h" #include "GLFramebuffer.h" #include "GLImage.h" @@ -423,10 +423,13 @@ GLESRenderEngine::GLESRenderEngine(uint32_t featureFlags, EGLDisplay display, EG mTraceGpuCompletion = true; mFlushTracer = std::make_unique(this); } + mImageManager = std::make_unique(this); mDrawingBuffer = createFramebuffer(); } GLESRenderEngine::~GLESRenderEngine() { + // Destroy the image manager first. + mImageManager = nullptr; std::lock_guard lock(mRenderingMutex); unbindFrameBuffer(mDrawingBuffer.get()); mDrawingBuffer = nullptr; @@ -619,19 +622,41 @@ void GLESRenderEngine::bindExternalTextureImage(uint32_t texName, const Image& i status_t GLESRenderEngine::bindExternalTextureBuffer(uint32_t texName, const sp& buffer, const sp& bufferFence) { + if (buffer == nullptr) { + return BAD_VALUE; + } + ATRACE_CALL(); - status_t cacheResult = cacheExternalTextureBuffer(buffer); - if (cacheResult != NO_ERROR) { - return cacheResult; + bool found = false; + { + std::lock_guard lock(mRenderingMutex); + auto cachedImage = mImageCache.find(buffer->getId()); + found = (cachedImage != mImageCache.end()); } + // If we couldn't find the image in the cache at this time, then either + // SurfaceFlinger messed up registering the buffer ahead of time or we got + // backed up creating other EGLImages. + if (!found) { + status_t cacheResult = mImageManager->cache(buffer); + if (cacheResult != NO_ERROR) { + return cacheResult; + } + } + + // Whether or not we needed to cache, re-check mImageCache to make sure that + // there's an EGLImage. The current threading model guarantees that we don't + // destroy a cached image until it's really not needed anymore (i.e. this + // function should not be called), so the only possibility is that something + // terrible went wrong and we should just bind something and move on. { std::lock_guard lock(mRenderingMutex); auto cachedImage = mImageCache.find(buffer->getId()); if (cachedImage == mImageCache.end()) { // We failed creating the image if we got here, so bail out. + ALOGE("Failed to create an EGLImage when rendering"); bindExternalTextureImage(texName, *createImage()); return NO_INIT; } @@ -663,7 +688,18 @@ status_t GLESRenderEngine::bindExternalTextureBuffer(uint32_t texName, return NO_ERROR; } -status_t GLESRenderEngine::cacheExternalTextureBuffer(const sp& buffer) { +void GLESRenderEngine::cacheExternalTextureBuffer(const sp& buffer) { + mImageManager->cacheAsync(buffer, nullptr); +} + +std::shared_ptr GLESRenderEngine::cacheExternalTextureBufferForTesting( + const sp& buffer) { + auto barrier = std::make_shared(); + mImageManager->cacheAsync(buffer, barrier); + return barrier; +} + +status_t GLESRenderEngine::cacheExternalTextureBufferInternal(const sp& buffer) { if (buffer == nullptr) { return BAD_VALUE; } @@ -703,12 +739,30 @@ status_t GLESRenderEngine::cacheExternalTextureBuffer(const sp& b } void GLESRenderEngine::unbindExternalTextureBuffer(uint64_t bufferId) { - std::lock_guard lock(mRenderingMutex); - const auto& cachedImage = mImageCache.find(bufferId); - if (cachedImage != mImageCache.end()) { - ALOGV("Destroying image for buffer: %" PRIu64, bufferId); - mImageCache.erase(bufferId); - return; + mImageManager->releaseAsync(bufferId, nullptr); +} + +std::shared_ptr GLESRenderEngine::unbindExternalTextureBufferForTesting( + uint64_t bufferId) { + auto barrier = std::make_shared(); + mImageManager->releaseAsync(bufferId, barrier); + return barrier; +} + +void GLESRenderEngine::unbindExternalTextureBufferInternal(uint64_t bufferId) { + std::unique_ptr image; + { + std::lock_guard lock(mRenderingMutex); + const auto& cachedImage = mImageCache.find(bufferId); + + if (cachedImage != mImageCache.end()) { + ALOGV("Destroying image for buffer: %" PRIu64, bufferId); + // Move the buffer out of cache first, so that we can destroy + // without holding the cache's lock. + image = std::move(cachedImage->second); + mImageCache.erase(bufferId); + return; + } } ALOGV("Failed to find image for buffer: %" PRIu64, bufferId); } diff --git a/libs/renderengine/gl/GLESRenderEngine.h b/libs/renderengine/gl/GLESRenderEngine.h index a01162009b..dd60e50c67 100644 --- a/libs/renderengine/gl/GLESRenderEngine.h +++ b/libs/renderengine/gl/GLESRenderEngine.h @@ -17,9 +17,7 @@ #ifndef SF_GLESRENDERENGINE_H_ #define SF_GLESRENDERENGINE_H_ -#include #include -#include #include #include #include @@ -30,8 +28,11 @@ #include #include #include +#include #include #include +#include +#include "ImageManager.h" #define EGL_NO_CONFIG ((EGLConfig)0) @@ -74,7 +75,7 @@ public: void bindExternalTextureImage(uint32_t texName, const Image& image) override; status_t bindExternalTextureBuffer(uint32_t texName, const sp& buffer, const sp& fence) EXCLUDES(mRenderingMutex); - status_t cacheExternalTextureBuffer(const sp& buffer) EXCLUDES(mRenderingMutex); + void cacheExternalTextureBuffer(const sp& buffer) EXCLUDES(mRenderingMutex); void unbindExternalTextureBuffer(uint64_t bufferId) EXCLUDES(mRenderingMutex); status_t bindFrameBuffer(Framebuffer* framebuffer) override; void unbindFrameBuffer(Framebuffer* framebuffer) override; @@ -101,6 +102,11 @@ public: // Returns true iff mFramebufferImageCache contains an image keyed by bufferId bool isFramebufferImageCachedForTesting(uint64_t bufferId) EXCLUDES(mFramebufferImageCacheMutex); + // These are wrappers around public methods above, but exposing Barrier + // objects so that tests can block. + std::shared_ptr cacheExternalTextureBufferForTesting( + const sp& buffer); + std::shared_ptr unbindExternalTextureBufferForTesting(uint64_t bufferId); protected: Framebuffer* getFramebufferForDrawing() override; @@ -147,6 +153,9 @@ private: void setScissor(const Rect& region); void disableScissor(); bool waitSync(EGLSyncKHR sync, EGLint flags); + status_t cacheExternalTextureBufferInternal(const sp& buffer) + EXCLUDES(mRenderingMutex); + void unbindExternalTextureBufferInternal(uint64_t bufferId) EXCLUDES(mRenderingMutex); // A data space is considered HDR data space if it has BT2020 color space // with PQ or HLG transfer function. @@ -250,7 +259,9 @@ private: bool mRunning = true; }; friend class FlushTracer; + friend class ImageManager; std::unique_ptr mFlushTracer; + std::unique_ptr mImageManager = std::make_unique(this); }; } // namespace gl diff --git a/libs/renderengine/gl/ImageManager.cpp b/libs/renderengine/gl/ImageManager.cpp new file mode 100644 index 0000000000..5af0e4f857 --- /dev/null +++ b/libs/renderengine/gl/ImageManager.cpp @@ -0,0 +1,140 @@ +/* + * Copyright 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#define ATRACE_TAG ATRACE_TAG_GRAPHICS + +#include + +#include +#include +#include "GLESRenderEngine.h" +#include "ImageManager.h" + +namespace android { +namespace renderengine { +namespace gl { + +ImageManager::ImageManager(GLESRenderEngine* engine) : mEngine(engine) { + pthread_setname_np(mThread.native_handle(), "ImageManager"); + // Use SCHED_FIFO to minimize jitter + struct sched_param param = {0}; + param.sched_priority = 2; + if (pthread_setschedparam(mThread.native_handle(), SCHED_FIFO, ¶m) != 0) { + ALOGE("Couldn't set SCHED_FIFO for ImageManager"); + } +} + +ImageManager::~ImageManager() { + { + std::lock_guard lock(mMutex); + mRunning = false; + } + mCondition.notify_all(); + if (mThread.joinable()) { + mThread.join(); + } +} + +void ImageManager::cacheAsync(const sp& buffer, + const std::shared_ptr& barrier) { + if (buffer == nullptr) { + { + std::lock_guard lock(barrier->mutex); + barrier->isOpen = true; + barrier->result = BAD_VALUE; + } + barrier->condition.notify_one(); + return; + } + ATRACE_CALL(); + QueueEntry entry = {QueueEntry::Operation::Insert, buffer, buffer->getId(), barrier}; + queueOperation(std::move(entry)); +} + +status_t ImageManager::cache(const sp& buffer) { + ATRACE_CALL(); + auto barrier = std::make_shared(); + cacheAsync(buffer, barrier); + std::lock_guard lock(barrier->mutex); + barrier->condition.wait(barrier->mutex, + [&]() REQUIRES(barrier->mutex) { return barrier->isOpen; }); + return barrier->result; +} + +void ImageManager::releaseAsync(uint64_t bufferId, const std::shared_ptr& barrier) { + ATRACE_CALL(); + QueueEntry entry = {QueueEntry::Operation::Delete, nullptr, bufferId, barrier}; + queueOperation(std::move(entry)); +} + +void ImageManager::queueOperation(const QueueEntry&& entry) { + { + std::lock_guard lock(mMutex); + mQueue.emplace(entry); + ATRACE_INT("ImageManagerQueueDepth", mQueue.size()); + } + mCondition.notify_one(); +} + +void ImageManager::threadMain() { + set_sched_policy(0, SP_FOREGROUND); + bool run; + { + std::lock_guard lock(mMutex); + run = mRunning; + } + while (run) { + QueueEntry entry; + { + std::lock_guard lock(mMutex); + mCondition.wait(mMutex, + [&]() REQUIRES(mMutex) { return !mQueue.empty() || !mRunning; }); + run = mRunning; + + if (!mRunning) { + // if mRunning is false, then ImageManager is being destroyed, so + // bail out now. + break; + } + + entry = mQueue.front(); + mQueue.pop(); + ATRACE_INT("ImageManagerQueueDepth", mQueue.size()); + } + + status_t result = NO_ERROR; + switch (entry.op) { + case QueueEntry::Operation::Delete: + mEngine->unbindExternalTextureBufferInternal(entry.bufferId); + break; + case QueueEntry::Operation::Insert: + result = mEngine->cacheExternalTextureBufferInternal(entry.buffer); + break; + } + if (entry.barrier != nullptr) { + { + std::lock_guard entryLock(entry.barrier->mutex); + entry.barrier->result = result; + entry.barrier->isOpen = true; + } + entry.barrier->condition.notify_one(); + } + } +} + +} // namespace gl +} // namespace renderengine +} // namespace android diff --git a/libs/renderengine/gl/ImageManager.h b/libs/renderengine/gl/ImageManager.h new file mode 100644 index 0000000000..b5ba554c4f --- /dev/null +++ b/libs/renderengine/gl/ImageManager.h @@ -0,0 +1,70 @@ +/* + * Copyright 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include +#include +#include + +#include + +namespace android { +namespace renderengine { +namespace gl { + +class GLESRenderEngine; + +class ImageManager { +public: + struct Barrier { + std::mutex mutex; + std::condition_variable_any condition; + bool isOpen GUARDED_BY(mutex) = false; + status_t result GUARDED_BY(mutex) = NO_ERROR; + }; + ImageManager(GLESRenderEngine* engine); + ~ImageManager(); + void cacheAsync(const sp& buffer, const std::shared_ptr& barrier) + EXCLUDES(mMutex); + status_t cache(const sp& buffer); + void releaseAsync(uint64_t bufferId, const std::shared_ptr& barrier) EXCLUDES(mMutex); + +private: + struct QueueEntry { + enum class Operation { Delete, Insert }; + + Operation op = Operation::Delete; + sp buffer = nullptr; + uint64_t bufferId = 0; + std::shared_ptr barrier = nullptr; + }; + + void queueOperation(const QueueEntry&& entry); + void threadMain(); + GLESRenderEngine* const mEngine; + std::thread mThread = std::thread([this]() { threadMain(); }); + std::condition_variable_any mCondition; + std::mutex mMutex; + std::queue mQueue GUARDED_BY(mMutex); + + bool mRunning GUARDED_BY(mMutex) = true; +}; + +} // namespace gl +} // namespace renderengine +} // namespace android diff --git a/libs/renderengine/include/renderengine/RenderEngine.h b/libs/renderengine/include/renderengine/RenderEngine.h index f92ccfbfc3..c6a7bd843a 100644 --- a/libs/renderengine/include/renderengine/RenderEngine.h +++ b/libs/renderengine/include/renderengine/RenderEngine.h @@ -116,10 +116,20 @@ public: const sp& fence) = 0; // Caches Image resources for this buffer, but does not bind the buffer to // a particular texture. - virtual status_t cacheExternalTextureBuffer(const sp& buffer) = 0; + // Note that work is deferred to an additional thread, i.e. this call + // is made asynchronously, but the caller can expect that cache/unbind calls + // are performed in a manner that's conflict serializable, i.e. unbinding + // a buffer should never occur before binding the buffer if the caller + // called {bind, cache}ExternalTextureBuffer before calling unbind. + virtual void cacheExternalTextureBuffer(const sp& buffer) = 0; // Removes internal resources referenced by the bufferId. This method should be // invoked when the caller will no longer hold a reference to a GraphicBuffer // and needs to clean up its resources. + // Note that work is deferred to an additional thread, i.e. this call + // is made asynchronously, but the caller can expect that cache/unbind calls + // are performed in a manner that's conflict serializable, i.e. unbinding + // a buffer should never occur before binding the buffer if the caller + // called {bind, cache}ExternalTextureBuffer before calling unbind. virtual void unbindExternalTextureBuffer(uint64_t bufferId) = 0; // When binding a native buffer, it must be done before setViewportAndProjection // Returns NO_ERROR when binds successfully, NO_MEMORY when there's no memory for allocation. diff --git a/libs/renderengine/include/renderengine/mock/RenderEngine.h b/libs/renderengine/include/renderengine/mock/RenderEngine.h index e33bcfd994..b4d3ef24ba 100644 --- a/libs/renderengine/include/renderengine/mock/RenderEngine.h +++ b/libs/renderengine/include/renderengine/mock/RenderEngine.h @@ -51,7 +51,7 @@ public: MOCK_METHOD2(genTextures, void(size_t, uint32_t*)); MOCK_METHOD2(deleteTextures, void(size_t, uint32_t const*)); MOCK_METHOD2(bindExternalTextureImage, void(uint32_t, const renderengine::Image&)); - MOCK_METHOD1(cacheExternalTextureBuffer, status_t(const sp&)); + MOCK_METHOD1(cacheExternalTextureBuffer, void(const sp&)); MOCK_METHOD3(bindExternalTextureBuffer, status_t(uint32_t, const sp&, const sp&)); MOCK_METHOD1(unbindExternalTextureBuffer, void(uint64_t)); diff --git a/libs/renderengine/tests/Android.bp b/libs/renderengine/tests/Android.bp index 9b483ef51d..e98babc30c 100644 --- a/libs/renderengine/tests/Android.bp +++ b/libs/renderengine/tests/Android.bp @@ -31,6 +31,7 @@ cc_test { "libgui", "liblog", "libnativewindow", + "libprocessgroup", "libsync", "libui", "libutils", diff --git a/libs/renderengine/tests/RenderEngineTest.cpp b/libs/renderengine/tests/RenderEngineTest.cpp index 7acaecf465..f47c7fd053 100644 --- a/libs/renderengine/tests/RenderEngineTest.cpp +++ b/libs/renderengine/tests/RenderEngineTest.cpp @@ -14,8 +14,10 @@ * limitations under the License. */ -#include +#include +#include +#include #include #include #include @@ -1001,8 +1003,15 @@ TEST_F(RenderEngineTest, drawLayers_fillsBufferAndCachesImages) { invokeDraw(settings, layers, mBuffer); uint64_t bufferId = layer.source.buffer.buffer->getId(); EXPECT_TRUE(sRE->isImageCachedForTesting(bufferId)); - sRE->unbindExternalTextureBuffer(bufferId); + std::shared_ptr barrier = + sRE->unbindExternalTextureBufferForTesting(bufferId); + std::lock_guard lock(barrier->mutex); + ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5), + [&]() REQUIRES(barrier->mutex) { + return barrier->isOpen; + })); EXPECT_FALSE(sRE->isImageCachedForTesting(bufferId)); + EXPECT_EQ(NO_ERROR, barrier->result); } TEST_F(RenderEngineTest, drawLayers_bindExternalBufferWithNullBuffer) { @@ -1019,21 +1028,52 @@ TEST_F(RenderEngineTest, drawLayers_bindExternalBufferCachesImages) { sRE->bindExternalTextureBuffer(texName, buf, nullptr); uint64_t bufferId = buf->getId(); EXPECT_TRUE(sRE->isImageCachedForTesting(bufferId)); - sRE->unbindExternalTextureBuffer(bufferId); + std::shared_ptr barrier = + sRE->unbindExternalTextureBufferForTesting(bufferId); + std::lock_guard lock(barrier->mutex); + ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5), + [&]() REQUIRES(barrier->mutex) { + return barrier->isOpen; + })); + EXPECT_EQ(NO_ERROR, barrier->result); EXPECT_FALSE(sRE->isImageCachedForTesting(bufferId)); } TEST_F(RenderEngineTest, drawLayers_cacheExternalBufferWithNullBuffer) { - status_t result = sRE->cacheExternalTextureBuffer(nullptr); - ASSERT_EQ(BAD_VALUE, result); + std::shared_ptr barrier = + sRE->cacheExternalTextureBufferForTesting(nullptr); + std::lock_guard lock(barrier->mutex); + ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5), + [&]() REQUIRES(barrier->mutex) { + return barrier->isOpen; + })); + EXPECT_TRUE(barrier->isOpen); + EXPECT_EQ(BAD_VALUE, barrier->result); } TEST_F(RenderEngineTest, drawLayers_cacheExternalBufferCachesImages) { sp buf = allocateSourceBuffer(1, 1); uint64_t bufferId = buf->getId(); - sRE->cacheExternalTextureBuffer(buf); + std::shared_ptr barrier = + sRE->cacheExternalTextureBufferForTesting(buf); + { + std::lock_guard lock(barrier->mutex); + ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5), + [&]() REQUIRES(barrier->mutex) { + return barrier->isOpen; + })); + EXPECT_EQ(NO_ERROR, barrier->result); + } EXPECT_TRUE(sRE->isImageCachedForTesting(bufferId)); - sRE->unbindExternalTextureBuffer(bufferId); + barrier = sRE->unbindExternalTextureBufferForTesting(bufferId); + { + std::lock_guard lock(barrier->mutex); + ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5), + [&]() REQUIRES(barrier->mutex) { + return barrier->isOpen; + })); + EXPECT_EQ(NO_ERROR, barrier->result); + } EXPECT_FALSE(sRE->isImageCachedForTesting(bufferId)); } diff --git a/services/surfaceflinger/BufferLayerConsumer.cpp b/services/surfaceflinger/BufferLayerConsumer.cpp index bd9bd81e2a..414814a6f4 100644 --- a/services/surfaceflinger/BufferLayerConsumer.cpp +++ b/services/surfaceflinger/BufferLayerConsumer.cpp @@ -494,7 +494,6 @@ void BufferLayerConsumer::onBufferAvailable(const BufferItem& item) { if (oldImage == nullptr || oldImage->graphicBuffer() == nullptr || oldImage->graphicBuffer()->getId() != item.mGraphicBuffer->getId()) { mImages[item.mSlot] = std::make_shared(item.mGraphicBuffer, mRE); - mRE.cacheExternalTextureBuffer(item.mGraphicBuffer); } } } @@ -531,6 +530,12 @@ void BufferLayerConsumer::dumpLocked(String8& result, const char* prefix) const ConsumerBase::dumpLocked(result, prefix); } +BufferLayerConsumer::Image::Image(const sp& graphicBuffer, + renderengine::RenderEngine& engine) + : mGraphicBuffer(graphicBuffer), mRE(engine) { + mRE.cacheExternalTextureBuffer(mGraphicBuffer); +} + BufferLayerConsumer::Image::~Image() { if (mGraphicBuffer != nullptr) { ALOGV("Destroying buffer: %" PRId64, mGraphicBuffer->getId()); diff --git a/services/surfaceflinger/BufferLayerConsumer.h b/services/surfaceflinger/BufferLayerConsumer.h index 901556a53f..617b1c2323 100644 --- a/services/surfaceflinger/BufferLayerConsumer.h +++ b/services/surfaceflinger/BufferLayerConsumer.h @@ -223,8 +223,7 @@ private: // Utility class for managing GraphicBuffer references into renderengine class Image { public: - Image(sp graphicBuffer, renderengine::RenderEngine& engine) - : mGraphicBuffer(graphicBuffer), mRE(engine) {} + Image(const sp& graphicBuffer, renderengine::RenderEngine& engine); virtual ~Image(); const sp& graphicBuffer() { return mGraphicBuffer; } -- cgit v1.2.3-59-g8ed1b From cdd0f9d88e8dec40629f60726212e26397a76475 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Fri, 9 Aug 2019 10:44:59 -0700 Subject: SurfaceFlinger: clamp frame refresh duration to min refresh duration Some applications may send doubles frames which are scheduled to be presented very close to each other. In this case to avoid calculating a very high fps for the layer, clamp the refresh duration to the minimal value allowed for the layer. Test: YouTube play "YouTube 60fps Tester" video. Bug: 139209733 Change-Id: I4bcdfad65b57782ec6e346e8d884bfb6e2abac4d --- services/surfaceflinger/Scheduler/LayerInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/Scheduler/LayerInfo.cpp b/services/surfaceflinger/Scheduler/LayerInfo.cpp index e782dd5361..723d71ff39 100644 --- a/services/surfaceflinger/Scheduler/LayerInfo.cpp +++ b/services/surfaceflinger/Scheduler/LayerInfo.cpp @@ -49,7 +49,7 @@ void LayerInfo::setLastPresentTime(nsecs_t lastPresentTime) { mLastPresentTime = lastPresentTime; // Ignore time diff that are too high - those are stale values if (timeDiff > OBSOLETE_TIME_EPSILON_NS.count()) return; - const nsecs_t refreshDuration = (timeDiff > 0) ? timeDiff : mMinRefreshDuration; + const nsecs_t refreshDuration = std::max(timeDiff, mMinRefreshDuration); const int fps = 1e9f / refreshDuration; mRefreshRateHistory.insertRefreshRate(fps); } -- cgit v1.2.3-59-g8ed1b From 4545a8a3ffece9db0732cb9183ed253dc22e8216 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 8 Aug 2019 20:05:32 -0700 Subject: [SurfaceFlinger] Callback to renderengine when erasing BLAST buffers Otherwise we may leak if BufferStateLayer is destroyed first. Bug: 137514000 Test: Over 61 hours, ran: while [ true ]; do am start -n \ com.android.chrome/com.google.android.apps.chrome.Main \ http://m.youtube.com; sleep 10; input tap 740 740 ; sleep 10; input \ keyevent HOME; sleep 0.5; am force-stop com.android.chrome; sleep 0.5; \ done Test: Over >30 minutes: while [ true ]; do am start -n \ com.android.chrome/com.google.android.apps.chrome.Main \ http://m.youtube.com; sleep 10; input tap 740 740; \ sleep 1; content insert --uri content://settings/system --bind \ name:s:user_rotation --bind value:i:1; sleep 4; content insert --uri \ content://settings/system --bind name:s:user_rotation --bind value:i:0; \ sleep 5; input keyevent HOME; sleep 0.5; \ am force-stop com.android.chrome; sleep 0.5; done Test: CtsViewTestCases:ASurfaceControlTest Test: CtsViewTestCases:SurfaceControlTest Test: Transaction_test Change-Id: I743eb8bd9887d17e08b6f1b8e8ec5874359df175 --- services/surfaceflinger/BufferLayer.h | 2 +- services/surfaceflinger/BufferStateLayer.cpp | 14 ++++++++++---- services/surfaceflinger/BufferStateLayer.h | 5 ++--- services/surfaceflinger/ClientCache.cpp | 18 ++++++++++-------- services/surfaceflinger/ClientCache.h | 4 ++-- services/surfaceflinger/Layer.h | 5 +---- services/surfaceflinger/SurfaceFlinger.cpp | 21 ++++++++++++++++----- services/surfaceflinger/SurfaceFlinger.h | 7 +++++-- 8 files changed, 47 insertions(+), 29 deletions(-) (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/BufferLayer.h b/services/surfaceflinger/BufferLayer.h index b679380b79..46a62eda91 100644 --- a/services/surfaceflinger/BufferLayer.h +++ b/services/surfaceflinger/BufferLayer.h @@ -48,7 +48,7 @@ namespace android { class BufferLayer : public Layer { public: explicit BufferLayer(const LayerCreationArgs& args); - ~BufferLayer() override; + virtual ~BufferLayer() override; // ----------------------------------------------------------------------- // Overriden from Layer diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index 2abc1a75a9..63a07c35ca 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -51,6 +51,16 @@ BufferStateLayer::BufferStateLayer(const LayerCreationArgs& args) mCurrentState.dataspace = ui::Dataspace::V0_SRGB; } +BufferStateLayer::~BufferStateLayer() { + if (mActiveBuffer != nullptr) { + // Ensure that mActiveBuffer is uncached from RenderEngine here, as + // RenderEngine may have been using the buffer as an external texture + // after the client uncached the buffer. + auto& engine(mFlinger->getRenderEngine()); + engine.unbindExternalTextureBuffer(mActiveBuffer->getId()); + } +} + // ----------------------------------------------------------------------- // Interface implementation for Layer // ----------------------------------------------------------------------- @@ -610,10 +620,6 @@ void BufferStateLayer::onFirstRef() { } } -void BufferStateLayer::bufferErased(const client_cache_t& clientCacheId) { - mFlinger->getRenderEngine().unbindExternalTextureBuffer(clientCacheId.id); -} - void BufferStateLayer::HwcSlotGenerator::bufferErased(const client_cache_t& clientCacheId) { std::lock_guard lock(mMutex); if (!clientCacheId.isValid()) { diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h index db8ae0d337..97e24cb277 100644 --- a/services/surfaceflinger/BufferStateLayer.h +++ b/services/surfaceflinger/BufferStateLayer.h @@ -35,6 +35,8 @@ class BufferStateLayer : public BufferLayer { public: explicit BufferStateLayer(const LayerCreationArgs&); + ~BufferStateLayer() override; + // ----------------------------------------------------------------------- // Interface implementation for Layer // ----------------------------------------------------------------------- @@ -102,9 +104,6 @@ public: bool fenceHasSignaled() const override; bool framePresentTimeIsCurrent() const override; - // Inherit from ClientCache::ErasedRecipient - void bufferErased(const client_cache_t& clientCacheId) override; - private: nsecs_t getDesiredPresentTime() override; std::shared_ptr getCurrentFenceTime() const override; diff --git a/services/surfaceflinger/ClientCache.cpp b/services/surfaceflinger/ClientCache.cpp index 77f2f5765c..16fe27c00a 100644 --- a/services/surfaceflinger/ClientCache.cpp +++ b/services/surfaceflinger/ClientCache.cpp @@ -55,16 +55,16 @@ bool ClientCache::getBuffer(const client_cache_t& cacheId, return true; } -void ClientCache::add(const client_cache_t& cacheId, const sp& buffer) { +bool ClientCache::add(const client_cache_t& cacheId, const sp& buffer) { auto& [processToken, id] = cacheId; if (processToken == nullptr) { ALOGE("failed to cache buffer: invalid process token"); - return; + return false; } if (!buffer) { ALOGE("failed to cache buffer: invalid buffer"); - return; + return false; } std::lock_guard lock(mMutex); @@ -77,13 +77,13 @@ void ClientCache::add(const client_cache_t& cacheId, const sp& bu token = processToken.promote(); if (!token) { ALOGE("failed to cache buffer: invalid token"); - return; + return false; } status_t err = token->linkToDeath(mDeathRecipient); if (err != NO_ERROR) { ALOGE("failed to cache buffer: could not link to death"); - return; + return false; } auto [itr, success] = mBuffers.emplace(processToken, std::unordered_map()); @@ -95,10 +95,11 @@ void ClientCache::add(const client_cache_t& cacheId, const sp& bu if (processBuffers.size() > BUFFER_CACHE_MAX_SIZE) { ALOGE("failed to cache buffer: cache is full"); - return; + return false; } processBuffers[id].buffer = buffer; + return true; } void ClientCache::erase(const client_cache_t& cacheId) { @@ -139,16 +140,17 @@ sp ClientCache::get(const client_cache_t& cacheId) { return buf->buffer; } -void ClientCache::registerErasedRecipient(const client_cache_t& cacheId, +bool ClientCache::registerErasedRecipient(const client_cache_t& cacheId, const wp& recipient) { std::lock_guard lock(mMutex); ClientCacheBuffer* buf = nullptr; if (!getBuffer(cacheId, &buf)) { ALOGE("failed to register erased recipient, could not retrieve buffer"); - return; + return false; } buf->recipients.insert(recipient); + return true; } void ClientCache::unregisterErasedRecipient(const client_cache_t& cacheId, diff --git a/services/surfaceflinger/ClientCache.h b/services/surfaceflinger/ClientCache.h index 9f057c45d4..aa6c80dfa7 100644 --- a/services/surfaceflinger/ClientCache.h +++ b/services/surfaceflinger/ClientCache.h @@ -36,7 +36,7 @@ class ClientCache : public Singleton { public: ClientCache(); - void add(const client_cache_t& cacheId, const sp& buffer); + bool add(const client_cache_t& cacheId, const sp& buffer); void erase(const client_cache_t& cacheId); sp get(const client_cache_t& cacheId); @@ -48,7 +48,7 @@ public: virtual void bufferErased(const client_cache_t& clientCacheId) = 0; }; - void registerErasedRecipient(const client_cache_t& cacheId, + bool registerErasedRecipient(const client_cache_t& cacheId, const wp& recipient); void unregisterErasedRecipient(const client_cache_t& cacheId, const wp& recipient); diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index ec7389ebad..b46eb112e7 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -93,7 +93,7 @@ struct LayerCreationArgs { LayerMetadata metadata; }; -class Layer : public virtual compositionengine::LayerFE, public ClientCache::ErasedRecipient { +class Layer : public virtual compositionengine::LayerFE { static std::atomic sSequence; public: @@ -700,9 +700,6 @@ public: compositionengine::OutputLayer* findOutputLayerForDisplay( const sp& display) const; - // Inherit from ClientCache::ErasedRecipient - void bufferErased(const client_cache_t& /*clientCacheId*/) override {} - protected: // constant sp mFlinger; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index e6d0dcb3a6..b31bc3813a 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3843,6 +3843,7 @@ void SurfaceFlinger::applyTransactionState(const Vector& states, if (uncacheBuffer.isValid()) { ClientCache::getInstance().erase(uncacheBuffer); + getRenderEngine().unbindExternalTextureBuffer(uncacheBuffer.id); } // If a synchronous transaction is explicitly requested without any changes, force a transaction @@ -4197,12 +4198,18 @@ uint32_t SurfaceFlinger::setClientStateLocked( bool bufferChanged = what & layer_state_t::eBufferChanged; bool cacheIdChanged = what & layer_state_t::eCachedBufferChanged; sp buffer; - if (bufferChanged && cacheIdChanged) { - ClientCache::getInstance().add(s.cachedBuffer, s.buffer); - ClientCache::getInstance().registerErasedRecipient(s.cachedBuffer, - wp(layer)); - getRenderEngine().cacheExternalTextureBuffer(s.buffer); + if (bufferChanged && cacheIdChanged && s.buffer != nullptr) { buffer = s.buffer; + bool success = ClientCache::getInstance().add(s.cachedBuffer, s.buffer); + if (success) { + getRenderEngine().cacheExternalTextureBuffer(s.buffer); + success = ClientCache::getInstance() + .registerErasedRecipient(s.cachedBuffer, + wp(this)); + if (!success) { + getRenderEngine().unbindExternalTextureBuffer(s.buffer->getId()); + } + } } else if (cacheIdChanged) { buffer = ClientCache::getInstance().get(s.cachedBuffer); } else if (bufferChanged) { @@ -6249,6 +6256,10 @@ sp SurfaceFlinger::fromHandle(const sp& handle) { return nullptr; } +void SurfaceFlinger::bufferErased(const client_cache_t& clientCacheId) { + getRenderEngine().unbindExternalTextureBuffer(clientCacheId.id); +} + } // namespace android #if defined(__gl_h_) diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 97d67aaf4f..0c6da0077f 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -170,9 +170,9 @@ public: class SurfaceFlinger : public BnSurfaceComposer, public PriorityDumper, + public ClientCache::ErasedRecipient, private IBinder::DeathRecipient, - private HWC2::ComposerCallback -{ + private HWC2::ComposerCallback { public: SurfaceFlingerBE& getBE() { return mBE; } const SurfaceFlingerBE& getBE() const { return mBE; } @@ -328,6 +328,9 @@ public: sp fromHandle(const sp& handle) REQUIRES(mStateLock); + // Inherit from ClientCache::ErasedRecipient + void bufferErased(const client_cache_t& clientCacheId) override; + private: friend class BufferLayer; friend class BufferQueueLayer; -- cgit v1.2.3-59-g8ed1b From 63edd27c0843a7c9b4268b86c3b991d78607f888 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 15 Aug 2019 18:24:55 -0700 Subject: Remove test image Bug: 139427228 Test: builds Change-Id: I72ff075bbf878c39a17803b2d74181fbe045a3fb --- services/surfaceflinger/test.png | Bin 3058273 -> 0 bytes 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 services/surfaceflinger/test.png (limited to 'services/surfaceflinger') diff --git a/services/surfaceflinger/test.png b/services/surfaceflinger/test.png deleted file mode 100644 index 773dcc3e72..0000000000 Binary files a/services/surfaceflinger/test.png and /dev/null differ -- cgit v1.2.3-59-g8ed1b