Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Thu, 28 Feb 2019 23:56:48 +0100 | Subject | Re: [PATCH 2/2][RFC v2] ACPI: Update cpuinfo.max after bootup if necessary |
| |
On Thu, Feb 28, 2019 at 6:59 PM Chen Yu <yu.c.chen@intel.com> wrote: > > On Dell Inc. XPS13 9333, the BIOS changes the value of > MSR_IA32_MISC_ENABLE_TURBO_DISABLE at runtime (e.g., when > the power source changes), the maximum frequency of the > CPU is not updated accordingly. This is due to the policy's > cpuinfo.max is not updated when _PPC notifier fires. > > Fix this problem by updating the policy's cpuinfo.max when > necessary. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=200759 > Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com> > Originally-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > --- > drivers/cpufreq/cpufreq.c | 2 ++ > drivers/cpufreq/intel_pstate.c | 15 +++++++++++++-- > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index e35a886e00bc..95e08816b512 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -2237,6 +2237,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, > > policy->min = new_policy->min; > policy->max = new_policy->max; > + policy->cpuinfo.max_freq = new_policy->cpuinfo.max_freq; > + policy->cpuinfo.min_freq = new_policy->cpuinfo.min_freq; > trace_cpu_frequency_limits(policy); > > policy->cached_target_freq = UINT_MAX; > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > index dd66decf2087..841c74f69f66 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -2081,11 +2081,17 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy, > > static int intel_pstate_verify_policy(struct cpufreq_policy *policy) > { > + int max_freq; > struct cpudata *cpu = all_cpu_data[policy->cpu]; > > update_turbo_state(); > + max_freq = intel_pstate_get_max_freq(cpu); > + > + if (acpi_ppc && max_freq != policy->cpuinfo.max_freq) > + policy->cpuinfo.max_freq = policy->max = max_freq;
Updating cpuinfo.max_freq here only causes the current limit to be reported via sysfs, because intel_pstate doesn't actually use cpuinfo.max_freq for anything and the core doesn't enforce it as a limit.
All of the computations in the active mode in the driver actually use the current limit anyway AFAICS.
> + > cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq, > - intel_pstate_get_max_freq(cpu)); > + max_freq); > > if (policy->policy != CPUFREQ_POLICY_POWERSAVE && > policy->policy != CPUFREQ_POLICY_PERFORMANCE) > @@ -2192,11 +2198,16 @@ static struct cpufreq_driver intel_pstate = { > > static int intel_cpufreq_verify_policy(struct cpufreq_policy *policy) > { > + int max_freq; > struct cpudata *cpu = all_cpu_data[policy->cpu]; > > update_turbo_state(); > + max_freq = intel_pstate_get_max_freq(cpu); > + > + if (acpi_ppc && max_freq != policy->cpuinfo.max_freq) > + policy->cpuinfo.max_freq = policy->max = max_freq;
In this case (passive mode) updating cpuinfo.max_freq will actually cause governors to use the new value in computations, so the P-state selection will work somewhat differently, but that isn't really consistent with what acpi-cpufreq does and with setting no_turbo in the intel_pstate sysfs to 1 without this patch.
With acpi-cpufreq cpuinfo.max_freq is always the max frequency in the _PSS table regardless of what the _PSS limit is. Also setting no_turbo to 1 in intel_pstate without this patch doesn't cause cpuinfo.max_freq to change and I don't really think that it should.
I would be tempted to always initialize cpuinfo.max_freq to the max turbo frequency, but there is a concern about systems in which MSR_IA32_MISC_ENABLE_TURBO_DISABLE is never set on the fly (just in the BIOS setup as it should be) and where it doesn't make sense to consider turbo frequencies at all.
> cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq, > - intel_pstate_get_max_freq(cpu)); > + max_freq); > > intel_pstate_adjust_policy_max(policy, cpu); > > --
It looks like I need to think about this a bit more.
| |