SF: avoid a composition cycle when the FrameRate votes updates
SF would still do a composition if the display mode changes, but
it is not required for every frame rate vote change.
Bug: 339759346
Test: android.platform.test.scenario.gmail.OpenCloseComposeEmailMicrobenchmark#testOpenCloseComposeEmail
Change-Id: Ia01a13b1a167b3a0a67cf7be3db5e64b9405580a
diff --git a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
index 4b0618e..52f8bea 100644
--- a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
+++ b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
@@ -41,7 +41,8 @@
return;
}
- mGlobalChanges |= RequestedLayerState::Changes::Hierarchy;
+ mGlobalChanges |= RequestedLayerState::Changes::Hierarchy |
+ RequestedLayerState::Changes::RequiresComposition;
for (auto& newLayer : newLayers) {
RequestedLayerState& layer = *newLayer.get();
auto [it, inserted] = mIdToLayer.try_emplace(layer.id, References{.owner = layer});
@@ -104,7 +105,8 @@
if (!layer.canBeDestroyed()) {
continue;
}
- layer.changes |= RequestedLayerState::Changes::Destroyed;
+ layer.changes |= RequestedLayerState::Changes::Destroyed |
+ RequestedLayerState::Changes::RequiresComposition;
layersToBeDestroyed.emplace_back(layerId);
}
@@ -112,7 +114,8 @@
return;
}
- mGlobalChanges |= RequestedLayerState::Changes::Hierarchy;
+ mGlobalChanges |= RequestedLayerState::Changes::Hierarchy |
+ RequestedLayerState::Changes::RequiresComposition;
for (size_t i = 0; i < layersToBeDestroyed.size(); i++) {
uint32_t layerId = layersToBeDestroyed[i];
auto it = mIdToLayer.find(layerId);
@@ -142,7 +145,8 @@
if (linkedLayer->parentId == layer.id) {
linkedLayer->parentId = UNASSIGNED_LAYER_ID;
if (linkedLayer->canBeDestroyed()) {
- linkedLayer->changes |= RequestedLayerState::Changes::Destroyed;
+ linkedLayer->changes |= RequestedLayerState::Changes::Destroyed |
+ RequestedLayerState::Changes::RequiresComposition;
layersToBeDestroyed.emplace_back(linkedLayer->id);
}
}
@@ -249,7 +253,8 @@
layer_state_t::eDataspaceChanged | layer_state_t::eAlphaChanged;
bgColorLayer->changes |= RequestedLayerState::Changes::Content;
mChangedLayers.push_back(bgColorLayer);
- mGlobalChanges |= RequestedLayerState::Changes::Content;
+ mGlobalChanges |= RequestedLayerState::Changes::Content |
+ RequestedLayerState::Changes::RequiresComposition;
}
}
@@ -407,7 +412,8 @@
layer.relativeParentId = unlinkLayer(layer.relativeParentId, layer.id);
layer.changes |=
RequestedLayerState::Changes::Hierarchy | RequestedLayerState::Changes::RelativeParent;
- mGlobalChanges |= RequestedLayerState::Changes::Hierarchy;
+ mGlobalChanges |= RequestedLayerState::Changes::Hierarchy |
+ RequestedLayerState::Changes::RequiresComposition;
}
// Some layers mirror the entire display stack. Since we don't have a single root layer per display
diff --git a/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp b/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp
index 5631fac..f5e5b02 100644
--- a/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp
+++ b/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp
@@ -58,7 +58,8 @@
parentId(args.parentId),
layerIdToMirror(args.layerIdToMirror) {
layerId = static_cast<int32_t>(args.sequence);
- changes |= RequestedLayerState::Changes::Created;
+ changes |= RequestedLayerState::Changes::Created |
+ RequestedLayerState::Changes::RequiresComposition;
metadata.merge(args.metadata);
changes |= RequestedLayerState::Changes::Metadata;
handleAlive = true;
@@ -248,7 +249,8 @@
if (hadSomethingToDraw != hasSomethingToDraw()) {
changes |= RequestedLayerState::Changes::Visibility |
- RequestedLayerState::Changes::VisibleRegion;
+ RequestedLayerState::Changes::VisibleRegion |
+ RequestedLayerState::Changes::RequiresComposition;
}
if (clientChanges & layer_state_t::HIERARCHY_CHANGES)
changes |= RequestedLayerState::Changes::Hierarchy;
@@ -258,6 +260,8 @@
changes |= RequestedLayerState::Changes::Geometry;
if (clientChanges & layer_state_t::AFFECTS_CHILDREN)
changes |= RequestedLayerState::Changes::AffectsChildren;
+ if (clientChanges & layer_state_t::REQUIRES_COMPOSITION)
+ changes |= RequestedLayerState::Changes::RequiresComposition;
if (clientChanges & layer_state_t::INPUT_CHANGES)
changes |= RequestedLayerState::Changes::Input;
if (clientChanges & layer_state_t::VISIBLE_REGION_CHANGES)
diff --git a/services/surfaceflinger/FrontEnd/RequestedLayerState.h b/services/surfaceflinger/FrontEnd/RequestedLayerState.h
index 09f33de..4829d4c 100644
--- a/services/surfaceflinger/FrontEnd/RequestedLayerState.h
+++ b/services/surfaceflinger/FrontEnd/RequestedLayerState.h
@@ -57,6 +57,7 @@
BufferSize = 1u << 18,
GameMode = 1u << 19,
BufferUsageFlags = 1u << 20,
+ RequiresComposition = 1u << 21,
};
static Rect reduce(const Rect& win, const Region& exclude);
RequestedLayerState(const LayerCreationArgs&);
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 66385d8..f13b4ce 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1411,11 +1411,12 @@
}
}
-void SurfaceFlinger::initiateDisplayModeChanges() {
+bool SurfaceFlinger::initiateDisplayModeChanges() {
ATRACE_CALL();
std::optional<PhysicalDisplayId> displayToUpdateImmediately;
+ bool mustComposite = false;
for (const auto& [id, physical] : mPhysicalDisplays) {
const auto display = getDisplayDeviceLocked(id);
if (!display) continue;
@@ -1472,7 +1473,11 @@
mScheduler->onNewVsyncPeriodChangeTimeline(outTimeline);
if (outTimeline.refreshRequired) {
- scheduleComposite(FrameHint::kNone);
+ if (FlagManager::getInstance().vrr_bugfix_24q4()) {
+ mustComposite = true;
+ } else {
+ scheduleComposite(FrameHint::kNone);
+ }
} else {
// TODO(b/255635711): Remove `displayToUpdateImmediately` to `finalizeDisplayModeChange`
// for all displays. This was only needed when the loop iterated over `mDisplays` rather
@@ -1490,6 +1495,8 @@
applyActiveMode(display);
}
}
+
+ return mustComposite;
}
void SurfaceFlinger::disableExpensiveRendering() {
@@ -2439,7 +2446,12 @@
mUpdateAttachedChoreographer = true;
}
outTransactionsAreEmpty = mLayerLifecycleManager.getGlobalChanges().get() == 0;
- mustComposite |= mLayerLifecycleManager.getGlobalChanges().get() != 0;
+ if (FlagManager::getInstance().vrr_bugfix_24q4()) {
+ mustComposite |= mLayerLifecycleManager.getGlobalChanges().test(
+ frontend::RequestedLayerState::Changes::RequiresComposition);
+ } else {
+ mustComposite |= mLayerLifecycleManager.getGlobalChanges().get() != 0;
+ }
bool newDataLatched = false;
ATRACE_NAME("DisplayCallbackAndStatsUpdates");
@@ -2664,7 +2676,7 @@
? &mLayerHierarchyBuilder.getHierarchy()
: nullptr,
updateAttachedChoreographer);
- initiateDisplayModeChanges();
+ mustComposite |= initiateDisplayModeChanges();
}
updateCursorAsync();
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index edcc8d3..a198768 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -736,7 +736,7 @@
status_t setActiveModeFromBackdoor(const sp<display::DisplayToken>&, DisplayModeId, Fps minFps,
Fps maxFps);
- void initiateDisplayModeChanges() REQUIRES(mStateLock, kMainThreadContext);
+ bool initiateDisplayModeChanges() REQUIRES(mStateLock, kMainThreadContext);
void finalizeDisplayModeChange(DisplayDevice&) REQUIRES(mStateLock, kMainThreadContext);
// TODO(b/241285191): Replace DisplayDevice with DisplayModeRequest, and move to Scheduler.
diff --git a/services/surfaceflinger/common/FlagManager.cpp b/services/surfaceflinger/common/FlagManager.cpp
index 0feacac..121629f 100644
--- a/services/surfaceflinger/common/FlagManager.cpp
+++ b/services/surfaceflinger/common/FlagManager.cpp
@@ -134,6 +134,7 @@
DUMP_READ_ONLY_FLAG(screenshot_fence_preservation);
DUMP_READ_ONLY_FLAG(vulkan_renderengine);
DUMP_READ_ONLY_FLAG(renderable_buffer_usage);
+ DUMP_READ_ONLY_FLAG(vrr_bugfix_24q4);
DUMP_READ_ONLY_FLAG(restore_blur_step);
DUMP_READ_ONLY_FLAG(dont_skip_on_early_ro);
DUMP_READ_ONLY_FLAG(protected_if_client);
@@ -234,6 +235,7 @@
FLAG_MANAGER_READ_ONLY_FLAG(restore_blur_step, "debug.renderengine.restore_blur_step")
FLAG_MANAGER_READ_ONLY_FLAG(dont_skip_on_early_ro, "")
FLAG_MANAGER_READ_ONLY_FLAG(protected_if_client, "")
+FLAG_MANAGER_READ_ONLY_FLAG(vrr_bugfix_24q4, "");
FLAG_MANAGER_READ_ONLY_FLAG(ce_fence_promise, "");
FLAG_MANAGER_READ_ONLY_FLAG(graphite_renderengine, "debug.renderengine.graphite")
FLAG_MANAGER_READ_ONLY_FLAG(latch_unsignaled_with_auto_refresh_changed, "");
diff --git a/services/surfaceflinger/common/include/common/FlagManager.h b/services/surfaceflinger/common/include/common/FlagManager.h
index 5850fe1..4cf4453 100644
--- a/services/surfaceflinger/common/include/common/FlagManager.h
+++ b/services/surfaceflinger/common/include/common/FlagManager.h
@@ -72,6 +72,7 @@
bool enable_layer_command_batching() const;
bool screenshot_fence_preservation() const;
bool vulkan_renderengine() const;
+ bool vrr_bugfix_24q4() const;
bool renderable_buffer_usage() const;
bool restore_blur_step() const;
bool dont_skip_on_early_ro() const;
diff --git a/services/surfaceflinger/surfaceflinger_flags_new.aconfig b/services/surfaceflinger/surfaceflinger_flags_new.aconfig
index 04f1d50..5b94f07 100644
--- a/services/surfaceflinger/surfaceflinger_flags_new.aconfig
+++ b/services/surfaceflinger/surfaceflinger_flags_new.aconfig
@@ -84,4 +84,15 @@
is_fixed_read_only: true
} # local_tonemap_screenshots
+flag {
+ name: "vrr_bugfix_24q4"
+ namespace: "core_graphics"
+ description: "bug fixes for VRR"
+ bug: "331513837"
+ is_fixed_read_only: true
+ metadata {
+ purpose: PURPOSE_BUGFIX
+ }
+} # vrr_bugfix_24q4
+
# IMPORTANT - please keep alphabetize to reduce merge conflicts
diff --git a/services/surfaceflinger/tests/unittests/LayerLifecycleManagerTest.cpp b/services/surfaceflinger/tests/unittests/LayerLifecycleManagerTest.cpp
index 867ff55..158db75 100644
--- a/services/surfaceflinger/tests/unittests/LayerLifecycleManagerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/LayerLifecycleManagerTest.cpp
@@ -461,8 +461,9 @@
HAL_PIXEL_FORMAT_RGBA_8888,
GRALLOC_USAGE_PROTECTED /*usage*/));
EXPECT_EQ(mLifecycleManager.getGlobalChanges().get(),
- ftl::Flags<RequestedLayerState::Changes>(RequestedLayerState::Changes::Buffer |
- RequestedLayerState::Changes::Content)
+ ftl::Flags<RequestedLayerState::Changes>(
+ RequestedLayerState::Changes::Buffer | RequestedLayerState::Changes::Content |
+ RequestedLayerState::Changes::RequiresComposition)
.get());
mLifecycleManager.commitChanges();
@@ -493,10 +494,11 @@
HAL_PIXEL_FORMAT_RGB_888,
GRALLOC_USAGE_PROTECTED /*usage*/));
EXPECT_EQ(mLifecycleManager.getGlobalChanges().get(),
- ftl::Flags<RequestedLayerState::Changes>(RequestedLayerState::Changes::Buffer |
- RequestedLayerState::Changes::Content |
- RequestedLayerState::Changes::VisibleRegion |
- RequestedLayerState::Changes::Visibility)
+ ftl::Flags<RequestedLayerState::Changes>(
+ RequestedLayerState::Changes::Buffer | RequestedLayerState::Changes::Content |
+ RequestedLayerState::Changes::VisibleRegion |
+ RequestedLayerState::Changes::Visibility |
+ RequestedLayerState::Changes::RequiresComposition)
.get());
mLifecycleManager.commitChanges();
}
@@ -520,7 +522,8 @@
RequestedLayerState::Changes::AffectsChildren |
RequestedLayerState::Changes::Content |
RequestedLayerState::Changes::Geometry |
- RequestedLayerState::Changes::VisibleRegion)
+ RequestedLayerState::Changes::VisibleRegion |
+ RequestedLayerState::Changes::RequiresComposition)
.get());
mLifecycleManager.commitChanges();
}
@@ -538,7 +541,8 @@
ftl::Flags<RequestedLayerState::Changes>(
RequestedLayerState::Changes::Content |
RequestedLayerState::Changes::AffectsChildren |
- RequestedLayerState::Changes::VisibleRegion)
+ RequestedLayerState::Changes::VisibleRegion |
+ RequestedLayerState::Changes::RequiresComposition)
.string());
EXPECT_EQ(mLifecycleManager.getChangedLayers()[0]->color.a, static_cast<half>(startingAlpha));
mLifecycleManager.commitChanges();
@@ -551,7 +555,8 @@
ftl::Flags<RequestedLayerState::Changes>(
RequestedLayerState::Changes::Content |
RequestedLayerState::Changes::AffectsChildren |
- RequestedLayerState::Changes::VisibleRegion)
+ RequestedLayerState::Changes::VisibleRegion |
+ RequestedLayerState::Changes::RequiresComposition)
.string());
EXPECT_EQ(mLifecycleManager.getChangedLayers()[0]->color.a, static_cast<half>(endingAlpha));
mLifecycleManager.commitChanges();
@@ -580,8 +585,9 @@
HAL_PIXEL_FORMAT_RGBA_8888,
GRALLOC_USAGE_SW_READ_NEVER /*usage*/));
EXPECT_EQ(mLifecycleManager.getGlobalChanges().get(),
- ftl::Flags<RequestedLayerState::Changes>(RequestedLayerState::Changes::Buffer |
- RequestedLayerState::Changes::Content)
+ ftl::Flags<RequestedLayerState::Changes>(
+ RequestedLayerState::Changes::Buffer | RequestedLayerState::Changes::Content |
+ RequestedLayerState::Changes::RequiresComposition)
.get());
mLifecycleManager.commitChanges();
diff --git a/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp b/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp
index 5ff6417..9dd38d6 100644
--- a/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp
+++ b/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp
@@ -254,7 +254,9 @@
TEST_F(LayerSnapshotTest, FastPathClearsPreviousChangeStates) {
setColor(11, {1._hf, 0._hf, 0._hf});
UPDATE_AND_VERIFY(mSnapshotBuilder, STARTING_ZORDER);
- EXPECT_EQ(getSnapshot(11)->changes, RequestedLayerState::Changes::Content);
+ EXPECT_EQ(getSnapshot(11)->changes,
+ RequestedLayerState::Changes::Content |
+ RequestedLayerState::Changes::RequiresComposition);
EXPECT_EQ(getSnapshot(11)->clientChanges, layer_state_t::eColorChanged);
EXPECT_EQ(getSnapshot(1)->changes.get(), 0u);
UPDATE_AND_VERIFY(mSnapshotBuilder, STARTING_ZORDER);
@@ -264,7 +266,9 @@
TEST_F(LayerSnapshotTest, FastPathSetsChangeFlagToContent) {
setColor(1, {1._hf, 0._hf, 0._hf});
UPDATE_AND_VERIFY(mSnapshotBuilder, STARTING_ZORDER);
- EXPECT_EQ(getSnapshot(1)->changes, RequestedLayerState::Changes::Content);
+ EXPECT_EQ(getSnapshot(1)->changes,
+ RequestedLayerState::Changes::Content |
+ RequestedLayerState::Changes::RequiresComposition);
EXPECT_EQ(getSnapshot(1)->clientChanges, layer_state_t::eColorChanged);
}