diff options
| -rw-r--r-- | services/surfaceflinger/DisplayHardware/DisplayMode.h | 5 | ||||
| -rw-r--r-- | services/surfaceflinger/FlagManager.cpp | 175 | ||||
| -rw-r--r-- | services/surfaceflinger/FlagManager.h | 47 | ||||
| -rw-r--r-- | services/surfaceflinger/RefreshRateOverlay.cpp | 5 | ||||
| -rw-r--r-- | services/surfaceflinger/Scheduler/EventThread.cpp | 3 | ||||
| -rw-r--r-- | services/surfaceflinger/Scheduler/LayerHistory.cpp | 6 | ||||
| -rw-r--r-- | services/surfaceflinger/Scheduler/RefreshRateSelector.cpp | 3 | ||||
| -rw-r--r-- | services/surfaceflinger/Scheduler/Scheduler.cpp | 3 | ||||
| -rw-r--r-- | services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp | 9 | ||||
| -rw-r--r-- | services/surfaceflinger/SurfaceFlinger.cpp | 40 | ||||
| -rw-r--r-- | services/surfaceflinger/SurfaceFlinger.h | 8 | ||||
| -rw-r--r-- | services/surfaceflinger/Utils/FlagUtils.h | 33 | ||||
| -rw-r--r-- | services/surfaceflinger/main_surfaceflinger.cpp | 3 | ||||
| -rw-r--r-- | services/surfaceflinger/tests/unittests/FlagManagerTest.cpp | 191 | ||||
| -rw-r--r-- | services/surfaceflinger/tests/unittests/FlagUtils.h | 4 |
15 files changed, 296 insertions, 239 deletions
diff --git a/services/surfaceflinger/DisplayHardware/DisplayMode.h b/services/surfaceflinger/DisplayHardware/DisplayMode.h index 1775a7a4a6..f32fb3a5c7 100644 --- a/services/surfaceflinger/DisplayHardware/DisplayMode.h +++ b/services/surfaceflinger/DisplayHardware/DisplayMode.h @@ -30,8 +30,8 @@ #include <scheduler/Fps.h> #include "DisplayHardware/Hal.h" +#include "FlagManager.h" #include "Scheduler/StrongTyping.h" -#include "Utils/FlagUtils.h" namespace android { @@ -50,7 +50,6 @@ using DisplayModeId = StrongTyping<ui::DisplayModeId, struct DisplayModeIdTag, C using DisplayModes = ftl::SmallMap<DisplayModeId, DisplayModePtr, 3>; using DisplayModeIterator = DisplayModes::const_iterator; -using namespace com::android::graphics::surfaceflinger; class DisplayMode { public: @@ -140,7 +139,7 @@ public: // Peak refresh rate represents the highest refresh rate that can be used // for the presentation. Fps getPeakFps() const { - return flagutils::vrrConfigEnabled() && mVrrConfig + return FlagManager::getInstance().vrr_config() && mVrrConfig ? Fps::fromPeriodNsecs(mVrrConfig->minFrameIntervalNs) : mVsyncRate; } diff --git a/services/surfaceflinger/FlagManager.cpp b/services/surfaceflinger/FlagManager.cpp index f8ad8f6941..a8f05bbd56 100644 --- a/services/surfaceflinger/FlagManager.cpp +++ b/services/surfaceflinger/FlagManager.cpp @@ -26,37 +26,21 @@ #include <server_configurable_flags/get_flags.h> #include <cinttypes> +#include <com_android_graphics_surfaceflinger_flags.h> + namespace android { +using namespace com::android::graphics::surfaceflinger; + static constexpr const char* kExperimentNamespace = "surface_flinger_native_boot"; -static constexpr const int64_t kDemoFlag = -1; -FlagManager::~FlagManager() = default; +std::unique_ptr<FlagManager> FlagManager::mInstance; +std::once_flag FlagManager::mOnce; -void FlagManager::dump(std::string& result) const { - base::StringAppendF(&result, "FlagManager values: \n"); - base::StringAppendF(&result, "demo_flag: %" PRId64 "\n", demo_flag()); - base::StringAppendF(&result, "use_adpf_cpu_hint: %s\n", use_adpf_cpu_hint() ? "true" : "false"); - base::StringAppendF(&result, "use_skia_tracing: %s\n", use_skia_tracing() ? "true" : "false"); -} +FlagManager::FlagManager(ConstructorTag) {} +FlagManager::~FlagManager() = default; namespace { -template <typename T> -std::optional<T> doParse(const char* str); - -template <> -[[maybe_unused]] std::optional<int32_t> doParse(const char* str) { - int32_t ret; - return base::ParseInt(str, &ret) ? std::make_optional(ret) : std::nullopt; -} - -template <> -[[maybe_unused]] std::optional<int64_t> doParse(const char* str) { - int64_t ret; - return base::ParseInt(str, &ret) ? std::make_optional(ret) : std::nullopt; -} - -template <> -[[maybe_unused]] std::optional<bool> doParse(const char* str) { +std::optional<bool> parseBool(const char* str) { base::ParseBoolResult parseResult = base::ParseBool(str); switch (parseResult) { case base::ParseBoolResult::kTrue: @@ -67,44 +51,129 @@ template <> return std::nullopt; } } + +bool getFlagValue(std::function<bool()> getter, std::optional<bool> overrideValue) { + if (overrideValue.has_value()) { + return *overrideValue; + } + + return getter(); +} + +void dumpFlag(std::string& result, const char* name, std::function<bool()> getter) { + base::StringAppendF(&result, "%s: %s\n", name, getter() ? "true" : "false"); +} + } // namespace -std::string FlagManager::getServerConfigurableFlag(const std::string& experimentFlagName) const { - return server_configurable_flags::GetServerConfigurableFlag(kExperimentNamespace, - experimentFlagName, ""); +const FlagManager& FlagManager::getInstance() { + return getMutableInstance(); } -template int32_t FlagManager::getValue<int32_t>(const std::string&, std::optional<int32_t>, - int32_t) const; -template int64_t FlagManager::getValue<int64_t>(const std::string&, std::optional<int64_t>, - int64_t) const; -template bool FlagManager::getValue<bool>(const std::string&, std::optional<bool>, bool) const; -template <typename T> -T FlagManager::getValue(const std::string& experimentFlagName, std::optional<T> systemPropertyOpt, - T defaultValue) const { - // System property takes precedence over the experiment config server value. - if (systemPropertyOpt.has_value()) { - return *systemPropertyOpt; - } - std::string str = getServerConfigurableFlag(experimentFlagName); - return str.empty() ? defaultValue : doParse<T>(str.c_str()).value_or(defaultValue); +FlagManager& FlagManager::getMutableInstance() { + std::call_once(mOnce, [&] { + LOG_ALWAYS_FATAL_IF(mInstance, "Instance already created"); + mInstance = std::make_unique<FlagManager>(ConstructorTag{}); + }); + + return *mInstance; } -int64_t FlagManager::demo_flag() const { - std::optional<int64_t> sysPropVal = std::nullopt; - return getValue("DemoFeature__demo_flag", sysPropVal, kDemoFlag); +void FlagManager::markBootCompleted() { + mBootCompleted = true; } -bool FlagManager::use_adpf_cpu_hint() const { - std::optional<bool> sysPropVal = - doParse<bool>(base::GetProperty("debug.sf.enable_adpf_cpu_hint", "").c_str()); - return getValue("AdpfFeature__adpf_cpu_hint", sysPropVal, false); +void FlagManager::setUnitTestMode() { + mUnitTestMode = true; + + // Also set boot completed as we don't really care about it in unit testing + mBootCompleted = true; +} + +void FlagManager::dump(std::string& result) const { +#define DUMP_FLAG(name) dumpFlag(result, #name, std::bind(&FlagManager::name, this)) + + base::StringAppendF(&result, "FlagManager values: \n"); + DUMP_FLAG(use_adpf_cpu_hint); + DUMP_FLAG(use_skia_tracing); + DUMP_FLAG(connected_display); + DUMP_FLAG(dont_skip_on_early); + DUMP_FLAG(enable_small_area_detection); + DUMP_FLAG(misc1); + DUMP_FLAG(late_boot_misc2); + DUMP_FLAG(vrr_config); + +#undef DUMP_FLAG +} + +std::optional<bool> FlagManager::getBoolProperty(const char* property) const { + return parseBool(base::GetProperty(property, "").c_str()); +} + +bool FlagManager::getServerConfigurableFlag(const char* experimentFlagName) const { + const auto value = server_configurable_flags::GetServerConfigurableFlag(kExperimentNamespace, + experimentFlagName, ""); + const auto res = parseBool(value.c_str()); + return res.has_value() && res.value(); } -bool FlagManager::use_skia_tracing() const { - std::optional<bool> sysPropVal = - doParse<bool>(base::GetProperty(PROPERTY_SKIA_ATRACE_ENABLED, "").c_str()); - return getValue("SkiaTracingFeature__use_skia_tracing", sysPropVal, false); +#define FLAG_MANAGER_LEGACY_SERVER_FLAG(name, syspropOverride, serverFlagName) \ + bool FlagManager::name() const { \ + LOG_ALWAYS_FATAL_IF(!mBootCompleted, \ + "Can't read %s before boot completed as it is server writable", \ + __func__); \ + const auto debugOverride = getBoolProperty(syspropOverride); \ + if (debugOverride.has_value()) return debugOverride.value(); \ + return getServerConfigurableFlag(serverFlagName); \ + } + +#define FLAG_MANAGER_FLAG_INTERNAL(name, syspropOverride, checkForBootCompleted) \ + bool FlagManager::name() const { \ + if (checkForBootCompleted) { \ + LOG_ALWAYS_FATAL_IF(!mBootCompleted, \ + "Can't read %s before boot completed as it is server writable", \ + __func__); \ + } \ + static std::optional<bool> debugOverride = getBoolProperty(syspropOverride); \ + static bool value = getFlagValue([] { return flags::name(); }, debugOverride); \ + if (mUnitTestMode) { \ + /* \ + * When testing, we don't want to rely on the cached values stored in the static \ + * variables. \ + */ \ + debugOverride = getBoolProperty(syspropOverride); \ + value = getFlagValue([] { return flags::name(); }, debugOverride); \ + } \ + return value; \ + } + +#define FLAG_MANAGER_SERVER_FLAG(name, syspropOverride) \ + FLAG_MANAGER_FLAG_INTERNAL(name, syspropOverride, true) + +#define FLAG_MANAGER_READ_ONLY_FLAG(name, syspropOverride) \ + FLAG_MANAGER_FLAG_INTERNAL(name, syspropOverride, false) + +/// Legacy server flags /// +FLAG_MANAGER_LEGACY_SERVER_FLAG(test_flag, "", "") +FLAG_MANAGER_LEGACY_SERVER_FLAG(use_adpf_cpu_hint, "debug.sf.enable_adpf_cpu_hint", + "AdpfFeature__adpf_cpu_hint") +FLAG_MANAGER_LEGACY_SERVER_FLAG(use_skia_tracing, PROPERTY_SKIA_ATRACE_ENABLED, + "SkiaTracingFeature__use_skia_tracing") + +/// Trunk stable readonly flags /// +FLAG_MANAGER_READ_ONLY_FLAG(connected_display, "") +FLAG_MANAGER_READ_ONLY_FLAG(enable_small_area_detection, "") +FLAG_MANAGER_READ_ONLY_FLAG(misc1, "") +FLAG_MANAGER_READ_ONLY_FLAG(vrr_config, "debug.sf.enable_vrr_config") + +/// Trunk stable server flags /// +FLAG_MANAGER_SERVER_FLAG(late_boot_misc2, "") + +/// Exceptions /// +bool FlagManager::dont_skip_on_early() const { + // Even though this is a server writable flag, we do call it before boot completed, but that's + // fine since the decision is done per frame. We can't do caching though. + return flags::dont_skip_on_early(); } } // namespace android diff --git a/services/surfaceflinger/FlagManager.h b/services/surfaceflinger/FlagManager.h index e8341424e7..5a353bbe6c 100644 --- a/services/surfaceflinger/FlagManager.h +++ b/services/surfaceflinger/FlagManager.h @@ -17,32 +17,59 @@ #pragma once #include <cstdint> +#include <mutex> #include <optional> #include <string> namespace android { // Manages flags for SurfaceFlinger, including default values, system properties, and Mendel -// experiment configuration values. +// experiment configuration values. Can be called from any thread. class FlagManager { +private: + // Effectively making the constructor private, while allowing std::make_unique to work + struct ConstructorTag {}; + public: - FlagManager() = default; + static const FlagManager& getInstance(); + static FlagManager& getMutableInstance(); + + FlagManager(ConstructorTag); virtual ~FlagManager(); + + void markBootCompleted(); void dump(std::string& result) const; - int64_t demo_flag() const; + void setUnitTestMode(); + /// Legacy server flags /// + bool test_flag() const; bool use_adpf_cpu_hint() const; - bool use_skia_tracing() const; + /// Trunk stable readonly flags /// + bool connected_display() const; + bool enable_small_area_detection() const; + bool misc1() const; + bool vrr_config() const; + + /// Trunk stable server flags /// + bool late_boot_misc2() const; + bool dont_skip_on_early() const; + +protected: + // overridden for unit tests + virtual std::optional<bool> getBoolProperty(const char*) const; + virtual bool getServerConfigurableFlag(const char*) const; + private: - friend class FlagManagerTest; + friend class TestableFlagManager; + + FlagManager(const FlagManager&) = delete; - // Wrapper for mocking in test. - virtual std::string getServerConfigurableFlag(const std::string& experimentFlagName) const; + std::atomic_bool mBootCompleted = false; + std::atomic_bool mUnitTestMode = false; - template <typename T> - T getValue(const std::string& experimentFlagName, std::optional<T> systemPropertyOpt, - T defaultValue) const; + static std::unique_ptr<FlagManager> mInstance; + static std::once_flag mOnce; }; } // namespace android diff --git a/services/surfaceflinger/RefreshRateOverlay.cpp b/services/surfaceflinger/RefreshRateOverlay.cpp index 42676c6936..2ac7319709 100644 --- a/services/surfaceflinger/RefreshRateOverlay.cpp +++ b/services/surfaceflinger/RefreshRateOverlay.cpp @@ -17,9 +17,9 @@ #include <algorithm> #include "Client.h" +#include "FlagManager.h" #include "Layer.h" #include "RefreshRateOverlay.h" -#include "Utils/FlagUtils.h" #include <SkSurface.h> @@ -268,7 +268,8 @@ void RefreshRateOverlay::changeRefreshRate(Fps vsyncRate, Fps renderFps) { } void RefreshRateOverlay::changeRenderRate(Fps renderFps) { - if (mFeatures.test(Features::RenderRate) && mVsyncRate && flagutils::vrrConfigEnabled()) { + if (mFeatures.test(Features::RenderRate) && mVsyncRate && + FlagManager::getInstance().vrr_config()) { mRenderFps = renderFps; const auto buffer = getOrCreateBuffers(*mVsyncRate, renderFps)[mFrame]; createTransaction().setBuffer(mSurfaceControl->get(), buffer).apply(); diff --git a/services/surfaceflinger/Scheduler/EventThread.cpp b/services/surfaceflinger/Scheduler/EventThread.cpp index 9a55c94424..7f627f829d 100644 --- a/services/surfaceflinger/Scheduler/EventThread.cpp +++ b/services/surfaceflinger/Scheduler/EventThread.cpp @@ -45,6 +45,7 @@ #include <scheduler/VsyncConfig.h> #include "DisplayHardware/DisplayMode.h" +#include "FlagManager.h" #include "FrameTimeline.h" #include "VSyncDispatch.h" #include "VSyncTracker.h" @@ -308,7 +309,7 @@ sp<EventThreadConnection> EventThread::createEventConnection( auto connection = sp<EventThreadConnection>::make(const_cast<EventThread*>(this), IPCThreadState::self()->getCallingUid(), eventRegistration); - if (flags::misc1()) { + if (FlagManager::getInstance().misc1()) { const int policy = SCHED_FIFO; connection->setMinSchedulerPolicy(policy, sched_get_priority_min(policy)); } diff --git a/services/surfaceflinger/Scheduler/LayerHistory.cpp b/services/surfaceflinger/Scheduler/LayerHistory.cpp index ff829143d3..21bbb08c1c 100644 --- a/services/surfaceflinger/Scheduler/LayerHistory.cpp +++ b/services/surfaceflinger/Scheduler/LayerHistory.cpp @@ -21,7 +21,6 @@ #include "LayerHistory.h" #include <android-base/stringprintf.h> -#include <com_android_graphics_surfaceflinger_flags.h> #include <cutils/properties.h> #include <gui/TraceUtils.h> #include <utils/Log.h> @@ -34,16 +33,15 @@ #include "../Layer.h" #include "EventThread.h" +#include "FlagManager.h" #include "LayerInfo.h" namespace android::scheduler { namespace { -using namespace com::android::graphics::surfaceflinger; - bool isLayerActive(const LayerInfo& info, nsecs_t threshold) { - if (flags::misc1() && !info.isVisible()) { + if (FlagManager::getInstance().misc1() && !info.isVisible()) { return false; } diff --git a/services/surfaceflinger/Scheduler/RefreshRateSelector.cpp b/services/surfaceflinger/Scheduler/RefreshRateSelector.cpp index eb69d0bf22..5892b2b44c 100644 --- a/services/surfaceflinger/Scheduler/RefreshRateSelector.cpp +++ b/services/surfaceflinger/Scheduler/RefreshRateSelector.cpp @@ -38,7 +38,6 @@ #include "../SurfaceFlingerProperties.h" #include "RefreshRateSelector.h" -#include "Utils/FlagUtils.h" #include <com_android_graphics_surfaceflinger_flags.h> @@ -115,7 +114,7 @@ std::pair<unsigned, unsigned> divisorRange(Fps vsyncRate, Fps peakFps, FpsRange using fps_approx_ops::operator/; // use signed type as `fps / range.max` might be 0 auto start = std::max(1, static_cast<int>(peakFps / range.max) - 1); - if (flagutils::vrrConfigEnabled()) { + if (FlagManager::getInstance().vrr_config()) { start = std::max(1, static_cast<int>(vsyncRate / std::min(range.max, peakFps, fps_approx_ops::operator<)) - diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index 5b36a5efa6..1a8713d4a9 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -47,6 +47,7 @@ #include "../Layer.h" #include "EventThread.h" +#include "FlagManager.h" #include "FrameRateOverrideMappings.h" #include "FrontEnd/LayerHandle.h" #include "OneShotTimer.h" @@ -210,7 +211,7 @@ void Scheduler::onFrameSignal(ICompositor& compositor, VsyncId vsyncId, targeters.try_emplace(id, &targeter); } - if (flagutils::vrrConfigEnabled() && + if (FlagManager::getInstance().vrr_config() && CC_UNLIKELY(mPacesetterFrameDurationFractionToSkip > 0.f)) { const auto period = pacesetterTargeter.target().expectedFrameDuration(); const auto skipDuration = Duration::fromNs( diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp index f4676706fd..186a6bc9d6 100644 --- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp +++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp @@ -25,16 +25,14 @@ #include <scheduler/TimeKeeper.h> +#include "FlagManager.h" #include "VSyncDispatchTimerQueue.h" #include "VSyncTracker.h" -#include <com_android_graphics_surfaceflinger_flags.h> - #undef LOG_TAG #define LOG_TAG "VSyncDispatch" namespace android::scheduler { -using namespace com::android::graphics::surfaceflinger; using base::StringAppendF; @@ -43,7 +41,8 @@ namespace { nsecs_t getExpectedCallbackTime(nsecs_t now, nsecs_t nextVsyncTime, const VSyncDispatch::ScheduleTiming& timing) { const auto expectedCallbackTime = nextVsyncTime - timing.readyDuration - timing.workDuration; - const auto baseTime = flags::dont_skip_on_early() ? now : expectedCallbackTime; + const auto baseTime = + FlagManager::getInstance().dont_skip_on_early() ? now : expectedCallbackTime; return std::max(baseTime, expectedCallbackTime); } @@ -105,7 +104,7 @@ ScheduleResult VSyncDispatchTimerQueueEntry::schedule(VSyncDispatch::ScheduleTim mArmedInfo && (nextVsyncTime > (mArmedInfo->mActualVsyncTime + mMinVsyncDistance)); bool const wouldSkipAWakeup = mArmedInfo && ((nextWakeupTime > (mArmedInfo->mActualWakeupTime + mMinVsyncDistance))); - if (flags::dont_skip_on_early()) { + if (FlagManager::getInstance().dont_skip_on_early()) { if (wouldSkipAVsyncTarget || wouldSkipAWakeup) { return getExpectedCallbackTime(now, mArmedInfo->mActualVsyncTime, timing); } diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 48be33c7ec..62eb17d2e7 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -157,15 +157,12 @@ #include "TimeStats/TimeStats.h" #include "TunnelModeEnabledReporter.h" #include "Utils/Dumper.h" -#include "Utils/FlagUtils.h" #include "WindowInfosListenerInvoker.h" #include <aidl/android/hardware/graphics/common/DisplayDecorationSupport.h> #include <aidl/android/hardware/graphics/composer3/DisplayCapability.h> #include <aidl/android/hardware/graphics/composer3/RenderIntent.h> -#include <com_android_graphics_surfaceflinger_flags.h> - #undef NO_THREAD_SAFETY_ANALYSIS #define NO_THREAD_SAFETY_ANALYSIS \ _Pragma("GCC error \"Prefer <ftl/fake_guard.h> or MutexUtils.h helpers.\"") @@ -175,8 +172,6 @@ #define DOES_CONTAIN_BORDER false namespace android { -using namespace com::android::graphics::surfaceflinger; - using namespace std::chrono_literals; using namespace std::string_literals; using namespace std::string_view_literals; @@ -509,11 +504,6 @@ SurfaceFlinger::SurfaceFlinger(Factory& factory) : SurfaceFlinger(factory, SkipI base::GetBoolProperty("persist.debug.sf.enable_layer_lifecycle_manager"s, true); mLegacyFrontEndEnabled = !mLayerLifecycleManagerEnabled || base::GetBoolProperty("persist.debug.sf.enable_legacy_frontend"s, false); - - // Trunk-Stable flags - mMiscFlagValue = flags::misc1(); - mConnectedDisplayFlagValue = flags::connected_display(); - mMisc2FlagEarlyBootValue = flags::late_boot_misc2(); } LatchUnsignaledConfig SurfaceFlinger::getLatchUnsignaledConfig() { @@ -689,6 +679,7 @@ void SurfaceFlinger::bootFinished() { return; } mBootFinished = true; + FlagManager::getMutableInstance().markBootCompleted(); if (mStartPropertySetThread->join() != NO_ERROR) { ALOGE("Join StartPropertySetThread failed!"); } @@ -702,7 +693,7 @@ void SurfaceFlinger::bootFinished() { mFrameTracer->initialize(); mFrameTimeline->onBootFinished(); - getRenderEngine().setEnableTracing(mFlagManager.use_skia_tracing()); + getRenderEngine().setEnableTracing(FlagManager::getInstance().use_skia_tracing()); // wait patiently for the window manager death const String16 name("window"); @@ -731,7 +722,7 @@ void SurfaceFlinger::bootFinished() { readPersistentProperties(); mPowerAdvisor->onBootFinished(); - const bool hintSessionEnabled = mFlagManager.use_adpf_cpu_hint(); + const bool hintSessionEnabled = FlagManager::getInstance().use_adpf_cpu_hint(); mPowerAdvisor->enablePowerHintSession(hintSessionEnabled); const bool hintSessionUsed = mPowerAdvisor->usePowerHintSession(); ALOGD("Power hint is %s", @@ -755,10 +746,6 @@ void SurfaceFlinger::bootFinished() { enableRefreshRateOverlay(true); } })); - - LOG_ALWAYS_FATAL_IF(flags::misc1() != mMiscFlagValue, "misc1 flag is not boot stable!"); - - mMisc2FlagLateBootValue = flags::late_boot_misc2(); } static std::optional<renderengine::RenderEngine::RenderEngineType> @@ -2101,7 +2088,7 @@ nsecs_t SurfaceFlinger::getVsyncPeriodFromHWC() const { void SurfaceFlinger::onComposerHalVsync(hal::HWDisplayId hwcDisplayId, int64_t timestamp, std::optional<hal::VsyncPeriodNanos> vsyncPeriod) { - if (mConnectedDisplayFlagValue) { + if (FlagManager::getInstance().connected_display()) { // use ~0 instead of -1 as AidlComposerHal.cpp passes the param as unsigned int32 if (mIsHotplugErrViaNegVsync && timestamp < 0 && vsyncPeriod.has_value() && vsyncPeriod.value() == ~0) { @@ -4065,7 +4052,7 @@ void SurfaceFlinger::initScheduler(const sp<const DisplayDevice>& display) { if (sysprop::use_content_detection_for_refresh_rate(false)) { features |= Feature::kContentDetection; - if (flags::enable_small_area_detection()) { + if (FlagManager::getInstance().enable_small_area_detection()) { features |= Feature::kSmallDirtyContentDetection; } } @@ -6475,17 +6462,6 @@ void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, const std::string& comp result.append("SurfaceFlinger global state:\n"); colorizer.reset(result); - StringAppendF(&result, "MiscFlagValue: %s\n", mMiscFlagValue ? "true" : "false"); - StringAppendF(&result, "ConnectedDisplayFlagValue: %s\n", - mConnectedDisplayFlagValue ? "true" : "false"); - StringAppendF(&result, "Misc2FlagValue: %s (%s after boot)\n", - mMisc2FlagLateBootValue ? "true" : "false", - mMisc2FlagEarlyBootValue == mMisc2FlagLateBootValue ? "stable" : "modified"); - StringAppendF(&result, "VrrConfigFlagValue: %s\n", - flagutils::vrrConfigEnabled() ? "true" : "false"); - StringAppendF(&result, "DontSkipOnEarlyFlagValue: %s\n", - flags::dont_skip_on_early() ? "true" : "false"); - getRenderEngine().dump(result); result.append("ClientCache state:\n"); @@ -6562,7 +6538,7 @@ void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, const std::string& comp /* * Dump flag/property manager state */ - mFlagManager.dump(result); + FlagManager::getInstance().dump(result); result.append(mTimeStats->miniDump()); result.append("\n"); @@ -7227,7 +7203,7 @@ status_t SurfaceFlinger::onTransact(uint32_t code, const Parcel& data, Parcel* r // Second argument is a delay in ms for triggering the jank. This is useful for working // with tools that steal the adb connection. This argument is optional. case 1045: { - if (flagutils::vrrConfigEnabled()) { + if (FlagManager::getInstance().vrr_config()) { float jankAmount = data.readFloat(); int32_t jankDelayMs = 0; if (data.readInt32(&jankDelayMs) != NO_ERROR) { @@ -9078,7 +9054,7 @@ binder::Status SurfaceComposerAIDL::createConnection(sp<gui::ISurfaceComposerCli const sp<Client> client = sp<Client>::make(mFlinger); if (client->initCheck() == NO_ERROR) { *outClient = client; - if (flags::misc1()) { + if (FlagManager::getInstance().misc1()) { const int policy = SCHED_FIFO; client->setMinSchedulerPolicy(policy, sched_get_priority_min(policy)); } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 96b67b8176..520bd221b3 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -1421,8 +1421,6 @@ private: const sp<WindowInfosListenerInvoker> mWindowInfosListenerInvoker; - FlagManager mFlagManager; - // returns the framerate of the layer with the given sequence ID float getLayerFramerate(nsecs_t now, int32_t id) const { return mScheduler->getLayerFramerate(now, id); @@ -1459,12 +1457,6 @@ private: void sfdo_setDebugFlash(int delay); void sfdo_scheduleComposite(); void sfdo_scheduleCommit(); - - // Trunk-Stable flags - bool mMiscFlagValue; - bool mConnectedDisplayFlagValue; - bool mMisc2FlagEarlyBootValue; - bool mMisc2FlagLateBootValue; }; class SurfaceComposerAIDL : public gui::BnSurfaceComposer { diff --git a/services/surfaceflinger/Utils/FlagUtils.h b/services/surfaceflinger/Utils/FlagUtils.h deleted file mode 100644 index 8435f04573..0000000000 --- a/services/surfaceflinger/Utils/FlagUtils.h +++ /dev/null @@ -1,33 +0,0 @@ -/** - * Copyright 2023 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#pragma once - -#include <android-base/properties.h> -#include <com_android_graphics_surfaceflinger_flags.h> -#include <string> - -namespace android::flagutils { - -using namespace std::literals::string_literals; -using namespace com::android::graphics::surfaceflinger; - -inline bool vrrConfigEnabled() { - static const bool enable_vrr_config = - base::GetBoolProperty("debug.sf.enable_vrr_config"s, false); - return flags::vrr_config() || enable_vrr_config; -} -} // namespace android::flagutils diff --git a/services/surfaceflinger/main_surfaceflinger.cpp b/services/surfaceflinger/main_surfaceflinger.cpp index 959945230d..9889cb9c5d 100644 --- a/services/surfaceflinger/main_surfaceflinger.cpp +++ b/services/surfaceflinger/main_surfaceflinger.cpp @@ -34,6 +34,7 @@ #include <errno.h> #include <hidl/LegacySupport.h> #include <processgroup/sched_policy.h> +#include "FlagManager.h" #include "SurfaceFlinger.h" #include "SurfaceFlingerFactory.h" #include "SurfaceFlingerProperties.h" @@ -149,7 +150,7 @@ int main(int, char**) { // publish gui::ISurfaceComposer, the new AIDL interface sp<SurfaceComposerAIDL> composerAIDL = sp<SurfaceComposerAIDL>::make(flinger); - if (flags::misc1()) { + if (FlagManager::getInstance().misc1()) { composerAIDL->setMinSchedulerPolicy(SCHED_FIFO, newPriority); } sm->addService(String16("SurfaceFlingerAIDL"), composerAIDL, false, diff --git a/services/surfaceflinger/tests/unittests/FlagManagerTest.cpp b/services/surfaceflinger/tests/unittests/FlagManagerTest.cpp index 0905cd14b1..aa37754300 100644 --- a/services/surfaceflinger/tests/unittests/FlagManagerTest.cpp +++ b/services/surfaceflinger/tests/unittests/FlagManagerTest.cpp @@ -14,130 +14,153 @@ * limitations under the License. */ -#include <cstdint> #undef LOG_TAG #define LOG_TAG "FlagManagerTest" #include "FlagManager.h" +#include "FlagUtils.h" -#include <android-base/properties.h> #include <gmock/gmock.h> #include <gtest/gtest.h> #include <log/log.h> -#include <server_configurable_flags/get_flags.h> -#include <optional> + +#include <com_android_graphics_surfaceflinger_flags.h> namespace android { using testing::Return; -class MockFlagManager : public FlagManager { +class TestableFlagManager : public FlagManager { public: - MockFlagManager() = default; - ~MockFlagManager() = default; + TestableFlagManager() : FlagManager(ConstructorTag{}) { markBootCompleted(); } + ~TestableFlagManager() = default; + + MOCK_METHOD(std::optional<bool>, getBoolProperty, (const char*), (const, override)); + MOCK_METHOD(bool, getServerConfigurableFlag, (const char*), (const, override)); - MOCK_METHOD(std::string, getServerConfigurableFlag, (const std::string& experimentFlagName), - (const, override)); + void markBootIncomplete() { mBootCompleted = false; } }; class FlagManagerTest : public testing::Test { public: - FlagManagerTest(); - ~FlagManagerTest() override; - std::unique_ptr<MockFlagManager> mFlagManager; - - template <typename T> - T getValue(const std::string& experimentFlagName, std::optional<T> systemPropertyOpt, - T defaultValue); + FlagManagerTest() { + const ::testing::TestInfo* const test_info = + ::testing::UnitTest::GetInstance()->current_test_info(); + ALOGD("**** Setting up for %s.%s\n", test_info->test_case_name(), test_info->name()); + } + ~FlagManagerTest() override { + const ::testing::TestInfo* const test_info = + ::testing::UnitTest::GetInstance()->current_test_info(); + ALOGD("**** Tearing down after %s.%s\n", test_info->test_case_name(), test_info->name()); + } + + TestableFlagManager mFlagManager; }; -FlagManagerTest::FlagManagerTest() { - const ::testing::TestInfo* const test_info = - ::testing::UnitTest::GetInstance()->current_test_info(); - ALOGD("**** Setting up for %s.%s\n", test_info->test_case_name(), test_info->name()); - mFlagManager = std::make_unique<MockFlagManager>(); +TEST_F(FlagManagerTest, isSingleton) { + EXPECT_EQ(&FlagManager::getInstance(), &FlagManager::getInstance()); } -FlagManagerTest::~FlagManagerTest() { - const ::testing::TestInfo* const test_info = - ::testing::UnitTest::GetInstance()->current_test_info(); - ALOGD("**** Tearing down after %s.%s\n", test_info->test_case_name(), test_info->name()); +TEST_F(FlagManagerTest, legacyCreashesIfQueriedBeforeBoot) { + mFlagManager.markBootIncomplete(); + EXPECT_DEATH(FlagManager::getInstance().test_flag(), ""); } -template <typename T> -T FlagManagerTest::getValue(const std::string& experimentFlagName, - std::optional<T> systemPropertyOpt, T defaultValue) { - return mFlagManager->getValue(experimentFlagName, systemPropertyOpt, defaultValue); -} +TEST_F(FlagManagerTest, legacyReturnsOverride) { + EXPECT_CALL(mFlagManager, getBoolProperty).WillOnce(Return(true)); + EXPECT_EQ(true, mFlagManager.test_flag()); -namespace { -TEST_F(FlagManagerTest, getValue_bool_default) { - EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("")); - const bool defaultValue = false; - std::optional<bool> systemPropertyValue = std::nullopt; - const bool result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue); - ASSERT_EQ(result, defaultValue); + EXPECT_CALL(mFlagManager, getBoolProperty).WillOnce(Return(false)); + EXPECT_EQ(false, mFlagManager.test_flag()); } -TEST_F(FlagManagerTest, getValue_bool_sysprop) { - const bool defaultValue = false; - std::optional<bool> systemPropertyValue = std::make_optional(true); - const bool result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue); - ASSERT_EQ(result, true); -} +TEST_F(FlagManagerTest, legacyReturnsValue) { + EXPECT_CALL(mFlagManager, getBoolProperty).WillRepeatedly(Return(std::nullopt)); + + EXPECT_CALL(mFlagManager, getServerConfigurableFlag).WillOnce(Return(true)); + EXPECT_EQ(true, mFlagManager.test_flag()); -TEST_F(FlagManagerTest, getValue_bool_experiment) { - EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("1")); - const bool defaultValue = false; - std::optional<bool> systemPropertyValue = std::nullopt; - const bool result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue); - ASSERT_EQ(result, true); + EXPECT_CALL(mFlagManager, getServerConfigurableFlag).WillOnce(Return(false)); + EXPECT_EQ(false, mFlagManager.test_flag()); } -TEST_F(FlagManagerTest, getValue_int32_default) { - EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("")); - int32_t defaultValue = 30; - std::optional<int32_t> systemPropertyValue = std::nullopt; - int32_t result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue); - ASSERT_EQ(result, defaultValue); +TEST_F(FlagManagerTest, creashesIfQueriedBeforeBoot) { + mFlagManager.markBootIncomplete(); + EXPECT_DEATH(FlagManager::getInstance().late_boot_misc2(), ""); } -TEST_F(FlagManagerTest, getValue_int32_sysprop) { - int32_t defaultValue = 30; - std::optional<int32_t> systemPropertyValue = std::make_optional(10); - int32_t result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue); - ASSERT_EQ(result, 10); +TEST_F(FlagManagerTest, returnsOverride) { + mFlagManager.setUnitTestMode(); + + // Twice, since the first call is to initialize the static variable + EXPECT_CALL(mFlagManager, getBoolProperty) + .Times((2)) + .WillOnce(Return(true)) + .WillOnce(Return(true)); + EXPECT_EQ(true, mFlagManager.late_boot_misc2()); + + EXPECT_CALL(mFlagManager, getBoolProperty).WillOnce(Return(false)); + EXPECT_EQ(false, mFlagManager.late_boot_misc2()); } -TEST_F(FlagManagerTest, getValue_int32_experiment) { - EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("50")); - std::int32_t defaultValue = 30; - std::optional<std::int32_t> systemPropertyValue = std::nullopt; - std::int32_t result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue); - ASSERT_EQ(result, 50); +TEST_F(FlagManagerTest, returnsValue) { + mFlagManager.setUnitTestMode(); + + EXPECT_CALL(mFlagManager, getBoolProperty).WillRepeatedly(Return(std::nullopt)); + + { + SET_FLAG_FOR_TEST(com::android::graphics::surfaceflinger::flags::late_boot_misc2, true); + EXPECT_EQ(true, mFlagManager.late_boot_misc2()); + } + + { + SET_FLAG_FOR_TEST(com::android::graphics::surfaceflinger::flags::late_boot_misc2, false); + EXPECT_EQ(false, mFlagManager.late_boot_misc2()); + } } -TEST_F(FlagManagerTest, getValue_int64_default) { - EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("")); - int64_t defaultValue = 30; - std::optional<int64_t> systemPropertyValue = std::nullopt; - int64_t result = getValue("flag_name", systemPropertyValue, defaultValue); - ASSERT_EQ(result, defaultValue); +TEST_F(FlagManagerTest, readonlyReturnsOverride) { + mFlagManager.setUnitTestMode(); + + // Twice, since the first call is to initialize the static variable + EXPECT_CALL(mFlagManager, getBoolProperty) + .Times(2) + .WillOnce(Return(true)) + .WillOnce(Return(true)); + EXPECT_EQ(true, mFlagManager.misc1()); + + EXPECT_CALL(mFlagManager, getBoolProperty).WillOnce(Return(false)); + EXPECT_EQ(false, mFlagManager.misc1()); } -TEST_F(FlagManagerTest, getValue_int64_sysprop) { - int64_t defaultValue = 30; - std::optional<int64_t> systemPropertyValue = std::make_optional(10); - int64_t result = getValue("flag_name", systemPropertyValue, defaultValue); - ASSERT_EQ(result, 10); +TEST_F(FlagManagerTest, readonlyReturnsValue) { + mFlagManager.setUnitTestMode(); + + EXPECT_CALL(mFlagManager, getBoolProperty).WillRepeatedly(Return(std::nullopt)); + + { + SET_FLAG_FOR_TEST(com::android::graphics::surfaceflinger::flags::misc1, true); + EXPECT_EQ(true, mFlagManager.misc1()); + } + + { + SET_FLAG_FOR_TEST(com::android::graphics::surfaceflinger::flags::misc1, false); + EXPECT_EQ(false, mFlagManager.misc1()); + } } -TEST_F(FlagManagerTest, getValue_int64_experiment) { - EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("50")); - int64_t defaultValue = 30; - std::optional<int64_t> systemPropertyValue = std::nullopt; - int64_t result = getValue("flag_name", systemPropertyValue, defaultValue); - ASSERT_EQ(result, 50); +TEST_F(FlagManagerTest, dontSkipOnEarlyIsNotCached) { + EXPECT_CALL(mFlagManager, getBoolProperty).WillRepeatedly(Return(std::nullopt)); + + const auto initialValue = com::android::graphics::surfaceflinger::flags::dont_skip_on_early(); + + com::android::graphics::surfaceflinger::flags::dont_skip_on_early(true); + EXPECT_EQ(true, mFlagManager.dont_skip_on_early()); + + com::android::graphics::surfaceflinger::flags::dont_skip_on_early(false); + EXPECT_EQ(false, mFlagManager.dont_skip_on_early()); + + com::android::graphics::surfaceflinger::flags::dont_skip_on_early(initialValue); } -} // namespace + } // namespace android diff --git a/services/surfaceflinger/tests/unittests/FlagUtils.h b/services/surfaceflinger/tests/unittests/FlagUtils.h index 7103684577..333e4e7e05 100644 --- a/services/surfaceflinger/tests/unittests/FlagUtils.h +++ b/services/surfaceflinger/tests/unittests/FlagUtils.h @@ -16,12 +16,16 @@ #pragma once +#include "FlagManager.h" + #define SET_FLAG_FOR_TEST(name, value) TestFlagSetter _testflag_((name), (name), (value)) namespace android { class TestFlagSetter { public: TestFlagSetter(bool (*getter)(), void((*setter)(bool)), bool flagValue) { + FlagManager::getMutableInstance().setUnitTestMode(); + const bool initialValue = getter(); setter(flagValue); mResetFlagValue = [=] { setter(initialValue); }; |