Common power-libperfmgr bringup

[rayavanindra]

Summary by CodeRabbit

• New Features

  • Introduced a power management service with AIDL integration for better performance and efficiency.
  • Added dynamic performance controls based on user interactions and application needs.
  • Implemented session management for power hints to optimize power usage.
  • Added new methods for managing power modes and boosts.

• Bug Fixes

  • Improved stability and responsiveness of power management with refined error handling.

• Documentation

  • Added manifest and configuration files for power HAL service, enhancing system integration.

• Chores

  • Created necessary files and classes to support the new power management functionalities.

[coderabbitai]

Walkthrough

The changes introduce a comprehensive power management framework in the LineageOS Android project. New classes and configurations enhance the hardware abstraction layer (HAL) for power services, allowing for dynamic performance tuning based on user interaction and system states. The structure facilitates better management of power modes, hints, and sessions, optimizing resource usage while ensuring responsiveness and efficiency in device performance.

Changes

Files	Change Summary
power-libperfmgr/Android.bp	Introduced build configuration for android.hardware.power-service.lineage-libperfmgr and libperfmgr-ext. Defined binary and static library properties, including source files and dependencies.
power-libperfmgr/aidl/InteractionHandler.*	Added InteractionHandler class to manage performance locks based on user interactions, implementing a state machine for dynamic control.
power-libperfmgr/aidl/Power.*	Implemented Power class for managing power modes and boosts, integrating with HintManager for performance hints.
power-libperfmgr/aidl/PowerExt.*	Introduced PowerExt class for extended power management functionalities, allowing device-specific mode and boost manipulation.
power-libperfmgr/aidl/PowerHintSession.*	Implemented PowerHintSession for managing CPU performance through power hints with PID control logic.
power-libperfmgr/aidl/PowerSessionManager.*	Developed PowerSessionManager to manage power hint sessions and associated threads, ensuring real-time adjustments to power management strategies.
power-libperfmgr/aidl/*.{xml,rc,cpp,h}	Added configuration and manifest files for establishing the power HAL service and defining its lifecycle and properties.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant InteractionHandler
    participant Power
    participant PowerSessionManager
    participant HintManager

    User->>InteractionHandler: Request performance lock
    InteractionHandler->>Power: Acquire lock
    Power->>HintManager: Apply performance hint
    PowerSessionManager->>Power: Update session state
    Note right of Power: Dynamic adjustments based on user interactions

Loading

In a world of circuits and light,
A rabbit hops with all its might.
Performance hints and locks anew,
Power dances, swift and true.
With every tweak, the systems hum,
Optimized joy, oh, how we run! 🐇✨

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 30

Outside diff range, codebase verification and nitpick comments (9)

power-libperfmgr/aidl/PowerModeExtension.cpp (1)

17-17: Include guard or pragma once.

Consider adding an include guard or #pragma once to prevent potential multiple inclusion issues.

#pragma once

power-libperfmgr/aidl/service.cpp (1)

17-17: Consider using a more descriptive log tag.

The log tag powerhal-libperfmgr is descriptive but could be more specific to the functionality.

power-libperfmgr/aidl/PowerExt.cpp (1)

17-17: Consider using a more descriptive log tag.

The log tag android.hardware.power-service.lineage.ext-libperfmgr is descriptive but could be more specific to the functionality.

power-libperfmgr/aidl/PowerSessionManager.h (5)

Line range hint 116-120:
Add error handling for the session parameter.

Consider adding error handling to check if the session parameter is valid before using it.

if (!session) {
    ALOGW("PowerSessionManager::removePowerSession: session is null");
    return;
}

---

Line range hint 154-157:
Add error handling for the session parameter.

Consider adding error handling to check if the session parameter is valid before using it.

if (!session) {
    ALOGW("PowerSessionManager::setUclampMin: session is null");
    return;
}

---

Line range hint 172-189:
Simplify the return type.

Consider returning a boolean value directly instead of using std::optional<bool>.

bool PowerSessionManager::isAnyAppSessionActive() {
    std::lock_guard<std::mutex> guard(mLock);
    for (PowerHintSession *s : mSessions) {
        if (s->isActive() && !s->isTimeout() && s->isAppSession()) {
            return true;
        }
    }
    return false;
}

---

Line range hint 191-201:
Add logging for debugging purposes.

Consider adding logging to help debug the message handling process.

void PowerSessionManager::handleMessage(const Message &message) {
    ALOGV("PowerSessionManager::handleMessage: received message");
    auto active = isAnyAppSessionActive();
    if (!active.has_value()) {
        return;
    }
    if (active.value()) {
        disableSystemTopAppBoost();
    } else {
        enableSystemTopAppBoost();
    }
}

---

Line range hint 203-223:
Add error handling for the file descriptor.

Consider adding error handling to check if the file descriptor is valid before using it.

if (fd < 0) {
    ALOGE("PowerSessionManager::dumpToFd: invalid file descriptor");
    return;
}

power-libperfmgr/aidl/InteractionHandler.cpp (1)

145-191: Consider adding more detailed logging.

The Acquire function performs several checks and operations. Adding more detailed logging can help with debugging and understanding the flow of execution.

void InteractionHandler::Acquire(int32_t duration) {
    ATRACE_CALL();

    std::lock_guard<std::mutex> lk(mLock);

    int inputDuration = duration + kDurationOffsetMs;
    int finalDuration;
    if (inputDuration > kMaxDurationMs)
        finalDuration = kMaxDurationMs;
    else if (inputDuration > kMinDurationMs)
        finalDuration = inputDuration;
    else
        finalDuration = kMinDurationMs;

    ALOGV("%s: input: %d, offset: %d, final duration: %d", __func__, duration, kDurationOffsetMs, finalDuration);

    // Fallback to do boost directly
    // 1) override property is set OR
    // 2) InteractionHandler not initialized
    if (!kDisplayIdleSupport || mState == INTERACTION_STATE_UNINITIALIZED) {
        HintManager::GetInstance()->DoHint("INTERACTION", std::chrono::milliseconds(finalDuration));
        return;
    }

    struct timespec cur_timespec;
    clock_gettime(CLOCK_MONOTONIC, &cur_timespec);
    if (mState != INTERACTION_STATE_IDLE && finalDuration <= mDurationMs) {
        size_t elapsed_time = CalcTimespecDiffMs(mLastTimespec, cur_timespec);
        // don't hint if previous hint's duration covers this hint's duration
        if (elapsed_time <= (mDurationMs - finalDuration)) {
            ALOGV("%s: Previous duration (%d) cover this (%d) elapsed: %lld", __func__,
                  static_cast<int>(mDurationMs), static_cast<int>(finalDuration),
                  static_cast<long long>(elapsed_time));
            return;
        }
    }
    mLastTimespec = cur_timespec;
    mDurationMs = finalDuration;

    ALOGV("%s: input: %d final duration: %d", __func__, duration, finalDuration);

    if (mState == INTERACTION_STATE_WAITING)
        AbortWaitLocked();
    else if (mState == INTERACTION_STATE_IDLE)
        PerfLock();

    mState = INTERACTION_STATE_INTERACTION;
    mCond.notify_one();
}

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9f6bf56 and f9a7951.

Files selected for processing (15)

• power-libperfmgr/Android.bp (1 hunks)
• power-libperfmgr/aidl/InteractionHandler.cpp (1 hunks)
• power-libperfmgr/aidl/InteractionHandler.h (1 hunks)
• power-libperfmgr/aidl/Power.cpp (1 hunks)
• power-libperfmgr/aidl/Power.h (1 hunks)
• power-libperfmgr/aidl/PowerExt.cpp (1 hunks)
• power-libperfmgr/aidl/PowerExt.h (1 hunks)
• power-libperfmgr/aidl/PowerHintSession.cpp (1 hunks)
• power-libperfmgr/aidl/PowerHintSession.h (1 hunks)
• power-libperfmgr/aidl/PowerModeExtension.cpp (1 hunks)
• power-libperfmgr/aidl/PowerSessionManager.cpp (1 hunks)
• power-libperfmgr/aidl/PowerSessionManager.h (1 hunks)
• power-libperfmgr/aidl/android.hardware.power-service.lineage-libperfmgr.rc (1 hunks)
• power-libperfmgr/aidl/android.hardware.power-service.lineage.xml (1 hunks)
• power-libperfmgr/aidl/service.cpp (1 hunks)

Additional comments not posted (45)

power-libperfmgr/aidl/android.hardware.power-service.lineage.xml (1)

1-7: Ensure XML manifest correctness.

The XML manifest appears to be correctly formatted and specifies the HAL for android.hardware.power with version 4. Ensure this aligns with the rest of the project requirements and configurations.

power-libperfmgr/aidl/PowerModeExtension.cpp (2)

1-15: Ensure license compliance.

The license header is correctly included and follows the Apache License, Version 2.0. Ensure compliance with the license terms.

---

19-45: Namespace and function implementations look good.

The namespace structure is well-defined, and the functions isDeviceSpecificModeSupported and setDeviceSpecificMode are correctly stubbed out. Ensure these functions are correctly integrated and implemented as needed.

power-libperfmgr/aidl/android.hardware.power-service.lineage-libperfmgr.rc (6)

1-5: Service definition looks good.

The service definition for vendor.power-hal-aidl is correctly specified with appropriate class, user, group, and priority settings.

---

7-9: Ensure service starts correctly on late-fs.

The on late-fs trigger to start the service is correctly specified. Ensure this aligns with the overall system initialization sequence.

---

10-16: Handle framework restart correctly.

The script correctly handles the restart of the powerHAL service when the framework restarts. Ensure this logic is tested thoroughly.

---

17-25: Ensure property handling for dalvik.vm properties.

The script handles dalvik.vm properties correctly. Ensure these properties are set and restored as intended.

---

26-29: Debug property handling looks good.

The script correctly handles the restart of the powerHAL service when debug properties are set. Ensure this logic is tested thoroughly.

---

30-32: Persisted debug property handling looks good.

The script correctly handles persisted debug properties. Ensure this logic is tested thoroughly.

power-libperfmgr/Android.bp (3)

7-12: Namespaces and imports look good.

The soong_namespace section correctly specifies the namespaces and imports.

---

14-47: Binary configuration looks good.

The cc_binary section correctly specifies the binary configuration, including dependencies, source files, and other settings.

---

49-58: Static library configuration looks good.

The cc_library_static section correctly specifies the static library configuration, including dependencies and source files.

power-libperfmgr/aidl/PowerExt.h (3)

1-25: File header and includes look good.

The file header contains correct licensing information, and the necessary headers are included.

---

26-39: Class definition and public methods look good.

The class PowerExt extends BnPowerExt and provides methods for setting and checking power modes and boosts. The methods are correctly defined.

---

41-42: Private section is correctly defined.

The private section of the class PowerExt is currently empty but correctly defined.

power-libperfmgr/aidl/InteractionHandler.h (4)

1-24: File header and includes look good.

The file header contains correct licensing information, and the necessary headers are included.

---

32-37: Enum definition looks good.

The enum InteractionState correctly defines various states for interaction management.

---

39-46: Class definition and public methods look good.

The class InteractionHandler provides methods for initializing, exiting, acquiring, and releasing interactions. The methods are correctly defined.

---

48-64: Private section looks good.

The private section of the class InteractionHandler contains private methods and member variables for managing interactions. The methods and variables are correctly defined.

power-libperfmgr/aidl/Power.h (3)

1-15: Ensure compliance with the Apache License.

The file includes the Apache License header, which is good practice.

---

19-25: Ensure all necessary headers are included.

The file includes several headers, which seem appropriate for the functionality.

---

38-55: Class Power implementation looks good.

The class Power extends BnPower and implements several methods related to power management.

power-libperfmgr/aidl/service.cpp (3)

1-15: Ensure compliance with the Apache License.

The file includes the Apache License header, which is good practice.

---

19-31: Ensure all necessary headers are included.

The file includes several headers, which seem appropriate for the functionality.

---

40-83: Main function implementation looks good.

The main function initializes the power HAL service, registers the service, and starts the necessary components.

power-libperfmgr/aidl/PowerExt.cpp (3)

1-15: Ensure compliance with the Apache License.

The file includes the Apache License header, which is good practice.

---

19-31: Ensure all necessary headers are included.

The file includes several headers, which seem appropriate for the functionality.

---

33-87: Class PowerExt implementation looks good.

The class PowerExt extends functionality for power management and implements several methods.

power-libperfmgr/aidl/PowerSessionManager.h (2)

100-104: Clarify the purpose of the method.

The method logs the boost and duration but does not perform any action. Clarify if this is intentional or if additional logic is required.

---

106-108: LGTM!

The method is simple and correctly implemented.

power-libperfmgr/aidl/PowerHintSession.h (1)

84-84: LGTM!

The method is simple and correctly implemented.

power-libperfmgr/aidl/PowerSessionManager.cpp (2)

100-104: Clarify the purpose of the method.

The method logs the boost and duration but does not perform any action. Clarify if this is intentional or if additional logic is required.

---

106-108: LGTM!

The method is simple and correctly implemented.

power-libperfmgr/aidl/InteractionHandler.cpp (3)

49-50: Ensure the property key is correct.

The property key vendor.powerhal.disp.idle_support is used to determine display idle support. Verify that this key is correct and exists in the system properties.

---

51-52: Confirm the validity of file paths.

The file paths /sys/class/drm/card0/device/idle_state and /sys/class/graphics/fb0/idle_state are used to check the display idle state. Ensure these paths are correct and accessible on the target devices.

---

53-60: Verify property keys and default values.

The property keys vendor.powerhal.disp.idle_wait, vendor.powerhal.interaction.min, vendor.powerhal.interaction.max, and vendor.powerhal.interaction.offset are used to configure various parameters. Ensure these keys are correct and the default values are appropriate.

power-libperfmgr/aidl/Power.cpp (5)

129-137: LGTM!

The isModeSupported method correctly checks for mode support.

---

177-181: LGTM!

The isBoostSupported method correctly checks for boost support.

---

188-201: LGTM!

The dump method correctly writes the current state to a file descriptor.

---

204-221: LGTM!

The createHintSession method correctly creates a new power hint session.

---

224-232: LGTM!

The getHintSessionPreferredRate method correctly retrieves the preferred rate for hint sessions.

power-libperfmgr/aidl/PowerHintSession.cpp (4)

150-152: LGTM!

The traceSessionVal method correctly traces session values.

---

154-157: LGTM!

The isAppSession method correctly checks if the session is an application session.

---

159-171: LGTM!

The updateUniveralBoostMode method correctly updates the universal boost mode.

---

173-179: LGTM!

The tryToSendPowerHint method correctly sends power hints.

[coderabbitai]

Consider using #ifndef guards instead of #pragma once.

While #pragma once is widely supported, using #ifndef guards can improve portability.

#ifndef AIDL_GOOGLE_HARDWARE_POWER_IMPL_PIXEL_POWER_H
#define AIDL_GOOGLE_HARDWARE_POWER_IMPL_PIXEL_POWER_H

// ... code ...

#endif // AIDL_GOOGLE_HARDWARE_POWER_IMPL_PIXEL_POWER_H

[coderabbitai]

Validate the mode string.

Consider adding validation for the mode string to ensure it matches expected values.

if (mode.empty()) {
    ALOGW("PowerSessionManager::updateHintMode: mode is empty");
    return;
}

[coderabbitai]

Add error handling for the session parameter.

Consider adding error handling to check if the session parameter is valid before using it.

if (!session) {
    ALOGW("PowerSessionManager::addPowerSession: session is null");
    return;
}

[coderabbitai]

Add validation for the input parameters.

Consider adding validation to check if the input parameters are valid before using them.

if (tgid < 0 || uid < 0) {
    ALOGW("AppHintDesc: invalid tgid or uid");
    return;
}

[coderabbitai]

Add validation for the input parameters.

Consider adding validation to check if the input parameters are valid before using them.

if (tgid < 0 || uid < 0) {
    ALOGW("PowerHintSession: invalid tgid or uid");
    return;
}

[coderabbitai]

Add error handling for HintManager methods.

The HintManager::DoHint and HintManager::EndHint methods might fail, and this is not currently handled.

if (enabled) {
    if (!HintManager::GetInstance()->DoHint(toString(type))) {
        LOG(ERROR) << "Failed to apply hint for mode: " << toString(type);
        // Handle the error appropriately
    }
} else {
    if (!HintManager::GetInstance()->EndHint(toString(type))) {
        LOG(ERROR) << "Failed to end hint for mode: " << toString(type);
        // Handle the error appropriately
    }
}

---

Add error handling for HintManager methods.

The HintManager::DoHint and HintManager::EndHint methods might fail, and this is not currently handled.

if (enabled) {
    if (!HintManager::GetInstance()->DoHint(toString(type))) {
        LOG(ERROR) << "Failed to apply hint for mode: " << toString(type);
        // Handle the error appropriately
    }
} else {
    if (!HintManager::GetInstance()->EndHint(toString(type))) {
        LOG(ERROR) << "Failed to end hint for mode: " << toString(type);
        // Handle the error appropriately
    }
}

[coderabbitai]

Add error handling for HintManager methods.

The HintManager::DoHint and HintManager::EndHint methods might fail, and this is not currently handled.

if (durationMs > 0) {
    if (!HintManager::GetInstance()->DoHint(toString(type), std::chrono::milliseconds(durationMs))) {
        LOG(ERROR) << "Failed to apply hint for boost: " << toString(type);
        // Handle the error appropriately
    }
} else if (durationMs == 0) {
    if (!HintManager::GetInstance()->DoHint(toString(type))) {
        LOG(ERROR) << "Failed to apply hint for boost: " << toString(type);
        // Handle the error appropriately
    }
} else {
    if (!HintManager::GetInstance()->EndHint(toString(type))) {
        LOG(ERROR) << "Failed to end hint for boost: " << toString(type);
        // Handle the error appropriately
    }
}

---

Add error handling for HintManager methods.

The HintManager::DoHint and HintManager::EndHint methods might fail, and this is not currently handled.

if (durationMs > 0) {
    if (!HintManager::GetInstance()->DoHint(toString(type), std::chrono::milliseconds(durationMs))) {
        LOG(ERROR) << "Failed to apply hint for boost: " << toString(type);
        // Handle the error appropriately
    }
} else if (durationMs == 0) {
    if (!HintManager::GetInstance()->DoHint(toString(type))) {
        LOG(ERROR) << "Failed to apply hint for boost: " << toString(type);
        // Handle the error appropriately
    }
} else {
    if (!HintManager::GetInstance()->EndHint(toString(type))) {
        LOG(ERROR) << "Failed to end hint for boost: " << toString(type);
        // Handle the error appropriately
    }
}

[coderabbitai]

Add error handling for potential issues in the PID calculation.

The PID control algorithm might encounter issues, such as division by zero or overflow, and this is not currently handled.

try {
    // PID control algorithm
    int64_t error = ns_to_100us(actualDurationNanos - targetDurationNanos);
    if (i >= d_start) {
        derivative_sum += error - previous_error;
    }
    if (i >= p_start) {
        err_sum += error;
    }
    if (i >= i_start) {
        integral_error += error * dt;
        integral_error = std::min(adpfConfig->getPidIHighDivI(), integral_error);
        integral_error = std::max(adpfConfig->getPidILowDivI(), integral_error);
    }
    previous_error = error;
} catch (const std::exception &e) {
    ALOGE("PID calculation error: %s", e.what());
    // Handle the error appropriately
}

[coderabbitai]

Add error handling for potential issues in the initialization.

The initialization might encounter issues, such as failed memory allocation or invalid arguments, and this is not currently handled.

try {
    mDescriptor = new AppHintDesc(tgid, uid, threadIds);
    mDescriptor->duration = std::chrono::nanoseconds(durationNanos);
    mIdString = StringPrintf("%" PRId32 "-%" PRId32 "-%" PRIxPTR, mDescriptor->tgid,
                             mDescriptor->uid, reinterpret_cast<uintptr_t>(this) & 0xffff);
    mPowerManagerHandler = PowerSessionManager::getInstance();
    mLastUpdatedTime.store(std::chrono::steady_clock::now());
    if (ATRACE_ENABLED()) {
        traceSessionVal("target", mDescriptor->duration.count());
        traceSessionVal("active", mDescriptor->is_active.load());
    }
    PowerSessionManager::getInstance()->addPowerSession(this);
    // init boost
    sendHint(SessionHint::CPU_LOAD_RESET);
    ALOGV("PowerHintSession created: %s", mDescriptor->toString().c_str());
} catch (const std::exception &e) {
    ALOGE("Initialization error: %s", e.what());
    // Handle the error appropriately
}

[coderabbitai]

Add error handling for potential issues in the cleanup.

The cleanup might encounter issues, such as failed resource deallocation, and this is not currently handled.

try {
    close();
    ALOGV("PowerHintSession deleted: %s", mDescriptor->toString().c_str());
    if (ATRACE_ENABLED()) {
        traceSessionVal("target", 0);
        traceSessionVal("actl_last", 0);
        traceSessionVal("active", 0);
    }
    delete mDescriptor;
} catch (const std::exception &e) {
    ALOGE("Cleanup error: %s", e.what());
    // Handle the error appropriately
}