Re: [PATCH v4 3/5] cpufreq/amd-pstate: Add support for platform profile class

From: Dhananjay Ugwekar
Date: Mon Mar 24 2025 - 06:31:12 EST

Next message: Jakub Kicinski: "Re: [PATCH net-next v11 09/10] selftest: tap: Add tests for virtio-net ioctls"
Previous message: Aatif Mushtaq/Aatif Mushtaq: "RE: [PATCH 1/3] dmaengine: Add support for 2D DMA operation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 3/21/2025 7:58 AM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@xxxxxxx>
>
> The platform profile core allows multiple drivers and devices to
> register platform profile support.
>
> When the legacy platform profile interface is used all drivers will
> adjust the platform profile as well.
>
> Add support for registering every CPU with the platform profile handler
> when dynamic EPP is enabled.
>
> The end result will be that changing the platform profile will modify
> EPP accordingly.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> Documentation/admin-guide/pm/amd-pstate.rst | 4 +-
> drivers/cpufreq/Kconfig.x86 | 1 +
> drivers/cpufreq/amd-pstate.c | 142 +++++++++++++++++---
> drivers/cpufreq/amd-pstate.h | 10 ++
> 4 files changed, 140 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index 8424e7119dd7e..36950fb6568c0 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -321,7 +321,9 @@ Whether this behavior is enabled by default with the kernel config option
> 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.
> +profile when the machine is running on battery or AC power. The driver will
> +also register with the platform profile handler to receive notifications of
> +user desired power state and react to those.
> 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.
>
> diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
> index 3a8bdc35f488a..8fc8319861bdf 100644
> --- a/drivers/cpufreq/Kconfig.x86
> +++ b/drivers/cpufreq/Kconfig.x86
> @@ -40,6 +40,7 @@ config X86_AMD_PSTATE
> select ACPI_PROCESSOR
> select ACPI_CPPC_LIB if X86_64
> select CPU_FREQ_GOV_SCHEDUTIL if SMP
> + select ACPI_PLATFORM_PROFILE
> help
> This driver adds a CPUFreq driver which utilizes a fine grain
> processor performance frequency control range instead of legacy
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 8172bd4b5952f..2a62b12148544 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -109,6 +109,7 @@ static struct quirk_entry *quirks;
> * 2 balance_performance
> * 3 balance_power
> * 4 power
> + * 5 custom (for raw EPP values)

Should we move these "custom" changes to the raw EPP patch ?

