Update the cpuinfo.max when power supply changes
Subsystems (3)
v2 cover Update the cpuinfo.max when power supply changes
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.
Firstly we should update the cpuinfo.max in this corner case, secondly we should broadcast the _PPC notifier to all online CPUs to keep information consistent.
Chen Yu (2):
ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC
ACPI: Update cpuinfo.max after bootup if necessary
drivers/acpi/processor_perflib.c | 16 ++++++++++++++-- drivers/cpufreq/cpufreq.c | 2 ++ drivers/cpufreq/intel_pstate.c | 15 +++++++++++++-- 3 files changed, 29 insertions(+), 4 deletions(-)
Signature · 1 line
2.17.1
RFC PATCH v2 1/2 ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC
On some problematic platforms, the _PPC notifier is
only delivered to one CPU, which might cause information
inconsistent between CPUs within the package. Thus introduce a boot up parameter to broadcast this _PPC notifier onto all online CPUs.
--- drivers/acpi/processor_perflib.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
drivers/acpi/processor_perflib.c +14 -2
@@ -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)
Signature · 1 line
2.17.1
Rafael J. Wysocki
maintainerRe: [PATCH 1/2][RFC v2] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC
Quoted 28 lines · Chen Yu
On some problematic platforms, the
_PPCnotifier is
only delivered to one CPU, which might cause information
inconsistent between CPUs within the package. Thus introduce a boot up parameter to broadcast this_PPCnotifier onto all online CPUs.Signed-off-by Chen Yu <[email protected]>--- drivers/acpi/processor_perflib.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)drivers/acpi/processor_perflib.c +4
-0@@ -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
drivers/acpi/processor_perflib.c void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) · line 192
Quoted 10 lines
drivers/acpi/processor_perflib.c +6
-2acpi_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);
This doesn't actually help AFAICS, because it only causes acpi_processor_ppc_notifier() to be called for all policies, but pr->performance_platform_limit is re-computed for the target CPU only anyway, so the limit will only be applied to that one.
What happens in the BZ is that invoking cpufreq_update_policy() for all CPUs causes ->verify() to run on all of them which triggers update_turbo_state() and cpuinfo.max_freq update, because global.turbo_disabled has changed.
That is rather less than straightforward and intel_pstate really doesn't need any _PPC change notifications to notice that the MSR_IA32_MISC_ENABLE_TURBO_DISABLE bit has changed as it checks that bit on every P-state update.
So no, we are not going to do this to fix the particular issue at hand.
Quoted 10 lines
drivers/acpi/processor_perflib.c +4
-1+ }else { +cpufreq_update_policy (pr->id); + } + } }int acpi_processor_get_bios_limit (int cpu,unsigned int *limit) --2.17.1
Rafael J. Wysocki
maintainerRe: [PATCH 1/2][RFC v2] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC
Quoted 51 lines · Rafael J. Wysocki
Quoted 28 lines · Chen Yu
On some problematic platforms, the
_PPCnotifier is
only delivered to one CPU, which might cause information
inconsistent between CPUs within the package. Thus introduce a boot up parameter to broadcast this_PPCnotifier onto all online CPUs.Signed-off-by Chen Yu <[email protected]>--- drivers/acpi/processor_perflib.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)drivers/acpi/processor_perflib.c +4
-0@@ -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 drivers/acpi/processor_perflib.c void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) · line 192
Quoted 10 lines
drivers/acpi/processor_perflib.c +6
-2acpi_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);This doesn't actually help AFAICS, because it only causes
acpi_processor_ppc_notifier()to be called for all policies, butpr->performance_platform_limitis re-computed for the target CPU only anyway, so the limit will only be applied to that one.What happens in the BZ is that invoking
cpufreq_update_policy()for all CPUs causes ->verify() to run on all of them which triggersupdate_turbo_state()and cpuinfo.max_freq update, because global.turbo_disabled has changed.
Quoted 4 lines
That is rather less than straightforward and
intel_pstatereally doesn't need any_PPCchange notifications to notice that theMSR_IA32_MISC_ENABLE_TURBO_DISABLEbit has changed as it checks that bit on every P-state update.
Which admittedly may not be necessary if notifications are delivered.
I still don't think that updating all policies from acpi_processor_ppc_has_changed() is a good idea, but yes, there is a problem that if MSR_IA32_MISC_ENABLE_TURBO_DISABLE goes from set to unset, all policies need to be updated to update policy->max accordingly, so looking at MSR_IA32_MISC_ENABLE_TURBO_DISABLE from within the driver without triggering a policy update is not sufficient.
Re: [PATCH 1/2][RFC v2] ACPI: add "processor.broadcast_ppc" hook to broadcast _PPC
Quoted 57 lines · Rafael J. Wysocki
Quoted 51 lines · Rafael J. Wysocki
Quoted 28 lines · Chen Yu
On some problematic platforms, the
_PPCnotifier is
only delivered to one CPU, which might cause information
inconsistent between CPUs within the package. Thus introduce a boot up parameter to broadcast this_PPCnotifier onto all online CPUs.Signed-off-by Chen Yu <[email protected]>--- drivers/acpi/processor_perflib.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)drivers/acpi/processor_perflib.c +4
-0@@ -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 drivers/acpi/processor_perflib.c void acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag) · line 192
Quoted 10 lines
drivers/acpi/processor_perflib.c +6
-2acpi_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);This doesn't actually help AFAICS, because it only causes
acpi_processor_ppc_notifier()to be called for all policies, butpr->performance_platform_limitis re-computed for the target CPU only anyway, so the limit will only be applied to that one.What happens in the BZ is that invoking
cpufreq_update_policy()for all CPUs causes ->verify() to run on all of them which triggersupdate_turbo_state()and cpuinfo.max_freq update, because global.turbo_disabled has changed.Quoted 4 lines
That is rather less than straightforward and
intel_pstatereally doesn't need any_PPCchange notifications to notice that theMSR_IA32_MISC_ENABLE_TURBO_DISABLEbit has changed as it checks that bit on every P-state update.
Quoted 7 lines
Which admittedly may not be necessary if notifications are delivered.
I still don't think that updating all policies from
acpi_processor_ppc_has_changed()is a good idea, but yes, there is a problem that ifMSR_IA32_MISC_ENABLE_TURBO_DISABLEgoes from set to unset, all policies need to be updated to updatepolicy->maxaccordingly,
Agree. policy->max might needed to be updated otherwise we might not get correct cpufreq limit range. According to the report log from bugzilla, if the system boots without AC and then plug the AC after boot up, the max_perf_ratio would be incorrect because policy->max is not updated.
# Plug the AC:
[ 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
Thanks,
Yu
Quoted 3 lines
so looking at
MSR_IA32_MISC_ENABLE_TURBO_DISABLEfrom within the driver without triggering a policy update is not sufficient.
RFC PATCH v2 2/2 ACPI: Update cpuinfo.max after bootup if necessary
In reply to Chen YuOn 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.
--- drivers/cpufreq/cpufreq.c | 2 ++ drivers/cpufreq/intel_pstate.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-)
drivers/cpufreq/cpufreq.c +2 -0
@@ -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 ;
drivers/cpufreq/intel_pstate.c +13 -2
@@ -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; +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;cpufreq_verify_within_limits (policy, policy->cpuinfo.min_freq, -intel_pstate_get_max_freq (cpu)); + max_freq);intel_pstate_adjust_policy_max (policy, cpu);
Signature · 1 line
2.17.1
Rafael J. Wysocki
maintainerRe: [PATCH 2/2][RFC v2] ACPI: Update cpuinfo.max after bootup if necessary
Quoted 38 lines · Chen Yu
On Dell Inc. XPS13 9333, the BIOS changes the value of
MSR_IA32_MISC_ENABLE_TURBO_DISABLEat 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_PPCnotifier fires.Fix this problem by updating the policy's cpuinfo.max when necessary.
Reported-and-tested-by Gabriele Mazzotta <[email protected]>Originally-by Srinivas Pandruvada <[email protected]>Signed-off-by Chen Yu <[email protected]>--- drivers/cpufreq/cpufreq.c | 2 ++ drivers/cpufreq/intel_pstate.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-)drivers/cpufreq/cpufreq.c +2
-0@@ -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 ;drivers/cpufreq/intel_pstate.c +0
-0@@ -2081,11 +2081,17 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,
drivers/cpufreq/intel_pstate.c static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy, · line 2091
Quoted 10 lines
drivers/cpufreq/intel_pstate.c +5
-0static 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.
Quoted 9 lines
drivers/cpufreq/intel_pstate.c +2
-1+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 = {
drivers/cpufreq/intel_pstate.c static struct cpufreq_driver intel_pstate = { · line 2208
Quoted 10 lines
drivers/cpufreq/intel_pstate.c +5
-0static 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.
drivers/cpufreq/intel_pstate.c static struct cpufreq_driver intel_pstate = {
Quoted 7 lines
drivers/cpufreq/intel_pstate.c +1
-2cpufreq_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.
Re: [PATCH 2/2][RFC v2] ACPI: Update cpuinfo.max after bootup if necessary
Quoted 55 lines · Rafael J. Wysocki
Quoted 38 lines · Chen Yu
On Dell Inc. XPS13 9333, the BIOS changes the value of
MSR_IA32_MISC_ENABLE_TURBO_DISABLEat 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_PPCnotifier fires.Fix this problem by updating the policy's cpuinfo.max when necessary.
Reported-and-tested-by Gabriele Mazzotta <[email protected]>Originally-by Srinivas Pandruvada <[email protected]>Signed-off-by Chen Yu <[email protected]>--- drivers/cpufreq/cpufreq.c | 2 ++ drivers/cpufreq/intel_pstate.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-)drivers/cpufreq/cpufreq.c +2
-0@@ -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 ;drivers/cpufreq/intel_pstate.c +0
-0@@ -2081,11 +2081,17 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,drivers/cpufreq/intel_pstate.c static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy, · line 2091
Quoted 10 lines
drivers/cpufreq/intel_pstate.c +5
-0static 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_pstatedoesn't actually use cpuinfo.max_freq for anything and the core doesn't enforce it as a limit.
Quoted 3 lines
All of the computations in the active mode in the driver actually use the current limit anyway AFAICS.
Yes, but it looks like we should also take care of the cpuinfo.max if it changes, I searched the code, it seems that other components might use the policy's cpuinfo.max for some purpose. They might use cpufreq_cpu_get() to get the policy, and use the cpuinfo.max_freq directly, no matter what the mode intel_pstate is in. Such as kvm might use it as the max tsc khz if the tsc is not constant. And i915 might consider the cpuinfo.max_freq to adjust the IA frequency on gen6 platforms. So changing cpuinfo.max might also impact not only cpufreq but also other components too.
Quoted 31 lines
Quoted 9 lines
drivers/cpufreq/intel_pstate.c +2
-1+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 = {drivers/cpufreq/intel_pstate.c static struct cpufreq_driver intel_pstate = { · line 2208
Quoted 10 lines
drivers/cpufreq/intel_pstate.c +5
-0static 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_turboin theintel_pstatesysfs to 1 without this patch.With acpi-cpufreq cpuinfo.max_freq is always the max frequency in the
_PSStable regardless of what the_PSSlimit is. Also settingno_turboto 1 inintel_pstatewithout this patch doesn't cause cpuinfo.max_freq to change and I don't really think that it should.
Quoted 5 lines
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_DISABLEis 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.
Ok, maybe we can check the bit during boot(consider BIOS's setting)?
Quoted 10 lines
drivers/cpufreq/intel_pstate.c static struct cpufreq_driver intel_pstate = {
Quoted 8 lines
drivers/cpufreq/intel_pstate.c +1
-2cpufreq_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.
Ok, I'll test the patch you sent out.
Thanks,
Yu
Rafael J. Wysocki
maintainerRe: [PATCH 2/2][RFC v2] ACPI: Update cpuinfo.max after bootup if necessary
Quoted 111 lines · Yu Chen
Quoted 55 lines · Rafael J. Wysocki
Quoted 38 lines · Chen Yu
On Dell Inc. XPS13 9333, the BIOS changes the value of
MSR_IA32_MISC_ENABLE_TURBO_DISABLEat 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_PPCnotifier fires.Fix this problem by updating the policy's cpuinfo.max when necessary.
Reported-and-tested-by Gabriele Mazzotta <[email protected]>Originally-by Srinivas Pandruvada <[email protected]>Signed-off-by Chen Yu <[email protected]>--- drivers/cpufreq/cpufreq.c | 2 ++ drivers/cpufreq/intel_pstate.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-)drivers/cpufreq/cpufreq.c +2
-0@@ -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 ;drivers/cpufreq/intel_pstate.c +0
-0@@ -2081,11 +2081,17 @@ static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy,drivers/cpufreq/intel_pstate.c static void intel_pstate_adjust_policy_max(struct cpufreq_policy *policy, · line 2091
Quoted 10 lines
drivers/cpufreq/intel_pstate.c +5
-0static 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_pstatedoesn't actually use cpuinfo.max_freq for anything and the core doesn't enforce it as a limit.Quoted 3 lines
All of the computations in the active mode in the driver actually use the current limit anyway AFAICS.
Yes, but it looks like we should also take care of the cpuinfo.max if it changes, I searched the code, it seems that other components might use the policy's cpuinfo.max for some purpose. They might use
cpufreq_cpu_get()to get the policy, and use the cpuinfo.max_freq directly, no matter what the modeintel_pstateis in. Such as kvm might use it as the max tsc khz if the tsc is not constant. And i915 might consider the cpuinfo.max_freq to adjust the IA frequency on gen6 platforms. So changing cpuinfo.max might also impact not only cpufreq but also other components too.Quoted 31 lines
Quoted 9 lines
drivers/cpufreq/intel_pstate.c +2
-1+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 = {drivers/cpufreq/intel_pstate.c static struct cpufreq_driver intel_pstate = { · line 2208
Quoted 10 lines
drivers/cpufreq/intel_pstate.c +5
-0static 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_turboin theintel_pstatesysfs to 1 without this patch.With acpi-cpufreq cpuinfo.max_freq is always the max frequency in the
_PSStable regardless of what the_PSSlimit is. Also settingno_turboto 1 inintel_pstatewithout this patch doesn't cause cpuinfo.max_freq to change and I don't really think that it should.Quoted 5 lines
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_DISABLEis 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.Ok, maybe we can check the bit during boot(consider BIOS's setting)?
Quoted 5 lines
Quoted 5 lines
drivers/cpufreq/intel_pstate.c +1
-1cpufreq_verify_within_limits (policy, policy->cpuinfo.min_freq, -intel_pstate_get_max_freq (cpu)); + max_freq);
Quoted 6 lines
Quoted 5 lines
drivers/cpufreq/intel_pstate.c static struct cpufreq_driver intel_pstate = {
Quoted 3 lines
drivers/cpufreq/intel_pstate.c +0
-1intel_pstate_adjust_policy_max (policy, cpu); --It looks like I need to think about this a bit more.
Ok, I'll test the patch you sent out.
Please do.