Turn off synthetic VSYNC when adjusting thread scheduling for performance

* This CL explicitly sets synthetic VSYNC state based on the new power state. Before, the synthetic VSYNC state was assumed based on the previous power state and in some cases was not set explicitly.
* The previous behaviour of enabling synthetic VSYNC if the only display is DOZE_SUSPEND is preserved.
* This also fixes a hypothetical gap where it would not optimise for performance if there were a display that needed it, while the primary display was DOZE_SUSPEND.

setPhysicalDisplayPowerMode now applies optimiation policy for any
internal display power mode change (external displays are out of scope
in this CL). This is necessary for foldables since the active display
has not always been updated at that point.

(This includes undoing commit 92bf32a,
fixing the issue it caused, and adding an additional flag)

Bug: 406147398

Bug: 342681202

Bug: 404073995

Flag: android.companion.virtualdevice.flags.correct_virtual_display_power_state

Flag: com.android.graphics.surfaceflinger.flags.disable_synthetic_vsync_for_performance

Test: ran failing tests (https://android-build.corp.google.com/builds/abtd/run/L41000030010571741) and SurfaceFlinger unit tests. Also verified logs include the call to applyOptimizationPolicy and SurfaceFlinger dumpsys not showing display in synthetic VSYNC when powered on.

Change-Id: I4ffcb42379fa97546ad307f8077455495382e878

Author: Dennis Kiilerich

services/surfaceflinger/DisplayDevice.cpp

@@ -50,17 +50,6 @@ namespace android {
 
 namespace hal = hardware::graphics::composer::hal;
 
-namespace gui {
-inline std::string_view to_string(ISurfaceComposer::OptimizationPolicy optimizationPolicy) {
-    switch (optimizationPolicy) {
-        case ISurfaceComposer::OptimizationPolicy::optimizeForPower:
-            return "optimizeForPower";
-        case ISurfaceComposer::OptimizationPolicy::optimizeForPerformance:
-            return "optimizeForPerformance";
-    }
-}
-} // namespace gui
-
 DisplayDeviceCreationArgs::DisplayDeviceCreationArgs(
         const sp<SurfaceFlinger>& flinger, HWComposer& hwComposer, const wp<IBinder>& displayToken,
         std::shared_ptr<compositionengine::Display> compositionDisplay)

services/surfaceflinger/DisplayDevice.h

@@ -67,6 +67,17 @@ namespace display {
 class DisplaySnapshot;
 } // namespace display
 
+namespace gui {
+inline const char* to_string(ISurfaceComposer::OptimizationPolicy optimizationPolicy) {
+    switch (optimizationPolicy) {
+        case ISurfaceComposer::OptimizationPolicy::optimizeForPower:
+            return "optimizeForPower";
+        case ISurfaceComposer::OptimizationPolicy::optimizeForPerformance:
+            return "optimizeForPerformance";
+    }
+}
+} // namespace gui
+
 class DisplayDevice : public RefBase {
 public:
     constexpr static float sDefaultMinLumiance = 0.0;

services/surfaceflinger/SurfaceFlinger.cpp

@@ -5678,7 +5678,15 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
         incRefreshableDisplays();
     }
 
+    const bool shouldApplyOptimizationPolicy =
+            FlagManager::getInstance().disable_synthetic_vsync_for_performance() &&
+            FlagManager::getInstance().correct_virtual_display_power_state();
+    if (isInternalDisplay && shouldApplyOptimizationPolicy) {
+        applyOptimizationPolicy(__func__);
+    }
+
     const auto activeMode = display->refreshRateSelector().getActiveMode().modePtr;
+    using OptimizationPolicy = gui::ISurfaceComposer::OptimizationPolicy;
     if (currentMode == hal::PowerMode::OFF) {
         // Turn on the display
 
@@ -5693,12 +5701,9 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
             onActiveDisplayChangedLocked(activeDisplay.get(), *display);
         }
 
-        if (displayId == mActiveDisplayId) {
-            if (FlagManager::getInstance().correct_virtual_display_power_state()) {
-                applyOptimizationPolicy("setPhysicalDisplayPowerMode(ON)");
-            } else {
-                disablePowerOptimizations("setPhysicalDisplayPowerMode(ON)");
-            }
+        if (displayId == mActiveDisplayId && !shouldApplyOptimizationPolicy) {
+            optimizeThreadScheduling("setPhysicalDisplayPowerMode(ON/DOZE)",
+                                     OptimizationPolicy::optimizeForPerformance);
         }
 
         getHwComposer().setPowerMode(displayId, mode);
@@ -5707,7 +5712,7 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
                     mScheduler->getVsyncSchedule(displayId)->getPendingHardwareVsyncState();
             requestHardwareVsync(displayId, enable);
 
-            if (displayId == mActiveDisplayId) {
+            if (displayId == mActiveDisplayId && !shouldApplyOptimizationPolicy) {
                 mScheduler->enableSyntheticVsync(false);
             }
 
@@ -5724,13 +5729,12 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
             if (const auto display = getActivatableDisplay()) {
                 onActiveDisplayChangedLocked(activeDisplay.get(), *display);
             } else {
-                if (FlagManager::getInstance().correct_virtual_display_power_state()) {
-                    applyOptimizationPolicy("setPhysicalDisplayPowerMode(OFF)");
-                } else {
-                    enablePowerOptimizations("setPhysicalDisplayPowerMode(OFF)");
+                if (!shouldApplyOptimizationPolicy) {
+                    optimizeThreadScheduling("setPhysicalDisplayPowerMode(OFF)",
+                                             OptimizationPolicy::optimizeForPower);
                 }
 
-                if (currentModeNotDozeSuspend) {
+                if (currentModeNotDozeSuspend && !shouldApplyOptimizationPolicy) {
                     mScheduler->enableSyntheticVsync();
                 }
             }
@@ -5758,7 +5762,9 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
                 ALOGI("Force repainting for DOZE_SUSPEND -> DOZE or ON.");
                 mVisibleRegionsDirty = true;
                 scheduleRepaint();
-                mScheduler->enableSyntheticVsync(false);
+                if (!shouldApplyOptimizationPolicy) {
+                    mScheduler->enableSyntheticVsync(false);
+                }
             }
             constexpr bool kAllowToEnable = true;
             mScheduler->resyncToHardwareVsync(displayId, kAllowToEnable, activeMode.get());
@@ -5768,7 +5774,7 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa
         constexpr bool kDisallow = true;
         mScheduler->disableHardwareVsync(displayId, kDisallow);
 
-        if (displayId == mActiveDisplayId) {
+        if (displayId == mActiveDisplayId && !shouldApplyOptimizationPolicy) {
             mScheduler->enableSyntheticVsync();
         }
         getHwComposer().setPowerMode(displayId, mode);
@@ -5807,43 +5813,44 @@ void SurfaceFlinger::setVirtualDisplayPowerMode(const sp<DisplayDevice>& display
           to_string(displayId).c_str());
 }
 
-bool SurfaceFlinger::shouldOptimizeForPerformance() {
-    for (const auto& [_, display] : mDisplays) {
-        // Displays that are optimized for power are always powered on and should not influence
-        // whether there is an active display for the purpose of power optimization, etc. If these
-        // displays are being shown somewhere, a different (physical or virtual) display that is
-        // optimized for performance will be powered on in addition. Displays optimized for
-        // performance will change power mode, so if they are off then they are not active.
-        if (display->isPoweredOn() &&
-            display->getOptimizationPolicy() ==
-                    gui::ISurfaceComposer::OptimizationPolicy::optimizeForPerformance) {
-            return true;
-        }
-    }
-    return false;
-}
-
-void SurfaceFlinger::enablePowerOptimizations(const char* whence) {
-    ALOGD("%s: Enabling power optimizations", whence);
-
-    setSchedAttr(false, whence);
-    setSchedFifo(false, whence);
-}
-
-void SurfaceFlinger::disablePowerOptimizations(const char* whence) {
-    ALOGD("%s: Disabling power optimizations", whence);
+void SurfaceFlinger::optimizeThreadScheduling(
+        const char* whence, gui::ISurfaceComposer::OptimizationPolicy optimizationPolicy) {
+    ALOGD("%s: Optimizing thread scheduling: %s", whence, to_string(optimizationPolicy));
 
+    const bool optimizeForPerformance =
+            optimizationPolicy == gui::ISurfaceComposer::OptimizationPolicy::optimizeForPerformance;
     // TODO: b/281692563 - Merge the syscalls. For now, keep uclamp in a separate syscall
     // and set it before SCHED_FIFO due to b/190237315.
-    setSchedAttr(true, whence);
-    setSchedFifo(true, whence);
+    setSchedAttr(optimizeForPerformance, whence);
+    setSchedFifo(optimizeForPerformance, whence);
 }
 
 void SurfaceFlinger::applyOptimizationPolicy(const char* whence) {
-    if (shouldOptimizeForPerformance()) {
-        disablePowerOptimizations(whence);
-    } else {
-        enablePowerOptimizations(whence);
+    using OptimizationPolicy = gui::ISurfaceComposer::OptimizationPolicy;
+
+    const bool optimizeForPerformance =
+            std::any_of(mDisplays.begin(), mDisplays.end(), [](const auto& pair) {
+                const auto& display = pair.second;
+                return display->isPoweredOn() &&
+                        display->getOptimizationPolicy() ==
+                        OptimizationPolicy::optimizeForPerformance;
+            });
+
+    optimizeThreadScheduling(whence,
+                             optimizeForPerformance ? OptimizationPolicy::optimizeForPerformance
+                                                    : OptimizationPolicy::optimizeForPower);
+
+    if (mScheduler) {
+        const bool disableSyntheticVsync =
+                std::any_of(mDisplays.begin(), mDisplays.end(), [](const auto& pair) {
+                    const auto& display = pair.second;
+                    const hal::PowerMode powerMode = display->getPowerMode();
+                    return powerMode != hal::PowerMode::OFF &&
+                            powerMode != hal::PowerMode::DOZE_SUSPEND &&
+                            display->getOptimizationPolicy() ==
+                            OptimizationPolicy::optimizeForPerformance;
+                });
+        mScheduler->enableSyntheticVsync(!disableSyntheticVsync);
     }
 }
 

services/surfaceflinger/SurfaceFlinger.h

@@ -733,19 +733,14 @@ class SurfaceFlinger : public BnSurfaceComposer,
     void setVirtualDisplayPowerMode(const sp<DisplayDevice>& display, hal::PowerMode mode)
             REQUIRES(mStateLock, kMainThreadContext);
 
-    // Returns whether to optimize globally for performance instead of power.
-    bool shouldOptimizeForPerformance() REQUIRES(mStateLock);
-
-    // Turns on power optimizations, for example when there are no displays to be optimized for
-    // performance.
-    static void enablePowerOptimizations(const char* whence);
-
-    // Turns off power optimizations.
-    static void disablePowerOptimizations(const char* whence);
+    // Adjusts thread scheduling according to the optimization policy
+    static void optimizeThreadScheduling(
+            const char* whence, gui::ISurfaceComposer::OptimizationPolicy optimizationPolicy);
 
     // Enables or disables power optimizations depending on whether there are displays that should
     // be optimized for performance.
-    void applyOptimizationPolicy(const char* whence) REQUIRES(mStateLock);
+    void applyOptimizationPolicy(const char* whence) REQUIRES(kMainThreadContext)
+            REQUIRES(mStateLock);
 
     // Returns the preferred mode for PhysicalDisplayId if the Scheduler has selected one for that
     // display. Falls back to the display's defaultModeId otherwise.

services/surfaceflinger/common/FlagManager.cpp

@@ -147,6 +147,7 @@ void FlagManager::dump(std::string& result) const {
     DUMP_ACONFIG_FLAG(deprecate_frame_tracker);
     DUMP_ACONFIG_FLAG(deprecate_vsync_sf);
     DUMP_ACONFIG_FLAG(detached_mirror);
+    DUMP_ACONFIG_FLAG(disable_synthetic_vsync_for_performance);
     DUMP_ACONFIG_FLAG(display_config_error_hal);
     DUMP_ACONFIG_FLAG(display_protected);
     DUMP_ACONFIG_FLAG(dont_skip_on_early_ro);
@@ -284,6 +285,7 @@ FLAG_MANAGER_ACONFIG_FLAG(latch_unsignaled_with_auto_refresh_changed, "");
 FLAG_MANAGER_ACONFIG_FLAG(deprecate_vsync_sf, "");
 FLAG_MANAGER_ACONFIG_FLAG(allow_n_vsyncs_in_targeter, "");
 FLAG_MANAGER_ACONFIG_FLAG(detached_mirror, "");
+FLAG_MANAGER_ACONFIG_FLAG(disable_synthetic_vsync_for_performance, "");
 FLAG_MANAGER_ACONFIG_FLAG(commit_not_composited, "");
 FLAG_MANAGER_ACONFIG_FLAG(correct_dpi_with_display_size, "");
 FLAG_MANAGER_ACONFIG_FLAG(local_tonemap_screenshots, "debug.sf.local_tonemap_screenshots");

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

@@ -81,6 +81,7 @@ class FlagManager {
     bool deprecate_frame_tracker() const;
     bool deprecate_vsync_sf() const;
     bool detached_mirror() const;
+    bool disable_synthetic_vsync_for_performance() const;
     bool display_config_error_hal() const;
     bool display_protected() const;
     bool dont_skip_on_early_ro() const;

services/surfaceflinger/surfaceflinger_flags_new.aconfig

@@ -133,6 +133,16 @@ flag {
   }
 } # detached_mirror
 
+flag {
+  name: "disable_synthetic_vsync_for_performance"
+  namespace: "core_graphics"
+  description: "Turn off synthetic VSYNC when adjusting thread scheduling for performance"
+  bug: "406147398"
+  metadata {
+    purpose: PURPOSE_BUGFIX
+  }
+} # disable_synthetic_vsync_for_performance
+
 flag {
   name: "display_config_error_hal"
   namespace: "core_graphics"

services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPhysicalDisplayPowerModeTest.cpp

@@ -17,6 +17,7 @@
 #undef LOG_TAG
 #define LOG_TAG "LibSurfaceFlingerUnittests"
 
+#include <android_companion_virtualdevice_flags.h>
 #include <com_android_graphics_surfaceflinger_flags.h>
 #include <common/test/FlagUtils.h>
 #include "DisplayTransactionTestHelpers.h"
@@ -78,11 +79,19 @@ struct EventThreadBaseSupportedVariant {
 struct EventThreadNotSupportedVariant : public EventThreadBaseSupportedVariant {
     static void setupEnableVsyncCallExpectations(DisplayTransactionTest* test) {
         EXPECT_CALL(test->mFlinger.scheduler()->mockRequestHardwareVsync, Call(_, true)).Times(1);
+        setupDisableSyntheticVsyncCallExpectations(test);
+    }
+
+    static void setupDisableSyntheticVsyncCallExpectations(DisplayTransactionTest* test) {
         EXPECT_CALL(*test->mEventThread, enableSyntheticVsync(_)).Times(0);
     }
 
     static void setupDisableVsyncCallExpectations(DisplayTransactionTest* test) {
         EXPECT_CALL(test->mFlinger.scheduler()->mockRequestHardwareVsync, Call(_, false)).Times(1);
+        setupEnableSyntheticVsyncCallExpectations(test);
+    }
+
+    static void setupEnableSyntheticVsyncCallExpectations(DisplayTransactionTest* test) {
         EXPECT_CALL(*test->mEventThread, enableSyntheticVsync(_)).Times(0);
     }
 };
@@ -91,12 +100,20 @@ struct EventThreadIsSupportedVariant : public EventThreadBaseSupportedVariant {
     static void setupEnableVsyncCallExpectations(DisplayTransactionTest* test) {
         // Expect to enable hardware VSYNC and disable synthetic VSYNC.
         EXPECT_CALL(test->mFlinger.scheduler()->mockRequestHardwareVsync, Call(_, true)).Times(1);
+        setupDisableSyntheticVsyncCallExpectations(test);
+    }
+
+    static void setupDisableSyntheticVsyncCallExpectations(DisplayTransactionTest* test) {
         EXPECT_CALL(*test->mEventThread, enableSyntheticVsync(false)).Times(1);
     }
 
     static void setupDisableVsyncCallExpectations(DisplayTransactionTest* test) {
         // Expect to disable hardware VSYNC and enable synthetic VSYNC.
         EXPECT_CALL(test->mFlinger.scheduler()->mockRequestHardwareVsync, Call(_, false)).Times(1);
+        setupEnableSyntheticVsyncCallExpectations(test);
+    }
+
+    static void setupEnableSyntheticVsyncCallExpectations(DisplayTransactionTest* test) {
         EXPECT_CALL(*test->mEventThread, enableSyntheticVsync(true)).Times(1);
     }
 };
@@ -151,7 +168,7 @@ struct TransitionOffToDozeSuspendVariant
     template <typename Case>
     static void setupCallExpectations(DisplayTransactionTest* test) {
         Case::setupComposerCallExpectations(test, Case::Doze::ACTUAL_POWER_MODE_FOR_DOZE_SUSPEND);
-        Case::EventThread::setupVsyncNoCallExpectations(test);
+        Case::EventThread::setupEnableSyntheticVsyncCallExpectations(test);
         Case::setupRepaintEverythingCallExpectations(test);
     }
 
@@ -176,7 +193,7 @@ struct TransitionDozeSuspendToOffVariant
       : public TransitionVariantCommon<PowerMode::DOZE_SUSPEND, PowerMode::OFF> {
     template <typename Case>
     static void setupCallExpectations(DisplayTransactionTest* test) {
-        Case::EventThread::setupVsyncNoCallExpectations(test);
+        Case::EventThread::setupEnableSyntheticVsyncCallExpectations(test);
         Case::setupComposerCallExpectations(test, IComposerClient::PowerMode::OFF);
     }
 
@@ -188,7 +205,7 @@ struct TransitionDozeSuspendToOffVariant
 struct TransitionOnToDozeVariant : public TransitionVariantCommon<PowerMode::ON, PowerMode::DOZE> {
     template <typename Case>
     static void setupCallExpectations(DisplayTransactionTest* test) {
-        Case::EventThread::setupVsyncNoCallExpectations(test);
+        Case::EventThread::setupDisableSyntheticVsyncCallExpectations(test);
         Case::setupComposerCallExpectations(test, Case::Doze::ACTUAL_POWER_MODE_FOR_DOZE);
     }
 };
@@ -206,7 +223,7 @@ struct TransitionDozeSuspendToDozeVariant
 struct TransitionDozeToOnVariant : public TransitionVariantCommon<PowerMode::DOZE, PowerMode::ON> {
     template <typename Case>
     static void setupCallExpectations(DisplayTransactionTest* test) {
-        Case::EventThread::setupVsyncNoCallExpectations(test);
+        Case::EventThread::setupDisableSyntheticVsyncCallExpectations(test);
         Case::setupComposerCallExpectations(test, IComposerClient::PowerMode::ON);
     }
 };
@@ -234,7 +251,7 @@ struct TransitionOnToUnknownVariant
       : public TransitionVariantCommon<PowerMode::ON, static_cast<PowerMode>(POWER_MODE_LEET)> {
     template <typename Case>
     static void setupCallExpectations(DisplayTransactionTest* test) {
-        Case::EventThread::setupVsyncNoCallExpectations(test);
+        Case::EventThread::setupDisableSyntheticVsyncCallExpectations(test);
         Case::setupNoComposerPowerModeCallExpectations(test);
     }
 };
@@ -335,6 +352,10 @@ void SetPhysicalDisplayPowerModeTest::transitionDisplayCommon() {
     // --------------------------------------------------------------------
     // Preconditions
 
+    SET_FLAG_FOR_TEST(android::companion::virtualdevice::flags::correct_virtual_display_power_state,
+                      true);
+    SET_FLAG_FOR_TEST(flags::disable_synthetic_vsync_for_performance, true);
+
     Case::Doze::setupComposerCallExpectations(this);
     auto display =
             Case::injectDisplayWithInitialPowerMode(this, Case::Transition::INITIAL_POWER_MODE);