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

11 files changed, 76 insertions, 26 deletions

diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h
index ca7acf9be4..ebdf2326d6 100644
--- a/libs/gui/include/gui/LayerState.h
+++ b/libs/gui/include/gui/LayerState.h
@@ -279,6 +279,9 @@ struct layer_state_t {
             layer_state_t::eDropInputModeChanged | layer_state_t::eTrustedOverlayChanged |
             layer_state_t::eLayerStackChanged;
 
+    // Changes requiring a composition pass.
+    static constexpr uint64_t REQUIRES_COMPOSITION = layer_state_t::CONTENT_DIRTY;
+
     // Changes that affect the visible region on a display.
     static constexpr uint64_t VISIBLE_REGION_CHANGES = layer_state_t::GEOMETRY_CHANGES |
             layer_state_t::HIERARCHY_CHANGES | layer_state_t::eAlphaChanged;
diff --git a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
index 4b0618e5aa..52f8bea4ba 100644
--- a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
+++ b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
@@ -41,7 +41,8 @@ void LayerLifecycleManager::addLayers(std::vector<std::unique_ptr<RequestedLayer
         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 @@ void LayerLifecycleManager::onHandlesDestroyed(
         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 @@ void LayerLifecycleManager::onHandlesDestroyed(
         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 @@ void LayerLifecycleManager::onHandlesDestroyed(
             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 @@ void LayerLifecycleManager::applyTransactions(const std::vector<TransactionState
                             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 @@ void LayerLifecycleManager::fixRelativeZLoop(uint32_t relativeRootId) {
     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 5631facdae..f5e5b0233e 100644
--- a/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp
+++ b/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp
@@ -58,7 +58,8 @@ RequestedLayerState::RequestedLayerState(const LayerCreationArgs& args)
         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 @@ void RequestedLayerState::merge(const ResolvedComposerState& resolvedComposerSta
 
     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 @@ void RequestedLayerState::merge(const ResolvedComposerState& resolvedComposerSta
         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 09f33de63b..4829d4c1e9 100644
--- a/services/surfaceflinger/FrontEnd/RequestedLayerState.h
+++ b/services/surfaceflinger/FrontEnd/RequestedLayerState.h
@@ -57,6 +57,7 @@ struct RequestedLayerState : layer_state_t {
         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 66385d8db5..f13b4ce9e8 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1411,11 +1411,12 @@ void SurfaceFlinger::applyActiveMode(const sp<DisplayDevice>& display) {
     }
 }
 
-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 @@ void SurfaceFlinger::initiateDisplayModeChanges() {
         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 @@ void SurfaceFlinger::initiateDisplayModeChanges() {
             applyActiveMode(display);
         }
     }
+
+    return mustComposite;
 }
 
 void SurfaceFlinger::disableExpensiveRendering() {
@@ -2439,7 +2446,12 @@ bool SurfaceFlinger::updateLayerSnapshots(VsyncId vsyncId, nsecs_t frameTimeNs,
         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 @@ bool SurfaceFlinger::commit(PhysicalDisplayId pacesetterId,
                                                         ? &mLayerHierarchyBuilder.getHierarchy()
                                                         : nullptr,
                                                 updateAttachedChoreographer);
-        initiateDisplayModeChanges();
+        mustComposite |= initiateDisplayModeChanges();
     }
 
     updateCursorAsync();
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index edcc8d3206..a1987682c2 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -736,7 +736,7 @@ private:
     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 0feacacd80..121629fd10 100644
--- a/services/surfaceflinger/common/FlagManager.cpp
+++ b/services/surfaceflinger/common/FlagManager.cpp
@@ -134,6 +134,7 @@ void FlagManager::dump(std::string& result) const {
     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(renderable_buffer_usage, "")
 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 5850fe1b02..4cf4453980 100644
--- a/services/surfaceflinger/common/include/common/FlagManager.h
+++ b/services/surfaceflinger/common/include/common/FlagManager.h
@@ -72,6 +72,7 @@ public:
     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 04f1d5080a..5b94f07dff 100644
--- a/services/surfaceflinger/surfaceflinger_flags_new.aconfig
+++ b/services/surfaceflinger/surfaceflinger_flags_new.aconfig
@@ -84,4 +84,15 @@ flag {
     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 867ff55632..158db752f4 100644
--- a/services/surfaceflinger/tests/unittests/LayerLifecycleManagerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/LayerLifecycleManagerTest.cpp
@@ -461,8 +461,9 @@ TEST_F(LayerLifecycleManagerTest, layerOpacityChangesSetsVisibilityChangeFlag) {
                                                                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 @@ TEST_F(LayerLifecycleManagerTest, bufferFormatChangesSetsVisibilityChangeFlag) {
                                                                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 @@ TEST_F(LayerLifecycleManagerTest, roundedCornerChangesSetsVisibilityChangeFlag)
                       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 @@ TEST_F(LayerLifecycleManagerTest, alphaChangesAlwaysSetsVisibleRegionFlag) {
               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 @@ TEST_F(LayerLifecycleManagerTest, alphaChangesAlwaysSetsVisibleRegionFlag) {
               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 @@ TEST_F(LayerLifecycleManagerTest, layerSecureChangesSetsVisibilityChangeFlag) {
                                                             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 5ff6417482..9dd38d60e7 100644
--- a/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp
+++ b/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp
@@ -254,7 +254,9 @@ TEST_F(LayerSnapshotTest, UpdateClearsPreviousChangeStates) {
 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, FastPathClearsPreviousChangeStates) {
 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);
 }