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

[Mario Limonciello]

On 3/24/2025 04:58, Dhananjay Ugwekar wrote:
> On 3/21/2025 7:58 AM, 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>
> > ---
> > 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);