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

[Gautham R. Shenoy]

On Sun, Mar 29, 2026 at 03:38:07PM -0500, Mario Limonciello (AMD) wrote:
> Dynamic energy performance preference changes 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.
> 
> When enabled, the driver exposes a sysfs toggle for dynamic EPP, blocks
> manual writes to energy_performance_preference, and keeps the policy in
> performance mode while it "owns" the EPP updates.
> 
> For non-server systems:
>     * the default EPP for AC mode is `performance`.
>     * the default EPP for DC mode is `balance_performance`.
> 
> For server systems dynamic EPP is mostly a no-op.
> 
> Signed-off-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx>
> ---
> v5->v6:
>  * Set the power supply notifier callback before registration
>  * Expand the changelog to cover the sysfs toggle and manual EPP blocking
>  * Add missing kdoc
> ---
>  Documentation/admin-guide/pm/amd-pstate.rst |  18 ++-
>  drivers/cpufreq/Kconfig.x86                 |  12 ++
>  drivers/cpufreq/amd-pstate.c                | 137 ++++++++++++++++++--
>  drivers/cpufreq/amd-pstate.h                |  10 +-
>  4 files changed, 163 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index d6c2f233ab239..0e4355fe13558 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -325,7 +325,7 @@ and user can change current preference according to energy or performance needs
>  Please get all support profiles list from
>  ``energy_performance_available_preferences`` attribute, all the profiles are
>  integer values defined between 0 to 255 when EPP feature is enabled by platform
> -firmware, if EPP feature is disabled, driver will ignore the written value
> +firmware, but if the dynamic EPP feature is enabled, driver will block writes.
>  This attribute is read-write.
>  
>  ``boost``
> @@ -347,6 +347,22 @@ boost or `1` to enable it, for the respective CPU using the sysfs path
>  Other performance and frequency values can be read back from
>  ``/sys/devices/system/cpu/cpuX/acpi_cppc/``, see :ref:`cppc_sysfs`.
>  
> +Dynamic energy performance profile
> +==================================
> +The amd-pstate driver supports dynamically selecting the energy performance
> +profile based on whether the machine is running on AC or DC power.
> +
> +Whether this behavior is enabled by default with the kernel config option
> +`CONFIG_X86_AMD_PSTATE_DYNAMIC_EPP`.

This sentence doesn't read right. Should it be

"Whether this behavior is enabled by default depends on the kernel
config option CONFIG_X86_AMD_PSTATE_DYNAMIC_EPP" ?


> This behavior can also be overridden
> +at runtime by the sysfs file ``/sys/devices/system/cpu/cpufreq/policyX/dynamic_epp``.
> +
> +When set to enabled, the driver will select a different energy performance
> +profile when the machine is running on battery or AC power.
> +When set to disabled, the driver will not change the energy performance profile
> +based on the power source and will not react to user desired power state.
> +
> +Attempting to manually write to the ``energy_performance_preference`` sysfs
> +file will fail when ``dynamic_epp`` is enabled.
>  


[..snip..]

> @@ -1715,22 +1824,20 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	if (amd_pstate_acpi_pm_profile_server() ||
>  	    amd_pstate_acpi_pm_profile_undefined()) {
>  		policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> -		cpudata->epp_default = amd_pstate_get_epp(cpudata);
> +		cpudata->epp_default_ac = cpudata->epp_default_dc = amd_pstate_get_epp(cpudata);
>  	} else {
>  		policy->policy = CPUFREQ_POLICY_POWERSAVE;
> -		cpudata->epp_default = AMD_CPPC_EPP_BALANCE_PERFORMANCE;
> +		cpudata->epp_default_ac = AMD_CPPC_EPP_PERFORMANCE;
> +		cpudata->epp_default_dc = AMD_CPPC_EPP_BALANCE_PERFORMANCE;
>  	}
>  
> -	ret = amd_pstate_set_epp(policy, cpudata->epp_default);
> +	if (dynamic_epp)
> +		ret = amd_pstate_set_dynamic_epp(policy);
> +	else
> +		ret = amd_pstate_set_epp(policy, amd_pstate_get_balanced_epp(policy));
>  	if (ret)
>  		goto free_cpudata1;
>  
> -	ret = amd_pstate_init_floor_perf(policy);
> -	if (ret) {
> -		dev_err(dev, "Failed to initialize Floor Perf (%d)\n", ret);
> -		goto free_cpudata1;
> -	}


Was the removal of amd_pstate_init_floor_perf() intentional? It looks
accidental since the call still exists in amd_pstate_cpu_init().

Before this patch, amd_pstate_epp_cpu_init() called
amd_pstate_init_floor_perf() which reads MSR_AMD_CPPC_REQ2 and
initializes bios_floor_perf, floor_freq, and cppc_req2_cached.
With this call removed these fields stay zero (from kzalloc) on
systems that support X86_FEATURE_CPPC_PERF_PRIO.

The bios_floor_perf is relied upon by amd_pstate_epp_cpu_exit(),
amd_pstate_suspend(), amd_pstate_epp_resume() functions.

Barring these two issues, I am ok with this patch.

-- 
Thanks and Regards
gautham.