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

[Mario Limonciello <superm1@kernel.org>]

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?

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