Rewrite concurrency logic in SurfaceFlinger PowerAdvisor
This patch aims to make concurrency safer by making sure all checks to
"hint session running" use mHintSession directly rather than a proxy
variable, and lock it. This requires all access to
"ensurePowerHintSessionRunning" to be locked, which requires refactoring
some methods, but should make the class much safer. This patch also aims
to ensure the session pointer is being directly checked for validity,
rather than relying on PowerHAL to always pass valid sessions.
Bug: 308030899
Test: atest libsurfaceflinger_unittest:PowerAdvisorTest
Change-Id: I0ebe55ed3c62c57cfd6fb94280beff4b9e8fcc13
diff --git a/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp b/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp
index e005ad3..dd228b4 100644
--- a/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp
+++ b/services/surfaceflinger/DisplayHardware/PowerAdvisor.cpp
@@ -50,7 +50,6 @@
namespace impl {
using aidl::android::hardware::power::Boost;
-using aidl::android::hardware::power::IPowerHintSession;
using aidl::android::hardware::power::Mode;
using aidl::android::hardware::power::SessionHint;
using aidl::android::hardware::power::WorkDuration;
@@ -144,11 +143,13 @@
if (!mBootFinished.load()) {
return;
}
- if (usePowerHintSession() && ensurePowerHintSessionRunning()) {
+ if (usePowerHintSession()) {
std::lock_guard lock(mHintSessionMutex);
- auto ret = mHintSession->sendHint(SessionHint::CPU_LOAD_UP);
- if (!ret.isOk()) {
- mHintSessionRunning = false;
+ if (ensurePowerHintSessionRunning()) {
+ auto ret = mHintSession->sendHint(SessionHint::CPU_LOAD_UP);
+ if (!ret.isOk()) {
+ mHintSession = nullptr;
+ }
}
}
}
@@ -162,11 +163,13 @@
if (mSendUpdateImminent.exchange(false)) {
ALOGV("AIDL notifyDisplayUpdateImminentAndCpuReset");
- if (usePowerHintSession() && ensurePowerHintSessionRunning()) {
+ if (usePowerHintSession()) {
std::lock_guard lock(mHintSessionMutex);
- auto ret = mHintSession->sendHint(SessionHint::CPU_LOAD_RESET);
- if (!ret.isOk()) {
- mHintSessionRunning = false;
+ if (ensurePowerHintSessionRunning()) {
+ auto ret = mHintSession->sendHint(SessionHint::CPU_LOAD_RESET);
+ if (!ret.isOk()) {
+ mHintSession = nullptr;
+ }
}
}
@@ -193,14 +196,12 @@
}
}
-// checks both if it supports and if it's enabled
bool PowerAdvisor::usePowerHintSession() {
// uses cached value since the underlying support and flag are unlikely to change at runtime
return mHintSessionEnabled.value_or(false) && supportsPowerHintSession();
}
bool PowerAdvisor::supportsPowerHintSession() {
- // cache to avoid needing lock every time
if (!mSupportsHintSession.has_value()) {
mSupportsHintSession = getPowerHal().getHintSessionPreferredRate().isOk();
}
@@ -208,10 +209,15 @@
}
bool PowerAdvisor::ensurePowerHintSessionRunning() {
- if (!mHintSessionRunning && !mHintSessionThreadIds.empty() && usePowerHintSession()) {
- startPowerHintSession(mHintSessionThreadIds);
+ if (mHintSession == nullptr && !mHintSessionThreadIds.empty() && usePowerHintSession()) {
+ auto ret = getPowerHal().createHintSession(getpid(), static_cast<int32_t>(getuid()),
+ mHintSessionThreadIds, mTargetDuration.ns());
+
+ if (ret.isOk()) {
+ mHintSession = ret.value();
+ }
}
- return mHintSessionRunning;
+ return mHintSession != nullptr;
}
void PowerAdvisor::updateTargetWorkDuration(Duration targetDuration) {
@@ -223,15 +229,16 @@
{
mTargetDuration = targetDuration;
if (sTraceHintSessionData) ATRACE_INT64("Time target", targetDuration.ns());
- if (ensurePowerHintSessionRunning() && (targetDuration != mLastTargetDurationSent)) {
+ if (targetDuration == mLastTargetDurationSent) return;
+ std::lock_guard lock(mHintSessionMutex);
+ if (ensurePowerHintSessionRunning()) {
ALOGV("Sending target time: %" PRId64 "ns", targetDuration.ns());
mLastTargetDurationSent = targetDuration;
- std::lock_guard lock(mHintSessionMutex);
auto ret = mHintSession->updateTargetWorkDuration(targetDuration.ns());
if (!ret.isOk()) {
ALOGW("Failed to set power hint target work duration with error: %s",
ret.getDescription().c_str());
- mHintSessionRunning = false;
+ mHintSession = nullptr;
}
}
}
@@ -244,21 +251,12 @@
}
ATRACE_CALL();
std::optional<Duration> actualDuration = estimateWorkDuration();
- if (!actualDuration.has_value() || actualDuration < 0ns || !ensurePowerHintSessionRunning()) {
+ if (!actualDuration.has_value() || actualDuration < 0ns) {
ALOGV("Failed to send actual work duration, skipping");
return;
}
actualDuration = std::make_optional(*actualDuration + sTargetSafetyMargin);
mActualDuration = actualDuration;
- WorkDuration duration;
- duration.workPeriodStartTimestampNanos = mCommitStartTimes[0].ns();
- // TODO(b/284324521): Correctly calculate total duration.
- duration.durationNanos = actualDuration->ns();
- duration.cpuDurationNanos = actualDuration->ns();
- // TODO(b/284324521): Calculate RenderEngine GPU time.
- duration.gpuDurationNanos = 0;
- duration.timeStampNanos = TimePoint::now().ns();
- mHintSessionQueue.push_back(duration);
if (sTraceHintSessionData) {
ATRACE_INT64("Measured duration", actualDuration->ns());
@@ -274,13 +272,34 @@
actualDuration->ns(), mLastTargetDurationSent.ns(),
Duration{*actualDuration - mLastTargetDurationSent}.ns());
+ if (mTimingTestingMode) {
+ mDelayReportActualMutexAcquisitonPromise.get_future().wait();
+ mDelayReportActualMutexAcquisitonPromise = std::promise<bool>{};
+ }
+
{
std::lock_guard lock(mHintSessionMutex);
+ if (!ensurePowerHintSessionRunning()) {
+ ALOGV("Hint session not running and could not be started, skipping");
+ return;
+ }
+
+ WorkDuration duration{
+ .timeStampNanos = TimePoint::now().ns(),
+ // TODO(b/284324521): Correctly calculate total duration.
+ .durationNanos = actualDuration->ns(),
+ .workPeriodStartTimestampNanos = mCommitStartTimes[0].ns(),
+ .cpuDurationNanos = actualDuration->ns(),
+ // TODO(b/284324521): Calculate RenderEngine GPU time.
+ .gpuDurationNanos = 0,
+ };
+ mHintSessionQueue.push_back(duration);
+
auto ret = mHintSession->reportActualWorkDuration(mHintSessionQueue);
if (!ret.isOk()) {
ALOGW("Failed to report actual work durations with error: %s",
ret.getDescription().c_str());
- mHintSessionRunning = false;
+ mHintSession = nullptr;
return;
}
}
@@ -291,7 +310,8 @@
mHintSessionEnabled = enabled;
}
-bool PowerAdvisor::startPowerHintSession(const std::vector<int32_t>& threadIds) {
+bool PowerAdvisor::startPowerHintSession(std::vector<int32_t>&& threadIds) {
+ mHintSessionThreadIds = threadIds;
if (!mBootFinished.load()) {
return false;
}
@@ -299,25 +319,14 @@
ALOGI("Cannot start power hint session: disabled or unsupported");
return false;
}
- if (mHintSessionRunning) {
+ LOG_ALWAYS_FATAL_IF(mHintSessionThreadIds.empty(),
+ "No thread IDs provided to power hint session!");
+ std::lock_guard lock(mHintSessionMutex);
+ if (mHintSession != nullptr) {
ALOGE("Cannot start power hint session: already running");
return false;
}
- LOG_ALWAYS_FATAL_IF(threadIds.empty(), "No thread IDs provided to power hint session!");
- {
- std::lock_guard lock(mHintSessionMutex);
- mHintSession = nullptr;
- mHintSessionThreadIds = threadIds;
-
- auto ret = getPowerHal().createHintSession(getpid(), static_cast<int32_t>(getuid()),
- threadIds, mTargetDuration.ns());
-
- if (ret.isOk()) {
- mHintSessionRunning = true;
- mHintSession = ret.value();
- }
- }
- return mHintSessionRunning;
+ return ensurePowerHintSessionRunning();
}
void PowerAdvisor::setGpuFenceTime(DisplayId displayId, std::unique_ptr<FenceTime>&& fenceTime) {
diff --git a/services/surfaceflinger/DisplayHardware/PowerAdvisor.h b/services/surfaceflinger/DisplayHardware/PowerAdvisor.h
index 05e4c8b..0276e44 100644
--- a/services/surfaceflinger/DisplayHardware/PowerAdvisor.h
+++ b/services/surfaceflinger/DisplayHardware/PowerAdvisor.h
@@ -46,16 +46,15 @@
// Initializes resources that cannot be initialized on construction
virtual void init() = 0;
+ // Used to indicate that power hints can now be reported
virtual void onBootFinished() = 0;
virtual void setExpensiveRenderingExpected(DisplayId displayId, bool expected) = 0;
virtual bool isUsingExpensiveRendering() = 0;
- virtual void notifyCpuLoadUp() = 0;
- virtual void notifyDisplayUpdateImminentAndCpuReset() = 0;
- // Checks both if it supports and if it's enabled
+ // Checks both if it's supported and if it's enabled; this is thread-safe since its values are
+ // set before onBootFinished, which gates all methods that run on threads other than SF main
virtual bool usePowerHintSession() = 0;
virtual bool supportsPowerHintSession() = 0;
- virtual bool ensurePowerHintSessionRunning() = 0;
// Sends a power hint that updates to the target work duration for the frame
virtual void updateTargetWorkDuration(Duration targetDuration) = 0;
// Sends a power hint for the actual known work duration at the end of the frame
@@ -63,7 +62,7 @@
// Sets whether the power hint session is enabled
virtual void enablePowerHintSession(bool enabled) = 0;
// Initializes the power hint session
- virtual bool startPowerHintSession(const std::vector<int32_t>& threadIds) = 0;
+ virtual bool startPowerHintSession(std::vector<int32_t>&& threadIds) = 0;
// Provides PowerAdvisor with a copy of the gpu fence so it can determine the gpu end time
virtual void setGpuFenceTime(DisplayId displayId, std::unique_ptr<FenceTime>&& fenceTime) = 0;
// Reports the start and end times of a hwc validate call this frame for a given display
@@ -94,6 +93,12 @@
virtual void setDisplays(std::vector<DisplayId>& displayIds) = 0;
// Sets the target duration for the entire pipeline including the gpu
virtual void setTotalFrameTargetWorkDuration(Duration targetDuration) = 0;
+
+ // --- The following methods may run on threads besides SF main ---
+ // Send a hint about an upcoming increase in the CPU workload
+ virtual void notifyCpuLoadUp() = 0;
+ // Send a hint about the imminent start of a new CPU workload
+ virtual void notifyDisplayUpdateImminentAndCpuReset() = 0;
};
namespace impl {
@@ -109,16 +114,13 @@
void onBootFinished() override;
void setExpensiveRenderingExpected(DisplayId displayId, bool expected) override;
bool isUsingExpensiveRendering() override { return mNotifiedExpensiveRendering; };
- void notifyCpuLoadUp() override;
- void notifyDisplayUpdateImminentAndCpuReset() override;
bool usePowerHintSession() override;
bool supportsPowerHintSession() override;
- bool ensurePowerHintSessionRunning() override;
void updateTargetWorkDuration(Duration targetDuration) override;
void reportActualWorkDuration() override;
void enablePowerHintSession(bool enabled) override;
- bool startPowerHintSession(const std::vector<int32_t>& threadIds) override;
- void setGpuFenceTime(DisplayId displayId, std::unique_ptr<FenceTime>&& fenceTime);
+ bool startPowerHintSession(std::vector<int32_t>&& threadIds) override;
+ void setGpuFenceTime(DisplayId displayId, std::unique_ptr<FenceTime>&& fenceTime) override;
void setHwcValidateTiming(DisplayId displayId, TimePoint validateStartTime,
TimePoint validateEndTime) override;
void setHwcPresentTiming(DisplayId displayId, TimePoint presentStartTime,
@@ -128,13 +130,16 @@
void setExpectedPresentTime(TimePoint expectedPresentTime) override;
void setSfPresentTiming(TimePoint presentFenceTime, TimePoint presentEndTime) override;
void setHwcPresentDelayedTime(DisplayId displayId, TimePoint earliestFrameStartTime) override;
-
void setFrameDelay(Duration frameDelayDuration) override;
void setCommitStart(TimePoint commitStartTime) override;
void setCompositeEnd(TimePoint compositeEndTime) override;
void setDisplays(std::vector<DisplayId>& displayIds) override;
void setTotalFrameTargetWorkDuration(Duration targetDuration) override;
+ // --- The following methods may run on threads besides SF main ---
+ void notifyCpuLoadUp() override;
+ void notifyDisplayUpdateImminentAndCpuReset() override;
+
private:
friend class PowerAdvisorTest;
@@ -220,6 +225,7 @@
// this normalizes them together and takes the max of the two
Duration combineTimingEstimates(Duration totalDuration, Duration flingerDuration);
+ bool ensurePowerHintSessionRunning() REQUIRES(mHintSessionMutex);
std::unordered_map<DisplayId, DisplayTimingData> mDisplayTimingData;
// Current frame's delay
@@ -242,9 +248,10 @@
// Ensure powerhal connection is initialized
power::PowerHalController& getPowerHal();
+ // These variables are set before mBootFinished and never mutated after, so it's safe to access
+ // from threaded methods.
std::optional<bool> mHintSessionEnabled;
std::optional<bool> mSupportsHintSession;
- bool mHintSessionRunning = false;
std::mutex mHintSessionMutex;
std::shared_ptr<aidl::android::hardware::power::IPowerHintSession> mHintSession
@@ -261,6 +268,11 @@
// The list of thread ids, stored so we can restart the session from this class if needed
std::vector<int32_t> mHintSessionThreadIds;
Duration mLastTargetDurationSent = kDefaultTargetDuration;
+
+ // Used to manage the execution ordering of reportActualWorkDuration for concurrency testing
+ std::promise<bool> mDelayReportActualMutexAcquisitonPromise;
+ bool mTimingTestingMode = false;
+
// Whether we should emit ATRACE_INT data for hint sessions
static const bool sTraceHintSessionData;