Re: [PATCH v4 1/5] cpufreq/amd-pstate: Add dynamic energy performance preference

[Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>]

On 3/25/2025 7:04 AM, Mario Limonciello wrote:
> On 3/24/2025 04:58, Dhananjay Ugwekar wrote:
>> On 3/21/2025 7:58 AM, Mario Limonciello wrote:
>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>
>>> Dynamic energy performance preference will change the EPP profile
>>> based on whether the machine is running on AC or DC power.
>>>
>>> A notification chain from the power supply core is used to adjust
>>> EPP values on plug in or plug out events.
>>>
>>> For non-server systems:
>>>      * the default EPP for AC mode is `performance`.
>>>      * the default EPP for DC mode is `balance_performance`.
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>> v3->v4:
>>>   * Handle Kconfig not being set
>>>   * Fix dynamic epp default on server
>>> v2-v3:
>>>   * Fix typo in Kconfig
>>> v1->v2:
>>>   * Change defaults to performance (AC) and balance_performance (DC)
>>>   * Default Kconfig to disabled for now
>>>   * Rebase on latest branch
>>> ---
>>>   Documentation/admin-guide/pm/amd-pstate.rst |  18 ++-
>>>   drivers/cpufreq/Kconfig.x86                 |  12 ++
>>>   drivers/cpufreq/amd-pstate.c                | 135 +++++++++++++++++++-
>>>   drivers/cpufreq/amd-pstate.h                |   5 +-
>>>   4 files changed, 161 insertions(+), 9 deletions(-)
>>>
>> [snip]
>>> @@ -1050,6 +1056,73 @@ static void amd_pstate_cpu_exit(struct cpufreq_policy *policy)
>>>       kfree(cpudata);
>>>   }
>>>   +static int amd_pstate_get_balanced_epp(struct cpufreq_policy *policy)
>>> +{
>>> +    struct amd_cpudata *cpudata = policy->driver_data;
>>> +
>>> +    if (power_supply_is_system_supplied())
>>> +        return cpudata->epp_default_ac;
>>> +    else
>>> +        return cpudata->epp_default_dc;
>>> +}
>>> +
>>> +static int amd_pstate_power_supply_notifier(struct notifier_block *nb,
>>> +                        unsigned long event, void *data)
>>> +{
>>> +    struct amd_cpudata *cpudata = container_of(nb, struct amd_cpudata, power_nb);
>>> +    struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
>>
>> For consistency, we should add "if (!policy)" check I think
>>
>>> +    u8 epp;
>>> +    int ret;
>>> +
>>> +    if (event != PSY_EVENT_PROP_CHANGED)
>>> +        return NOTIFY_OK;
>>> +
>>> +    epp = amd_pstate_get_balanced_epp(policy);
>>> +
>>> +    ret = amd_pstate_set_epp(policy, epp);
>>> +    if (ret)
>>> +        pr_warn("Failed to set CPU %d EPP %u: %d\n", cpudata->cpu, epp, ret);
>>> +
>>> +    return NOTIFY_OK;
>>> +}
>> [snip]
>>> @@ -1364,6 +1444,32 @@ static ssize_t prefcore_show(struct device *dev,
>>>       return sysfs_emit(buf, "%s\n", str_enabled_disabled(amd_pstate_prefcore));
>>>   }
>>>   +static ssize_t dynamic_epp_show(struct device *dev,
>>> +                struct device_attribute *attr, char *buf)
>>> +{
>>> +    return sysfs_emit(buf, "%s\n", str_enabled_disabled(dynamic_epp));
>>> +}
>>> +
>>> +static ssize_t dynamic_epp_store(struct device *a, struct device_attribute *b,
>>> +                 const char *buf, size_t count)
>>> +{
>>> +    bool enabled;
>>> +    int ret;
>>> +
>>> +    ret = kstrtobool(buf, &enabled);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (dynamic_epp == enabled)
>>> +        return -EINVAL;
>>> +
>>> +    /* reinitialize with desired dynamic EPP value */
>>> +    dynamic_epp = enabled;
>>> +    ret = amd_pstate_change_driver_mode(cppc_state);
>>
>> I think implicitly changing the driver mode when we write to dynamic_epp file might lead to some confusions.
> 
> How about only allowing to write dynamic_epp attribute when in active mode already?

Yes, I think we should allow, dynamic_epp only with "active mode + powersave governor". And when user tries 
to enable dynamic_epp in the wrong combination, we should fail, right?

> 
>>
>>> +
>>> +    return ret ? ret : count;
>>> +}
>>> +
>>>   cpufreq_freq_attr_ro(amd_pstate_max_freq);
>>>   cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
>