From 9953e060ba211798cd5efe04afb1bcf4ef5b600a Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Wed, 24 Jul 2024 22:56:22 -0700 Subject: SF: add a work duration slack when missing a frame When missing a frame, SF usually ends up waiting 1ms for the previous fence to fire. This causes sometimes skipping the next frame as the work duration with the 1ms pushes the vsync one more frame. Bug: 354007767 Flag: com.android.graphics.surfaceflinger.flags.allow_n_vsyncs_in_targeter Test: SF unit tests Change-Id: Ia4e2791c420c17bcbe123bd61cc569695702a40c --- services/surfaceflinger/Scheduler/MessageQueue.cpp | 5 +++-- services/surfaceflinger/Scheduler/MessageQueue.h | 4 ++-- services/surfaceflinger/SurfaceFlinger.cpp | 9 ++++++--- services/surfaceflinger/SurfaceFlinger.h | 2 +- .../unittests/FrameRateSelectionStrategyTest.cpp | 9 +++++---- .../tests/unittests/SetFrameRateTest.cpp | 20 ++++++++++---------- .../unittests/SurfaceFlinger_CreateDisplayTest.cpp | 6 +++--- .../unittests/SurfaceFlinger_DestroyDisplayTest.cpp | 2 +- .../tests/unittests/SurfaceFlinger_HotplugTest.cpp | 6 +++--- .../SurfaceFlinger_InitializeDisplaysTest.cpp | 2 +- .../SurfaceFlinger_SetPowerModeInternalTest.cpp | 2 +- .../tests/unittests/TestableScheduler.h | 2 +- .../tests/unittests/TransactionApplicationTest.cpp | 10 +++++----- 13 files changed, 42 insertions(+), 37 deletions(-) diff --git a/services/surfaceflinger/Scheduler/MessageQueue.cpp b/services/surfaceflinger/Scheduler/MessageQueue.cpp index 6a67ac5d42..2e1f938126 100644 --- a/services/surfaceflinger/Scheduler/MessageQueue.cpp +++ b/services/surfaceflinger/Scheduler/MessageQueue.cpp @@ -188,12 +188,13 @@ void MessageQueue::scheduleConfigure() { postMessage(sp::make(mCompositor)); } -void MessageQueue::scheduleFrame() { +void MessageQueue::scheduleFrame(Duration workDurationSlack) { SFTRACE_CALL(); std::lock_guard lock(mVsync.mutex); + const auto workDuration = Duration(mVsync.workDuration.get() - workDurationSlack); mVsync.scheduledFrameTimeOpt = - mVsync.registration->schedule({.workDuration = mVsync.workDuration.get().count(), + mVsync.registration->schedule({.workDuration = workDuration.ns(), .readyDuration = 0, .lastVsync = mVsync.lastCallbackTime.ns()}); } diff --git a/services/surfaceflinger/Scheduler/MessageQueue.h b/services/surfaceflinger/Scheduler/MessageQueue.h index c5fc371d3a..ba1efbe58f 100644 --- a/services/surfaceflinger/Scheduler/MessageQueue.h +++ b/services/surfaceflinger/Scheduler/MessageQueue.h @@ -74,7 +74,7 @@ public: virtual void postMessage(sp&&) = 0; virtual void postMessageDelayed(sp&&, nsecs_t uptimeDelay) = 0; virtual void scheduleConfigure() = 0; - virtual void scheduleFrame() = 0; + virtual void scheduleFrame(Duration workDurationSlack = Duration::fromNs(0)) = 0; virtual std::optional getScheduledFrameResult() const = 0; }; @@ -149,7 +149,7 @@ public: void postMessageDelayed(sp&&, nsecs_t uptimeDelay) override; void scheduleConfigure() override; - void scheduleFrame() override; + void scheduleFrame(Duration workDurationSlack = Duration::fromNs(0)) override; std::optional getScheduledFrameResult() const override; }; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index e32202f788..b2fe5ae5a6 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2164,12 +2164,12 @@ sp SurfaceFlinger::createDisplayEventConnection( return mScheduler->createDisplayEventConnection(cycle, eventRegistration, layerHandle); } -void SurfaceFlinger::scheduleCommit(FrameHint hint) { +void SurfaceFlinger::scheduleCommit(FrameHint hint, Duration workDurationSlack) { if (hint == FrameHint::kActive) { mScheduler->resetIdleTimer(); } mPowerAdvisor->notifyDisplayUpdateImminentAndCpuReset(); - mScheduler->scheduleFrame(); + mScheduler->scheduleFrame(workDurationSlack); } void SurfaceFlinger::scheduleComposite(FrameHint hint) { @@ -2629,7 +2629,10 @@ bool SurfaceFlinger::commit(PhysicalDisplayId pacesetterId, mScheduler->getVsyncSchedule()->getTracker().onFrameMissed( pacesetterFrameTarget.expectedPresentTime()); } - scheduleCommit(FrameHint::kNone); + const Duration slack = FlagManager::getInstance().allow_n_vsyncs_in_targeter() + ? TimePoint::now() - pacesetterFrameTarget.frameBeginTime() + : Duration::fromNs(0); + scheduleCommit(FrameHint::kNone, slack); return false; } } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index b17fd49463..7263aaaaca 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -273,7 +273,7 @@ public: enum class FrameHint { kNone, kActive }; // Schedule commit of transactions on the main thread ahead of the next VSYNC. - void scheduleCommit(FrameHint); + void scheduleCommit(FrameHint, Duration workDurationSlack = Duration::fromNs(0)); // As above, but also force composite regardless if transactions were committed. void scheduleComposite(FrameHint); // As above, but also force dirty geometry to repaint. diff --git a/services/surfaceflinger/tests/unittests/FrameRateSelectionStrategyTest.cpp b/services/surfaceflinger/tests/unittests/FrameRateSelectionStrategyTest.cpp index 5c742d7360..866eb08913 100644 --- a/services/surfaceflinger/tests/unittests/FrameRateSelectionStrategyTest.cpp +++ b/services/surfaceflinger/tests/unittests/FrameRateSelectionStrategyTest.cpp @@ -28,6 +28,7 @@ namespace android { +using testing::_; using testing::DoAll; using testing::Mock; using testing::SetArgPointee; @@ -91,7 +92,7 @@ INSTANTIATE_TEST_SUITE_P(PerLayerType, FrameRateSelectionStrategyTest, PrintToStringParamName); TEST_P(FrameRateSelectionStrategyTest, SetAndGet) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); auto layer = mLayers.emplace_back(layerFactory->createLayer(mFlinger)); @@ -104,7 +105,7 @@ TEST_P(FrameRateSelectionStrategyTest, SetAndGet) { } TEST_P(FrameRateSelectionStrategyTest, SetChildOverrideChildren) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); auto parent = mLayers.emplace_back(layerFactory->createLayer(mFlinger)); @@ -128,7 +129,7 @@ TEST_P(FrameRateSelectionStrategyTest, SetChildOverrideChildren) { } TEST_P(FrameRateSelectionStrategyTest, SetParentOverrideChildren) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); auto layer1 = mLayers.emplace_back(layerFactory->createLayer(mFlinger)); @@ -169,7 +170,7 @@ TEST_P(FrameRateSelectionStrategyTest, SetParentOverrideChildren) { } TEST_P(FrameRateSelectionStrategyTest, OverrideChildrenAndSelf) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); auto layer1 = mLayers.emplace_back(layerFactory->createLayer(mFlinger)); diff --git a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp index 9899d4290b..4705dd1cc7 100644 --- a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp +++ b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp @@ -35,7 +35,7 @@ #include "mock/MockVsyncController.h" namespace android { - +using testing::_; using testing::DoAll; using testing::Mock; using testing::SetArgPointee; @@ -93,7 +93,7 @@ void SetFrameRateTest::commitTransaction() { namespace { TEST_P(SetFrameRateTest, SetAndGet) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); @@ -104,7 +104,7 @@ TEST_P(SetFrameRateTest, SetAndGet) { } TEST_P(SetFrameRateTest, SetAndGetParent) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); @@ -129,7 +129,7 @@ TEST_P(SetFrameRateTest, SetAndGetParent) { } TEST_P(SetFrameRateTest, SetAndGetParentAllVote) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); @@ -168,7 +168,7 @@ TEST_P(SetFrameRateTest, SetAndGetParentAllVote) { } TEST_P(SetFrameRateTest, SetAndGetChild) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); @@ -193,7 +193,7 @@ TEST_P(SetFrameRateTest, SetAndGetChild) { } TEST_P(SetFrameRateTest, SetAndGetChildAllVote) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); @@ -232,7 +232,7 @@ TEST_P(SetFrameRateTest, SetAndGetChildAllVote) { } TEST_P(SetFrameRateTest, SetAndGetChildAddAfterVote) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); @@ -262,7 +262,7 @@ TEST_P(SetFrameRateTest, SetAndGetChildAddAfterVote) { } TEST_P(SetFrameRateTest, SetAndGetChildRemoveAfterVote) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); @@ -293,7 +293,7 @@ TEST_P(SetFrameRateTest, SetAndGetChildRemoveAfterVote) { } TEST_P(SetFrameRateTest, SetAndGetParentNotInTree) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); @@ -352,7 +352,7 @@ TEST_P(SetFrameRateTest, SetOnParentActivatesTree) { } TEST_P(SetFrameRateTest, addChildForParentWithTreeVote) { - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); const auto& layerFactory = GetParam(); diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp index e5f2a9138d..2d3ebb47bd 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp @@ -97,7 +97,7 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForNonsecureDisplay) { // Cleanup conditions // Creating the display commits a display transaction. - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); } TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForSecureDisplay) { @@ -129,7 +129,7 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForSecureDisplay) { // Cleanup conditions // Creating the display commits a display transaction. - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); } TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForUniqueId) { @@ -159,7 +159,7 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForUniqueId) { // Cleanup conditions // Creating the display commits a display transaction. - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); } // Requesting 0 tells SF not to do anything, i.e., default to refresh as physical displays diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DestroyDisplayTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DestroyDisplayTest.cpp index f8ad8e1e1b..df8f68f2b6 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DestroyDisplayTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DestroyDisplayTest.cpp @@ -38,7 +38,7 @@ TEST_F(DestroyDisplayTest, destroyDisplayClearsCurrentStateForDisplay) { // Call Expectations // Destroying the display commits a display transaction. - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); // -------------------------------------------------------------------- // Invocation diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp index 897f9a0319..aef467ab9d 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp @@ -48,7 +48,7 @@ TEST_F(HotplugTest, schedulesConfigureToProcessHotplugEvents) { TEST_F(HotplugTest, schedulesFrameToCommitDisplayTransaction) { EXPECT_CALL(*mFlinger.scheduler(), scheduleConfigure()).Times(1); - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); constexpr HWDisplayId displayId1 = 456; mFlinger.onComposerHalHotplugEvent(displayId1, DisplayHotplugEvent::DISCONNECTED); @@ -73,7 +73,7 @@ TEST_F(HotplugTest, ignoresDuplicateDisconnection) { .WillOnce(Return(Error::NONE)); // A single commit should be scheduled for both configure calls. - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); ExternalDisplay::injectPendingHotplugEvent(this, Connection::CONNECTED); mFlinger.configure(); @@ -116,7 +116,7 @@ TEST_F(HotplugTest, rejectsHotplugIfFailedToLoadDisplayModes) { setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE)) .WillOnce(Return(Error::NONE)); - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); ExternalDisplay::injectPendingHotplugEvent(this, Connection::CONNECTED); mFlinger.configure(); diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_InitializeDisplaysTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_InitializeDisplaysTest.cpp index eaf468432c..5231965277 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_InitializeDisplaysTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_InitializeDisplaysTest.cpp @@ -28,7 +28,7 @@ struct InitializeDisplaysTest : DualDisplayTransactionTest( mFlinger.scheduler()->getVsyncSchedule()->getTracker()), diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPowerModeInternalTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPowerModeInternalTest.cpp index 83e2f980ce..fed7b2e767 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPowerModeInternalTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPowerModeInternalTest.cpp @@ -271,7 +271,7 @@ struct DisplayPowerCase { } static void setupRepaintEverythingCallExpectations(DisplayTransactionTest* test) { - EXPECT_CALL(*test->mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*test->mFlinger.scheduler(), scheduleFrame(_)).Times(1); } static void setupComposerCallExpectations(DisplayTransactionTest* test, PowerMode mode) { diff --git a/services/surfaceflinger/tests/unittests/TestableScheduler.h b/services/surfaceflinger/tests/unittests/TestableScheduler.h index f0638094c7..0814e3d6fe 100644 --- a/services/surfaceflinger/tests/unittests/TestableScheduler.h +++ b/services/surfaceflinger/tests/unittests/TestableScheduler.h @@ -62,7 +62,7 @@ public: } MOCK_METHOD(void, scheduleConfigure, (), (override)); - MOCK_METHOD(void, scheduleFrame, (), (override)); + MOCK_METHOD(void, scheduleFrame, (Duration), (override)); MOCK_METHOD(void, postMessage, (sp&&), (override)); void doFrameSignal(ICompositor& compositor, VsyncId vsyncId) { diff --git a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp index e13fe4958c..fab1f6d451 100644 --- a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp +++ b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp @@ -105,7 +105,7 @@ public: void NotPlacedOnTransactionQueue(uint32_t flags) { ASSERT_TRUE(mFlinger.getTransactionQueue().isEmpty()); - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); TransactionInfo transaction; setupSingle(transaction, flags, /*desiredPresentTime*/ systemTime(), /*isAutoTimestamp*/ true, @@ -129,7 +129,7 @@ public: void PlaceOnTransactionQueue(uint32_t flags) { ASSERT_TRUE(mFlinger.getTransactionQueue().isEmpty()); - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); // first check will see desired present time has not passed, // but afterwards it will look like the desired present time has passed @@ -155,7 +155,7 @@ public: void BlockedByPriorTransaction(uint32_t flags) { ASSERT_TRUE(mFlinger.getTransactionQueue().isEmpty()); nsecs_t time = systemTime(); - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(2); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(2); // transaction that should go on the pending thread TransactionInfo transactionA; @@ -217,7 +217,7 @@ public: TEST_F(TransactionApplicationTest, AddToPendingQueue) { ASSERT_TRUE(mFlinger.getTransactionQueue().isEmpty()); - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); TransactionInfo transactionA; // transaction to go on pending queue setupSingle(transactionA, /*flags*/ 0, /*desiredPresentTime*/ s2ns(1), false, @@ -238,7 +238,7 @@ TEST_F(TransactionApplicationTest, AddToPendingQueue) { TEST_F(TransactionApplicationTest, Flush_RemovesFromQueue) { ASSERT_TRUE(mFlinger.getTransactionQueue().isEmpty()); - EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); TransactionInfo transactionA; // transaction to go on pending queue setupSingle(transactionA, /*flags*/ 0, /*desiredPresentTime*/ s2ns(1), false, -- cgit v1.2.3-59-g8ed1b