From fe6cde1c9631de1bd5e0d927538b0998413c1db1 Mon Sep 17 00:00:00 2001 From: Jimmy Shiu Date: Wed, 7 Jul 2021 12:00:43 +0800 Subject: [PATCH] ADPF: fix abnormal high uclamp "GPU completion" task inherits a high uclamp value from RenderThread. But it's not in the ADPF thread list, so it remains a high uclamp value. Use SetTaskProfiles("ResetUclampGrp") and SetTaskProfiles("NoResetUclampGrp") to manage the uclamp_fork_reset for tasks. Bug: 191973176 Bug: 192149875 Test: vendor/google_testing/pts/tests/common/utils/perf/run_pts/jank_test.sh Test: adb shell cat /proc/vendor_sched/dump_task Change-Id: I6aed171e88c0a6db5f762e7c791344bb3f4b7a90 --- aidl/power-libperfmgr/Android.mk | 1 + aidl/power-libperfmgr/PowerHintSession.cpp | 91 +++++++++++-------- aidl/power-libperfmgr/PowerHintSession.h | 8 +- aidl/power-libperfmgr/PowerSessionManager.cpp | 29 ++++++ aidl/power-libperfmgr/PowerSessionManager.h | 1 + 5 files changed, 90 insertions(+), 40 deletions(-) diff --git a/aidl/power-libperfmgr/Android.mk b/aidl/power-libperfmgr/Android.mk index f5aa65a..8e5c21f 100644 --- a/aidl/power-libperfmgr/Android.mk +++ b/aidl/power-libperfmgr/Android.mk @@ -31,6 +31,7 @@ LOCAL_SHARED_LIBRARIES := \ libdl \ liblog \ libperfmgr \ + libprocessgroup \ libutils \ pixel-power-ext-V1-ndk_platform diff --git a/aidl/power-libperfmgr/PowerHintSession.cpp b/aidl/power-libperfmgr/PowerHintSession.cpp index 7b398e8..053aeda 100644 --- a/aidl/power-libperfmgr/PowerHintSession.cpp +++ b/aidl/power-libperfmgr/PowerHintSession.cpp @@ -40,15 +40,18 @@ using std::chrono::duration_cast; using std::chrono::nanoseconds; using std::literals::chrono_literals::operator""s; -constexpr char kPowerHalAdpfPidOffset[] = "vendor.powerhal.adpf.pid.offset"; -constexpr char kPowerHalAdpfPidP[] = "vendor.powerhal.adpf.pid.p"; -constexpr char kPowerHalAdpfPidI[] = "vendor.powerhal.adpf.pid.i"; -constexpr char kPowerHalAdpfPidIClamp[] = "vendor.powerhal.adpf.pid.i_clamp"; -constexpr char kPowerHalAdpfPidD[] = "vendor.powerhal.adpf.pid.d"; -constexpr char kPowerHalAdpfPidInitialIntegral[] = "vendor.powerhal.adpf.pid.i_init"; +constexpr char kPowerHalAdpfPidPOver[] = "vendor.powerhal.adpf.pid_p.over"; +constexpr char kPowerHalAdpfPidPUnder[] = "vendor.powerhal.adpf.pid_p.under"; +constexpr char kPowerHalAdpfPidI[] = "vendor.powerhal.adpf.pid_i"; +constexpr char kPowerHalAdpfPidDOver[] = "vendor.powerhal.adpf.pid_d.over"; +constexpr char kPowerHalAdpfPidDUnder[] = "vendor.powerhal.adpf.pid_d.under"; +constexpr char kPowerHalAdpfPidIInit[] = "vendor.powerhal.adpf.pid_i.init"; +constexpr char kPowerHalAdpfPidIHighLimit[] = "vendor.powerhal.adpf.pid_i.high_limit"; +constexpr char kPowerHalAdpfPidILowLimit[] = "vendor.powerhal.adpf.pid_i.low_limit"; constexpr char kPowerHalAdpfUclampEnable[] = "vendor.powerhal.adpf.uclamp"; -constexpr char kPowerHalAdpfUclampBoostCap[] = "vendor.powerhal.adpf.uclamp.boost_cap"; -constexpr char kPowerHalAdpfUclampGranularity[] = "vendor.powerhal.adpf.uclamp.granularity"; +constexpr char kPowerHalAdpfUclampMinGranularity[] = "vendor.powerhal.adpf.uclamp_min.granularity"; +constexpr char kPowerHalAdpfUclampMinHighLimit[] = "vendor.powerhal.adpf.uclamp_min.high_limit"; +constexpr char kPowerHalAdpfUclampMinLowLimit[] = "vendor.powerhal.adpf.uclamp_min.low_limit"; constexpr char kPowerHalAdpfStaleTimeFactor[] = "vendor.powerhal.adpf.stale_timeout_factor"; constexpr char kPowerHalAdpfPSamplingWindow[] = "vendor.powerhal.adpf.p.window"; constexpr char kPowerHalAdpfISamplingWindow[] = "vendor.powerhal.adpf.i.window"; @@ -91,24 +94,32 @@ static double getDoubleProperty(const char *prop, double value) { return value; } -static double sPidOffset = getDoubleProperty(kPowerHalAdpfPidOffset, 0.0); -static double sPidP = getDoubleProperty(kPowerHalAdpfPidP, 2.0); +static double sPidPOver = getDoubleProperty(kPowerHalAdpfPidPOver, 2.0); +static double sPidPUnder = getDoubleProperty(kPowerHalAdpfPidPUnder, 2.0); static double sPidI = getDoubleProperty(kPowerHalAdpfPidI, 0.001); -static double sPidD = getDoubleProperty(kPowerHalAdpfPidD, 100.0); +static double sPidDOver = getDoubleProperty(kPowerHalAdpfPidDOver, 100.0); +static double sPidDUnder = getDoubleProperty(kPowerHalAdpfPidDUnder, 0.0); static const int64_t sPidIInit = (sPidI == 0) ? 0 : static_cast(::android::base::GetIntProperty( - kPowerHalAdpfPidInitialIntegral, 100) / + kPowerHalAdpfPidIInit, 200) / sPidI); -static const int64_t sPidIClamp = +static const int64_t sPidIHighLimit = (sPidI == 0) ? 0 - : std::abs(static_cast(::android::base::GetIntProperty( - kPowerHalAdpfPidIClamp, 512) / - sPidI)); -static const int32_t sUclampCap = - ::android::base::GetUintProperty(kPowerHalAdpfUclampBoostCap, 512); -static const uint32_t sUclampGranularity = - ::android::base::GetUintProperty(kPowerHalAdpfUclampGranularity, 5); + : static_cast(::android::base::GetIntProperty( + kPowerHalAdpfPidIHighLimit, 512) / + sPidI); +static const int64_t sPidILowLimit = + (sPidI == 0) ? 0 + : static_cast(::android::base::GetIntProperty( + kPowerHalAdpfPidILowLimit, -512) / + sPidI); +static const int32_t sUclampMinHighLimit = + ::android::base::GetUintProperty(kPowerHalAdpfUclampMinHighLimit, 512); +static const int32_t sUclampMinLowLimit = + ::android::base::GetUintProperty(kPowerHalAdpfUclampMinLowLimit, 0); +static const uint32_t sUclampMinGranularity = + ::android::base::GetUintProperty(kPowerHalAdpfUclampMinGranularity, 5); static const int64_t sStaleTimeFactor = ::android::base::GetUintProperty(kPowerHalAdpfStaleTimeFactor, 20); static const int64_t sPSamplingWindow = @@ -123,7 +134,7 @@ static const int64_t sDSamplingWindow = PowerHintSession::PowerHintSession(int32_t tgid, int32_t uid, const std::vector &threadIds, int64_t durationNanos, const nanoseconds adpfRate) : kAdpfRate(adpfRate) { - mDescriptor = new AppHintDesc(tgid, uid, threadIds, sUclampCap); + mDescriptor = new AppHintDesc(tgid, uid, threadIds); mDescriptor->duration = std::chrono::nanoseconds(durationNanos); mStaleHandler = sp(new StaleHandler(this)); mPowerManagerHandler = PowerSessionManager::getInstance(); @@ -137,7 +148,7 @@ PowerHintSession::PowerHintSession(int32_t tgid, int32_t uid, const std::vector< } PowerSessionManager::getInstance()->addPowerSession(this); // init boost - setUclamp(sUclampCap, 1024); + setUclamp(sUclampMinHighLimit); ALOGV("PowerHintSession created: %s", mDescriptor->toString().c_str()); } @@ -191,6 +202,7 @@ int PowerHintSession::setUclamp(int32_t min, int32_t max) { } ALOGV("PowerHintSession tid: %d, uclamp(%d, %d)", tid, min, max); } + mDescriptor->current_min = min; return 0; } @@ -198,7 +210,7 @@ ndk::ScopedAStatus PowerHintSession::pause() { if (!mDescriptor->is_active.load()) return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_STATE); // Reset to default uclamp value. - setUclamp(0, 1024); + setUclamp(0); mDescriptor->is_active.store(false); if (ATRACE_ENABLED()) { const std::string idstr = getIdString(); @@ -215,7 +227,7 @@ ndk::ScopedAStatus PowerHintSession::resume() { mDescriptor->is_active.store(true); mDescriptor->integral_error = std::max(sPidIInit, mDescriptor->integral_error); // resume boost - setUclamp(sUclampCap, 1024); + setUclamp(sUclampMinHighLimit); if (ATRACE_ENABLED()) { const std::string idstr = getIdString(); std::string sz = StringPrintf("adpf.%s-active", idstr.c_str()); @@ -228,7 +240,7 @@ ndk::ScopedAStatus PowerHintSession::resume() { ndk::ScopedAStatus PowerHintSession::close() { PowerHintMonitor::getInstance()->getLooper()->removeMessages(mStaleHandler); // Reset to (0, 1024) uclamp value -- instead of threads' original setting. - setUclamp(0, 1024); + setUclamp(0); PowerSessionManager::getInstance()->removePowerSession(this); updateUniveralBoostMode(); return ndk::ScopedAStatus::ok(); @@ -295,8 +307,7 @@ ndk::ScopedAStatus PowerHintSession::reportActualWorkDuration( actualDurationNanos, targetDurationNanos); } // PID control algorithm - int64_t error = ns_to_100us(actualDurationNanos - targetDurationNanos) + - static_cast(sPidOffset); + int64_t error = ns_to_100us(actualDurationNanos - targetDurationNanos); if (i >= d_start) { derivative_sum += error - mDescriptor->previous_error; } @@ -305,14 +316,16 @@ ndk::ScopedAStatus PowerHintSession::reportActualWorkDuration( } if (i >= i_start) { mDescriptor->integral_error = mDescriptor->integral_error + error * dt; - mDescriptor->integral_error = std::min(sPidIClamp, mDescriptor->integral_error); - mDescriptor->integral_error = std::max(-sPidIClamp, mDescriptor->integral_error); + mDescriptor->integral_error = std::min(sPidIHighLimit, mDescriptor->integral_error); + mDescriptor->integral_error = std::max(sPidILowLimit, mDescriptor->integral_error); } mDescriptor->previous_error = error; } - int64_t pOut = static_cast(sPidP * err_sum / (length - p_start)); + int64_t pOut = static_cast((err_sum > 0 ? sPidPOver : sPidPUnder) * err_sum / + (length - p_start)); int64_t iOut = static_cast(sPidI * mDescriptor->integral_error); - int64_t dOut = static_cast(sPidD * derivative_sum / dt / (length - d_start)); + int64_t dOut = static_cast((derivative_sum > 0 ? sPidDOver : sPidDUnder) * + derivative_sum / dt / (length - d_start)); int64_t output = pOut + iOut + dOut; if (ATRACE_ENABLED()) { @@ -340,11 +353,11 @@ ndk::ScopedAStatus PowerHintSession::reportActualWorkDuration( /* apply to all the threads in the group */ if (output != 0) { - int next_min = std::min(sUclampCap, mDescriptor->current_min + static_cast(output)); - next_min = std::max(0, next_min); - if (std::abs(mDescriptor->current_min - next_min) > sUclampGranularity) { - setUclamp(next_min, 1024); - mDescriptor->current_min = next_min; + int next_min = + std::min(sUclampMinHighLimit, mDescriptor->current_min + static_cast(output)); + next_min = std::max(sUclampMinLowLimit, next_min); + if (std::abs(mDescriptor->current_min - next_min) > sUclampMinGranularity) { + setUclamp(next_min); } } @@ -381,6 +394,10 @@ bool PowerHintSession::isStale() { return now >= mStaleHandler->getStaleTime(); } +const std::vector &PowerHintSession::getTidList() const { + return mDescriptor->threadIds; +} + void PowerHintSession::setStale() { if (ATRACE_ENABLED()) { const std::string idstr = getIdString(); @@ -388,7 +405,7 @@ void PowerHintSession::setStale() { ATRACE_INT(sz.c_str(), 1); } // Reset to default uclamp value. - setUclamp(0, 1024); + setUclamp(0); // Deliver a task to check if all sessions are inactive. updateUniveralBoostMode(); } diff --git a/aidl/power-libperfmgr/PowerHintSession.h b/aidl/power-libperfmgr/PowerHintSession.h index e850634..6ad2652 100644 --- a/aidl/power-libperfmgr/PowerHintSession.h +++ b/aidl/power-libperfmgr/PowerHintSession.h @@ -41,13 +41,14 @@ using std::chrono::nanoseconds; using std::chrono::steady_clock; using std::chrono::time_point; +static const int32_t kMaxUclampValue = 1024; struct AppHintDesc { - AppHintDesc(int32_t tgid, int32_t uid, std::vector threadIds, int uclamp_min) + AppHintDesc(int32_t tgid, int32_t uid, std::vector threadIds) : tgid(tgid), uid(uid), threadIds(std::move(threadIds)), duration(0LL), - current_min(uclamp_min), + current_min(0), is_active(true), update_count(0), integral_error(0), @@ -79,6 +80,7 @@ class PowerHintSession : public BnPowerHintSession { const std::vector &actualDurations) override; bool isActive(); bool isStale(); + const std::vector &getTidList() const; private: class StaleHandler : public MessageHandler { @@ -99,7 +101,7 @@ class PowerHintSession : public BnPowerHintSession { private: void setStale(); void updateUniveralBoostMode(); - int setUclamp(int32_t max, int32_t min); + int setUclamp(int32_t min, int32_t max = kMaxUclampValue); std::string getIdString() const; AppHintDesc *mDescriptor = nullptr; sp mStaleHandler; diff --git a/aidl/power-libperfmgr/PowerSessionManager.cpp b/aidl/power-libperfmgr/PowerSessionManager.cpp index ff853ff..fc73c01 100644 --- a/aidl/power-libperfmgr/PowerSessionManager.cpp +++ b/aidl/power-libperfmgr/PowerSessionManager.cpp @@ -18,6 +18,7 @@ #define ATRACE_TAG (ATRACE_TAG_POWER | ATRACE_TAG_HAL) #include +#include #include #include "PowerSessionManager.h" @@ -55,11 +56,39 @@ int PowerSessionManager::getDisplayRefreshRate() { void PowerSessionManager::addPowerSession(PowerHintSession *session) { std::lock_guard guard(mLock); + for (auto t : session->getTidList()) { + if (mTidRefCountMap.find(t) == mTidRefCountMap.end()) { + if (!SetTaskProfiles(t, {"ResetUclampGrp"})) { + ALOGW("Failed to set ResetUclampGrp task profile for tid:%d", t); + } else { + mTidRefCountMap[t] = 1; + } + continue; + } + if (mTidRefCountMap[t] <= 0) { + ALOGE("Error! Unexpected zero/negative RefCount:%d for tid:%d", mTidRefCountMap[t], t); + continue; + } + mTidRefCountMap[t]++; + } mSessions.insert(session); } void PowerSessionManager::removePowerSession(PowerHintSession *session) { std::lock_guard guard(mLock); + for (auto t : session->getTidList()) { + if (mTidRefCountMap.find(t) == mTidRefCountMap.end()) { + ALOGE("Unexpected Error! Failed to look up tid:%d in TidRefCountMap", t); + continue; + } + mTidRefCountMap[t]--; + if (mTidRefCountMap[t] <= 0) { + if (!SetTaskProfiles(t, {"NoResetUclampGrp"})) { + ALOGW("Failed to set NoResetUclampGrp task profile for tid:%d", t); + } + mTidRefCountMap.erase(t); + } + } mSessions.erase(session); } diff --git a/aidl/power-libperfmgr/PowerSessionManager.h b/aidl/power-libperfmgr/PowerSessionManager.h index a705b68..4b7c36d 100644 --- a/aidl/power-libperfmgr/PowerSessionManager.h +++ b/aidl/power-libperfmgr/PowerSessionManager.h @@ -66,6 +66,7 @@ class PowerSessionManager : public MessageHandler { const std::string kDisableBoostHintName; std::shared_ptr mHintManager; std::unordered_set mSessions; // protected by mLock + std::unordered_map mTidRefCountMap; // protected by mLock std::mutex mLock; int mDisplayRefreshRate; bool mActive; // protected by mLock