> */
> enum energy_perf_value_index {
> EPP_INDEX_DEFAULT = 0,
> @@ -116,6 +117,7 @@ enum energy_perf_value_index {
> EPP_INDEX_BALANCE_PERFORMANCE,
> EPP_INDEX_BALANCE_POWERSAVE,
> EPP_INDEX_POWERSAVE,
> + EPP_INDEX_CUSTOM,
> };
>
> static const char * const energy_perf_strings[] = {
> @@ -124,6 +126,7 @@ static const char * const energy_perf_strings[] = {
> [EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
> [EPP_INDEX_BALANCE_POWERSAVE] = "balance_power",
> [EPP_INDEX_POWERSAVE] = "power",
> + [EPP_INDEX_CUSTOM] = "custom",
> NULL
> };
>
> @@ -1077,6 +1080,10 @@ static int amd_pstate_power_supply_notifier(struct notifier_block *nb,
> if (event != PSY_EVENT_PROP_CHANGED)
> return NOTIFY_OK;
>
> + /* dynamic actions are only applied while platform profile is in balanced */
> + if (cpudata->current_profile != PLATFORM_PROFILE_BALANCED)
> + return 0;
> +
> epp = amd_pstate_get_balanced_epp(policy);
>
> ret = amd_pstate_set_epp(policy, epp);
> @@ -1085,14 +1092,84 @@ static int amd_pstate_power_supply_notifier(struct notifier_block *nb,
>
> return NOTIFY_OK;
> }
> -static void amd_pstate_clear_dynamic_epp(struct cpufreq_policy *policy)
> +
> +static int amd_pstate_profile_probe(void *drvdata, unsigned long *choices)
> +{
> + set_bit(PLATFORM_PROFILE_LOW_POWER, choices);
> + set_bit(PLATFORM_PROFILE_BALANCED, choices);
> + set_bit(PLATFORM_PROFILE_PERFORMANCE, choices);
> +
> + return 0;
> +}
> +
> +static int amd_pstate_profile_get(struct device *dev,
> + enum platform_profile_option *profile)
> +{
> + struct amd_cpudata *cpudata = dev_get_drvdata(dev);
> +
> + *profile = cpudata->current_profile;
> +
> + return 0;
> +}
> +
> +static int amd_pstate_profile_set(struct device *dev,
> + enum platform_profile_option profile)
> +{
> + struct amd_cpudata *cpudata = dev_get_drvdata(dev);
> + struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);

We should add "if (!policy)" check here?

> + int ret;
> +
> + switch (profile) {
> + case PLATFORM_PROFILE_LOW_POWER:
> + if (cpudata->policy != CPUFREQ_POLICY_POWERSAVE)
> + cpudata->policy = CPUFREQ_POLICY_POWERSAVE;
> + ret = amd_pstate_set_epp(policy, AMD_CPPC_EPP_POWERSAVE);
> + if (ret)
> + return ret;
> + break;
> + case PLATFORM_PROFILE_BALANCED:
> + if (cpudata->policy != CPUFREQ_POLICY_POWERSAVE)
> + cpudata->policy = CPUFREQ_POLICY_POWERSAVE;
> + ret = amd_pstate_set_epp(policy,
> + amd_pstate_get_balanced_epp(policy));
> + if (ret)
> + return ret;
> + break;
> + case PLATFORM_PROFILE_PERFORMANCE:
> + ret = amd_pstate_set_epp(policy, AMD_CPPC_EPP_PERFORMANCE);
> + if (ret)
> + return ret;
> + break;
> + default:
> + pr_err("Unknown Platform Profile %d\n", profile);
> + return -EOPNOTSUPP;
> + }
> +
> + cpudata->current_profile = profile;
> +
> + return 0;
> +}
> +
[snip]
> @@ -1228,16 +1329,22 @@ static ssize_t store_energy_performance_preference(
> if (ret != 1)
> return -EINVAL;
>
> - ret = match_string(energy_perf_strings, -1, str_preference);
> - if (ret < 0)
> - return -EINVAL;
> -
> - if (ret)
> - epp = epp_values[ret];
> - else
> - epp = amd_pstate_get_balanced_epp(policy);
> + /*
> + * if the value matches a number, use that, otherwise see if
> + * matches an index in the energy_perf_strings array
> + */
> + ret = kstrtou8(str_preference, 0, &epp);
> + if (ret) {
> + ret = match_string(energy_perf_strings, -1, str_preference);
> + if (ret < 0 || ret == EPP_INDEX_CUSTOM)
> + return -EINVAL;
> + if (ret)
> + epp = epp_values[ret];
> + else
> + epp = amd_pstate_get_balanced_epp(policy);
> + }
>
> - if (epp > 0 && policy->policy == CPUFREQ_POLICY_PERFORMANCE) {
> + if (epp > 0 && cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
^^^^^^^ this change wont make any difference right?

> pr_debug("EPP cannot be set under performance policy\n");
> return -EBUSY;
> }
> @@ -1248,9 +1355,9 @@ static ssize_t store_energy_performance_preference(
>
> return ret ? ret : count;
> }
> +EXPORT_SYMBOL_GPL(store_energy_performance_preference);
>
> -static ssize_t show_energy_performance_preference(
> - struct cpufreq_policy *policy, char *buf)
> +ssize_t show_energy_performance_preference(struct cpufreq_policy *policy, char *buf)
> {
> struct amd_cpudata *cpudata = policy->driver_data;
> u8 preference, epp;
> @@ -1271,11 +1378,12 @@ static ssize_t show_energy_performance_preference(
> preference = EPP_INDEX_POWERSAVE;
> break;
> default:
> - return -EINVAL;
> + return sysfs_emit(buf, "%u\n", epp);
> }
>
> return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
> }
> +EXPORT_SYMBOL_GPL(show_energy_performance_preference);
>
> static void amd_pstate_driver_cleanup(void)
> {
> @@ -1599,10 +1707,12 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
> amd_pstate_acpi_pm_profile_undefined()) {
> policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> cpudata->epp_default_ac = cpudata->epp_default_dc = amd_pstate_get_epp(cpudata);

Why dont we just set the defaults to "AMD_CPPC_EPP_BALANCE_PERFORMANCE" for server systems, so
that the governor, profile and the EPP values are aligned at initialization. Any historical
context for using amd_pstate_get_epp() to init the default epp value?, do we want to preserve
the initial value set by the SMU?

> + cpudata->current_profile = PLATFORM_PROFILE_PERFORMANCE;
> } else {
> policy->policy = CPUFREQ_POLICY_POWERSAVE;
> cpudata->epp_default_ac = AMD_CPPC_EPP_PERFORMANCE;
> cpudata->epp_default_dc = AMD_CPPC_EPP_BALANCE_PERFORMANCE;
> + cpudata->current_profile = PLATFORM_PROFILE_BALANCED;
> }
>
> if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
> index 6882876f895de..b4c5374762110 100644
> --- a/drivers/cpufreq/amd-pstate.h
> +++ b/drivers/cpufreq/amd-pstate.h
> @@ -9,6 +9,7 @@
> #define _LINUX_AMD_PSTATE_H
>
> #include <linux/pm_qos.h>
> +#include <linux/platform_profile.h>
>
> /*********************************************************************
> * AMD P-state INTERFACE *
> @@ -108,6 +109,11 @@ struct amd_cpudata {
> u8 epp_default_dc;
> bool dynamic_epp;
> struct notifier_block power_nb;
> +
> + /* platform profile */
> + enum platform_profile_option current_profile;
> + struct device *ppdev;
> + char *profile_name;
> };
>
> /*
> @@ -123,5 +129,9 @@ enum amd_pstate_mode {
> };
> const char *amd_pstate_get_mode_string(enum amd_pstate_mode mode);
> int amd_pstate_update_status(const char *buf, size_t size);
> +ssize_t store_energy_performance_preference(struct cpufreq_policy *policy,
> + const char *buf, size_t count);
> +ssize_t show_energy_performance_preference(struct cpufreq_policy *policy, char *buf);
> +void amd_pstate_clear_dynamic_epp(struct cpufreq_policy *policy);
>
> #endif /* _LINUX_AMD_PSTATE_H */

Next message: Jakub Kicinski: "Re: [PATCH net-next v11 09/10] selftest: tap: Add tests for virtio-net ioctls"
Previous message: Aatif Mushtaq/Aatif Mushtaq: "RE: [PATCH 1/3] dmaengine: Add support for 2D DMA operation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]