Skip to content

Commit b4fbc2d

Browse files
Dominik LaskowskiAndroid (Google) Code Review
Dominik Laskowski
authored and
Android (Google) Code Review
committed
Merge "SF: Remove multithreaded_present flag" into main
2 parents 329c7f9 + db4e6ad commit b4fbc2d

11 files changed

Lines changed: 37 additions & 82 deletions

File tree

‎services/surfaceflinger/CompositionEngine/src/CompositionEngine.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ nsecs_t CompositionEngine::getLastFrameRefreshTimestamp() const {
9191

9292
namespace {
9393
void offloadOutputs(Outputs& outputs) {
94-
if (!FlagManager::getInstance().multithreaded_present() || outputs.size() < 2) {
94+
if (outputs.size() < 2) {
9595
return;
9696
}
9797

‎services/surfaceflinger/CompositionEngine/tests/CompositionEngineTest.cpp‎

Lines changed: 27 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ using ::testing::ReturnRef;
4343
using ::testing::SaveArg;
4444
using ::testing::StrictMock;
4545

46+
static constexpr PhysicalDisplayId kDisplayId1 = PhysicalDisplayId::fromPort(123u);
47+
static constexpr PhysicalDisplayId kDisplayId2 = PhysicalDisplayId::fromPort(234u);
48+
static constexpr PhysicalDisplayId kDisplayId3 = PhysicalDisplayId::fromPort(567u);
49+
4650
struct CompositionEngineTest : public testing::Test {
4751
std::shared_ptr<TimeStats> mTimeStats;
4852

@@ -52,6 +56,26 @@ struct CompositionEngineTest : public testing::Test {
5256
std::shared_ptr<mock::Output> mOutput1{std::make_shared<StrictMock<mock::Output>>()};
5357
std::shared_ptr<mock::Output> mOutput2{std::make_shared<StrictMock<mock::Output>>()};
5458
std::shared_ptr<mock::Output> mOutput3{std::make_shared<StrictMock<mock::Output>>()};
59+
60+
std::array<impl::OutputCompositionState, 3> mOutputStates;
61+
62+
void SetUp() override {
63+
EXPECT_CALL(*mOutput1, getDisplayId)
64+
.WillRepeatedly(Return(std::make_optional<DisplayId>(kDisplayId1)));
65+
EXPECT_CALL(*mOutput2, getDisplayId)
66+
.WillRepeatedly(Return(std::make_optional<DisplayId>(kDisplayId2)));
67+
EXPECT_CALL(*mOutput3, getDisplayId)
68+
.WillRepeatedly(Return(std::make_optional<DisplayId>(kDisplayId3)));
69+
70+
// Most tests will depend on the outputs being enabled.
71+
for (auto& state : mOutputStates) {
72+
state.isEnabled = true;
73+
}
74+
75+
EXPECT_CALL(*mOutput1, getState).WillRepeatedly(ReturnRef(mOutputStates[0]));
76+
EXPECT_CALL(*mOutput2, getState).WillRepeatedly(ReturnRef(mOutputStates[1]));
77+
EXPECT_CALL(*mOutput3, getState).WillRepeatedly(ReturnRef(mOutputStates[2]));
78+
}
5579
};
5680

5781
TEST_F(CompositionEngineTest, canInstantiateCompositionEngine) {
@@ -94,15 +118,15 @@ struct CompositionEnginePresentTest : public CompositionEngineTest {
94118
StrictMock<CompositionEnginePartialMock> mEngine;
95119
};
96120

97-
TEST_F(CompositionEnginePresentTest, worksWithEmptyRequest) {
121+
TEST_F(CompositionEnginePresentTest, zeroOutputs) {
98122
// present() always calls preComposition() and postComposition()
99123
EXPECT_CALL(mEngine, preComposition(Ref(mRefreshArgs)));
100124
EXPECT_CALL(mEngine, postComposition(Ref(mRefreshArgs)));
101125

102126
mEngine.present(mRefreshArgs);
103127
}
104128

105-
TEST_F(CompositionEnginePresentTest, worksAsExpected) {
129+
TEST_F(CompositionEnginePresentTest, threeOutputs) {
106130
// Expect calls to in a certain sequence
107131
InSequence seq;
108132

@@ -114,9 +138,7 @@ TEST_F(CompositionEnginePresentTest, worksAsExpected) {
114138
EXPECT_CALL(*mOutput2, prepare(Ref(mRefreshArgs), _));
115139
EXPECT_CALL(*mOutput3, prepare(Ref(mRefreshArgs), _));
116140

117-
// All of mOutput<i> are StrictMocks. If the flag is true, it will introduce
118-
// calls to getDisplayId, which are not relevant to this test.
119-
SET_FLAG_FOR_TEST(flags::multithreaded_present, false);
141+
EXPECT_CALL(*mOutput1, supportsOffloadPresent).WillOnce(Return(false));
120142

121143
// The last step is to actually present each output.
122144
EXPECT_CALL(*mOutput1, present(Ref(mRefreshArgs)))
@@ -284,8 +306,6 @@ struct CompositionEngineOffloadTest : public testing::Test {
284306
std::shared_ptr<mock::Output> mVirtualDisplay{std::make_shared<StrictMock<mock::Output>>()};
285307
std::shared_ptr<mock::Output> mHalVirtualDisplay{std::make_shared<StrictMock<mock::Output>>()};
286308

287-
static constexpr PhysicalDisplayId kDisplayId1 = PhysicalDisplayId::fromPort(123u);
288-
static constexpr PhysicalDisplayId kDisplayId2 = PhysicalDisplayId::fromPort(234u);
289309
static constexpr GpuVirtualDisplayId kGpuVirtualDisplayId{789u};
290310
static constexpr HalVirtualDisplayId kHalVirtualDisplayId{456u};
291311

@@ -332,7 +352,6 @@ TEST_F(CompositionEngineOffloadTest, basic) {
332352
EXPECT_CALL(*mDisplay1, offloadPresentNextFrame).Times(1);
333353
EXPECT_CALL(*mDisplay2, offloadPresentNextFrame).Times(0);
334354

335-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
336355
setOutputs({mDisplay1, mDisplay2});
337356

338357
mEngine.present(mRefreshArgs);
@@ -345,7 +364,6 @@ TEST_F(CompositionEngineOffloadTest, dependsOnSupport) {
345364
EXPECT_CALL(*mDisplay1, offloadPresentNextFrame).Times(0);
346365
EXPECT_CALL(*mDisplay2, offloadPresentNextFrame).Times(0);
347366

348-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
349367
setOutputs({mDisplay1, mDisplay2});
350368

351369
mEngine.present(mRefreshArgs);
@@ -358,20 +376,6 @@ TEST_F(CompositionEngineOffloadTest, dependsOnSupport2) {
358376
EXPECT_CALL(*mDisplay1, offloadPresentNextFrame).Times(0);
359377
EXPECT_CALL(*mDisplay2, offloadPresentNextFrame).Times(0);
360378

361-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
362-
setOutputs({mDisplay1, mDisplay2});
363-
364-
mEngine.present(mRefreshArgs);
365-
}
366-
367-
TEST_F(CompositionEngineOffloadTest, dependsOnFlag) {
368-
EXPECT_CALL(*mDisplay1, supportsOffloadPresent).Times(0);
369-
EXPECT_CALL(*mDisplay2, supportsOffloadPresent).Times(0);
370-
371-
EXPECT_CALL(*mDisplay1, offloadPresentNextFrame).Times(0);
372-
EXPECT_CALL(*mDisplay2, offloadPresentNextFrame).Times(0);
373-
374-
SET_FLAG_FOR_TEST(flags::multithreaded_present, false);
375379
setOutputs({mDisplay1, mDisplay2});
376380

377381
mEngine.present(mRefreshArgs);
@@ -382,7 +386,6 @@ TEST_F(CompositionEngineOffloadTest, oneDisplay) {
382386

383387
EXPECT_CALL(*mDisplay1, offloadPresentNextFrame).Times(0);
384388

385-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
386389
setOutputs({mDisplay1});
387390

388391
mEngine.present(mRefreshArgs);
@@ -397,7 +400,6 @@ TEST_F(CompositionEngineOffloadTest, virtualDisplay) {
397400
EXPECT_CALL(*mDisplay2, offloadPresentNextFrame).Times(0);
398401
EXPECT_CALL(*mVirtualDisplay, offloadPresentNextFrame).Times(0);
399402

400-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
401403
setOutputs({mDisplay1, mDisplay2, mVirtualDisplay});
402404

403405
mEngine.present(mRefreshArgs);
@@ -410,7 +412,6 @@ TEST_F(CompositionEngineOffloadTest, virtualDisplay2) {
410412
EXPECT_CALL(*mDisplay1, offloadPresentNextFrame).Times(0);
411413
EXPECT_CALL(*mVirtualDisplay, offloadPresentNextFrame).Times(0);
412414

413-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
414415
setOutputs({mDisplay1, mVirtualDisplay});
415416

416417
mEngine.present(mRefreshArgs);
@@ -423,7 +424,6 @@ TEST_F(CompositionEngineOffloadTest, halVirtual) {
423424
EXPECT_CALL(*mDisplay1, offloadPresentNextFrame).Times(1);
424425
EXPECT_CALL(*mHalVirtualDisplay, offloadPresentNextFrame).Times(0);
425426

426-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
427427
setOutputs({mDisplay1, mHalVirtualDisplay});
428428

429429
mEngine.present(mRefreshArgs);
@@ -440,7 +440,6 @@ TEST_F(CompositionEngineOffloadTest, ordering) {
440440
EXPECT_CALL(*mDisplay1, offloadPresentNextFrame).Times(1);
441441
EXPECT_CALL(*mDisplay2, offloadPresentNextFrame).Times(0);
442442

443-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
444443
setOutputs({mVirtualDisplay, mHalVirtualDisplay, mDisplay1, mDisplay2});
445444

446445
mEngine.present(mRefreshArgs);
@@ -458,7 +457,6 @@ TEST_F(CompositionEngineOffloadTest, dependsOnEnabled) {
458457
EXPECT_CALL(*mDisplay1, offloadPresentNextFrame).Times(0);
459458
EXPECT_CALL(*mDisplay2, offloadPresentNextFrame).Times(0);
460459

461-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
462460
setOutputs({mDisplay1, mDisplay2});
463461

464462
mEngine.present(mRefreshArgs);
@@ -478,7 +476,6 @@ TEST_F(CompositionEngineOffloadTest, disabledDisplaysDoNotPreventOthersFromOfflo
478476
EXPECT_CALL(*mDisplay2, offloadPresentNextFrame).Times(0);
479477
EXPECT_CALL(*mHalVirtualDisplay, offloadPresentNextFrame).Times(0);
480478

481-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
482479
setOutputs({mDisplay1, mDisplay2, mHalVirtualDisplay});
483480

484481
mEngine.present(mRefreshArgs);

‎services/surfaceflinger/DisplayHardware/AidlComposerHal.cpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1773,7 +1773,6 @@ void AidlComposer::onHotplugDisconnect(Display display) {
17731773
}
17741774

17751775
bool AidlComposer::hasMultiThreadedPresentSupport(Display display) {
1776-
if (!FlagManager::getInstance().multithreaded_present()) return false;
17771776
const auto displayId = translate<int64_t>(display);
17781777
std::vector<AidlDisplayCapability> capabilities;
17791778
const auto status = mAidlComposerClient->getDisplayCapabilities(displayId, &capabilities);

‎services/surfaceflinger/Scheduler/Scheduler.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -554,8 +554,7 @@ void Scheduler::resyncAllToHardwareVsync(bool allowToEnable) {
554554
ftl::FakeGuard guard(kMainThreadContext);
555555

556556
for (const auto& [id, display] : mDisplays) {
557-
if (display.powerMode != hal::PowerMode::OFF ||
558-
!FlagManager::getInstance().multithreaded_present()) {
557+
if (display.powerMode != hal::PowerMode::OFF) {
559558
resyncToHardwareVsyncLocked(id, allowToEnable);
560559
}
561560
}

‎services/surfaceflinger/SurfaceFlinger.cpp‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5724,8 +5724,7 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
57245724
}
57255725

57265726
getHwComposer().setPowerMode(displayId, mode);
5727-
if (mode != hal::PowerMode::DOZE_SUSPEND &&
5728-
(displayId == mActiveDisplayId || FlagManager::getInstance().multithreaded_present())) {
5727+
if (mode != hal::PowerMode::DOZE_SUSPEND) {
57295728
const bool enable =
57305729
mScheduler->getVsyncSchedule(displayId)->getPendingHardwareVsyncState();
57315730
requestHardwareVsync(displayId, enable);
@@ -5754,14 +5753,11 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
57545753
}
57555754

57565755
if (currentModeNotDozeSuspend) {
5757-
if (!FlagManager::getInstance().multithreaded_present()) {
5758-
mScheduler->disableHardwareVsync(displayId, true);
5759-
}
57605756
mScheduler->enableSyntheticVsync();
57615757
}
57625758
}
57635759
}
5764-
if (currentModeNotDozeSuspend && FlagManager::getInstance().multithreaded_present()) {
5760+
if (currentModeNotDozeSuspend) {
57655761
constexpr bool kDisallow = true;
57665762
mScheduler->disableHardwareVsync(displayId, kDisallow);
57675763
}
@@ -5779,8 +5775,7 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
57795775
} else if (mode == hal::PowerMode::DOZE || mode == hal::PowerMode::ON) {
57805776
// Update display while dozing
57815777
getHwComposer().setPowerMode(displayId, mode);
5782-
if (currentMode == hal::PowerMode::DOZE_SUSPEND &&
5783-
(displayId == mActiveDisplayId || FlagManager::getInstance().multithreaded_present())) {
5778+
if (currentMode == hal::PowerMode::DOZE_SUSPEND) {
57845779
if (displayId == mActiveDisplayId) {
57855780
ALOGI("Force repainting for DOZE_SUSPEND -> DOZE or ON.");
57865781
mVisibleRegionsDirty = true;
@@ -5792,10 +5787,9 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
57925787
}
57935788
} else if (mode == hal::PowerMode::DOZE_SUSPEND) {
57945789
// Leave display going to doze
5795-
if (displayId == mActiveDisplayId || FlagManager::getInstance().multithreaded_present()) {
5796-
constexpr bool kDisallow = true;
5797-
mScheduler->disableHardwareVsync(displayId, kDisallow);
5798-
}
5790+
constexpr bool kDisallow = true;
5791+
mScheduler->disableHardwareVsync(displayId, kDisallow);
5792+
57995793
if (displayId == mActiveDisplayId) {
58005794
mScheduler->enableSyntheticVsync();
58015795
}

‎services/surfaceflinger/common/FlagManager.cpp‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,6 @@ void FlagManager::dump(std::string& result) const {
164164
DUMP_ACONFIG_FLAG(latch_unsignaled_with_auto_refresh_changed);
165165
DUMP_ACONFIG_FLAG(local_tonemap_screenshots);
166166
DUMP_ACONFIG_FLAG(misc1);
167-
DUMP_ACONFIG_FLAG(multithreaded_present);
168167
DUMP_ACONFIG_FLAG(no_vsyncs_on_screen_off);
169168
DUMP_ACONFIG_FLAG(override_trusted_overlay);
170169
DUMP_ACONFIG_FLAG(protected_if_client);
@@ -259,7 +258,6 @@ FLAG_MANAGER_ACONFIG_FLAG(frame_rate_category_mrr, "debug.sf.frame_rate_category
259258
FLAG_MANAGER_ACONFIG_FLAG(misc1, "")
260259
FLAG_MANAGER_ACONFIG_FLAG(vrr_config, "debug.sf.enable_vrr_config")
261260
FLAG_MANAGER_ACONFIG_FLAG(hdcp_level_hal, "")
262-
FLAG_MANAGER_ACONFIG_FLAG(multithreaded_present, "debug.sf.multithreaded_present")
263261
FLAG_MANAGER_ACONFIG_FLAG(add_sf_skipped_frames_to_trace, "")
264262
FLAG_MANAGER_ACONFIG_FLAG(use_known_refresh_rate_for_fps_consistency, "")
265263
FLAG_MANAGER_ACONFIG_FLAG(cache_when_source_crop_layer_only_moved,

‎services/surfaceflinger/common/include/common/FlagManager.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,6 @@ class FlagManager {
9999
bool local_tonemap_screenshots() const;
100100
bool luts_api() const;
101101
bool misc1() const;
102-
bool multithreaded_present() const;
103102
bool no_vsyncs_on_screen_off() const;
104103
bool override_trusted_overlay() const;
105104
bool protected_if_client() const;

‎services/surfaceflinger/tests/unittests/FlagManagerTest.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,13 +125,13 @@ TEST_F(FlagManagerTest, DISABLED_returnsOverrideFalse) {
125125
TEST_F(FlagManagerTest, ignoresOverrideInUnitTestMode) {
126126
mFlagManager.setUnitTestMode();
127127

128-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
128+
SET_FLAG_FOR_TEST(flags::no_vsyncs_on_screen_off, true);
129129

130130
// If this has not been called in this process, it will be called.
131131
// Regardless, the result is ignored.
132132
EXPECT_CALL(mFlagManager, getBoolProperty).WillRepeatedly(Return(false));
133133

134-
EXPECT_EQ(true, mFlagManager.multithreaded_present());
134+
EXPECT_EQ(true, mFlagManager.no_vsyncs_on_screen_off());
135135
}
136136

137137
TEST_F(FlagManagerTest, returnsValue) {

‎services/surfaceflinger/tests/unittests/SchedulerTest.cpp‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -741,8 +741,6 @@ TEST_F(SchedulerTest, resyncAllDoNotAllow) FTL_FAKE_GUARD(kMainThreadContext) {
741741
}
742742

743743
TEST_F(SchedulerTest, resyncAllSkipsOffDisplays) FTL_FAKE_GUARD(kMainThreadContext) {
744-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
745-
746744
// resyncAllToHardwareVsync will result in requesting hardware VSYNC on display 1, which is on,
747745
// but not on display 2, which is off.
748746
EXPECT_CALL(mScheduler->mockRequestHardwareVsync, Call(kDisplayId1, true)).Times(1);
@@ -763,28 +761,6 @@ TEST_F(SchedulerTest, resyncAllSkipsOffDisplays) FTL_FAKE_GUARD(kMainThreadConte
763761
mScheduler->resyncAllToHardwareVsync(kAllowToEnable);
764762
}
765763

766-
TEST_F(SchedulerTest, resyncAllLegacyAppliesToOffDisplays) FTL_FAKE_GUARD(kMainThreadContext) {
767-
SET_FLAG_FOR_TEST(flags::multithreaded_present, false);
768-
769-
// In the legacy code, prior to the flag, resync applied to OFF displays.
770-
EXPECT_CALL(mScheduler->mockRequestHardwareVsync, Call(kDisplayId1, true)).Times(1);
771-
EXPECT_CALL(mScheduler->mockRequestHardwareVsync, Call(kDisplayId2, true)).Times(1);
772-
773-
mScheduler->setDisplayPowerMode(kDisplayId1, hal::PowerMode::ON);
774-
775-
mScheduler->registerDisplay(kDisplayId2,
776-
std::make_shared<RefreshRateSelector>(kDisplay2Modes,
777-
kDisplay2Mode60->getId()));
778-
ASSERT_EQ(hal::PowerMode::OFF, mScheduler->getDisplayPowerMode(kDisplayId2));
779-
780-
static constexpr bool kDisallow = true;
781-
mScheduler->disableHardwareVsync(kDisplayId1, kDisallow);
782-
mScheduler->disableHardwareVsync(kDisplayId2, kDisallow);
783-
784-
static constexpr bool kAllowToEnable = true;
785-
mScheduler->resyncAllToHardwareVsync(kAllowToEnable);
786-
}
787-
788764
class AttachedChoreographerTest : public SchedulerTest {
789765
protected:
790766
void frameRateTestScenario(Fps layerFps, int8_t frameRateCompatibility, Fps displayFps,

‎services/surfaceflinger/tests/unittests/SurfaceFlinger_FoldableTest.cpp‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,6 @@ TEST_F(FoldableTest, requestsHardwareVsyncForBothDisplays) {
167167
}
168168

169169
TEST_F(FoldableTest, requestVsyncOnPowerOn) {
170-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
171170
EXPECT_CALL(mFlinger.scheduler()->mockRequestHardwareVsync, Call(kInnerDisplayId, true))
172171
.Times(1);
173172
EXPECT_CALL(mFlinger.scheduler()->mockRequestHardwareVsync, Call(kOuterDisplayId, true))
@@ -178,7 +177,6 @@ TEST_F(FoldableTest, requestVsyncOnPowerOn) {
178177
}
179178

180179
TEST_F(FoldableTest, disableVsyncOnPowerOffPacesetter) {
181-
SET_FLAG_FOR_TEST(flags::multithreaded_present, true);
182180
// When the device boots, the inner display should be the pacesetter.
183181
ASSERT_EQ(mFlinger.scheduler()->pacesetterDisplayId(), kInnerDisplayId);
184182

0 commit comments

Comments
 (0)