| Message ID | 8366982.T7Z3S40VBb@kreacher (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | intel_pstate: Turbo disabled handling rework | expand |
On Mon, 2024-03-25 at 18:02 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The global.turbo_disabled is updated quite often, especially in the > passive mode in which case it is updated every time the scheduler calls > into the driver. However, this is generally not necessary and it adds > MSR read overhead to scheduler code paths (and that particular MSR is > slow to read). > > For this reason, make the driver read MSR_IA32_MISC_ENABLE_TURBO_DISABLE > just once at the cpufreq driver registration time and remove all of the > in-flight updates of global.turbo_disabled. Hi Rafael and Srinivas, Thanks for the clean up, but unfortunately on one of my laptops (based on i5-11300H) MSR_IA32_MISC_ENABLE_TURBO_DISABLE is mysteriously changing from 1 to 0 in about one minute after system boot. I've no idea why this is happening (firmware is doing some stupid thing?) I've noticed the issue before and "hacked it around" (https://bugzilla.kernel.org/show_bug.cgi?id=218702). But after this change I can no longer hack it around and the system is much slower. Is it possible to hack it around again?
Hi Xi, On Sun, 2024-06-02 at 11:21 +0800, Xi Ruoyao wrote: > On Mon, 2024-03-25 at 18:02 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The global.turbo_disabled is updated quite often, especially in the > > passive mode in which case it is updated every time the scheduler > > calls > > into the driver. However, this is generally not necessary and it > > adds > > MSR read overhead to scheduler code paths (and that particular MSR > > is > > slow to read). > > > > For this reason, make the driver read > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE > > just once at the cpufreq driver registration time and remove all of > > the > > in-flight updates of global.turbo_disabled. > > Hi Rafael and Srinivas, > > Thanks for the clean up, but unfortunately on one of my laptops > (based > on i5-11300H) MSR_IA32_MISC_ENABLE_TURBO_DISABLE is mysteriously > changing from 1 to 0 in about one minute after system boot. I've no > idea why this is happening (firmware is doing some stupid thing?) > > I've noticed the issue before and "hacked it around" > (https://bugzilla.kernel.org/show_bug.cgi?id=218702). But after this > change I can no longer hack it around and the system is much slower. > > Is it possible to hack it around again? > Please try the attached diff and build kernel and try. git apply update_max_freq.diff Then build kernel and install. Thanks, Srinivas
On Sat, 2024-06-01 at 21:03 -0700, srinivas pandruvada wrote: > Hi Xi, > > On Sun, 2024-06-02 at 11:21 +0800, Xi Ruoyao wrote: > > On Mon, 2024-03-25 at 18:02 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The global.turbo_disabled is updated quite often, especially in the > > > passive mode in which case it is updated every time the scheduler > > > calls > > > into the driver. However, this is generally not necessary and it > > > adds > > > MSR read overhead to scheduler code paths (and that particular MSR > > > is > > > slow to read). > > > > > > For this reason, make the driver read > > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE > > > just once at the cpufreq driver registration time and remove all of > > > the > > > in-flight updates of global.turbo_disabled. > > > > Hi Rafael and Srinivas, > > > > Thanks for the clean up, but unfortunately on one of my laptops > > (based > > on i5-11300H) MSR_IA32_MISC_ENABLE_TURBO_DISABLE is mysteriously > > changing from 1 to 0 in about one minute after system boot. I've no > > idea why this is happening (firmware is doing some stupid thing?) > > > > I've noticed the issue before and "hacked it around" > > (https://bugzilla.kernel.org/show_bug.cgi?id=218702). But after this > > change I can no longer hack it around and the system is much slower. > > > > Is it possible to hack it around again? > > > Please try the attached diff and build kernel and try. > > git apply update_max_freq.diff > > Then build kernel and install. Unfortunately it didn't work. Then I tried: @@ -1304,6 +1310,10 @@ static ssize_t store_no_turbo(struct kobject *a, struct kobj_attribute *b, if (no_turbo == global.no_turbo) goto unlock_driver; + global.turbo_disabled = turbo_is_disabled(); + global.no_turbo = global.turbo_disabled; + arch_set_max_freq_ratio(global.turbo_disabled); + if (global.turbo_disabled) { pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n"); count = -EPERM; and my old hack worked again. Curiously after I writing 0 to /sys/devices/system/cpu/intel_pstate/no_turbo successfully, your code is triggered. $ dmesg | grep intel_pstate [ 0.554425] intel_pstate: Intel P-state driver initializing [ 0.554877] intel_pstate: HWP enabled [ 1.780021] intel_pstate: Turbo disabled by BIOS or unavailable on processor [ 21.789044] intel_pstate: intel_pstate_update_limits cpu:0 [ 21.789053] intel_pstate: intel_pstate_update_limits cpu:1 [ 21.789060] intel_pstate: intel_pstate_update_limits cpu:2 [ 21.789189] intel_pstate: intel_pstate_update_limits cpu:3 [ 21.789198] intel_pstate: intel_pstate_update_limits cpu:4 [ 21.789203] intel_pstate: intel_pstate_update_limits cpu:5 [ 21.789209] intel_pstate: intel_pstate_update_limits cpu:6 [ 21.789276] intel_pstate: intel_pstate_update_limits cpu:7 The message at [1.780021] is from the first attempt writing 0 to /sys/devices/system/cpu/intel_pstate/no_turbo when MSR_IA32_MISC_ENABLE_TURBO_DISABLE is still 1.
On Sun, 2024-06-02 at 12:25 +0800, Xi Ruoyao wrote: > On Sat, 2024-06-01 at 21:03 -0700, srinivas pandruvada wrote: > > Hi Xi, > > > > On Sun, 2024-06-02 at 11:21 +0800, Xi Ruoyao wrote: > > > On Mon, 2024-03-25 at 18:02 +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > The global.turbo_disabled is updated quite often, especially in > > > > the > > > > passive mode in which case it is updated every time the > > > > scheduler > > > > calls > > > > into the driver. However, this is generally not necessary and > > > > it > > > > adds > > > > MSR read overhead to scheduler code paths (and that particular > > > > MSR > > > > is > > > > slow to read). > > > > > > > > For this reason, make the driver read > > > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE > > > > just once at the cpufreq driver registration time and remove > > > > all of > > > > the > > > > in-flight updates of global.turbo_disabled. > > > > > > Hi Rafael and Srinivas, > > > > > > Thanks for the clean up, but unfortunately on one of my laptops > > > (based > > > on i5-11300H) MSR_IA32_MISC_ENABLE_TURBO_DISABLE is mysteriously > > > changing from 1 to 0 in about one minute after system boot. I've > > > no > > > idea why this is happening (firmware is doing some stupid thing?) > > > > > > I've noticed the issue before and "hacked it around" > > > (https://bugzilla.kernel.org/show_bug.cgi?id=218702). But after > > > this > > > change I can no longer hack it around and the system is much > > > slower. > > > > > > Is it possible to hack it around again? > > > > > Please try the attached diff and build kernel and try. > > > > git apply update_max_freq.diff > > > > Then build kernel and install. > > Unfortunately it didn't work. Then I tried: > > @@ -1304,6 +1310,10 @@ static ssize_t store_no_turbo(struct kobject > *a, struct kobj_attribute *b, > if (no_turbo == global.no_turbo) > goto unlock_driver; > > + global.turbo_disabled = turbo_is_disabled(); > + global.no_turbo = global.turbo_disabled; > + arch_set_max_freq_ratio(global.turbo_disabled); > + > if (global.turbo_disabled) { > pr_notice_once("Turbo disabled by BIOS or unavailable > on processor\n"); > count = -EPERM; > > and my old hack worked again. Curiously after I writing 0 to > /sys/devices/system/cpu/intel_pstate/no_turbo successfully, your code > is > triggered. > > $ dmesg | grep intel_pstate > [ 0.554425] intel_pstate: Intel P-state driver initializing > [ 0.554877] intel_pstate: HWP enabled > [ 1.780021] intel_pstate: Turbo disabled by BIOS or unavailable on > processor > [ 21.789044] intel_pstate: intel_pstate_update_limits cpu:0 > [ 21.789053] intel_pstate: intel_pstate_update_limits cpu:1 > [ 21.789060] intel_pstate: intel_pstate_update_limits cpu:2 > [ 21.789189] intel_pstate: intel_pstate_update_limits cpu:3 > [ 21.789198] intel_pstate: intel_pstate_update_limits cpu:4 > [ 21.789203] intel_pstate: intel_pstate_update_limits cpu:5 > [ 21.789209] intel_pstate: intel_pstate_update_limits cpu:6 > [ 21.789276] intel_pstate: intel_pstate_update_limits cpu:7 > > The message at [1.780021] is from the first attempt writing 0 to > /sys/devices/system/cpu/intel_pstate/no_turbo when > MSR_IA32_MISC_ENABLE_TURBO_DISABLE is still 1 This requires user action, Please add a pr_info() to https://elixir.bootlin.com/linux/v6.10-rc1/C/ident/acpi_processor_notify Check if you got any message Also what is cat /proc/cpuinfo and cpuid -1 Thanks, Srinivas
On Sun, 2024-06-02 at 06:40 -0700, srinivas pandruvada wrote: /* snip */ > This requires user action, > Please add a > pr_info() to > https://elixir.bootlin.com/linux/v6.10-rc1/C/ident/acpi_processor_notify > > Check if you got any message With diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index 67db60eda370..4585eb6566c8 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -57,6 +57,8 @@ static void acpi_processor_notify(acpi_handle handle, u32 event, void *data) struct acpi_processor *pr; int saved; + pr_info("acpi_processor_notify: %d\n", event); + if (device->handle != handle) return; I get nothing. > Also what is > cat /proc/cpuinfo > and > cpuid -1 Attached.
On Mon, 2024-06-03 at 00:07 +0800, Xi Ruoyao wrote: > On Sun, 2024-06-02 at 06:40 -0700, srinivas pandruvada wrote: > > /* snip */ > > > This requires user action, > > Please add a > > pr_info() to > > https://elixir.bootlin.com/linux/v6.10-rc1/C/ident/acpi_processor_notify > > > > Check if you got any message > > With > > diff --git a/drivers/acpi/processor_driver.c > b/drivers/acpi/processor_driver.c > index 67db60eda370..4585eb6566c8 100644 > --- a/drivers/acpi/processor_driver.c > +++ b/drivers/acpi/processor_driver.c > @@ -57,6 +57,8 @@ static void acpi_processor_notify(acpi_handle > handle, u32 event, void *data) > struct acpi_processor *pr; > int saved; > > + pr_info("acpi_processor_notify: %d\n", event); > + > if (device->handle != handle) > return; > > > I get nothing. > What is the output of: grep . /sys/devices/system/cpu/intel_pstate/* Also rdmsr 0x771 rdmsr 0x774 Try these three patches. Don't worry about the commit description for this issue. Please send me full dmesg after you see the issue. Thanks, Srinivas > > Also what is > > cat /proc/cpuinfo > > and > > cpuid -1 > > Attached. > >
On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote: /* snip */ > What is the output of: > grep . /sys/devices/system/cpu/intel_pstate/* > > Also > rdmsr 0x771 > rdmsr 0x774 > > > Try these three patches. Don't worry about the commit description for > this issue. Unfortunately they still do not fix the issue for me. The outputs of grep and rdmsr commands are initially: /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 /sys/devices/system/cpu/intel_pstate/no_turbo:1 /sys/devices/system/cpu/intel_pstate/num_pstates:41 /sys/devices/system/cpu/intel_pstate/status:active /sys/devices/system/cpu/intel_pstate/turbo_pct:33 rdmsr 0x771: 10d1f2c rdmsr 0x774: 1f04 But it then changes to: /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 /sys/devices/system/cpu/intel_pstate/no_turbo:1 /sys/devices/system/cpu/intel_pstate/num_pstates:41 /sys/devices/system/cpu/intel_pstate/status:active /sys/devices/system/cpu/intel_pstate/turbo_pct:33 rdmsr 0x771: 10c1f2c rdmsr 0x774: 1f04 It seems only the output of rdmsr 0x771 has changed. And if I read the SDM correctly it's a "Most_Efficient_Performance" change. > Please send me full dmesg after you see the issue. Attached.
On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote: > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote: > > /* snip */ > > > What is the output of: > > grep . /sys/devices/system/cpu/intel_pstate/* > > > > Also > > rdmsr 0x771 > > rdmsr 0x774 > > > > > > Try these three patches. Don't worry about the commit description > > for > > this issue. > > Unfortunately they still do not fix the issue for me. > > The outputs of grep and rdmsr commands are initially: > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > /sys/devices/system/cpu/intel_pstate/status:active > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > rdmsr 0x771: 10d1f2c > rdmsr 0x774: 1f04 > > But it then changes to: > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > /sys/devices/system/cpu/intel_pstate/status:active > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > rdmsr 0x771: 10c1f2c > rdmsr 0x774: 1f04 > > It seems only the output of rdmsr 0x771 has changed. And if I read > the > SDM correctly it's a "Most_Efficient_Performance" change. That is fine. We don't have any notifications either via ACPI or via HWP interrupt. I think it was working by chance before this change as by the cpufreq core is trying to set policy, the turbo is enabled by the firmware. What is this laptop make and model? Thanks, Srinivas > > > Please send me full dmesg after you see the issue. > > Attached. >
On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote: > On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote: > > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote: > > > > /* snip */ > > > > > What is the output of: > > > grep . /sys/devices/system/cpu/intel_pstate/* > > > > > > Also > > > rdmsr 0x771 > > > rdmsr 0x774 > > > > > > > > > Try these three patches. Don't worry about the commit description > > > for > > > this issue. > > > > Unfortunately they still do not fix the issue for me. > > > > The outputs of grep and rdmsr commands are initially: > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > /sys/devices/system/cpu/intel_pstate/status:active > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > rdmsr 0x771: 10d1f2c > > rdmsr 0x774: 1f04 > > > > But it then changes to: > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > /sys/devices/system/cpu/intel_pstate/status:active > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > rdmsr 0x771: 10c1f2c > > rdmsr 0x774: 1f04 > > > > It seems only the output of rdmsr 0x771 has changed. And if I read > > the > > SDM correctly it's a "Most_Efficient_Performance" change. > That is fine. > > We don't have any notifications either via ACPI or via HWP interrupt. > I think it was working by chance before this change as by the cpufreq > core is trying to set policy, the turbo is enabled by the firmware. > > What is this laptop make and model? It's a Hasee X5-2021S5H. Hasee is known for producing some laptops very cheap but often having "minor" issues. So I guess the firmware is doing some stupid thing. But turbo works just fine on Windows 11 so it'd be better if we could make it work for Linux too.
On Mon, Jun 3, 2024 at 7:44 PM Xi Ruoyao <xry111@xry111.site> wrote: > > On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote: > > On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote: > > > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote: > > > > > > /* snip */ > > > > > > > What is the output of: > > > > grep . /sys/devices/system/cpu/intel_pstate/* > > > > > > > > Also > > > > rdmsr 0x771 > > > > rdmsr 0x774 > > > > > > > > > > > > Try these three patches. Don't worry about the commit description > > > > for > > > > this issue. > > > > > > Unfortunately they still do not fix the issue for me. > > > > > > The outputs of grep and rdmsr commands are initially: > > > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > > /sys/devices/system/cpu/intel_pstate/status:active > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > > rdmsr 0x771: 10d1f2c > > > rdmsr 0x774: 1f04 > > > > > > But it then changes to: > > > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > > /sys/devices/system/cpu/intel_pstate/status:active > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > > rdmsr 0x771: 10c1f2c > > > rdmsr 0x774: 1f04 > > > > > > It seems only the output of rdmsr 0x771 has changed. And if I read > > > the > > > SDM correctly it's a "Most_Efficient_Performance" change. > > That is fine. > > > > We don't have any notifications either via ACPI or via HWP interrupt. > > I think it was working by chance before this change as by the cpufreq > > core is trying to set policy, the turbo is enabled by the firmware. > > > > What is this laptop make and model? > > It's a Hasee X5-2021S5H. > > Hasee is known for producing some laptops very cheap but often having > "minor" issues. So I guess the firmware is doing some stupid thing. > > But turbo works just fine on Windows 11 so it'd be better if we could > make it work for Linux too. In principle, there are two things that can be done about this. First, MSR_IA32_MISC_ENABLE_TURBO_DISABLE on this system can be ignored altogether, but that would require adding a quirk. Second, a delayed work can be added to check the MSR long enough after initialization and update global.turbo_disabled if it is 1. However, that would require some code surgery.
On Mon, 2024-06-03 at 20:32 +0200, Rafael J. Wysocki wrote: > On Mon, Jun 3, 2024 at 7:44 PM Xi Ruoyao <xry111@xry111.site> wrote: > > > > On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote: > > > On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote: > > > > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote: > > > > > > > > /* snip */ > > > > > > > > > What is the output of: > > > > > grep . /sys/devices/system/cpu/intel_pstate/* > > > > > > > > > > Also > > > > > rdmsr 0x771 > > > > > rdmsr 0x774 > > > > > > > > > > > > > > > Try these three patches. Don't worry about the commit > > > > > description > > > > > for > > > > > this issue. > > > > > > > > Unfortunately they still do not fix the issue for me. > > > > > > > > The outputs of grep and rdmsr commands are initially: > > > > > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > > > /sys/devices/system/cpu/intel_pstate/status:active > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > > > rdmsr 0x771: 10d1f2c > > > > rdmsr 0x774: 1f04 > > > > > > > > But it then changes to: > > > > > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > > > /sys/devices/system/cpu/intel_pstate/status:active > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > > > rdmsr 0x771: 10c1f2c > > > > rdmsr 0x774: 1f04 > > > > > > > > It seems only the output of rdmsr 0x771 has changed. And if I > > > > read > > > > the > > > > SDM correctly it's a "Most_Efficient_Performance" change. > > > That is fine. > > > > > > We don't have any notifications either via ACPI or via HWP > > > interrupt. > > > I think it was working by chance before this change as by the > > > cpufreq > > > core is trying to set policy, the turbo is enabled by the > > > firmware. > > > > > > What is this laptop make and model? > > > > It's a Hasee X5-2021S5H. > > > > Hasee is known for producing some laptops very cheap but often > > having > > "minor" issues. So I guess the firmware is doing some stupid > > thing. > > > > But turbo works just fine on Windows 11 so it'd be better if we > > could > > make it work for Linux too. > > In principle, there are two things that can be done about this. > > First, MSR_IA32_MISC_ENABLE_TURBO_DISABLE on this system can be > ignored altogether, but that would require adding a quirk. > > Second, a delayed work can be added to check the MSR long enough > after > initialization and update global.turbo_disabled if it is 1. However, > that would require some code surgery. I was about to send this suggestion. For the first one we can always program the HWP_REQ.max to HWP_CAP.max and let hardware do the clipping. But this is not friendly to passive mode. But display of scalig_max_freq still should reflect the reality. Thanks, Srinivas >
On Mon, 2024-06-03 at 11:47 -0700, srinivas pandruvada wrote: > On Mon, 2024-06-03 at 20:32 +0200, Rafael J. Wysocki wrote: > > On Mon, Jun 3, 2024 at 7:44 PM Xi Ruoyao <xry111@xry111.site> > > wrote: > > > > > > On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote: > > > > On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote: > > > > > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote: > > > > > > > > > > /* snip */ > > > > > > > > > > > What is the output of: > > > > > > grep . /sys/devices/system/cpu/intel_pstate/* > > > > > > > > > > > > Also > > > > > > rdmsr 0x771 > > > > > > rdmsr 0x774 > > > > > > > > > > > > > > > > > > Try these three patches. Don't worry about the commit > > > > > > description > > > > > > for > > > > > > this issue. > > > > > > > > > > Unfortunately they still do not fix the issue for me. > > > > > > > > > > The outputs of grep and rdmsr commands are initially: > > > > > > > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > > > > /sys/devices/system/cpu/intel_pstate/status:active > > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > > > > rdmsr 0x771: 10d1f2c > > > > > rdmsr 0x774: 1f04 > > > > > > > > > > But it then changes to: > > > > > > > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > > > > /sys/devices/system/cpu/intel_pstate/status:active > > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > > > > rdmsr 0x771: 10c1f2c > > > > > rdmsr 0x774: 1f04 > > > > > > > > > > It seems only the output of rdmsr 0x771 has changed. And if > > > > > I > > > > > read > > > > > the > > > > > SDM correctly it's a "Most_Efficient_Performance" change. > > > > That is fine. > > > > > > > > We don't have any notifications either via ACPI or via HWP > > > > interrupt. > > > > I think it was working by chance before this change as by the > > > > cpufreq > > > > core is trying to set policy, the turbo is enabled by the > > > > firmware. > > > > > > > > What is this laptop make and model? > > > > > > It's a Hasee X5-2021S5H. > > > > > > Hasee is known for producing some laptops very cheap but often > > > having > > > "minor" issues. So I guess the firmware is doing some stupid > > > thing. > > > > > > But turbo works just fine on Windows 11 so it'd be better if we > > > could > > > make it work for Linux too. > > > > In principle, there are two things that can be done about this. > > > > First, MSR_IA32_MISC_ENABLE_TURBO_DISABLE on this system can be > > ignored altogether, but that would require adding a quirk. > > > > Second, a delayed work can be added to check the MSR long enough > > after > > initialization and update global.turbo_disabled if it is 1. > > However, > > that would require some code surgery. > Try the attached diff for this case. Thanks, Srinivas > I was about to send this suggestion. > > For the first one we can always program the HWP_REQ.max to > HWP_CAP.max > and let hardware do the clipping. But this is not friendly to > passive > mode. But display of scalig_max_freq still should reflect the > reality. > > > Thanks, > Srinivas > > > > >
On Mon, 2024-06-03 at 11:47 -0700, srinivas pandruvada wrote: > On Mon, 2024-06-03 at 20:32 +0200, Rafael J. Wysocki wrote: > > On Mon, Jun 3, 2024 at 7:44 PM Xi Ruoyao <xry111@xry111.site> > > wrote: > > > > > > On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote: > > > > On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote: > > > > > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote: > > > > > > > > > > /* snip */ > > > > > > > > > > > What is the output of: > > > > > > grep . /sys/devices/system/cpu/intel_pstate/* > > > > > > > > > > > > Also > > > > > > rdmsr 0x771 > > > > > > rdmsr 0x774 > > > > > > > > > > > > > > > > > > Try these three patches. Don't worry about the commit > > > > > > description > > > > > > for > > > > > > this issue. > > > > > > > > > > Unfortunately they still do not fix the issue for me. > > > > > > > > > > The outputs of grep and rdmsr commands are initially: > > > > > > > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > > > > /sys/devices/system/cpu/intel_pstate/status:active > > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > > > > rdmsr 0x771: 10d1f2c > > > > > rdmsr 0x774: 1f04 > > > > > > > > > > But it then changes to: > > > > > > > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0 > > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100 > > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9 > > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1 > > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41 > > > > > /sys/devices/system/cpu/intel_pstate/status:active > > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33 > > > > > rdmsr 0x771: 10c1f2c > > > > > rdmsr 0x774: 1f04 > > > > > > > > > > It seems only the output of rdmsr 0x771 has changed. And if > > > > > I > > > > > read > > > > > the > > > > > SDM correctly it's a "Most_Efficient_Performance" change. > > > > That is fine. > > > > > > > > We don't have any notifications either via ACPI or via HWP > > > > interrupt. > > > > I think it was working by chance before this change as by the > > > > cpufreq > > > > core is trying to set policy, the turbo is enabled by the > > > > firmware. > > > > > > > > What is this laptop make and model? > > > > > > It's a Hasee X5-2021S5H. > > > > > > Hasee is known for producing some laptops very cheap but often > > > having > > > "minor" issues. So I guess the firmware is doing some stupid > > > thing. > > > > > > But turbo works just fine on Windows 11 so it'd be better if we > > > could > > > make it work for Linux too. > > > > In principle, there are two things that can be done about this. > > > > First, MSR_IA32_MISC_ENABLE_TURBO_DISABLE on this system can be > > ignored altogether, but that would require adding a quirk. > > > > Second, a delayed work can be added to check the MSR long enough > > after > > initialization and update global.turbo_disabled if it is 1. > > However, > > that would require some code surgery. > Something like the attached which does same way as user space no_turbo update. Thanks, Srinivas > I was about to send this suggestion. > > For the first one we can always program the HWP_REQ.max to > HWP_CAP.max > and let hardware do the clipping. But this is not friendly to > passive > mode. But display of scalig_max_freq still should reflect the > reality. > > > Thanks, > Srinivas > > > > >
On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote: > > > Second, a delayed work can be added to check the MSR long enough > > > after > > > initialization and update global.turbo_disabled if it is 1. > > > However, > > > that would require some code surgery. > > > Something like the attached which does same way as user space no_turbo > update. > static int intel_pstate_register_driver(struct cpufreq_driver *driver) > { > int ret; > @@ -3114,6 +3137,9 @@ static int intel_pstate_register_driver(struct cpufreq_driver *driver) > global.turbo_disabled = turbo_is_disabled(); > global.no_turbo = global.turbo_disabled; > > + if (global.turbo_disabled) > + schedule_delayed_work(&turbo_work, HZ); > + I have to change it to 20 * HZ to make it work for me. 15 * HZ does not work.
On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote: > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote: > > > > > Second, a delayed work can be added to check the MSR long > > > > enough > > > > after > > > > initialization and update global.turbo_disabled if it is 1. > > > > However, > > > > that would require some code surgery. > > > > > Something like the attached which does same way as user space > > no_turbo > > update. > > > static int intel_pstate_register_driver(struct cpufreq_driver > > *driver) > > { > > int ret; > > @@ -3114,6 +3137,9 @@ static int > > intel_pstate_register_driver(struct cpufreq_driver *driver) > > global.turbo_disabled = turbo_is_disabled(); > > global.no_turbo = global.turbo_disabled; > > > > + if (global.turbo_disabled) > > + schedule_delayed_work(&turbo_work, HZ); > > + > > I have to change it to 20 * HZ to make it work for me. 15 * HZ does > not > work. Is there any consistency or it is changing every time? Thanks, Srinivas >
On Tue, 2024-06-04 at 03:29 -0700, srinivas pandruvada wrote: > On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote: > > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote: > > > > > > > Second, a delayed work can be added to check the MSR long > > > > > enough > > > > > after > > > > > initialization and update global.turbo_disabled if it is 1. > > > > > However, > > > > > that would require some code surgery. > > > > > > > Something like the attached which does same way as user space > > > no_turbo > > > update. > > > > > static int intel_pstate_register_driver(struct cpufreq_driver > > > *driver) > > > { > > > int ret; > > > @@ -3114,6 +3137,9 @@ static int > > > intel_pstate_register_driver(struct cpufreq_driver *driver) > > > global.turbo_disabled = turbo_is_disabled(); > > > global.no_turbo = global.turbo_disabled; > > > > > > + if (global.turbo_disabled) > > > + schedule_delayed_work(&turbo_work, HZ); > > > + > > > > I have to change it to 20 * HZ to make it work for me. 15 * HZ does > > not > > work. > > Is there any consistency or it is changing every time? It seems consistent.
On Tue, 2024-06-04 at 18:32 +0800, Xi Ruoyao wrote: > On Tue, 2024-06-04 at 03:29 -0700, srinivas pandruvada wrote: > > On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote: > > > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote: > > > > > > > > > Second, a delayed work can be added to check the MSR long > > > > > > enough > > > > > > after > > > > > > initialization and update global.turbo_disabled if it is 1. > > > > > > However, > > > > > > that would require some code surgery. > > > > > > > > > Something like the attached which does same way as user space > > > > no_turbo > > > > update. > > > > > > > static int intel_pstate_register_driver(struct cpufreq_driver > > > > *driver) > > > > { > > > > int ret; > > > > @@ -3114,6 +3137,9 @@ static int > > > > intel_pstate_register_driver(struct cpufreq_driver *driver) > > > > global.turbo_disabled = turbo_is_disabled(); > > > > global.no_turbo = global.turbo_disabled; > > > > > > > > + if (global.turbo_disabled) > > > > + schedule_delayed_work(&turbo_work, HZ); > > > > + > > > > > > I have to change it to 20 * HZ to make it work for me. 15 * HZ > > > does > > > not > > > work. > > > > Is there any consistency or it is changing every time? > > It seems consistent. With such a delay, I am not sure how this even worked before. Can you revert the patch in question and use kernel dynamic debug dyndbg="file intel_pstate.c +p" kernel command line and collect log for 30 seconds? Thanks, Srinivas > >
On Tue, Jun 4, 2024 at 6:41 PM srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > On Tue, 2024-06-04 at 18:32 +0800, Xi Ruoyao wrote: > > On Tue, 2024-06-04 at 03:29 -0700, srinivas pandruvada wrote: > > > On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote: > > > > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote: > > > > > > > > > > > Second, a delayed work can be added to check the MSR long > > > > > > > enough > > > > > > > after > > > > > > > initialization and update global.turbo_disabled if it is 1. > > > > > > > However, > > > > > > > that would require some code surgery. > > > > > > > > > > > Something like the attached which does same way as user space > > > > > no_turbo > > > > > update. > > > > > > > > > static int intel_pstate_register_driver(struct cpufreq_driver > > > > > *driver) > > > > > { > > > > > int ret; > > > > > @@ -3114,6 +3137,9 @@ static int > > > > > intel_pstate_register_driver(struct cpufreq_driver *driver) > > > > > global.turbo_disabled = turbo_is_disabled(); > > > > > global.no_turbo = global.turbo_disabled; > > > > > > > > > > + if (global.turbo_disabled) > > > > > + schedule_delayed_work(&turbo_work, HZ); > > > > > + > > > > > > > > I have to change it to 20 * HZ to make it work for me. 15 * HZ > > > > does > > > > not > > > > work. > > > > > > Is there any consistency or it is changing every time? > > > > It seems consistent. > With such a delay, I am not sure how this even worked before. > Can you revert the patch in question and use kernel dynamic debug > dyndbg="file intel_pstate.c +p" kernel command line and collect log for > 30 seconds? I think that it worked because the MSR was read every time intel_pstate ran, so it got updated at one point and stayed that way.
On Tue, 2024-06-04 at 18:46 +0200, Rafael J. Wysocki wrote: > On Tue, Jun 4, 2024 at 6:41 PM srinivas pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > > > On Tue, 2024-06-04 at 18:32 +0800, Xi Ruoyao wrote: > > > On Tue, 2024-06-04 at 03:29 -0700, srinivas pandruvada wrote: > > > > On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote: > > > > > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote: > > > > > > > > > > > > > Second, a delayed work can be added to check the MSR > > > > > > > > long > > > > > > > > enough > > > > > > > > after > > > > > > > > initialization and update global.turbo_disabled if it > > > > > > > > is 1. > > > > > > > > However, > > > > > > > > that would require some code surgery. > > > > > > > > > > > > > Something like the attached which does same way as user > > > > > > space > > > > > > no_turbo > > > > > > update. > > > > > > > > > > > static int intel_pstate_register_driver(struct > > > > > > cpufreq_driver > > > > > > *driver) > > > > > > { > > > > > > int ret; > > > > > > @@ -3114,6 +3137,9 @@ static int > > > > > > intel_pstate_register_driver(struct cpufreq_driver *driver) > > > > > > global.turbo_disabled = turbo_is_disabled(); > > > > > > global.no_turbo = global.turbo_disabled; > > > > > > > > > > > > + if (global.turbo_disabled) > > > > > > + schedule_delayed_work(&turbo_work, HZ); > > > > > > + > > > > > > > > > > I have to change it to 20 * HZ to make it work for me. 15 * > > > > > HZ > > > > > does > > > > > not > > > > > work. > > > > > > > > Is there any consistency or it is changing every time? > > > > > > It seems consistent. > > With such a delay, I am not sure how this even worked before. > > Can you revert the patch in question and use kernel dynamic debug > > dyndbg="file intel_pstate.c +p" kernel command line and collect log > > for > > 30 seconds? > > I think that it worked because the MSR was read every time > intel_pstate ran, so it got updated at one point and stayed that way. But here HWP in active mode is getting used. So it should have fewer request calls to set accept via cpufreq set_policy() callback or with some HWP interrupt.
On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote: > > > With such a delay, I am not sure how this even worked before. It didn't work out of box but it worked after manually writing 0 to no_turbo after 20 seconds, see https://bugzilla.kernel.org/show_bug.cgi?id=218702. > > > Can you revert the patch in question and use kernel dynamic debug > > > dyndbg="file intel_pstate.c +p" kernel command line and collect log > > > for > > > 30 seconds? Attached. The related part seems: [ 0.553606] intel_pstate: Intel P-state driver initializing [ 0.553630] intel_pstate: controlling: cpu 0 [ 0.553640] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 0.553642] intel_pstate: cpu:0 min_policy_perf:4 max_policy_perf:31 [ 0.553643] intel_pstate: cpu:0 global_min:0 global_max:44 [ 0.553644] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4 [ 0.553680] intel_pstate: controlling: cpu 1 [ 0.553702] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 0.553703] intel_pstate: cpu:1 min_policy_perf:4 max_policy_perf:31 [ 0.553704] intel_pstate: cpu:1 global_min:0 global_max:44 [ 0.553705] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4 [ 0.553742] intel_pstate: controlling: cpu 2 [ 0.553763] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 0.553764] intel_pstate: cpu:2 min_policy_perf:4 max_policy_perf:31 [ 0.553765] intel_pstate: cpu:2 global_min:0 global_max:44 [ 0.553766] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4 [ 0.553809] intel_pstate: controlling: cpu 3 [ 0.553830] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 0.553831] intel_pstate: cpu:3 min_policy_perf:4 max_policy_perf:31 [ 0.553831] intel_pstate: cpu:3 global_min:0 global_max:44 [ 0.553832] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4 [ 0.553868] intel_pstate: controlling: cpu 4 [ 0.553892] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 0.553893] intel_pstate: cpu:4 min_policy_perf:4 max_policy_perf:31 [ 0.553894] intel_pstate: cpu:4 global_min:0 global_max:44 [ 0.553895] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4 [ 0.553943] intel_pstate: controlling: cpu 5 [ 0.553967] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 0.553968] intel_pstate: cpu:5 min_policy_perf:4 max_policy_perf:31 [ 0.553968] intel_pstate: cpu:5 global_min:0 global_max:44 [ 0.553969] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4 [ 0.554009] intel_pstate: controlling: cpu 6 [ 0.554034] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 0.554035] intel_pstate: cpu:6 min_policy_perf:4 max_policy_perf:31 [ 0.554036] intel_pstate: cpu:6 global_min:0 global_max:44 [ 0.554037] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4 [ 0.554077] intel_pstate: controlling: cpu 7 [ 0.554098] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 0.554099] intel_pstate: cpu:7 min_policy_perf:4 max_policy_perf:31 [ 0.554099] intel_pstate: cpu:7 global_min:0 global_max:44 [ 0.554100] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4 [ 0.554104] intel_pstate: HWP enabled [ 2.183669] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 2.183675] intel_pstate: cpu:6 min_policy_perf:4 max_policy_perf:31 [ 2.183677] intel_pstate: cpu:6 global_min:4 global_max:44 [ 2.183679] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4 [ 2.183710] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 2.183711] intel_pstate: cpu:4 min_policy_perf:4 max_policy_perf:31 [ 2.183713] intel_pstate: cpu:4 global_min:4 global_max:44 [ 2.183715] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4 [ 2.183742] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 2.183744] intel_pstate: cpu:2 min_policy_perf:4 max_policy_perf:31 [ 2.183745] intel_pstate: cpu:2 global_min:4 global_max:44 [ 2.183747] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4 [ 2.183773] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 2.183775] intel_pstate: cpu:0 min_policy_perf:4 max_policy_perf:31 [ 2.183776] intel_pstate: cpu:0 global_min:4 global_max:44 [ 2.183777] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4 [ 2.183803] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 2.183805] intel_pstate: cpu:7 min_policy_perf:4 max_policy_perf:31 [ 2.183806] intel_pstate: cpu:7 global_min:4 global_max:44 [ 2.183807] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4 [ 2.183831] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 2.183833] intel_pstate: cpu:5 min_policy_perf:4 max_policy_perf:31 [ 2.183834] intel_pstate: cpu:5 global_min:4 global_max:44 [ 2.183836] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4 [ 2.183862] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 2.183864] intel_pstate: cpu:3 min_policy_perf:4 max_policy_perf:31 [ 2.183865] intel_pstate: cpu:3 global_min:4 global_max:44 [ 2.183867] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4 [ 2.183893] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000 [ 2.183895] intel_pstate: cpu:1 min_policy_perf:4 max_policy_perf:31 [ 2.183896] intel_pstate: cpu:1 global_min:4 global_max:44 [ 2.183898] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4 > > I think that it worked because the MSR was read every time > > intel_pstate ran, so it got updated at one point and stayed that way. > > But here HWP in active mode is getting used. So it should have fewer > request calls to set accept via cpufreq set_policy() > > callback or with some HWP interrupt.
On Wed, 2024-06-05 at 13:21 +0800, Xi Ruoyao wrote: > On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote: > > > > With such a delay, I am not sure how this even worked before. > > It didn't work out of box but it worked after manually writing 0 to > no_turbo after 20 seconds, see > https://bugzilla.kernel.org/show_bug.cgi?id=218702. That make sense. So it never worked out of box. The store_no_turbo() has additional read for turbo flag before, which is removed now. I think adding that back will will restore old behavior. diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index 4b986c044741..0d5330e5b96b 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -1301,6 +1301,8 @@ static ssize_t store_no_turbo(struct kobject *a, struct kobj_attribute *b, no_turbo = !!clamp_t(int, input, 0, 1); + global.turbo_disabled = turbo_is_disabled(); + if (no_turbo == global.no_turbo) goto unlock_driver; Need to adjust the mutex around it also. Regarding the bugzilla, I disabled turbo and no_turbo shows 1. Not sure how it shows "0" to you. Thanks, Srinivas > > > > > Can you revert the patch in question and use kernel dynamic > > > > debug > > > > dyndbg="file intel_pstate.c +p" kernel command line and collect > > > > log > > > > for > > > > 30 seconds? > > Attached. The related part seems: > > [ 0.553606] intel_pstate: Intel P-state driver initializing > [ 0.553630] intel_pstate: controlling: cpu 0 > [ 0.553640] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 0.553642] intel_pstate: cpu:0 min_policy_perf:4 > max_policy_perf:31 > [ 0.553643] intel_pstate: cpu:0 global_min:0 global_max:44 > [ 0.553644] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4 > [ 0.553680] intel_pstate: controlling: cpu 1 > [ 0.553702] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 0.553703] intel_pstate: cpu:1 min_policy_perf:4 > max_policy_perf:31 > [ 0.553704] intel_pstate: cpu:1 global_min:0 global_max:44 > [ 0.553705] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4 > [ 0.553742] intel_pstate: controlling: cpu 2 > [ 0.553763] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 0.553764] intel_pstate: cpu:2 min_policy_perf:4 > max_policy_perf:31 > [ 0.553765] intel_pstate: cpu:2 global_min:0 global_max:44 > [ 0.553766] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4 > [ 0.553809] intel_pstate: controlling: cpu 3 > [ 0.553830] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 0.553831] intel_pstate: cpu:3 min_policy_perf:4 > max_policy_perf:31 > [ 0.553831] intel_pstate: cpu:3 global_min:0 global_max:44 > [ 0.553832] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4 > [ 0.553868] intel_pstate: controlling: cpu 4 > [ 0.553892] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 0.553893] intel_pstate: cpu:4 min_policy_perf:4 > max_policy_perf:31 > [ 0.553894] intel_pstate: cpu:4 global_min:0 global_max:44 > [ 0.553895] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4 > [ 0.553943] intel_pstate: controlling: cpu 5 > [ 0.553967] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 0.553968] intel_pstate: cpu:5 min_policy_perf:4 > max_policy_perf:31 > [ 0.553968] intel_pstate: cpu:5 global_min:0 global_max:44 > [ 0.553969] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4 > [ 0.554009] intel_pstate: controlling: cpu 6 > [ 0.554034] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 0.554035] intel_pstate: cpu:6 min_policy_perf:4 > max_policy_perf:31 > [ 0.554036] intel_pstate: cpu:6 global_min:0 global_max:44 > [ 0.554037] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4 > [ 0.554077] intel_pstate: controlling: cpu 7 > [ 0.554098] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 0.554099] intel_pstate: cpu:7 min_policy_perf:4 > max_policy_perf:31 > [ 0.554099] intel_pstate: cpu:7 global_min:0 global_max:44 > [ 0.554100] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4 > [ 0.554104] intel_pstate: HWP enabled > [ 2.183669] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 2.183675] intel_pstate: cpu:6 min_policy_perf:4 > max_policy_perf:31 > [ 2.183677] intel_pstate: cpu:6 global_min:4 global_max:44 > [ 2.183679] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4 > [ 2.183710] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 2.183711] intel_pstate: cpu:4 min_policy_perf:4 > max_policy_perf:31 > [ 2.183713] intel_pstate: cpu:4 global_min:4 global_max:44 > [ 2.183715] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4 > [ 2.183742] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 2.183744] intel_pstate: cpu:2 min_policy_perf:4 > max_policy_perf:31 > [ 2.183745] intel_pstate: cpu:2 global_min:4 global_max:44 > [ 2.183747] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4 > [ 2.183773] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 2.183775] intel_pstate: cpu:0 min_policy_perf:4 > max_policy_perf:31 > [ 2.183776] intel_pstate: cpu:0 global_min:4 global_max:44 > [ 2.183777] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4 > [ 2.183803] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 2.183805] intel_pstate: cpu:7 min_policy_perf:4 > max_policy_perf:31 > [ 2.183806] intel_pstate: cpu:7 global_min:4 global_max:44 > [ 2.183807] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4 > [ 2.183831] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 2.183833] intel_pstate: cpu:5 min_policy_perf:4 > max_policy_perf:31 > [ 2.183834] intel_pstate: cpu:5 global_min:4 global_max:44 > [ 2.183836] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4 > [ 2.183862] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 2.183864] intel_pstate: cpu:3 min_policy_perf:4 > max_policy_perf:31 > [ 2.183865] intel_pstate: cpu:3 global_min:4 global_max:44 > [ 2.183867] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4 > [ 2.183893] intel_pstate: set_policy cpuinfo.max 3100000 policy- > >max 3100000 > [ 2.183895] intel_pstate: cpu:1 min_policy_perf:4 > max_policy_perf:31 > [ 2.183896] intel_pstate: cpu:1 global_min:4 global_max:44 > [ 2.183898] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4 > > > > I think that it worked because the MSR was read every time > > > intel_pstate ran, so it got updated at one point and stayed that > > > way. > > > > But here HWP in active mode is getting used. So it should have > > fewer > > request calls to set accept via cpufreq set_policy() > > > > callback or with some HWP interrupt. >
On Wed, Jun 5, 2024 at 2:05 PM srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > On Wed, 2024-06-05 at 13:21 +0800, Xi Ruoyao wrote: > > On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote: > > > > > With such a delay, I am not sure how this even worked before. > > > > It didn't work out of box but it worked after manually writing 0 to > > no_turbo after 20 seconds, see > > https://bugzilla.kernel.org/show_bug.cgi?id=218702. > > That make sense. So it never worked out of box. The store_no_turbo() > has additional read for turbo flag before, which is removed now. I > think adding that back will will restore old behavior. > > diff --git a/drivers/cpufreq/intel_pstate.c > b/drivers/cpufreq/intel_pstate.c > index 4b986c044741..0d5330e5b96b 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -1301,6 +1301,8 @@ static ssize_t store_no_turbo(struct kobject *a, > struct kobj_attribute *b, > > no_turbo = !!clamp_t(int, input, 0, 1); > > + global.turbo_disabled = turbo_is_disabled(); > + > if (no_turbo == global.no_turbo) > goto unlock_driver; > > > Need to adjust the mutex around it also. Anyhow, it can be made work. global.turbo_disabled can be updated right before it is checked in store_no_turbo(), so if 0 is written to no_turbo (and global.no_turbo is 1), it will succeed if global.turbo_disabled changes from 1 to 0. Something like the attached (untested) patch.
On Fri, 2024-06-07 at 17:04 +0200, Rafael J. Wysocki wrote: > On Wed, Jun 5, 2024 at 2:05 PM srinivas pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > > > On Wed, 2024-06-05 at 13:21 +0800, Xi Ruoyao wrote: > > > On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote: > > > > > > With such a delay, I am not sure how this even worked > > > > > > before. > > > > > > It didn't work out of box but it worked after manually writing 0 > > > to > > > no_turbo after 20 seconds, see > > > https://bugzilla.kernel.org/show_bug.cgi?id=218702. > > > > That make sense. So it never worked out of box. The > > store_no_turbo() > > has additional read for turbo flag before, which is removed now. I > > think adding that back will will restore old behavior. > > > > diff --git a/drivers/cpufreq/intel_pstate.c > > b/drivers/cpufreq/intel_pstate.c > > index 4b986c044741..0d5330e5b96b 100644 > > --- a/drivers/cpufreq/intel_pstate.c > > +++ b/drivers/cpufreq/intel_pstate.c > > @@ -1301,6 +1301,8 @@ static ssize_t store_no_turbo(struct kobject > > *a, > > struct kobj_attribute *b, > > > > no_turbo = !!clamp_t(int, input, 0, 1); > > > > + global.turbo_disabled = turbo_is_disabled(); > > + > > if (no_turbo == global.no_turbo) > > goto unlock_driver; > > > > > > Need to adjust the mutex around it also. > > Anyhow, it can be made work. > > global.turbo_disabled can be updated right before it is checked in > store_no_turbo(), so if 0 is written to no_turbo (and global.no_turbo > is 1), it will succeed if global.turbo_disabled changes from 1 to 0. > > Something like the attached (untested) patch. Should work. Xi, Please test so that we can close this issue. Thanks, Srinivas
On Fri, 2024-06-07 at 08:18 -0700, srinivas pandruvada wrote: > On Fri, 2024-06-07 at 17:04 +0200, Rafael J. Wysocki wrote: > > On Wed, Jun 5, 2024 at 2:05 PM srinivas pandruvada > > <srinivas.pandruvada@linux.intel.com> wrote: > > > > > > On Wed, 2024-06-05 at 13:21 +0800, Xi Ruoyao wrote: > > > > On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote: > > > > > > > With such a delay, I am not sure how this even worked > > > > > > > before. > > > > > > > > It didn't work out of box but it worked after manually writing 0 > > > > to > > > > no_turbo after 20 seconds, see > > > > https://bugzilla.kernel.org/show_bug.cgi?id=218702. > > > > > > That make sense. So it never worked out of box. The > > > store_no_turbo() > > > has additional read for turbo flag before, which is removed now. I > > > think adding that back will will restore old behavior. > > > > > > diff --git a/drivers/cpufreq/intel_pstate.c > > > b/drivers/cpufreq/intel_pstate.c > > > index 4b986c044741..0d5330e5b96b 100644 > > > --- a/drivers/cpufreq/intel_pstate.c > > > +++ b/drivers/cpufreq/intel_pstate.c > > > @@ -1301,6 +1301,8 @@ static ssize_t store_no_turbo(struct kobject > > > *a, > > > struct kobj_attribute *b, > > > > > > no_turbo = !!clamp_t(int, input, 0, 1); > > > > > > + global.turbo_disabled = turbo_is_disabled(); > > > + > > > if (no_turbo == global.no_turbo) > > > goto unlock_driver; > > > > > > > > > Need to adjust the mutex around it also. > > > > Anyhow, it can be made work. > > > > global.turbo_disabled can be updated right before it is checked in > > store_no_turbo(), so if 0 is written to no_turbo (and global.no_turbo > > is 1), it will succeed if global.turbo_disabled changes from 1 to 0. > > > > Something like the attached (untested) patch. > > Should work. > > Xi, > Please test so that we can close this issue. Yes it restores the old behavior for me. Tested-by: Xi Ruoyao <xry111@xry111.site>
Index: linux-pm/drivers/cpufreq/intel_pstate.c =================================================================== --- linux-pm.orig/drivers/cpufreq/intel_pstate.c +++ linux-pm/drivers/cpufreq/intel_pstate.c @@ -173,7 +173,6 @@ struct vid_data { * based on the MSR_IA32_MISC_ENABLE value and whether or * not the maximum reported turbo P-state is different from * the maximum reported non-turbo one. - * @turbo_disabled_mf: The @turbo_disabled value reflected by cpuinfo.max_freq. * @min_perf_pct: Minimum capacity limit in percent of the maximum turbo * P-state capacity. * @max_perf_pct: Maximum capacity limit in percent of the maximum turbo @@ -182,7 +181,6 @@ struct vid_data { struct global_params { bool no_turbo; bool turbo_disabled; - bool turbo_disabled_mf; int max_perf_pct; int min_perf_pct; }; @@ -594,12 +592,13 @@ static void intel_pstate_hybrid_hwp_adju cpu->pstate.min_pstate = intel_pstate_freq_to_hwp(cpu, freq); } -static inline void update_turbo_state(void) +static bool turbo_is_disabled(void) { u64 misc_en; rdmsrl(MSR_IA32_MISC_ENABLE, misc_en); - global.turbo_disabled = misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE; + + return !!(misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE); } static int min_perf_pct_min(void) @@ -1154,40 +1153,16 @@ static void intel_pstate_update_policies static void __intel_pstate_update_max_freq(struct cpudata *cpudata, struct cpufreq_policy *policy) { - policy->cpuinfo.max_freq = global.turbo_disabled_mf ? + policy->cpuinfo.max_freq = global.turbo_disabled ? cpudata->pstate.max_freq : cpudata->pstate.turbo_freq; refresh_frequency_limits(policy); } -static void intel_pstate_update_max_freq(unsigned int cpu) -{ - struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu); - - if (!policy) - return; - - __intel_pstate_update_max_freq(all_cpu_data[cpu], policy); - - cpufreq_cpu_release(policy); -} - static void intel_pstate_update_limits(unsigned int cpu) { mutex_lock(&intel_pstate_driver_lock); - update_turbo_state(); - /* - * If turbo has been turned on or off globally, policy limits for - * all CPUs need to be updated to reflect that. - */ - if (global.turbo_disabled_mf != global.turbo_disabled) { - global.turbo_disabled_mf = global.turbo_disabled; - arch_set_max_freq_ratio(global.turbo_disabled); - for_each_possible_cpu(cpu) - intel_pstate_update_max_freq(cpu); - } else { - cpufreq_update_policy(cpu); - } + cpufreq_update_policy(cpu); mutex_unlock(&intel_pstate_driver_lock); } @@ -1287,7 +1262,6 @@ static ssize_t show_no_turbo(struct kobj return -EAGAIN; } - update_turbo_state(); if (global.turbo_disabled) ret = sprintf(buf, "%u\n", global.turbo_disabled); else @@ -1317,7 +1291,6 @@ static ssize_t store_no_turbo(struct kob mutex_lock(&intel_pstate_limits_lock); - update_turbo_state(); if (global.turbo_disabled) { pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n"); mutex_unlock(&intel_pstate_limits_lock); @@ -2281,8 +2254,6 @@ static void intel_pstate_adjust_pstate(s struct sample *sample; int target_pstate; - update_turbo_state(); - target_pstate = get_target_pstate(cpu); target_pstate = intel_pstate_prepare_request(cpu, target_pstate); trace_cpu_frequency(target_pstate * cpu->pstate.scaling, cpu->cpu); @@ -2593,7 +2564,6 @@ static int intel_pstate_set_policy(struc * be invoked on them. */ intel_pstate_clear_update_util_hook(policy->cpu); - update_turbo_state(); intel_pstate_set_pstate(cpu, pstate); } else { intel_pstate_set_update_util_hook(policy->cpu); @@ -2637,7 +2607,6 @@ static void intel_pstate_verify_cpu_poli { int max_freq; - update_turbo_state(); if (hwp_active) { intel_pstate_get_hwp_cap(cpu); max_freq = global.no_turbo || global.turbo_disabled ? @@ -2734,8 +2703,6 @@ static int __intel_pstate_cpu_init(struc /* cpuinfo and default policy values */ policy->cpuinfo.min_freq = cpu->pstate.min_freq; - update_turbo_state(); - global.turbo_disabled_mf = global.turbo_disabled; policy->cpuinfo.max_freq = global.turbo_disabled ? cpu->pstate.max_freq : cpu->pstate.turbo_freq; @@ -2901,8 +2868,6 @@ static int intel_cpufreq_target(struct c struct cpufreq_freqs freqs; int target_pstate; - update_turbo_state(); - freqs.old = policy->cur; freqs.new = target_freq; @@ -2924,8 +2889,6 @@ static unsigned int intel_cpufreq_fast_s struct cpudata *cpu = all_cpu_data[policy->cpu]; int target_pstate; - update_turbo_state(); - target_pstate = intel_pstate_freq_to_hwp(cpu, target_freq); target_pstate = intel_cpufreq_update_pstate(policy, target_pstate, true); @@ -2943,7 +2906,6 @@ static void intel_cpufreq_adjust_perf(un int old_pstate = cpu->pstate.current_pstate; int cap_pstate, min_pstate, max_pstate, target_pstate; - update_turbo_state(); cap_pstate = global.turbo_disabled ? HWP_GUARANTEED_PERF(hwp_cap) : HWP_HIGHEST_PERF(hwp_cap); @@ -3131,6 +3093,9 @@ static int intel_pstate_register_driver( memset(&global, 0, sizeof(global)); global.max_perf_pct = 100; + global.turbo_disabled = turbo_is_disabled(); + + arch_set_max_freq_ratio(global.turbo_disabled); intel_pstate_driver = driver; ret = cpufreq_register_driver(intel_pstate_driver);