Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3783 bytes --]

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
> 
> > 
> 
> 


[-- Attachment #2: delayed_turbo_check.diff --]
[-- Type: text/x-patch, Size: 2202 bytes --]

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 65d3f79104bd..8d61ff261c61 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1305,7 +1305,7 @@ static ssize_t store_no_turbo(struct kobject *a, struct kobj_attribute *b,
 	if (no_turbo == global.no_turbo)
 		goto unlock_driver;
 
-	if (global.turbo_disabled) {
+	if (READ_ONCE(global.turbo_disabled)) {
 		pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n");
 		count = -EPERM;
 		goto unlock_driver;
@@ -1762,7 +1762,7 @@ static u64 atom_get_val(struct cpudata *cpudata, int pstate)
 	u32 vid;
 
 	val = (u64)pstate << 8;
-	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !READ_ONCE(global.turbo_disabled))
 		val |= (u64)1 << 32;
 
 	vid_fp = cpudata->vid.min + mul_fp(
@@ -1927,7 +1927,7 @@ static u64 core_get_val(struct cpudata *cpudata, int pstate)
 	u64 val;
 
 	val = (u64)pstate << 8;
-	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !READ_ONCE(global.turbo_disabled))
 		val |= (u64)1 << 32;
 
 	return val;
@@ -3102,6 +3102,29 @@ static void intel_pstate_driver_cleanup(void)
 	intel_pstate_driver = NULL;
 }
 
+static void check_turbo_work_handler(struct work_struct *work)
+{
+	bool no_turbo;
+	
+	no_turbo = turbo_is_disabled();
+
+	if (no_turbo)
+		return;
+
+	/* Same processing as sysfs no_turbo update */
+	mutex_lock(&intel_pstate_driver_lock);
+
+	WRITE_ONCE(global.turbo_disabled, no_turbo);
+	WRITE_ONCE(global.no_turbo, no_turbo);
+
+	intel_pstate_update_limits_for_all();
+	arch_set_max_freq_ratio(false);
+
+	mutex_unlock(&intel_pstate_driver_lock);
+}
+
+DECLARE_DELAYED_WORK(turbo_work, check_turbo_work_handler);
+
 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);
+
 	arch_set_max_freq_ratio(global.turbo_disabled);
 
 	intel_pstate_driver = driver;