| Message ID | 20190209120232.21582-1-yu.c.chen@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC to all online CPUs | expand |
On 09-02-19, 20:02, Chen Yu 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 > and broadcast the _PPC notifier to all online CPUs. > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > drivers/cpufreq/cpufreq.c | 2 ++ > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > 3 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > index a303fd0e108c..737dbf5aa7f7 100644 > --- a/drivers/acpi/processor_perflib.c > +++ b/drivers/acpi/processor_perflib.c > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > "limited by BIOS, this should help"); > > +static int broadcast_ppc; > +module_param(broadcast_ppc, int, 0644); > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > + > #define PPC_REGISTERED 1 > #define PPC_IN_USE 2 > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > else > acpi_processor_ppc_ost(pr->handle, 0); > } > - if (ret >= 0) > - cpufreq_update_policy(pr->id); > + if (ret >= 0) { > + if (broadcast_ppc) { > + int cpu; > + > + for_each_possible_cpu(cpu) > + cpufreq_update_policy(cpu); > + } else { > + cpufreq_update_policy(pr->id); > + } > + } > } > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > 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..e1881313c396 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -2081,11 +2081,24 @@ 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 && policy->max == policy->cpuinfo.max_freq && > + max_freq != policy->cpuinfo.max_freq) { > + /* > + * System was not running under any constraints, but the > + * current max possible frequency is changed. So reset > + * policy limits. > + */ > + policy->cpuinfo.max_freq = policy->max = max_freq; > + } > + > 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) By TURBO I believe this is about boost-frequencies and you should use that infrastructure to make it work, isn't it ?
On Mon, Feb 11, 2019 at 10:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 09-02-19, 20:02, Chen Yu 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 > > and broadcast the _PPC notifier to all online CPUs. > > > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > > drivers/cpufreq/cpufreq.c | 2 ++ > > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > > 3 files changed, 30 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > > index a303fd0e108c..737dbf5aa7f7 100644 > > --- a/drivers/acpi/processor_perflib.c > > +++ b/drivers/acpi/processor_perflib.c > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > > "limited by BIOS, this should help"); > > > > +static int broadcast_ppc; > > +module_param(broadcast_ppc, int, 0644); > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > > + > > #define PPC_REGISTERED 1 > > #define PPC_IN_USE 2 > > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > > else > > acpi_processor_ppc_ost(pr->handle, 0); > > } > > - if (ret >= 0) > > - cpufreq_update_policy(pr->id); > > + if (ret >= 0) { > > + if (broadcast_ppc) { > > + int cpu; > > + > > + for_each_possible_cpu(cpu) > > + cpufreq_update_policy(cpu); > > + } else { > > + cpufreq_update_policy(pr->id); > > + } > > + } > > } > > > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > > 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..e1881313c396 100644 > > --- a/drivers/cpufreq/intel_pstate.c > > +++ b/drivers/cpufreq/intel_pstate.c > > @@ -2081,11 +2081,24 @@ 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 && policy->max == policy->cpuinfo.max_freq && > > + max_freq != policy->cpuinfo.max_freq) { > > + /* > > + * System was not running under any constraints, but the > > + * current max possible frequency is changed. So reset > > + * policy limits. > > + */ > > + policy->cpuinfo.max_freq = policy->max = max_freq; > > + } > > + > > 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) > > By TURBO I believe this is about boost-frequencies and you should use > that infrastructure to make it work, isn't it ? I guess you mean the the "boost" attribute in the core, but that's not applicable to intel_pstate.
On 11-02-19, 11:20, Rafael J. Wysocki wrote: > On Mon, Feb 11, 2019 at 10:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 09-02-19, 20:02, Chen Yu 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 > > > and broadcast the _PPC notifier to all online CPUs. > > > > > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > > > drivers/cpufreq/cpufreq.c | 2 ++ > > > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > > > 3 files changed, 30 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > > > index a303fd0e108c..737dbf5aa7f7 100644 > > > --- a/drivers/acpi/processor_perflib.c > > > +++ b/drivers/acpi/processor_perflib.c > > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > > > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > > > "limited by BIOS, this should help"); > > > > > > +static int broadcast_ppc; > > > +module_param(broadcast_ppc, int, 0644); > > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > > > + > > > #define PPC_REGISTERED 1 > > > #define PPC_IN_USE 2 > > > > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > > > else > > > acpi_processor_ppc_ost(pr->handle, 0); > > > } > > > - if (ret >= 0) > > > - cpufreq_update_policy(pr->id); > > > + if (ret >= 0) { > > > + if (broadcast_ppc) { > > > + int cpu; > > > + > > > + for_each_possible_cpu(cpu) > > > + cpufreq_update_policy(cpu); > > > + } else { > > > + cpufreq_update_policy(pr->id); > > > + } > > > + } > > > } > > > > > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > > > 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..e1881313c396 100644 > > > --- a/drivers/cpufreq/intel_pstate.c > > > +++ b/drivers/cpufreq/intel_pstate.c > > > @@ -2081,11 +2081,24 @@ 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 && policy->max == policy->cpuinfo.max_freq && > > > + max_freq != policy->cpuinfo.max_freq) { > > > + /* > > > + * System was not running under any constraints, but the > > > + * current max possible frequency is changed. So reset > > > + * policy limits. > > > + */ > > > + policy->cpuinfo.max_freq = policy->max = max_freq; > > > + } > > > + > > > 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) > > > > By TURBO I believe this is about boost-frequencies and you should use > > that infrastructure to make it work, isn't it ? > > I guess you mean the the "boost" attribute in the core, but that's not > applicable to intel_pstate. Ahh, I missed the part that this is for the !has_target() platforms :(
On 09-02-19, 20:02, Chen Yu 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 > and broadcast the _PPC notifier to all online CPUs. > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > drivers/cpufreq/cpufreq.c | 2 ++ > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > 3 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > index a303fd0e108c..737dbf5aa7f7 100644 > --- a/drivers/acpi/processor_perflib.c > +++ b/drivers/acpi/processor_perflib.c > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > "limited by BIOS, this should help"); > > +static int broadcast_ppc; > +module_param(broadcast_ppc, int, 0644); > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > + > #define PPC_REGISTERED 1 > #define PPC_IN_USE 2 > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > else > acpi_processor_ppc_ost(pr->handle, 0); > } > - if (ret >= 0) > - cpufreq_update_policy(pr->id); > + if (ret >= 0) { > + if (broadcast_ppc) { > + int cpu; > + > + for_each_possible_cpu(cpu) > + cpufreq_update_policy(cpu); > + } else { > + cpufreq_update_policy(pr->id); > + } > + } > } > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > 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..e1881313c396 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -2081,11 +2081,24 @@ 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 && policy->max == policy->cpuinfo.max_freq && Why do have this check for policy->max here ? > + max_freq != policy->cpuinfo.max_freq) { > + /* > + * System was not running under any constraints, but the > + * current max possible frequency is changed. So reset > + * policy limits. > + */ > + policy->cpuinfo.max_freq = policy->max = max_freq; > + } > + > 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) > -- > 2.17.1
On Sat, Feb 9, 2019 at 12:54 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 > and broadcast the _PPC notifier to all online CPUs. > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > drivers/cpufreq/cpufreq.c | 2 ++ > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > 3 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > index a303fd0e108c..737dbf5aa7f7 100644 > --- a/drivers/acpi/processor_perflib.c > +++ b/drivers/acpi/processor_perflib.c > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > "limited by BIOS, this should help"); > > +static int broadcast_ppc; > +module_param(broadcast_ppc, int, 0644); > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > + > #define PPC_REGISTERED 1 > #define PPC_IN_USE 2 > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > else > acpi_processor_ppc_ost(pr->handle, 0); > } > - if (ret >= 0) > - cpufreq_update_policy(pr->id); > + if (ret >= 0) { > + if (broadcast_ppc) { > + int cpu; > + > + for_each_possible_cpu(cpu) > + cpufreq_update_policy(cpu); > + } else { > + cpufreq_update_policy(pr->id); > + } > + } > } > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > 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..e1881313c396 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -2081,11 +2081,24 @@ 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(); Well, update_turbo_state() should handle the case at hand already. That's what it's for actually: It checks if MSR_IA32_MISC_ENABLE_TURBO_DISABLE is set and sets global.turbo_disabled is that's the case. Why isn't that sufficient? > + max_freq = intel_pstate_get_max_freq(cpu); > + > + if (acpi_ppc && policy->max == policy->cpuinfo.max_freq && > + max_freq != policy->cpuinfo.max_freq) { > + /* > + * System was not running under any constraints, but the > + * current max possible frequency is changed. So reset > + * policy limits. > + */ > + policy->cpuinfo.max_freq = policy->max = max_freq; > + } Why does policy->cpuinfo.max_freq need to be updated? > + > 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) > --
Hi Rafael, On Mon, Feb 11, 2019 at 11:41:26AM +0100, Rafael J. Wysocki wrote: > On Sat, Feb 9, 2019 at 12:54 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 > > and broadcast the _PPC notifier to all online CPUs. > > > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > > drivers/cpufreq/cpufreq.c | 2 ++ > > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > > 3 files changed, 30 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > > index a303fd0e108c..737dbf5aa7f7 100644 > > --- a/drivers/acpi/processor_perflib.c > > +++ b/drivers/acpi/processor_perflib.c > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > > "limited by BIOS, this should help"); > > > > +static int broadcast_ppc; > > +module_param(broadcast_ppc, int, 0644); > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > > + > > #define PPC_REGISTERED 1 > > #define PPC_IN_USE 2 > > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > > else > > acpi_processor_ppc_ost(pr->handle, 0); > > } > > - if (ret >= 0) > > - cpufreq_update_policy(pr->id); > > + if (ret >= 0) { > > + if (broadcast_ppc) { > > + int cpu; > > + > > + for_each_possible_cpu(cpu) > > + cpufreq_update_policy(cpu); > > + } else { > > + cpufreq_update_policy(pr->id); > > + } > > + } > > } > > > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > > 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..e1881313c396 100644 > > --- a/drivers/cpufreq/intel_pstate.c > > +++ b/drivers/cpufreq/intel_pstate.c > > @@ -2081,11 +2081,24 @@ 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(); > > Well, update_turbo_state() should handle the case at hand already. > > That's what it's for actually: It checks if > MSR_IA32_MISC_ENABLE_TURBO_DISABLE is set and sets > global.turbo_disabled is that's the case. > > Why isn't that sufficient? > update_turbo_state() changes the flag of global.turbo_diabled but we need to also leverage it to adjust the policy.max accordingly. This is why we add intel_pstate_get_max_freq() to get the updated max freq in intel_pstate_verify_policy(). > > + max_freq = intel_pstate_get_max_freq(cpu); > > + > > + if (acpi_ppc && policy->max == policy->cpuinfo.max_freq && > > + max_freq != policy->cpuinfo.max_freq) { > > + /* > > + * System was not running under any constraints, but the > > + * current max possible frequency is changed. So reset > > + * policy limits. > > + */ > > + policy->cpuinfo.max_freq = policy->max = max_freq; > > + } > > Why does policy->cpuinfo.max_freq need to be updated? > This is my understanding: There's a corner case that, if the system boots with battery, the max cpu frequency will not scale up if we plug the AC later. According to the log provided by Gabriele Mazzotta, if the system boot up with battery, then plug the AC after boot up, the max perf ratio and policy->cpuinfo.max will remain 17 rather than increasing to 30(when AC plugged thus turbo enabled): [ 52.158810] CPU 0: _PPC is 6 - frequency limited [ 52.158822] intel_pstate: set_policy cpuinfo.max 1700000 policy->max 1700000 [ 52.158825] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:17 [ 52.158827] intel_pstate: cpu:0 global_min:8 global_max:30 [ 52.158829] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8 This is caused by: In current intel_pstate, there's only one chance for policy.cpuinfo.max to get updated which is during boot up in __intel_pstate_cpu_init(). If the turbo status changes, we might need to also update the policy->cpuinfo.max to tell user that the hardware status has changed. So we give it a chance to adjust the policy.cpuinfo.max and policy.max in cpufreq_driver->verify() according to turbo status, this is what this patch mainly aims to do. Besides, since on this platform there's only one _PPC notification for one CPU, it is necessary to broadcast the notification to all CPUs on this package. And this patch broadcast it to all online CPUs to make the change simpler. Best, Yu
Hi Viresh, On Mon, Feb 11, 2019 at 04:03:07PM +0530, Viresh Kumar wrote: > On 09-02-19, 20:02, Chen Yu 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 > > and broadcast the _PPC notifier to all online CPUs. > > > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > > drivers/cpufreq/cpufreq.c | 2 ++ > > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > > 3 files changed, 30 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > > index a303fd0e108c..737dbf5aa7f7 100644 > > --- a/drivers/acpi/processor_perflib.c > > +++ b/drivers/acpi/processor_perflib.c > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > > "limited by BIOS, this should help"); > > > > +static int broadcast_ppc; > > +module_param(broadcast_ppc, int, 0644); > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > > + > > #define PPC_REGISTERED 1 > > #define PPC_IN_USE 2 > > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > > else > > acpi_processor_ppc_ost(pr->handle, 0); > > } > > - if (ret >= 0) > > - cpufreq_update_policy(pr->id); > > + if (ret >= 0) { > > + if (broadcast_ppc) { > > + int cpu; > > + > > + for_each_possible_cpu(cpu) > > + cpufreq_update_policy(cpu); > > + } else { > > + cpufreq_update_policy(pr->id); > > + } > > + } > > } > > > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > > 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..e1881313c396 100644 > > --- a/drivers/cpufreq/intel_pstate.c > > +++ b/drivers/cpufreq/intel_pstate.c > > @@ -2081,11 +2081,24 @@ 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 && policy->max == policy->cpuinfo.max_freq && > > Why do have this check for policy->max here ? > Thanks for looking at this change, I've replied to another email in detail of the scenario that, this is due to corner case that if the system boots with battery and plug the AC after boot up, the cpufreq max limit will not increase even the turbo has been enabled after the AC plugged. Best, Yu
On 14-02-19, 00:55, Yu Chen wrote: > Hi Viresh, > On Mon, Feb 11, 2019 at 04:03:07PM +0530, Viresh Kumar wrote: > > On 09-02-19, 20:02, Chen Yu 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 > > > and broadcast the _PPC notifier to all online CPUs. > > > > > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > > > drivers/cpufreq/cpufreq.c | 2 ++ > > > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > > > 3 files changed, 30 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > > > index a303fd0e108c..737dbf5aa7f7 100644 > > > --- a/drivers/acpi/processor_perflib.c > > > +++ b/drivers/acpi/processor_perflib.c > > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > > > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > > > "limited by BIOS, this should help"); > > > > > > +static int broadcast_ppc; > > > +module_param(broadcast_ppc, int, 0644); > > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > > > + > > > #define PPC_REGISTERED 1 > > > #define PPC_IN_USE 2 > > > > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > > > else > > > acpi_processor_ppc_ost(pr->handle, 0); > > > } > > > - if (ret >= 0) > > > - cpufreq_update_policy(pr->id); > > > + if (ret >= 0) { > > > + if (broadcast_ppc) { > > > + int cpu; > > > + > > > + for_each_possible_cpu(cpu) > > > + cpufreq_update_policy(cpu); > > > + } else { > > > + cpufreq_update_policy(pr->id); > > > + } > > > + } > > > } > > > > > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > > > 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..e1881313c396 100644 > > > --- a/drivers/cpufreq/intel_pstate.c > > > +++ b/drivers/cpufreq/intel_pstate.c > > > @@ -2081,11 +2081,24 @@ 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 && policy->max == policy->cpuinfo.max_freq && > > > > Why do have this check for policy->max here ? > > > Thanks for looking at this change, I've replied to another email in detail of > the scenario that, this is due to corner case that if the system boots > with battery and plug the AC after boot up, the cpufreq max limit will not > increase even the turbo has been enabled after the AC plugged. Yeah, but I asked a different question I believe, why is this comparison necessary ? policy->max == policy->cpuinfo.max_freq
On Wed, Feb 13, 2019 at 5:44 PM Yu Chen <yu.c.chen@intel.com> wrote: > > Hi Rafael, > On Mon, Feb 11, 2019 at 11:41:26AM +0100, Rafael J. Wysocki wrote: > > On Sat, Feb 9, 2019 at 12:54 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 > > > and broadcast the _PPC notifier to all online CPUs. > > > > > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > > > drivers/cpufreq/cpufreq.c | 2 ++ > > > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > > > 3 files changed, 30 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > > > index a303fd0e108c..737dbf5aa7f7 100644 > > > --- a/drivers/acpi/processor_perflib.c > > > +++ b/drivers/acpi/processor_perflib.c > > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > > > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > > > "limited by BIOS, this should help"); > > > > > > +static int broadcast_ppc; > > > +module_param(broadcast_ppc, int, 0644); > > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > > > + > > > #define PPC_REGISTERED 1 > > > #define PPC_IN_USE 2 > > > > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > > > else > > > acpi_processor_ppc_ost(pr->handle, 0); > > > } > > > - if (ret >= 0) > > > - cpufreq_update_policy(pr->id); > > > + if (ret >= 0) { > > > + if (broadcast_ppc) { > > > + int cpu; > > > + > > > + for_each_possible_cpu(cpu) > > > + cpufreq_update_policy(cpu); > > > + } else { > > > + cpufreq_update_policy(pr->id); > > > + } > > > + } > > > } > > > > > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > > > 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..e1881313c396 100644 > > > --- a/drivers/cpufreq/intel_pstate.c > > > +++ b/drivers/cpufreq/intel_pstate.c > > > @@ -2081,11 +2081,24 @@ 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(); > > > > Well, update_turbo_state() should handle the case at hand already. > > > > That's what it's for actually: It checks if > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE is set and sets > > global.turbo_disabled is that's the case. > > > > Why isn't that sufficient? > > > update_turbo_state() changes the flag of global.turbo_diabled but we > need to also leverage it to adjust the policy.max accordingly. This is why > we add intel_pstate_get_max_freq() to get the updated max freq in > intel_pstate_verify_policy(). Yes, that's why intel_pstate_verify_policy() passes the return value of intel_pstate_get_max_freq() as the second arg cpufreq_verify_within_limits(), so really my question was about why cpuinfo.max_freq needed to be updated (below). > > > + max_freq = intel_pstate_get_max_freq(cpu); > > > + > > > + if (acpi_ppc && policy->max == policy->cpuinfo.max_freq && > > > + max_freq != policy->cpuinfo.max_freq) { > > > + /* > > > + * System was not running under any constraints, but the > > > + * current max possible frequency is changed. So reset > > > + * policy limits. > > > + */ > > > + policy->cpuinfo.max_freq = policy->max = max_freq; > > > + } > > > > Why does policy->cpuinfo.max_freq need to be updated? > > > This is my understanding: > There's a corner case that, if the system boots with battery, > the max cpu frequency will not scale up if we plug the AC later. I see. The *initial* cpuinfo.max_freq may be too low. This part is missing from your patch changelog. The driver is not expected to update cpuinfo.max_freq after init. That may not actually break anything, even though it is racy in principle, but if it is done, it needs to be done in the "passive" mode too and that may be more problematic. Anyway, this is more fundamental than you seem to be thinking. > According to the log provided by Gabriele Mazzotta, if the system > boot up with battery, then plug the AC after boot up, the max perf ratio > and policy->cpuinfo.max will remain 17 rather than increasing to > 30(when AC plugged thus turbo enabled): > > [ 52.158810] CPU 0: _PPC is 6 - frequency limited > [ 52.158822] intel_pstate: set_policy cpuinfo.max 1700000 policy->max 1700000 > [ 52.158825] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:17 > [ 52.158827] intel_pstate: cpu:0 global_min:8 global_max:30 > [ 52.158829] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8 > > This is caused by: > In current intel_pstate, there's only one chance for policy.cpuinfo.max to get updated > which is during boot up in __intel_pstate_cpu_init(). If the turbo status changes, > we might need to also update the policy->cpuinfo.max to tell user that the hardware > status has changed. > > So we give it a chance to adjust the policy.cpuinfo.max and policy.max in > cpufreq_driver->verify() according to turbo status, this is what this patch mainly > aims to do. > > Besides, since on this platform there's only one _PPC notification for one CPU, it is > necessary to broadcast the notification to all CPUs on this package. And this patch > broadcast it to all online CPUs to make the change simpler. You're trying to make two substantial changes in one go, broadcasting _PPC and updating cpuinfo.max_freq. Don't do that, they need to be separate changes. Moreover, we may want to address the initial cpuinfo.max_freq issue in a different way. Thanks, Rafael
On Thu, Feb 14, 2019 at 12:36:48PM +0530, Viresh Kumar wrote: > On 14-02-19, 00:55, Yu Chen wrote: > > Hi Viresh, > > On Mon, Feb 11, 2019 at 04:03:07PM +0530, Viresh Kumar wrote: > > > On 09-02-19, 20:02, Chen Yu 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 > > > > and broadcast the _PPC notifier to all online CPUs. > > > > > > > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > > > > drivers/cpufreq/cpufreq.c | 2 ++ > > > > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > > > > 3 files changed, 30 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > > > > index a303fd0e108c..737dbf5aa7f7 100644 > > > > --- a/drivers/acpi/processor_perflib.c > > > > +++ b/drivers/acpi/processor_perflib.c > > > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > > > > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > > > > "limited by BIOS, this should help"); > > > > > > > > +static int broadcast_ppc; > > > > +module_param(broadcast_ppc, int, 0644); > > > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > > > > + > > > > #define PPC_REGISTERED 1 > > > > #define PPC_IN_USE 2 > > > > > > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > > > > else > > > > acpi_processor_ppc_ost(pr->handle, 0); > > > > } > > > > - if (ret >= 0) > > > > - cpufreq_update_policy(pr->id); > > > > + if (ret >= 0) { > > > > + if (broadcast_ppc) { > > > > + int cpu; > > > > + > > > > + for_each_possible_cpu(cpu) > > > > + cpufreq_update_policy(cpu); > > > > + } else { > > > > + cpufreq_update_policy(pr->id); > > > > + } > > > > + } > > > > } > > > > > > > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > > > > 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..e1881313c396 100644 > > > > --- a/drivers/cpufreq/intel_pstate.c > > > > +++ b/drivers/cpufreq/intel_pstate.c > > > > @@ -2081,11 +2081,24 @@ 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 && policy->max == policy->cpuinfo.max_freq && > > > > > > Why do have this check for policy->max here ? > > > > > Thanks for looking at this change, I've replied to another email in detail of > > the scenario that, this is due to corner case that if the system boots > > with battery and plug the AC after boot up, the cpufreq max limit will not > > increase even the turbo has been enabled after the AC plugged. > > Yeah, but I asked a different question I believe, why is this > comparison necessary ? > Sorry for late response, I was caught in another issue. The reason for why checking if policy->max equals to policy->cpuinfo.max_freq is to bypass unnecessary assignment(it turns out to be a hacky logic here): Say, if the system boots up with AC attached, after bootup the cpuinfo.max is always the highest, and there is no need to reassign the cpuinfo.max. However if the system boots up with AC deattached, then the cpuinfo.max will not scale up if the AC is attached after bootup. Thus in former case, the cpuinfo.max does not equals to policy.max, and we don't need to update cpuinfo.max. While in latter case, the cpuinfo.max might equal to policy.max, and in this situation we need to update the cpuinfo.max. Case 1: Boot up AC attached, cpuinfo.max does not equal to policy.max # Unplug [ 25.775643] CPU 0: _PPC is 6 - frequency limited [ 25.775660] intel_pstate: set_policy cpuinfo.max 3000000 policy->max 1700000 [ 25.775666] intel_pstate: cpu:0 max_state 17 min_policy_perf:8 max_policy_perf:17 [ 25.775670] intel_pstate: cpu:0 global_min:8 global_max:30 [ 25.775674] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8 "REDUCED FREQUENCY ABOVE AFTER UNPLUG" # Re-plug [ 36.979264] CPU 0: _PPC is 6 - frequency limited [ 36.979276] intel_pstate: policy->max > max non turbo frequency [ 36.979280] intel_pstate: set_policy cpuinfo.max 3000000 policy->max 3000000 [ 36.979283] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:30 [ 36.979286] intel_pstate: cpu:0 global_min:8 global_max:30 [ 36.979289] intel_pstate: cpu:0 max_perf_ratio:30 min_perf_ratio:8 Case 2: boot up without AC attached, cpuinfo.max equals to policy.max: # Plug [ 52.158810] CPU 0: _PPC is 6 - frequency limited [ 52.158822] intel_pstate: set_policy cpuinfo.max 1700000 policy->max 1700000 [ 52.158825] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:17 [ 52.158827] intel_pstate: cpu:0 global_min:8 global_max:30 [ 52.158829] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8 As we see, the check is a little hacky, because it does not do with the logic on how to check if the system's cpuinfo.max is abnormal. In v2 version, I've removed this check to make the code easier to be understood. Best, Yu > policy->max == policy->cpuinfo.max_freq > > -- > viresh
On Thu, Feb 14, 2019 at 11:21:13AM +0100, Rafael J. Wysocki wrote: > On Wed, Feb 13, 2019 at 5:44 PM Yu Chen <yu.c.chen@intel.com> wrote: > > > > Hi Rafael, > > On Mon, Feb 11, 2019 at 11:41:26AM +0100, Rafael J. Wysocki wrote: > > > On Sat, Feb 9, 2019 at 12:54 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 > > > > and broadcast the _PPC notifier to all online CPUs. > > > > > > > > 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/acpi/processor_perflib.c | 16 ++++++++++++++-- > > > > drivers/cpufreq/cpufreq.c | 2 ++ > > > > drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- > > > > 3 files changed, 30 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > > > > index a303fd0e108c..737dbf5aa7f7 100644 > > > > --- a/drivers/acpi/processor_perflib.c > > > > +++ b/drivers/acpi/processor_perflib.c > > > > @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); > > > > MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ > > > > "limited by BIOS, this should help"); > > > > > > > > +static int broadcast_ppc; > > > > +module_param(broadcast_ppc, int, 0644); > > > > +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); > > > > + > > > > #define PPC_REGISTERED 1 > > > > #define PPC_IN_USE 2 > > > > > > > > @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) > > > > else > > > > acpi_processor_ppc_ost(pr->handle, 0); > > > > } > > > > - if (ret >= 0) > > > > - cpufreq_update_policy(pr->id); > > > > + if (ret >= 0) { > > > > + if (broadcast_ppc) { > > > > + int cpu; > > > > + > > > > + for_each_possible_cpu(cpu) > > > > + cpufreq_update_policy(cpu); > > > > + } else { > > > > + cpufreq_update_policy(pr->id); > > > > + } > > > > + } > > > > } > > > > > > > > int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) > > > > 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..e1881313c396 100644 > > > > --- a/drivers/cpufreq/intel_pstate.c > > > > +++ b/drivers/cpufreq/intel_pstate.c > > > > @@ -2081,11 +2081,24 @@ 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(); > > > > > > Well, update_turbo_state() should handle the case at hand already. > > > > > > That's what it's for actually: It checks if > > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE is set and sets > > > global.turbo_disabled is that's the case. > > > > > > Why isn't that sufficient? > > > > > update_turbo_state() changes the flag of global.turbo_diabled but we > > need to also leverage it to adjust the policy.max accordingly. This is why > > we add intel_pstate_get_max_freq() to get the updated max freq in > > intel_pstate_verify_policy(). > > Yes, that's why intel_pstate_verify_policy() passes the return value > of intel_pstate_get_max_freq() as the second arg > cpufreq_verify_within_limits(), so really my question was about why > cpuinfo.max_freq needed to be updated (below). > Ok. > > > > + max_freq = intel_pstate_get_max_freq(cpu); > > > > + > > > > + if (acpi_ppc && policy->max == policy->cpuinfo.max_freq && > > > > + max_freq != policy->cpuinfo.max_freq) { > > > > + /* > > > > + * System was not running under any constraints, but the > > > > + * current max possible frequency is changed. So reset > > > > + * policy limits. > > > > + */ > > > > + policy->cpuinfo.max_freq = policy->max = max_freq; > > > > + } > > > > > > Why does policy->cpuinfo.max_freq need to be updated? > > > > > This is my understanding: > > There's a corner case that, if the system boots with battery, > > the max cpu frequency will not scale up if we plug the AC later. > > I see. The *initial* cpuinfo.max_freq may be too low. This part is > missing from your patch changelog. > > The driver is not expected to update cpuinfo.max_freq after init. > That may not actually break anything, even though it is racy in > principle, but if it is done, it needs to be done in the "passive" > mode too and that may be more problematic. > Do you mean updating it for "passive" mode might not be suitable? > Anyway, this is more fundamental than you seem to be thinking. > > > According to the log provided by Gabriele Mazzotta, if the system > > boot up with battery, then plug the AC after boot up, the max perf ratio > > and policy->cpuinfo.max will remain 17 rather than increasing to > > 30(when AC plugged thus turbo enabled): > > > > [ 52.158810] CPU 0: _PPC is 6 - frequency limited > > [ 52.158822] intel_pstate: set_policy cpuinfo.max 1700000 policy->max 1700000 > > [ 52.158825] intel_pstate: cpu:0 max_state 30 min_policy_perf:8 max_policy_perf:17 > > [ 52.158827] intel_pstate: cpu:0 global_min:8 global_max:30 > > [ 52.158829] intel_pstate: cpu:0 max_perf_ratio:17 min_perf_ratio:8 > > > > This is caused by: > > In current intel_pstate, there's only one chance for policy.cpuinfo.max to get updated > > which is during boot up in __intel_pstate_cpu_init(). If the turbo status changes, > > we might need to also update the policy->cpuinfo.max to tell user that the hardware > > status has changed. > > > > So we give it a chance to adjust the policy.cpuinfo.max and policy.max in > > cpufreq_driver->verify() according to turbo status, this is what this patch mainly > > aims to do. > > > > Besides, since on this platform there's only one _PPC notification for one CPU, it is > > necessary to broadcast the notification to all CPUs on this package. And this patch > > broadcast it to all online CPUs to make the change simpler. > > You're trying to make two substantial changes in one go, broadcasting > _PPC and updating cpuinfo.max_freq. Don't do that, they need to be > separate changes. Ok, I'll send version 2 out which is composed of two modifications to address different problems. > > Moreover, we may want to address the initial cpuinfo.max_freq issue in > a different way. Adjust the cpuinfo.max_freq in .verify() seems to be a proper place since this callback is invoked at a very early stage for both active and passive mode, and I did not see race condition for them. (Could not quite catch what we talked last time on this, if you have proposed another suggestion on how to update the cpuinfo.max_freq) :) Thanks, Ryan(Yu) > > Thanks, > Rafael
diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c index a303fd0e108c..737dbf5aa7f7 100644 --- a/drivers/acpi/processor_perflib.c +++ b/drivers/acpi/processor_perflib.c @@ -63,6 +63,10 @@ module_param(ignore_ppc, int, 0644); MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets wrongly" \ "limited by BIOS, this should help"); +static int broadcast_ppc; +module_param(broadcast_ppc, int, 0644); +MODULE_PARM_DESC(broadcast_ppc, "Broadcast the ppc to all online CPUs"); + #define PPC_REGISTERED 1 #define PPC_IN_USE 2 @@ -180,8 +184,16 @@ void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) else acpi_processor_ppc_ost(pr->handle, 0); } - if (ret >= 0) - cpufreq_update_policy(pr->id); + if (ret >= 0) { + if (broadcast_ppc) { + int cpu; + + for_each_possible_cpu(cpu) + cpufreq_update_policy(cpu); + } else { + cpufreq_update_policy(pr->id); + } + } } int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) 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..e1881313c396 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -2081,11 +2081,24 @@ 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 && policy->max == policy->cpuinfo.max_freq && + max_freq != policy->cpuinfo.max_freq) { + /* + * System was not running under any constraints, but the + * current max possible frequency is changed. So reset + * policy limits. + */ + policy->cpuinfo.max_freq = policy->max = max_freq; + } + 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)
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 and broadcast the _PPC notifier to all online CPUs. 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/acpi/processor_perflib.c | 16 ++++++++++++++-- drivers/cpufreq/cpufreq.c | 2 ++ drivers/cpufreq/intel_pstate.c | 15 ++++++++++++++- 3 files changed, 30 insertions(+), 3 deletions(-)