[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <784b2e21-10fb-44d8-b874-b6bc2ee238c6@kernel.org>
Date: Mon, 24 Mar 2025 20:34:16 -0500
From: Mario Limonciello <superm1@...nel.org>
To: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>,
"Gautham R . Shenoy" <gautham.shenoy@....com>,
Perry Yuan <perry.yuan@....com>
Cc: "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
<linux-kernel@...r.kernel.org>,
"open list:CPU FREQUENCY SCALING FRAMEWORK" <linux-pm@...r.kernel.org>,
Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v4 1/5] cpufreq/amd-pstate: Add dynamic energy performance
preference
On 3/24/2025 04:58, Dhananjay Ugwekar wrote:
> On 3/21/2025 7:58 AM, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@....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@....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);
Powered by blists - more mailing lists