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

[Dhananjay Ugwekar]

On 3/19/2025 1:06 AM, Mario Limonciello wrote:
> On 3/12/2025 07:16, Dhananjay Ugwekar wrote:
>> On 3/4/2025 8:53 PM, Mario Limonciello wrote:
>>> From: Mario Limonciello <mario.limonciello@xxxxxxx>
>>>
>>> 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@xxxxxxx>
>>> ---
>>> 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                | 129 ++++++++++++++++++--
>>>   drivers/cpufreq/amd-pstate.h                |   5 +-
>>>   4 files changed, 155 insertions(+), 9 deletions(-)
>>>
>> [Snip]
>>> @@ -1556,6 +1667,10 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>>>       if (!policy->cpuinfo.max_freq)
>>>           return -ENODEV;
>>>   +    /* policy can't be changed to performance policy while dynamic epp is enabled */
>>> +    if (policy->policy == CPUFREQ_POLICY_PERFORMANCE && cpudata->dynamic_epp)
>>> +        return -EBUSY;
>>
>> We might need to tweak this condition, because if we enable "CONFIG_X86_AMD_PSTATE_DYNAMIC_EPP" in config
>> and boot with "amd_pstate=active" it lands here (cpufreq_online()->amd_pstate_epp_set_policy()) driver init fails
>> as the default governor is performance.
>>
> 
> The check is important to make sure that you can't go to performance mode after init.
> 
> I think this is the way I would want to solve it.
> Set policy to powersave before enabling dynamic epp for amd_pstate_epp_cpu_init().
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 824756ac0010e..4a0f561d0e2d1 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1729,8 +1729,10 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>                 WRITE_ONCE(cpudata->cppc_req_cached, value);
>         }
> 
> -       if (dynamic_epp)
> +       if (dynamic_epp) {
> +               policy->policy = CPUFREQ_POLICY_POWERSAVE;
>                 ret = amd_pstate_set_dynamic_epp(policy);
> +       }
>         else
>                 ret = amd_pstate_set_epp(policy, amd_pstate_get_balanced_epp(policy));
			^^^^^^^^^^^^ (mentioned below)
>         if (ret)
> 
> Thoughts?

Yes, this looks good, because anyway there is no point in having performance governor and dynamic 
epp set at the same time.

I found one related quirk though, we are setting performance governor for server platforms in 
amd_pstate_epp_cpu_init() and then setting epp at the line highlighted above. We dont have a 
check in *_set_epp() functions for performance governor. This could alter the performance governor 
behavior if we set a "balanced" epp for it. I haven't tested this part yet. 

Thanks,
Dhananjay

> 
>>> +
>>>       cpudata->policy = policy->policy;
>>>         ret = amd_pstate_epp_update_limit(policy);
>>> diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
>>> index fbe1c08d3f061..6882876f895de 100644
>>> --- a/drivers/cpufreq/amd-pstate.h
>>> +++ b/drivers/cpufreq/amd-pstate.h
>>> @@ -104,7 +104,10 @@ struct amd_cpudata {
>>>       /* EPP feature related attributes*/
>>>       u32    policy;
>>>       bool    suspended;
>>> -    u8    epp_default;
>>> +    u8    epp_default_ac;
>>> +    u8    epp_default_dc;
>>> +    bool    dynamic_epp;
>>> +    struct notifier_block power_nb;
>>>   };
>>>     /*
>>
>