| Message ID | 20241012174519.897-1-mario.limonciello@amd.com |
|---|---|
| State | Accepted |
| Commit | 18d9b52271213890da295a7c63ef8880ed570cd8 |
| Headers | show |
| Series | [1/4] cpufreq/amd-pstate: Use nominal perf for limits when boost is disabled | expand |
Hello Mario, On 10/12/2024 11:15 PM, Mario Limonciello wrote: > The EPP value doesn't need to be cached to the CPPC request in > amd_pstate_epp_update_limit() because it's passed as an argument > at the end to amd_pstate_set_epp() and stored at that time. > Tested this on an AMD Zen4 EPYC server system, ran some sanity tests, both modes (active and passive) seem to be working fine with the boost disabled and enabled. You may add, Tested-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com> Regards, Dhananjay > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 8d2541f2c74b..90868c8b214e 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -1528,12 +1528,6 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy) > if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) > epp = 0; > > - /* Set initial EPP value */ > - if (cpu_feature_enabled(X86_FEATURE_CPPC)) { > - value &= ~GENMASK_ULL(31, 24); > - value |= (u64)epp << 24; > - } > - > WRITE_ONCE(cpudata->cppc_req_cached, value); > return amd_pstate_set_epp(cpudata, epp); > }
Hello Mario, On Sat, Oct 12, 2024 at 12:45:16PM -0500, Mario Limonciello wrote: > When boost has been disabled the limit for perf should be nominal perf not > the highest perf. Using the latter to do calculations will lead to > incorrect values that are still above nominal. > > Fixes: ad4caad58d91 ("cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()") > Reported-by: Peter Jung <ptr1337@cachyos.org> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219348 > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> The patch looks good to me. Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> -- Thanks and Regards gautham. > --- > drivers/cpufreq/amd-pstate.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 30415c30d8b4..dfa9a146769b 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -536,11 +536,16 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy) > > static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy) > { > - u32 max_limit_perf, min_limit_perf, lowest_perf; > + u32 max_limit_perf, min_limit_perf, lowest_perf, max_perf; > struct amd_cpudata *cpudata = policy->driver_data; > > - max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata->max_freq); > - min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata->max_freq); > + if (cpudata->boost_supported && !policy->boost_enabled) > + max_perf = READ_ONCE(cpudata->nominal_perf); > + else > + max_perf = READ_ONCE(cpudata->highest_perf); > + > + max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq); > + min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq); > > lowest_perf = READ_ONCE(cpudata->lowest_perf); > if (min_limit_perf < lowest_perf) > @@ -1506,10 +1511,13 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy) > u64 value; > s16 epp; > > - max_perf = READ_ONCE(cpudata->highest_perf); > + if (cpudata->boost_supported && !policy->boost_enabled) > + max_perf = READ_ONCE(cpudata->nominal_perf); > + else > + max_perf = READ_ONCE(cpudata->highest_perf); > min_perf = READ_ONCE(cpudata->lowest_perf); > - max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata->max_freq); > - min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata->max_freq); > + max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq); > + min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq); > > if (min_limit_perf < min_perf) > min_limit_perf = min_perf; > -- > 2.43.0 >
[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Limonciello, Mario <Mario.Limonciello@amd.com> > Sent: Sunday, October 13, 2024 1:45 AM > To: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com> > Cc: Yuan, Perry <Perry.Yuan@amd.com>; linux-kernel@vger.kernel.org; linux- > pm@vger.kernel.org; Ugwekar, Dhananjay <Dhananjay.Ugwekar@amd.com>; > Limonciello, Mario <Mario.Limonciello@amd.com>; Peter Jung > <ptr1337@cachyos.org> > Subject: [PATCH 1/4] cpufreq/amd-pstate: Use nominal perf for limits when boost is > disabled > > When boost has been disabled the limit for perf should be nominal perf not the > highest perf. Using the latter to do calculations will lead to incorrect values that are > still above nominal. > > Fixes: ad4caad58d91 ("cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() > into amd_get_boost_ratio_numerator()") > Reported-by: Peter Jung <ptr1337@cachyos.org> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219348 > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index > 30415c30d8b4..dfa9a146769b 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -536,11 +536,16 @@ static int amd_pstate_verify(struct cpufreq_policy_data > *policy) > > static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy) { > - u32 max_limit_perf, min_limit_perf, lowest_perf; > + u32 max_limit_perf, min_limit_perf, lowest_perf, max_perf; > struct amd_cpudata *cpudata = policy->driver_data; > > - max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata- > >max_freq); > - min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata- > >max_freq); > + if (cpudata->boost_supported && !policy->boost_enabled) > + max_perf = READ_ONCE(cpudata->nominal_perf); > + else > + max_perf = READ_ONCE(cpudata->highest_perf); > + > + max_limit_perf = div_u64(policy->max * max_perf, policy- > >cpuinfo.max_freq); > + min_limit_perf = div_u64(policy->min * max_perf, > +policy->cpuinfo.max_freq); > > lowest_perf = READ_ONCE(cpudata->lowest_perf); > if (min_limit_perf < lowest_perf) > @@ -1506,10 +1511,13 @@ static int amd_pstate_epp_update_limit(struct > cpufreq_policy *policy) > u64 value; > s16 epp; > > - max_perf = READ_ONCE(cpudata->highest_perf); > + if (cpudata->boost_supported && !policy->boost_enabled) > + max_perf = READ_ONCE(cpudata->nominal_perf); > + else > + max_perf = READ_ONCE(cpudata->highest_perf); > min_perf = READ_ONCE(cpudata->lowest_perf); > - max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata- > >max_freq); > - min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata- > >max_freq); > + max_limit_perf = div_u64(policy->max * max_perf, policy- > >cpuinfo.max_freq); > + min_limit_perf = div_u64(policy->min * max_perf, > +policy->cpuinfo.max_freq); > > if (min_limit_perf < min_perf) > min_limit_perf = min_perf; > -- > 2.43.0 LGTM, thanks for the fix. Reviewed-by: Perry Yuan <perry.yuan@amd.com>
Hello Mario, [..snip..] > > > > When boost is changed the CPPC value is changed in > > amd_pstate_cpu_boost_update() but then changed again when > > refresh_frequency_limits() and all it's callbacks occur. The first is a pointless write, > > so instead just update the limits for the policy and let the policy refresh anchor > > everything properly. > > > > Fixes: c8c68c38b56f ("cpufreq: amd-pstate: initialize core precision boost state") > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > drivers/cpufreq/amd-pstate.c | 24 +----------------------- > > 1 file changed, 1 insertion(+), 23 deletions(-) > > > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index > > dfa9a146769b..13dec8b1e7a8 100644 > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -665,34 +665,12 @@ static void amd_pstate_adjust_perf(unsigned int cpu, > > static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on) { > > struct amd_cpudata *cpudata = policy->driver_data; > > - struct cppc_perf_ctrls perf_ctrls; > > - u32 highest_perf, nominal_perf, nominal_freq, max_freq; > > + u32 nominal_freq, max_freq; > > int ret = 0; > > > > - highest_perf = READ_ONCE(cpudata->highest_perf); > > - nominal_perf = READ_ONCE(cpudata->nominal_perf); > > nominal_freq = READ_ONCE(cpudata->nominal_freq); > > max_freq = READ_ONCE(cpudata->max_freq); > > > > - if (boot_cpu_has(X86_FEATURE_CPPC)) { > > - u64 value = READ_ONCE(cpudata->cppc_req_cached); > > - > > - value &= ~GENMASK_ULL(7, 0); > > - value |= on ? highest_perf : nominal_perf; > > - WRITE_ONCE(cpudata->cppc_req_cached, value); > > - > > - wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value); So, for amd-pstate-epp driver refresh_frequency_limits() --> cpufreq_set_policy() --> amd_pstate_epp_set_policy() --> amd_pstate_epp_update_limit() will ensure that the MSR is updated with the is updated with the value after Patch 1. > > - } else { > > - perf_ctrls.max_perf = on ? highest_perf : nominal_perf; > > - ret = cppc_set_perf(cpudata->cpu, &perf_ctrls); refresh_frequency_limits() --> cpufreq_start_governor() --> governor->limits() --> cpufreq_policy_apply_limits() --> __cpufreq_driver_target() --> amd_pstate_target() --> amd_pstate_update_freq() --> amd_pstate_update() So these updates in amd_pstate_cpu_boost_update() seem redundant. Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> -- Thanks and Regards gautham.
On Sat, Oct 12, 2024 at 12:45:19PM -0500, Mario Limonciello wrote: > The EPP value doesn't need to be cached to the CPPC request in > amd_pstate_epp_update_limit() because it's passed as an argument > at the end to amd_pstate_set_epp() and stored at that time. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> Thanks for cleaning it up. Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> -- Thanks and Regards gautham. > --- > drivers/cpufreq/amd-pstate.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 8d2541f2c74b..90868c8b214e 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -1528,12 +1528,6 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy) > if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) > epp = 0; > > - /* Set initial EPP value */ > - if (cpu_feature_enabled(X86_FEATURE_CPPC)) { > - value &= ~GENMASK_ULL(31, 24); > - value |= (u64)epp << 24; > - } > - > WRITE_ONCE(cpudata->cppc_req_cached, value); > return amd_pstate_set_epp(cpudata, epp); > } > -- > 2.43.0 >
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 30415c30d8b4..dfa9a146769b 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -536,11 +536,16 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy) static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy) { - u32 max_limit_perf, min_limit_perf, lowest_perf; + u32 max_limit_perf, min_limit_perf, lowest_perf, max_perf; struct amd_cpudata *cpudata = policy->driver_data; - max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata->max_freq); - min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata->max_freq); + if (cpudata->boost_supported && !policy->boost_enabled) + max_perf = READ_ONCE(cpudata->nominal_perf); + else + max_perf = READ_ONCE(cpudata->highest_perf); + + max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq); + min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq); lowest_perf = READ_ONCE(cpudata->lowest_perf); if (min_limit_perf < lowest_perf) @@ -1506,10 +1511,13 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy) u64 value; s16 epp; - max_perf = READ_ONCE(cpudata->highest_perf); + if (cpudata->boost_supported && !policy->boost_enabled) + max_perf = READ_ONCE(cpudata->nominal_perf); + else + max_perf = READ_ONCE(cpudata->highest_perf); min_perf = READ_ONCE(cpudata->lowest_perf); - max_limit_perf = div_u64(policy->max * cpudata->highest_perf, cpudata->max_freq); - min_limit_perf = div_u64(policy->min * cpudata->highest_perf, cpudata->max_freq); + max_limit_perf = div_u64(policy->max * max_perf, policy->cpuinfo.max_freq); + min_limit_perf = div_u64(policy->min * max_perf, policy->cpuinfo.max_freq); if (min_limit_perf < min_perf) min_limit_perf = min_perf;
When boost has been disabled the limit for perf should be nominal perf not the highest perf. Using the latter to do calculations will lead to incorrect values that are still above nominal. Fixes: ad4caad58d91 ("cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()") Reported-by: Peter Jung <ptr1337@cachyos.org> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219348 Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/cpufreq/amd-pstate.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)