commit 50e2e4d5f75fb481b729ef05808008adad7de10a
author Dominik Laskowski <domlaskowski@google.com> Wed Oct 04 10:58:28 2023 -0400
committer Dominik Laskowski <domlaskowski@google.com> Fri Oct 06 11:29:01 2023 -0400
tree 5124e6f47f5c738feccb4047315f569aba86a615
parent cc89e85f3aa9225d977d3f78197307ec2f6f47e1

SF: Fix freezing after follower display mode set

When a mode set is initiated for any display, SF skips committing until
HWC confirms the mode set by signaling the corresponding present fence.

However, if a concurrent follower display misses a frame (i.e. does not
signal the fence) as its mode is set, SF::commit skips endlessly, since
the Scheduler only checked for missed frames on followers after commit.

Fix this by having all displays FrameTargeter::beginFrame before commit.

Fixes: 301082260
Test: Mode set on external display does not sporadically freeze SF.
Test: SchedulerTest.onFrameSignalMultipleDisplays
Change-Id: Ic97f33975f6d30279cc65cefe5d7ccc2561c45aa

services/surfaceflinger/Scheduler/Scheduler.cpp

diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 76f1af9..b5b20fb 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -192,7 +192,8 @@
     for (const auto& [id, display] : mDisplays) {
         if (id == pacesetterId) continue;
 
-        const FrameTargeter& targeter = *display.targeterPtr;
+        FrameTargeter& targeter = *display.targeterPtr;
+        targeter.beginFrame(beginFrameArgs, *display.schedulePtr);
         targets.try_emplace(id, &targeter.target());
     }
 
@@ -206,8 +207,6 @@
         if (id == pacesetterId) continue;
 
         FrameTargeter& targeter = *display.targeterPtr;
-        targeter.beginFrame(beginFrameArgs, *display.schedulePtr);
-
         targeters.try_emplace(id, &targeter);
     }
 

services/surfaceflinger/tests/unittests/SchedulerTest.cpp

diff --git a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
index 173f941..0df41ed 100644
--- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
@@ -444,6 +444,63 @@
     }
 }
 
+TEST_F(SchedulerTest, onFrameSignalMultipleDisplays) {
+    mScheduler->registerDisplay(kDisplayId1,
+                                std::make_shared<RefreshRateSelector>(kDisplay1Modes,
+                                                                      kDisplay1Mode60->getId()));
+    mScheduler->registerDisplay(kDisplayId2,
+                                std::make_shared<RefreshRateSelector>(kDisplay2Modes,
+                                                                      kDisplay2Mode60->getId()));
+
+    using VsyncIds = std::vector<std::pair<PhysicalDisplayId, VsyncId>>;
+
+    struct Compositor final : ICompositor {
+        VsyncIds vsyncIds;
+        bool committed = true;
+
+        void configure() override {}
+
+        bool commit(PhysicalDisplayId, const scheduler::FrameTargets& targets) override {
+            vsyncIds.clear();
+
+            for (const auto& [id, target] : targets) {
+                vsyncIds.emplace_back(id, target->vsyncId());
+            }
+
+            return committed;
+        }
+
+        CompositeResultsPerDisplay composite(PhysicalDisplayId,
+                                             const scheduler::FrameTargeters&) override {
+            CompositeResultsPerDisplay results;
+
+            for (const auto& [id, _] : vsyncIds) {
+                results.try_emplace(id,
+                                    CompositeResult{.compositionCoverage =
+                                                            CompositionCoverage::Hwc});
+            }
+
+            return results;
+        }
+
+        void sample() override {}
+    } compositor;
+
+    mScheduler->doFrameSignal(compositor, VsyncId(42));
+
+    const auto makeVsyncIds = [](VsyncId vsyncId) -> VsyncIds {
+        return {{kDisplayId1, vsyncId}, {kDisplayId2, vsyncId}};
+    };
+
+    EXPECT_EQ(makeVsyncIds(VsyncId(42)), compositor.vsyncIds);
+
+    compositor.committed = false;
+    mScheduler->doFrameSignal(compositor, VsyncId(43));
+
+    // FrameTargets should be updated despite the skipped commit.
+    EXPECT_EQ(makeVsyncIds(VsyncId(43)), compositor.vsyncIds);
+}
+
 class AttachedChoreographerTest : public SchedulerTest {
 protected:
     void frameRateTestScenario(Fps layerFps, int8_t frameRateCompatibility, Fps displayFps,

services/surfaceflinger/tests/unittests/TestableScheduler.h

diff --git a/services/surfaceflinger/tests/unittests/TestableScheduler.h b/services/surfaceflinger/tests/unittests/TestableScheduler.h
index 151b178..014d07c 100644
--- a/services/surfaceflinger/tests/unittests/TestableScheduler.h
+++ b/services/surfaceflinger/tests/unittests/TestableScheduler.h
@@ -59,6 +59,12 @@
     MOCK_METHOD(void, scheduleFrame, (), (override));
     MOCK_METHOD(void, postMessage, (sp<MessageHandler>&&), (override));
 
+    void doFrameSignal(ICompositor& compositor, VsyncId vsyncId) {
+        ftl::FakeGuard guard1(kMainThreadContext);
+        ftl::FakeGuard guard2(mDisplayLock);
+        Scheduler::onFrameSignal(compositor, vsyncId, TimePoint());
+    }
+
     // Used to inject mock event thread.
     ConnectionHandle createConnection(std::unique_ptr<EventThread> eventThread) {
         return Scheduler::createConnection(std::move(eventThread));