Re: [PATCH v4 4/5] cpufreq/amd-pstate: Add support for raw EPP writes

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

On 3/21/2025 7:58 AM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> The energy performance preference field of the CPPC request MSR
> supports values from 0 to 255, but the strings only offer 4 values.
> 
> The other values are useful for tuning the performance of some
> workloads.
> 
> Add support for writing the raw energy performance preference value
> to the sysfs file.  If the last value written was an integer then
> an integer will be returned.  If the last value written was a string
> then a string will be returned.

LGTM

Reviewed-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  Documentation/admin-guide/pm/amd-pstate.rst | 16 +++++++++++-----
>  drivers/cpufreq/amd-pstate.c                | 11 +++++++++--
>  drivers/cpufreq/amd-pstate.h                |  1 +
>  3 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index 36950fb6568c0..0e4d2e0aaeff7 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -280,16 +280,22 @@ A list of all the supported EPP preferences that could be used for
>  These profiles represent different hints that are provided
>  to the low-level firmware about the user's desired energy vs efficiency
>  tradeoff.  ``default`` represents the epp value is set by platform
> -firmware. This attribute is read-only.
> +firmware. ``custom`` designates that integer values 0-255 may be written
> +as well.  This attribute is read-only.
>  
>  ``energy_performance_preference``
>  
>  The current energy performance preference can be read from this attribute.
>  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, but if the dynamic EPP feature is enabled, driver will block writes.
> +Coarse named profiles are available in the attribute
> +``energy_performance_available_preferences``.
> +Users can also write individual integer values between 0 to 255.
> +When EPP feature is enabled by platform firmware but if the dynamic EPP feature is
> +enabled, driver will ignore the written value. Lower epp values shift the bias
> +towards improved performance while a higher epp value shifts the bias towards
> +power-savings. The exact impact can change from one platform to the other.
> +If a valid integer was last written, then a number will be returned on future reads.
> +If a valid string was last written then a string will be returned on future reads.
>  This attribute is read-write.
>  
>  ``boost``
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 2a62b12148544..b0de50f390e07 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1318,6 +1318,7 @@ ssize_t store_energy_performance_preference(struct cpufreq_policy *policy,
>  	struct amd_cpudata *cpudata = policy->driver_data;
>  	char str_preference[21];
>  	ssize_t ret;
> +	bool raw_epp = FALSE;
>  	u8 epp;
>  
>  	if (cpudata->dynamic_epp) {
> @@ -1334,6 +1335,7 @@ ssize_t store_energy_performance_preference(struct cpufreq_policy *policy,
>  	 * matches an index in the energy_perf_strings array
>  	 */
>  	ret = kstrtou8(str_preference, 0, &epp);
> +	raw_epp = !ret;
>  	if (ret) {
>  		ret = match_string(energy_perf_strings, -1, str_preference);
>  		if (ret < 0 || ret == EPP_INDEX_CUSTOM)
> @@ -1353,7 +1355,9 @@ ssize_t store_energy_performance_preference(struct cpufreq_policy *policy,
>  	if (ret)
>  		return ret;
>  
> -	return ret ? ret : count;
> +	cpudata->raw_epp = raw_epp;
> +
> +	return count;
>  }
>  EXPORT_SYMBOL_GPL(store_energy_performance_preference);
>  
> @@ -1364,6 +1368,9 @@ ssize_t show_energy_performance_preference(struct cpufreq_policy *policy, char *
>  
>  	epp = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached);
>  
> +	if (cpudata->raw_epp)
> +		return sysfs_emit(buf, "%u\n", epp);
> +
>  	switch (epp) {
>  	case AMD_CPPC_EPP_PERFORMANCE:
>  		preference = EPP_INDEX_PERFORMANCE;
> @@ -1378,7 +1385,7 @@ ssize_t show_energy_performance_preference(struct cpufreq_policy *policy, char *
>  		preference = EPP_INDEX_POWERSAVE;
>  		break;
>  	default:
> -		return sysfs_emit(buf, "%u\n", epp);
> +		return -EINVAL;
>  	}
>  
>  	return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
> diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
> index b4c5374762110..b6be2b8fbffbf 100644
> --- a/drivers/cpufreq/amd-pstate.h
> +++ b/drivers/cpufreq/amd-pstate.h
> @@ -108,6 +108,7 @@ struct amd_cpudata {
>  	u8	epp_default_ac;
>  	u8	epp_default_dc;
>  	bool	dynamic_epp;
> +	bool	raw_epp;
>  	struct notifier_block power_nb;
>  
>  	/* platform profile */