Re: [PATCH 08/15] cpufreq/amd-pstate: store all values in cpudata struct in khz

From: Gautham R. Shenoy
Date: Fri Dec 06 2024 - 01:43:30 EST

Next message: Gautham R. Shenoy: "Re: [PATCH 09/15] cpufreq/amd-pstate: Change amd_pstate_update_perf() to return an int"
Previous message: Greg KH: "Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device"
In reply to: Mario Limonciello: "[PATCH 08/15] cpufreq/amd-pstate: store all values in cpudata struct in khz"
Next in thread: Mario Limonciello: "[PATCH 15/15] cpufreq/amd-pstate: Set different default EPP policy for Epyc and Ryzen"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Dec 05, 2024 at 04:28:40PM -0600, Mario Limonciello wrote:
> Storing values in the cpudata structure in different units leads
> to confusion and hardcoded conversions elsewhere. After ratios are
> calculated store everything in khz for any future use. Adjust all
> relevant consumers for this change as well.
>
> Suggested-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@xxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>

Thanks for cleaning this up.

This also makes the code in-line with the comment for cpudata->nominal_freq which says

* @nominal_freq: the frequency (in khz) that mapped to nominal_perf

Couple of minor nits. See below.

> ---
> drivers/cpufreq/amd-pstate-ut.c | 12 +++++-------
> drivers/cpufreq/amd-pstate.c | 30 +++++++++++++++---------------
> 2 files changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
> index a261d7300951e..3a0a380c3590c 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -207,7 +207,6 @@ static void amd_pstate_ut_check_freq(u32 index)
> int cpu = 0;
> struct cpufreq_policy *policy = NULL;
> struct amd_cpudata *cpudata = NULL;
> - u32 nominal_freq_khz;
>
> for_each_possible_cpu(cpu) {
> policy = cpufreq_cpu_get(cpu);
> @@ -215,14 +214,13 @@ static void amd_pstate_ut_check_freq(u32 index)
> break;
> cpudata = policy->driver_data;
>
> - nominal_freq_khz = cpudata->nominal_freq*1000;
> - if (!((cpudata->max_freq >= nominal_freq_khz) &&
> - (nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
> + if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
> + (cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
> (cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
> (cpudata->min_freq > 0))) {
> amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
> - __func__, cpu, cpudata->max_freq, nominal_freq_khz,
> + __func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
> cpudata->lowest_nonlinear_freq, cpudata->min_freq);
> goto skip_test;
> }
> @@ -236,13 +234,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>
> if (cpudata->boost_supported) {
> if ((policy->max == cpudata->max_freq) ||
> - (policy->max == nominal_freq_khz))
> + (policy->max == cpudata->nominal_freq))
> amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
> else {
> amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
> __func__, cpu, policy->max, cpudata->max_freq,
> - nominal_freq_khz);
> + cpudata->nominal_freq);
> goto skip_test;
> }
> } else {
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ce70d1bfa55d0..464db6745d84e 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -739,8 +739,8 @@ static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
>
> if (on)
> policy->cpuinfo.max_freq = max_freq;
> - else if (policy->cpuinfo.max_freq > nominal_freq * 1000)
> - policy->cpuinfo.max_freq = nominal_freq * 1000;
> + else if (policy->cpuinfo.max_freq > nominal_freq)
> + policy->cpuinfo.max_freq = nominal_freq;
>
> policy->max = policy->cpuinfo.max_freq;
>
> @@ -940,29 +940,29 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> return ret;
>
> if (quirks && quirks->lowest_freq)
> - min_freq = quirks->lowest_freq * 1000;
> + min_freq = quirks->lowest_freq;
> else
> - min_freq = cppc_perf.lowest_freq * 1000;
> + min_freq = cppc_perf.lowest_freq;
>
> if (quirks && quirks->nominal_freq)
> - nominal_freq = quirks->nominal_freq ;
> + nominal_freq = quirks->nominal_freq;
> else
> nominal_freq = cppc_perf.nominal_freq;
>
> nominal_perf = READ_ONCE(cpudata->nominal_perf);
>
> boost_ratio = div_u64(cpudata->highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf);
> - max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> + max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT);
>
> lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
> nominal_perf);
> - lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> + lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT);
>
> - WRITE_ONCE(cpudata->min_freq, min_freq);
> - WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
> - WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
> - WRITE_ONCE(cpudata->max_freq, max_freq);
> + WRITE_ONCE(cpudata->min_freq, min_freq * 1000);
> + WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq * 1000);
> + WRITE_ONCE(cpudata->nominal_freq, nominal_freq * 1000);
> + WRITE_ONCE(cpudata->max_freq, max_freq * 1000);
>
> /**
> * Below values need to be initialized correctly, otherwise driver will fail to load
> @@ -970,15 +970,15 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
> * Check _CPC in ACPI table objects if any values are incorrect
> */
> - if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) {

> + if (min_freq <= 0 || max_freq <= 0 || cpudata->nominal_freq <= 0 || min_freq > max_freq) {
> pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n",
> - min_freq, max_freq, nominal_freq * 1000);
> + min_freq, max_freq, cpudata->nominal_freq);
> return -EINVAL;

We don't need this change. This looks odd since we are using min_freq
and max_freq from local variables, but nominal_freq from cpudata.

We may just as well use the local variables in all the cases since
"cpudata->nominal_freq = nominal_freq * 1000", and thus won't make any
difference to the checks above.

> }
>
> - if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) {
^^^^^^^^^^^^^^^^^^^^

Simply replace this with nominal_freq ?

Because both lowest_nonlinear_freq and nominal_freq are in MHz at this
point.

> + if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > cpudata->nominal_freq) {
> pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n",
> - lowest_nonlinear_freq, min_freq, nominal_freq * 1000);
> + lowest_nonlinear_freq, min_freq, cpudata->nominal_freq);
^^^^^^^^^^^^^^^^^^^^^^

Likewise, this can just be nominal_freq
> return -EINVAL;
> }
>
> --
> 2.43.0
>

Other than that,
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@xxxxxxx>

--
Thanks and Regards
gautham.

Next message: Gautham R. Shenoy: "Re: [PATCH 09/15] cpufreq/amd-pstate: Change amd_pstate_update_perf() to return an int"
Previous message: Greg KH: "Re: [PATCH v3 1/5] rust: miscdevice: Provide accessor to pull out miscdevice::this_device"
In reply to: Mario Limonciello: "[PATCH 08/15] cpufreq/amd-pstate: store all values in cpudata struct in khz"
Next in thread: Mario Limonciello: "[PATCH 15/15] cpufreq/amd-pstate: Set different default EPP policy for Epyc and Ryzen"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]