| Message ID | fa430f6288744c760145f4acab952c2bd0f947ad.1706255676.git.perry.yuan@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | AMD Pstate Driver Core Performance Boost | expand |
On 1/26/2024 02:08, Perry Yuan wrote: > From: Perry Yuan <Perry.Yuan@amd.com> > > Add suspend and resume support for `passive` mode driver which can save > the previous CPU Pstate values and restore them while resuming, on some > old platforms, the highest perf needs to be restored from driver side, > otherwise the highest frequency could be changed during suspend. So this sounds like a BIOS bug, right? Do you know how far back this problem exists? Should it be a quirked behavior to only run on the broken platforms so we don't need to run the callback on modern ones without it? > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 48 ++++++++++++++++++++++++++++++++---- > 1 file changed, 43 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 5cbbc2999d9a..bba7640d46e0 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -785,23 +785,61 @@ static int amd_pstate_cpu_exit(struct cpufreq_policy *policy) > > static int amd_pstate_cpu_resume(struct cpufreq_policy *policy) > { > + struct cppc_perf_ctrls perf_ctrls; > + struct amd_cpudata *cpudata = policy->driver_data; > + u64 value, max_perf; > int ret; > > - ret = amd_pstate_enable(true); > - if (ret) > - pr_err("failed to enable amd-pstate during resume, return %d\n", ret); > + if (cpudata->suspended) { > + mutex_lock(&amd_pstate_driver_lock); > > - return ret; > + ret = amd_pstate_enable(true); > + if (ret) { > + pr_err("failed to enable amd-pstate during resume, return %d\n", ret); > + mutex_unlock(&amd_pstate_driver_lock); > + return ret; > + } This /looks/ like an unintended logic change to me. Previously amd_pstate_enable(true) would be called in all modes, but now it will only be called in passive mode. Is that right? > + > + value = READ_ONCE(cpudata->cppc_req_cached); > + max_perf = READ_ONCE(cpudata->highest_perf); > + > + if (boot_cpu_has(X86_FEATURE_CPPC)) { > + wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value); > + } else { > + perf_ctrls.max_perf = max_perf; > + cppc_set_perf(cpudata->cpu, &perf_ctrls); > + } > + > + cpudata->suspended = false; > + mutex_unlock(&amd_pstate_driver_lock); > + } > + > + return 0; > } > > static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy) > { > + struct amd_cpudata *cpudata = policy->driver_data; > int ret; > > + /* avoid suspending when EPP is not enabled */ The logic seems right, but shouldn't the comment be: /* only run suspend callbacks for passive mode */ Because this stuff won't run in guided mode or disable mode either > + if (cppc_state != AMD_PSTATE_PASSIVE) > + return 0; > + > + mutex_lock(&amd_pstate_driver_lock); > + > + /* set this flag to avoid calling core offline function > + * when system is suspending, use this flag to skip offline function > + * called > + */ > + cpudata->suspended = true; > + > ret = amd_pstate_enable(false); > if (ret) > pr_err("failed to disable amd-pstate during suspend, return %d\n", ret); > > + mutex_unlock(&amd_pstate_driver_lock); > + > return ret; > } > > @@ -1460,7 +1498,7 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) > if (cppc_state != AMD_PSTATE_ACTIVE) > return 0; > > - /* set this flag to avoid setting core offline*/ > + /* set this flag to avoid setting core offline */ > cpudata->suspended = true; > > /* disable CPPC in lowlevel firmware */
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 5cbbc2999d9a..bba7640d46e0 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -785,23 +785,61 @@ static int amd_pstate_cpu_exit(struct cpufreq_policy *policy) static int amd_pstate_cpu_resume(struct cpufreq_policy *policy) { + struct cppc_perf_ctrls perf_ctrls; + struct amd_cpudata *cpudata = policy->driver_data; + u64 value, max_perf; int ret; - ret = amd_pstate_enable(true); - if (ret) - pr_err("failed to enable amd-pstate during resume, return %d\n", ret); + if (cpudata->suspended) { + mutex_lock(&amd_pstate_driver_lock); - return ret; + ret = amd_pstate_enable(true); + if (ret) { + pr_err("failed to enable amd-pstate during resume, return %d\n", ret); + mutex_unlock(&amd_pstate_driver_lock); + return ret; + } + + value = READ_ONCE(cpudata->cppc_req_cached); + max_perf = READ_ONCE(cpudata->highest_perf); + + if (boot_cpu_has(X86_FEATURE_CPPC)) { + wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value); + } else { + perf_ctrls.max_perf = max_perf; + cppc_set_perf(cpudata->cpu, &perf_ctrls); + } + + cpudata->suspended = false; + mutex_unlock(&amd_pstate_driver_lock); + } + + return 0; } static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy) { + struct amd_cpudata *cpudata = policy->driver_data; int ret; + /* avoid suspending when EPP is not enabled */ + if (cppc_state != AMD_PSTATE_PASSIVE) + return 0; + + mutex_lock(&amd_pstate_driver_lock); + + /* set this flag to avoid calling core offline function + * when system is suspending, use this flag to skip offline function + * called + */ + cpudata->suspended = true; + ret = amd_pstate_enable(false); if (ret) pr_err("failed to disable amd-pstate during suspend, return %d\n", ret); + mutex_unlock(&amd_pstate_driver_lock); + return ret; } @@ -1460,7 +1498,7 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) if (cppc_state != AMD_PSTATE_ACTIVE) return 0; - /* set this flag to avoid setting core offline*/ + /* set this flag to avoid setting core offline */ cpudata->suspended = true; /* disable CPPC in lowlevel firmware */