amd-pstate cleanups

From: Mario Limonciello <[email protected]>

During resume it's possible the firmware didn't restore the CPPC request MSR but the kernel thinks the values line up. This leads to incorrect performance after resume from suspend.

To fix the issue invalidate the cached value at suspend. During resume use the saved values programmed as cached limits.

Reported-by Miroslav Pavleski <[email protected]>

Closes https://bugzilla.kernel.org/show_bug.cgi?id=217931

Signed-off-by Mario Limonciello <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/cpufreq/amd-pstate.c +0 -0

index f425fb7ec77d7..12fb63169a24c 100644

drivers/cpufreq/amd-pstate.c +4 -1

-	return amd_pstate_update_perf(cpudata, 0, 0, max_perf, cpudata->epp_cached, false);
+	return amd_pstate_epp_update_limit(policy);
 }
 
 static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)

@@ -1660,6 +1660,9 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
 	if (cppc_state != AMD_PSTATE_ACTIVE)
 		return 0;
 
+	/* invalidate to ensure it's rewritten during resume */
+	cpudata->cppc_req_cached = 0;
+
 	/* set this flag to avoid setting core offline*/
 	cpudata->suspended = true;
 

Signature · 1 line

2.43.0

Hello Mario,

Quoted 21 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

During resume it's possible the firmware didn't restore the CPPC request MSR but the kernel thinks the values line up. This leads to incorrect performance after resume from suspend.

To fix the issue invalidate the cached value at suspend. During resume use the saved values programmed as cached limits.

Reported-by Miroslav Pavleski <[email protected]>

Closes https://bugzilla.kernel.org/show_bug.cgi?id=217931

Signed-off-by Mario Limonciello <[email protected]>

---
  drivers/cpufreq/amd-pstate.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/cpufreq/amd-pstate.c +0 -0

index f425fb7ec77d7..12fb63169a24c 100644

drivers/cpufreq/amd-pstate.c static int amd_pstate_epp_reenable(struct cpufreq_policy *policy) · line 1612

Quoted 2 lines

drivers/cpufreq/amd-pstate.c +0 -0

@@ -1611,7 +1611,7 @@ static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
  					  max_perf, policy->boost_enabled);
  	}

You can also remove the tracing code from amd_pstate_epp_reenable(), i.e,

	if (trace_amd_pstate_epp_perf_enabled()) {
		trace_amd_pstate_epp_perf(cpudata->cpu, cpudata->highest_perf,
					  cpudata->epp_cached,
					  FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cpudata->cppc_req_cached),
					  max_perf, policy->boost_enabled);
	}

Since amd_pstate_epp_update_limit() also has the the tracing code.

The patch looks good to me otherwise.

Reviewed-by Gautham R. Shenoy <[email protected]>

From: Mario Limonciello <[email protected]>

During resume it's possible the firmware didn't restore the CPPC request MSR but the kernel thinks the values line up. This leads to incorrect performance after resume from suspend.

To fix the issue invalidate the cached value at suspend. During resume use the saved values programmed as cached limits.

Reported-by Miroslav Pavleski <[email protected]>

Closes https://bugzilla.kernel.org/show_bug.cgi?id=217931

Signed-off-by Mario Limonciello <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/cpufreq/amd-pstate.c +0 -0

index f425fb7ec77d7..12fb63169a24c 100644

drivers/cpufreq/amd-pstate.c +3 -0

 }
 
 static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)

@@ -1660,6 +1660,9 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
 	if (cppc_state != AMD_PSTATE_ACTIVE)
 		return 0;
 
+	/* invalidate to ensure it's rewritten during resume */
+	cpudata->cppc_req_cached = 0;
+
 	/* set this flag to avoid setting core offline*/
 	cpudata->suspended = true;
 

Quoted 24 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

During resume it's possible the firmware didn't restore the CPPC request MSR but the kernel thinks the values line up. This leads to incorrect performance after resume from suspend.

To fix the issue invalidate the cached value at suspend. During resume use the saved values programmed as cached limits.

Reported-by Miroslav Pavleski <[email protected]>

Closes https://bugzilla.kernel.org/show_bug.cgi?id=217931

Signed-off-by Mario Limonciello <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/cpufreq/amd-pstate.c +0 -0

index f425fb7ec77d7..12fb63169a24c 100644
@@ -1611,7 +1611,7 @@ static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
 					  max_perf, policy->boost_enabled);
 	}
 

drivers/cpufreq/amd-pstate.c static int amd_pstate_epp_reenable(struct cpufreq_policy *policy) · line 1614

Quoted 2 lines

drivers/cpufreq/amd-pstate.c +1 -1

-	return amd_pstate_update_perf(cpudata, 0, 0, max_perf, cpudata->epp_cached, false);
+	return amd_pstate_epp_update_limit(policy);

Can we also add the check "if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)" in "amd_pstate_epp_update_limit()" before calling "amd_pstate_update_min_max_limit()". I think it would help in avoiding some unnecessary calls to the update_min_max_limit() function.

Patch looks good to me otherwise.

Thanks,
Dhananjay

Quoted 13 lines

drivers/cpufreq/amd-pstate.c +3 -0

 }
 
 static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)

@@ -1660,6 +1660,9 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
 	if (cppc_state != AMD_PSTATE_ACTIVE)
 		return 0;
 
+	/* invalidate to ensure it's rewritten during resume */
+	cpudata->cppc_req_cached = 0;
+
 	/* set this flag to avoid setting core offline*/
 	cpudata->suspended = true;
 

From: Mario Limonciello <[email protected]>

I came across a system that MSR_AMD_CPPC_CAP1 for some CPUs isn't populated. This is an unexpected behavior that is most likely a BIOS bug. In the event it happens I'd like users to report bugs to properly root cause and get this fixed.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/cpufreq/amd-pstate.c +2 -0

index 12fb63169a24c..87c605348a3dc 100644
@@ -1034,6 +1034,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 free_cpudata2:
 	freq_qos_remove_request(&cpudata->req[0]);
 free_cpudata1:
+	pr_warn("Failed to initialize CPU %d: %d\n", policy->cpu, ret);
 	kfree(cpudata);
 	return ret;
 }

@@ -1527,6 +1528,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	return 0;
 
 free_cpudata1:
+	pr_warn("Failed to initialize CPU %d: %d\n", policy->cpu, ret);
 	kfree(cpudata);
 	return ret;
 }

From: Mario Limonciello <[email protected]>

Use the perf_to_freq helpers to calculate this on the fly.

Signed-off-by Mario Limonciello <[email protected]>

From: Mario Limonciello <[email protected]>

Use the perf_to_freq helpers to calculate this on the fly.

Signed-off-by Mario Limonciello <[email protected]>

--

v3:
* Fix calc error for min_freq
v2:
* Keep cached limits

---
 drivers/cpufreq/amd-pstate-ut.c | 14 +++----
 drivers/cpufreq/amd-pstate.c    | 70 +++++++++++----------------------
 drivers/cpufreq/amd-pstate.h    |  9 +----
 3 files changed, 32 insertions(+), 61 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +7 -7

index 3a0a380c3590c..445278cf40b61 100644
@@ -214,14 +214,14 @@ static void amd_pstate_ut_check_freq(u32 index)
 			break;
 		cpudata = policy->driver_data;
 
-		if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
+		if (!((policy->cpuinfo.max_freq >= cpudata->nominal_freq) &&
 			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
-			(cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
-			(cpudata->min_freq > 0))) {
+			(cpudata->lowest_nonlinear_freq > policy->cpuinfo.min_freq) &&
+			(policy->cpuinfo.min_freq > 0))) {
 			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
 			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
-				__func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
-				cpudata->lowest_nonlinear_freq, cpudata->min_freq);
+				__func__, cpu, policy->cpuinfo.max_freq, cpudata->nominal_freq,
+				cpudata->lowest_nonlinear_freq, policy->cpuinfo.min_freq);
 			goto skip_test;
 		}
 

@@ -233,13 +233,13 @@ static void amd_pstate_ut_check_freq(u32 index)
 		}
 
 		if (cpudata->boost_supported) {
-			if ((policy->max == cpudata->max_freq) ||
+			if ((policy->max == policy->cpuinfo.max_freq) ||
 					(policy->max == cpudata->nominal_freq))
 				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
 			else {
 				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
 				pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
-					__func__, cpu, policy->max, cpudata->max_freq,
+					__func__, cpu, policy->max, policy->cpuinfo.max_freq,
 					cpudata->nominal_freq);
 				goto skip_test;
 			}

drivers/cpufreq/amd-pstate.c +2 -9

index 87c605348a3dc..a7c41f915b46e 100644
@@ -717,7 +717,7 @@ static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
 	int ret = 0;
 
 	nominal_freq = READ_ONCE(cpudata->nominal_freq);
-	max_freq = READ_ONCE(cpudata->max_freq);
+	max_freq = perf_to_freq(cpudata, READ_ONCE(cpudata->highest_perf));
 
 	if (on)
 		policy->cpuinfo.max_freq = max_freq;

@@ -901,35 +901,26 @@ static u32 amd_pstate_get_transition_latency(unsigned int cpu)
 static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 {
 	int ret;
-	u32 min_freq, max_freq;
-	u32 nominal_freq, lowest_nonlinear_freq;
+	u32 min_freq, nominal_freq, lowest_nonlinear_freq;
 	struct cppc_perf_caps cppc_perf;
 
 	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
 	if (ret)
 		return ret;
 
-	if (quirks && quirks->lowest_freq)
-		min_freq = quirks->lowest_freq;
-	else
-		min_freq = cppc_perf.lowest_freq;
-
 	if (quirks && quirks->nominal_freq)
 		nominal_freq = quirks->nominal_freq;
 	else
 		nominal_freq = cppc_perf.nominal_freq;
 
-	min_freq *= 1000;
 	nominal_freq *= 1000;

drivers/cpufreq/amd-pstate.c +16 -30

-		pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n",
-			min_freq, max_freq, nominal_freq);
+	if (nominal_freq <= 0) {
+		pr_err("nominal_freq(%d) value is incorrect\n",
+			nominal_freq);
 		return -EINVAL;
 	}
 
+	lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
+	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
+
 	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq) {
 		pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n",
 			lowest_nonlinear_freq, min_freq, nominal_freq);

@@ -954,9 +948,9 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 
 static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 {
-	int min_freq, max_freq, ret;
-	struct device *dev;
 	struct amd_cpudata *cpudata;
+	struct device *dev;
+	int ret;
 
 	/*
 	 * Resetting PERF_CTL_MSR will put the CPU in P0 frequency,

@@ -987,17 +981,11 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 	if (ret)
 		goto free_cpudata1;
 
-	min_freq = READ_ONCE(cpudata->min_freq);
-	max_freq = READ_ONCE(cpudata->max_freq);
-
 	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
 	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
 
-	policy->min = min_freq;
-	policy->max = max_freq;
-
-	policy->cpuinfo.min_freq = min_freq;
-	policy->cpuinfo.max_freq = max_freq;
+	policy->cpuinfo.min_freq = policy->min = perf_to_freq(cpudata, cpudata->lowest_perf);
+	policy->cpuinfo.max_freq = policy->max = perf_to_freq(cpudata, cpudata->highest_perf);
 
 	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
 

@@ -1021,9 +1009,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 		goto free_cpudata2;
 	}
 
-	cpudata->max_limit_freq = max_freq;
-	cpudata->min_limit_freq = min_freq;
-
 	policy->driver_data = cpudata;
 
 	if (!current_pstate_driver->adjust_perf)

@@ -1081,14 +1066,10 @@ static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
 static ssize_t show_amd_pstate_max_freq(struct cpufreq_policy *policy,
 					char *buf)
 {
-	int max_freq;
 	struct amd_cpudata *cpudata = policy->driver_data;
 
-	max_freq = READ_ONCE(cpudata->max_freq);
-	if (max_freq < 0)
-		return max_freq;
 
-	return sysfs_emit(buf, "%u\n", max_freq);
+	return sysfs_emit(buf, "%u\n", perf_to_freq(cpudata, READ_ONCE(cpudata->highest_perf)));
 }
 
 static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *policy,

@@ -1446,10 +1427,10 @@ static bool amd_pstate_acpi_pm_profile_undefined(void)
 
 static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 {
-	int min_freq, max_freq, ret;
 	struct amd_cpudata *cpudata;
 	struct device *dev;
 	u64 value;
+	int ret;
 
 	/*
 	 * Resetting PERF_CTL_MSR will put the CPU in P0 frequency,

@@ -1480,19 +1461,13 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	if (ret)
 		goto free_cpudata1;
 
-	min_freq = READ_ONCE(cpudata->min_freq);
-	max_freq = READ_ONCE(cpudata->max_freq);
-
-	policy->cpuinfo.min_freq = min_freq;
-	policy->cpuinfo.max_freq = max_freq;
+	policy->cpuinfo.min_freq = policy->min = perf_to_freq(cpudata, cpudata->lowest_perf);
+	policy->cpuinfo.max_freq = policy->max = perf_to_freq(cpudata, cpudata->highest_perf);
 	/* It will be updated by governor */
 	policy->cur = policy->cpuinfo.min_freq;
 
 	policy->driver_data = cpudata;
 
-	policy->min = policy->cpuinfo.min_freq;
-	policy->max = policy->cpuinfo.max_freq;
-
 	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
 
 	/*

@@ -1550,7 +1525,8 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 	struct amd_cpudata *cpudata = policy->driver_data;
 	u8 epp;
 
-	amd_pstate_update_min_max_limit(policy);
+	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
+		amd_pstate_update_min_max_limit(policy);
 
 	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
 		epp = 0;

drivers/cpufreq/amd-pstate.h +2 -7

index 19d405c6d805e..0149933692458 100644
@@ -46,8 +46,6 @@ struct amd_aperf_mperf {
  * @max_limit_perf: Cached value of the performance corresponding to policy->max
  * @min_limit_freq: Cached value of policy->min (in khz)
  * @max_limit_freq: Cached value of policy->max (in khz)
- * @max_freq: the frequency (in khz) that mapped to highest_perf
- * @min_freq: the frequency (in khz) that mapped to lowest_perf
  * @nominal_freq: the frequency (in khz) that mapped to nominal_perf
  * @lowest_nonlinear_freq: the frequency (in khz) that mapped to lowest_nonlinear_perf
  * @cur: Difference of Aperf/Mperf/tsc count between last and current sample

@@ -77,11 +75,8 @@ struct amd_cpudata {
 	u8	prefcore_ranking;
 	u8	min_limit_perf;
 	u8	max_limit_perf;
-	u32     min_limit_freq;
-	u32     max_limit_freq;
-
-	u32	max_freq;
-	u32	min_freq;
+	u32	min_limit_freq;
+	u32	max_limit_freq;
 	u32	nominal_freq;
 	u32	lowest_nonlinear_freq;
 

Quoted 3 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

Use the perf_to_freq helpers to calculate this on the fly.

Can we call out the below change (in the -1550,7 +1525,8 code chunk) in the commit message, or split it out to different patch

* Adding the if check in amd_pstate_epp_update_limit()

Quoted 3 lines

Signed-off-by Mario Limonciello <[email protected]>

--

We need one more hyphen here I think i.e."---", otherwise the version info is showing up in the commit message

Quoted 96 lines

v3:
* Fix calc error for min_freq
v2:
* Keep cached limits

---
  drivers/cpufreq/amd-pstate-ut.c | 14 +++----
  drivers/cpufreq/amd-pstate.c    | 70 +++++++++++----------------------
  drivers/cpufreq/amd-pstate.h    |  9 +----
  3 files changed, 32 insertions(+), 61 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +7 -7

index 3a0a380c3590c..445278cf40b61 100644
@@ -214,14 +214,14 @@ static void amd_pstate_ut_check_freq(u32 index)
  			break;
  		cpudata = policy->driver_data;
 
-		if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
+		if (!((policy->cpuinfo.max_freq >= cpudata->nominal_freq) &&
  			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
-			(cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
-			(cpudata->min_freq > 0))) {
+			(cpudata->lowest_nonlinear_freq > policy->cpuinfo.min_freq) &&
+			(policy->cpuinfo.min_freq > 0))) {
  			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
  			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
-				__func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
-				cpudata->lowest_nonlinear_freq, cpudata->min_freq);
+				__func__, cpu, policy->cpuinfo.max_freq, cpudata->nominal_freq,
+				cpudata->lowest_nonlinear_freq, policy->cpuinfo.min_freq);
  			goto skip_test;
  		}
 

@@ -233,13 +233,13 @@ static void amd_pstate_ut_check_freq(u32 index)
  		}
 
  		if (cpudata->boost_supported) {
-			if ((policy->max == cpudata->max_freq) ||
+			if ((policy->max == policy->cpuinfo.max_freq) ||
  					(policy->max == cpudata->nominal_freq))
  				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
  			else {
  				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
  				pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
-					__func__, cpu, policy->max, cpudata->max_freq,
+					__func__, cpu, policy->max, policy->cpuinfo.max_freq,
  					cpudata->nominal_freq);
  				goto skip_test;
  			}

drivers/cpufreq/amd-pstate.c +2 -14

index 87c605348a3dc..a7c41f915b46e 100644
@@ -717,7 +717,7 @@ static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
  	int ret = 0;
 
  	nominal_freq = READ_ONCE(cpudata->nominal_freq);
-	max_freq = READ_ONCE(cpudata->max_freq);
+	max_freq = perf_to_freq(cpudata, READ_ONCE(cpudata->highest_perf));
 
  	if (on)
  		policy->cpuinfo.max_freq = max_freq;

@@ -901,35 +901,26 @@ static u32 amd_pstate_get_transition_latency(unsigned int cpu)
  static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
  {
  	int ret;
-	u32 min_freq, max_freq;
-	u32 nominal_freq, lowest_nonlinear_freq;
+	u32 min_freq, nominal_freq, lowest_nonlinear_freq;
  	struct cppc_perf_caps cppc_perf;
 
  	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
  	if (ret)
  		return ret;
 
-	if (quirks && quirks->lowest_freq)
-		min_freq = quirks->lowest_freq;
-	else
-		min_freq = cppc_perf.lowest_freq;
-
  	if (quirks && quirks->nominal_freq)
  		nominal_freq = quirks->nominal_freq;
  	else
  		nominal_freq = cppc_perf.nominal_freq;
 
-	min_freq *= 1000;
  	nominal_freq *= 1000;
-
  	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
-	WRITE_ONCE(cpudata->min_freq, min_freq);
-
-	max_freq = perf_to_freq(cpudata, cpudata->highest_perf);
-	lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
 

drivers/cpufreq/amd-pstate.c static u32 amd_pstate_get_transition_latency(unsigned int cpu) · line 919

Quoted 3 lines

drivers/cpufreq/amd-pstate.c +1 -2

-	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
-	WRITE_ONCE(cpudata->max_freq, max_freq);
+	if (quirks && quirks->lowest_freq) {

We can avoid the "{" for single line if statement, to keep checkpatch happy

drivers/cpufreq/amd-pstate.c static u32 amd_pstate_get_transition_latency(unsigned int cpu) · line 923

Quoted 5 lines

drivers/cpufreq/amd-pstate.c +4 -0

+		min_freq = quirks->lowest_freq;
+	} else
+		min_freq = cppc_perf.lowest_freq;
+	min_freq *= 1000;

I see that this min_freq part of the code is unchanged, just moved few lines below. If the moving is unintended can we avoid it, so that the diff is optimal.

drivers/cpufreq/amd-pstate.c static int amd_pstate_init_freq(struct amd_cpudata *cpudata) · line 940

Quoted 9 lines

drivers/cpufreq/amd-pstate.c +0 -1

  	/**
  	 * Below values need to be initialized correctly, otherwise driver will fail to load

@@ -937,12 +928,15 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
  	 * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
  	 * Check _CPC in ACPI table objects if any values are incorrect
  	 */
-	if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) {

Shouldn't we retain these sanity checks for min_freq and max_freq?

Thanks,
Dhananjay

Quoted 143 lines

drivers/cpufreq/amd-pstate.c +16 -30

-		pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n",
-			min_freq, max_freq, nominal_freq);
+	if (nominal_freq <= 0) {
+		pr_err("nominal_freq(%d) value is incorrect\n",
+			nominal_freq);
  		return -EINVAL;
  	}
 
+	lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
+	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
+
  	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq) {
  		pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n",
  			lowest_nonlinear_freq, min_freq, nominal_freq);

@@ -954,9 +948,9 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 
  static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
  {
-	int min_freq, max_freq, ret;
-	struct device *dev;
  	struct amd_cpudata *cpudata;
+	struct device *dev;
+	int ret;
 
  	/*
  	 * Resetting PERF_CTL_MSR will put the CPU in P0 frequency,

@@ -987,17 +981,11 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
  	if (ret)
  		goto free_cpudata1;
 
-	min_freq = READ_ONCE(cpudata->min_freq);
-	max_freq = READ_ONCE(cpudata->max_freq);
-
  	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
  	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
 
-	policy->min = min_freq;
-	policy->max = max_freq;
-
-	policy->cpuinfo.min_freq = min_freq;
-	policy->cpuinfo.max_freq = max_freq;
+	policy->cpuinfo.min_freq = policy->min = perf_to_freq(cpudata, cpudata->lowest_perf);
+	policy->cpuinfo.max_freq = policy->max = perf_to_freq(cpudata, cpudata->highest_perf);
 
  	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
 

@@ -1021,9 +1009,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
  		goto free_cpudata2;
  	}
 
-	cpudata->max_limit_freq = max_freq;
-	cpudata->min_limit_freq = min_freq;
-
  	policy->driver_data = cpudata;
 
  	if (!current_pstate_driver->adjust_perf)

@@ -1081,14 +1066,10 @@ static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
  static ssize_t show_amd_pstate_max_freq(struct cpufreq_policy *policy,
  					char *buf)
  {
-	int max_freq;
  	struct amd_cpudata *cpudata = policy->driver_data;
 
-	max_freq = READ_ONCE(cpudata->max_freq);
-	if (max_freq < 0)
-		return max_freq;
 
-	return sysfs_emit(buf, "%u\n", max_freq);
+	return sysfs_emit(buf, "%u\n", perf_to_freq(cpudata, READ_ONCE(cpudata->highest_perf)));
  }
 
  static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *policy,

@@ -1446,10 +1427,10 @@ static bool amd_pstate_acpi_pm_profile_undefined(void)
 
  static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
  {
-	int min_freq, max_freq, ret;
  	struct amd_cpudata *cpudata;
  	struct device *dev;
  	u64 value;
+	int ret;
 
  	/*
  	 * Resetting PERF_CTL_MSR will put the CPU in P0 frequency,

@@ -1480,19 +1461,13 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
  	if (ret)
  		goto free_cpudata1;
 
-	min_freq = READ_ONCE(cpudata->min_freq);
-	max_freq = READ_ONCE(cpudata->max_freq);
-
-	policy->cpuinfo.min_freq = min_freq;
-	policy->cpuinfo.max_freq = max_freq;
+	policy->cpuinfo.min_freq = policy->min = perf_to_freq(cpudata, cpudata->lowest_perf);
+	policy->cpuinfo.max_freq = policy->max = perf_to_freq(cpudata, cpudata->highest_perf);
  	/* It will be updated by governor */
  	policy->cur = policy->cpuinfo.min_freq;
 
  	policy->driver_data = cpudata;
 
-	policy->min = policy->cpuinfo.min_freq;
-	policy->max = policy->cpuinfo.max_freq;
-
  	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
 
  	/*

@@ -1550,7 +1525,8 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
  	struct amd_cpudata *cpudata = policy->driver_data;
  	u8 epp;
 
-	amd_pstate_update_min_max_limit(policy);
+	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
+		amd_pstate_update_min_max_limit(policy);
 
  	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
  		epp = 0;

drivers/cpufreq/amd-pstate.h +2 -7

index 19d405c6d805e..0149933692458 100644
@@ -46,8 +46,6 @@ struct amd_aperf_mperf {
   * @max_limit_perf: Cached value of the performance corresponding to policy->max
   * @min_limit_freq: Cached value of policy->min (in khz)
   * @max_limit_freq: Cached value of policy->max (in khz)
- * @max_freq: the frequency (in khz) that mapped to highest_perf
- * @min_freq: the frequency (in khz) that mapped to lowest_perf
   * @nominal_freq: the frequency (in khz) that mapped to nominal_perf
   * @lowest_nonlinear_freq: the frequency (in khz) that mapped to lowest_nonlinear_perf
   * @cur: Difference of Aperf/Mperf/tsc count between last and current sample

@@ -77,11 +75,8 @@ struct amd_cpudata {
  	u8	prefcore_ranking;
  	u8	min_limit_perf;
  	u8	max_limit_perf;
-	u32     min_limit_freq;
-	u32     max_limit_freq;
-
-	u32	max_freq;
-	u32	min_freq;
+	u32	min_limit_freq;
+	u32	max_limit_freq;
  	u32	nominal_freq;
  	u32	lowest_nonlinear_freq;
 

From: Mario Limonciello <[email protected]>

By storing perf values in a union all the writes and reads can be done atomically, removing the need for some concurrency protections.

While making this change, also drop the cached frequency values, using inline helpers to calculate them on demand from perf value.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
v3:
* Pick up tag
v2:
* cache perf variable in unit tests
* Drop unnecessary check from amd_pstate_update_min_max_limit()
* Consistency with READ_ONCE()
* Drop unneeded policy checks
* add kdoc

---
 drivers/cpufreq/amd-pstate-ut.c |  18 +--
 drivers/cpufreq/amd-pstate.c    | 195 ++++++++++++++++++--------------
 drivers/cpufreq/amd-pstate.h    |  49 +++++---
 3 files changed, 151 insertions(+), 111 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +1 -0

index 445278cf40b61..ba3e06f349c6d 100644
@@ -129,6 +129,7 @@ static void amd_pstate_ut_check_perf(u32 index)
 	struct cppc_perf_caps cppc_perf;
 	struct cpufreq_policy *policy = NULL;
 	struct amd_cpudata *cpudata = NULL;
+	union perf_cached cur_perf;
 
 	for_each_possible_cpu(cpu) {
 		policy = cpufreq_cpu_get(cpu);

drivers/cpufreq/amd-pstate-ut.c +1 -3

 			goto skip_test;
 		}
-		if ((nominal_perf != READ_ONCE(cpudata->nominal_perf)) ||
-			(lowest_nonlinear_perf != READ_ONCE(cpudata->lowest_nonlinear_perf)) ||
-			(lowest_perf != READ_ONCE(cpudata->lowest_perf))) {
+		if (nominal_perf != cur_perf.nominal_perf ||

drivers/cpufreq/amd-pstate-ut.c +0 -0

 			goto skip_test;
 		}
 

drivers/cpufreq/amd-pstate-ut.c +7 -8

@@ -888,25 +896,24 @@ static u32 amd_pstate_get_transition_latency(unsigned int cpu)
 }
 
 /*
- * amd_pstate_init_freq: Initialize the max_freq, min_freq,
- *                       nominal_freq and lowest_nonlinear_freq for
- *                       the @cpudata object.
+ * amd_pstate_init_freq: Initialize the nominal_freq and lowest_nonlinear_freq
+ *			 for the @cpudata object.
  *
- *  Requires: highest_perf, lowest_perf, nominal_perf and
- *            lowest_nonlinear_perf members of @cpudata to be
- *            initialized.
+ * Requires: all perf members of @cpudata to be initialized.
  *
- *  Returns 0 on success, non-zero value on failure.
+ * Returns 0 on success, non-zero value on failure.
  */
 static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 {
-	int ret;
 	u32 min_freq, nominal_freq, lowest_nonlinear_freq;
 	struct cppc_perf_caps cppc_perf;
+	union perf_cached perf;
+	int ret;
 
 	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
 	if (ret)
 		return ret;
+	perf = READ_ONCE(cpudata->perf);
 
 	if (quirks && quirks->nominal_freq)
 		nominal_freq = quirks->nominal_freq;

drivers/cpufreq/amd-pstate-ut.c +1 -1

 	} else
 		min_freq = cppc_perf.lowest_freq;
 	min_freq *= 1000;

@@ -934,7 +942,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 		return -EINVAL;
 	}
 
-	lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
+	lowest_nonlinear_freq = perf_to_freq(perf, nominal_freq, perf.lowest_nonlinear_perf);
 	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
 
 	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq) {

drivers/cpufreq/amd-pstate.h +0 -0

index 0149933692458..8421c83c07919 100644

drivers/cpufreq/amd-pstate.h +7 -0

+ *		  calculated wrongly. we take the fixed value as the highest_perf.
+ * @nominal_perf: the maximum sustained performance level of the processor,
+ *		  assuming ideal operating conditions
+ * @lowest_nonlinear_perf: the lowest performance level at which nonlinear power
+ *			   savings are achieved
+ * @lowest_perf: the absolute lowest performance level of the processor
+ * @min_limit_perf: Cached value of the performance corresponding to policy->min

drivers/cpufreq/amd-pstate.h +7 -18

+	};
+	u64	val;
+};
+
 /**
  * struct  amd_aperf_mperf
  * @aperf: actual performance frequency clock count

@@ -30,20 +58,9 @@ struct amd_aperf_mperf {
  * @cpu: CPU number
  * @req: constraint request to apply
  * @cppc_req_cached: cached performance request hints
- * @highest_perf: the maximum performance an individual processor may reach,
- *		  assuming ideal conditions
- *		  For platforms that do not support the preferred core feature, the
- *		  highest_pef may be configured with 166 or 255, to avoid max frequency
- *		  calculated wrongly. we take the fixed value as the highest_perf.
- * @nominal_perf: the maximum sustained performance level of the processor,
- *		  assuming ideal operating conditions
- * @lowest_nonlinear_perf: the lowest performance level at which nonlinear power
- *			   savings are achieved
- * @lowest_perf: the absolute lowest performance level of the processor
+ * @perf: cached performance-related data
  * @prefcore_ranking: the preferred core ranking, the higher value indicates a higher
  * 		  priority.
- * @min_limit_perf: Cached value of the performance corresponding to policy->min
- * @max_limit_perf: Cached value of the performance corresponding to policy->max
  * @min_limit_freq: Cached value of policy->min (in khz)
  * @max_limit_freq: Cached value of policy->max (in khz)
  * @nominal_freq: the frequency (in khz) that mapped to nominal_perf

@@ -68,13 +85,9 @@ struct amd_cpudata {
 	struct	freq_qos_request req[2];
 	u64	cppc_req_cached;
 
-	u8	highest_perf;
-	u8	nominal_perf;
-	u8	lowest_nonlinear_perf;
-	u8	lowest_perf;
+	union perf_cached perf;
+
 	u8	prefcore_ranking;
-	u8	min_limit_perf;
-	u8	max_limit_perf;
 	u32	min_limit_freq;
 	u32	max_limit_freq;
 	u32	nominal_freq;

Quoted 41 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

By storing perf values in a union all the writes and reads can be done atomically, removing the need for some concurrency protections.

While making this change, also drop the cached frequency values, using inline helpers to calculate them on demand from perf value.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
v3:
* Pick up tag
v2:
* cache perf variable in unit tests
* Drop unnecessary check from amd_pstate_update_min_max_limit()
* Consistency with READ_ONCE()
* Drop unneeded policy checks
* add kdoc

---
  drivers/cpufreq/amd-pstate-ut.c |  18 +--
  drivers/cpufreq/amd-pstate.c    | 195 ++++++++++++++++++--------------
  drivers/cpufreq/amd-pstate.h    |  49 +++++---
  3 files changed, 151 insertions(+), 111 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +1 -0

index 445278cf40b61..ba3e06f349c6d 100644
@@ -129,6 +129,7 @@ static void amd_pstate_ut_check_perf(u32 index)
  	struct cppc_perf_caps cppc_perf;
  	struct cpufreq_policy *policy = NULL;
  	struct amd_cpudata *cpudata = NULL;
+	union perf_cached cur_perf;
 
  	for_each_possible_cpu(cpu) {
  		policy = cpufreq_cpu_get(cpu);

@@ -162,19 +163,20 @@ static void amd_pstate_ut_check_perf(u32 index)
  			lowest_perf = AMD_CPPC_LOWEST_PERF(cap1);
  		}
 

drivers/cpufreq/amd-pstate-ut.c static void amd_pstate_ut_check_perf(u32 index) · line 169

Quoted 6 lines

drivers/cpufreq/amd-pstate-ut.c +3 -2

-		if (highest_perf != READ_ONCE(cpudata->highest_perf) && !cpudata->hw_prefcore) {
+		cur_perf = READ_ONCE(cpudata->perf);
+		if (highest_perf != cur_perf.highest_perf && !cpudata->hw_prefcore) {
  			pr_err("%s cpu%d highest=%d %d highest perf doesn't match\n",
-				__func__, cpu, highest_perf, cpudata->highest_perf);
+				__func__, cpu, highest_perf, cpudata->perf.highest_perf);

Can we use cur_perf.highest_perf here ?

Quoted 10 lines

drivers/cpufreq/amd-pstate-ut.c +3 -3

  			goto skip_test;
  		}
-		if ((nominal_perf != READ_ONCE(cpudata->nominal_perf)) ||
-			(lowest_nonlinear_perf != READ_ONCE(cpudata->lowest_nonlinear_perf)) ||
-			(lowest_perf != READ_ONCE(cpudata->lowest_perf))) {
+		if (nominal_perf != cur_perf.nominal_perf ||
+		   (lowest_nonlinear_perf != cur_perf.lowest_nonlinear_perf) ||
+		   (lowest_perf != cur_perf.lowest_perf)) {
  			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;

drivers/cpufreq/amd-pstate-ut.c static void amd_pstate_ut_check_perf(u32 index) · line 179

Quoted 7 lines

drivers/cpufreq/amd-pstate-ut.c +3 -3

  			pr_err("%s cpu%d nominal=%d %d lowest_nonlinear=%d %d lowest=%d %d, they should be equal!\n",
-				__func__, cpu, nominal_perf, cpudata->nominal_perf,
-				lowest_nonlinear_perf, cpudata->lowest_nonlinear_perf,
-				lowest_perf, cpudata->lowest_perf);
+				__func__, cpu, nominal_perf, cpudata->perf.nominal_perf,
+				lowest_nonlinear_perf, cpudata->perf.lowest_nonlinear_perf,
+				lowest_perf, cpudata->perf.lowest_perf);

Can we use cur_perf.(nominal/lowest_nonlinear/lowest)_perf here as well ?

Quoted 4 lines

drivers/cpufreq/amd-pstate-ut.c +0 -0

  			goto skip_test;
  		}
 

[Snip]

Quoted 35 lines

drivers/cpufreq/amd-pstate-ut.c +7 -8

@@ -888,25 +896,24 @@ static u32 amd_pstate_get_transition_latency(unsigned int cpu)
  }
 
  /*
- * amd_pstate_init_freq: Initialize the max_freq, min_freq,
- *                       nominal_freq and lowest_nonlinear_freq for
- *                       the @cpudata object.
+ * amd_pstate_init_freq: Initialize the nominal_freq and lowest_nonlinear_freq
+ *			 for the @cpudata object.
   *
- *  Requires: highest_perf, lowest_perf, nominal_perf and
- *            lowest_nonlinear_perf members of @cpudata to be
- *            initialized.
+ * Requires: all perf members of @cpudata to be initialized.
   *
- *  Returns 0 on success, non-zero value on failure.
+ * Returns 0 on success, non-zero value on failure.
   */
  static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
  {
-	int ret;
  	u32 min_freq, nominal_freq, lowest_nonlinear_freq;
  	struct cppc_perf_caps cppc_perf;
+	union perf_cached perf;
+	int ret;
 
  	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
  	if (ret)
  		return ret;
+	perf = READ_ONCE(cpudata->perf);
 
  	if (quirks && quirks->nominal_freq)
  		nominal_freq = quirks->nominal_freq;

@@ -918,6 +925,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 

drivers/cpufreq/amd-pstate-ut.c static int amd_pstate_init_freq(struct amd_cpudata *cpudata) · line 928

Quoted 3 lines

drivers/cpufreq/amd-pstate-ut.c +1 -0

  	if (quirks && quirks->lowest_freq) {
  		min_freq = quirks->lowest_freq;
+		perf.lowest_perf = freq_to_perf(perf, nominal_freq, min_freq);

I think we forgot to write back this value to the cpudata->perf variable

Quoted 13 lines

drivers/cpufreq/amd-pstate-ut.c +1 -1

  	} else
  		min_freq = cppc_perf.lowest_freq;
  	min_freq *= 1000;

@@ -934,7 +942,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
  		return -EINVAL;
  	}
 
-	lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
+	lowest_nonlinear_freq = perf_to_freq(perf, nominal_freq, perf.lowest_nonlinear_perf);
  	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
 
  	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq) {

[Snip]

Quoted 5 lines

drivers/cpufreq/amd-pstate.h +0 -0

index 0149933692458..8421c83c07919 100644

drivers/cpufreq/amd-pstate.h line 22

Quoted 10 lines

drivers/cpufreq/amd-pstate.h +7 -0

@@ -13,6 +13,34 @@
  /*********************************************************************
   *                        AMD P-state INTERFACE                       *
   *********************************************************************/
+
+/**
+ * union perf_cached - A union to cache performance-related data.
+ * @highest_perf: the maximum performance an individual processor may reach,
+ *		  assuming ideal conditions
+ *		  For platforms that do not support the preferred core feature, the
+ *		  highest_pef may be configured with 166 or 255, to avoid max frequency

s/highest_pef/highest_perf/

Also I think this statement is bit confusing, how about,

"For platforms that support the preferred core feature, the highest_perf value maybe configured to any value in the range 166-256 by the firmware (because the preferred core ranking is encoded in the highest_perf value). To maintain consistency across all platforms, we split the highest_perf and preferred core ranking values into cpudata->perf.highest_perf and cpudata->prefcore_ranking."

Quoted 8 lines

drivers/cpufreq/amd-pstate.h +7 -0

+ *		  calculated wrongly. we take the fixed value as the highest_perf.
+ * @nominal_perf: the maximum sustained performance level of the processor,
+ *		  assuming ideal operating conditions
+ * @lowest_nonlinear_perf: the lowest performance level at which nonlinear power
+ *			   savings are achieved
+ * @lowest_perf: the absolute lowest performance level of the processor
+ * @min_limit_perf: Cached value of the performance corresponding to policy->min

drivers/cpufreq/amd-pstate.h line 39

Quoted 10 lines

drivers/cpufreq/amd-pstate.h +10 -0

+ * @max_limit_perf: Cached value of the performance corresponding to policy->max
+ */
+union perf_cached {
+	struct {
+		u8	highest_perf;
+		u8	nominal_perf;
+		u8	lowest_nonlinear_perf;
+		u8	lowest_perf;
+		u8	min_limit_perf;
+		u8	max_limit_perf;

Just a thought, how about adding the "u8 desired_perf" (last requested) and "u8 prefcore_ranking" in this. We can pursue it as a separate patch if you want.

I think there is value in adding desired_perf atleast, so that when we are caching the min, max limits in the perf_cached variable, desired perf level is also atomically updated into the shared cpudata structure.

Thanks,
Dhananjay

Quoted 45 lines

drivers/cpufreq/amd-pstate.h +7 -18

+	};
+	u64	val;
+};
+
  /**
   * struct  amd_aperf_mperf
   * @aperf: actual performance frequency clock count

@@ -30,20 +58,9 @@ struct amd_aperf_mperf {
   * @cpu: CPU number
   * @req: constraint request to apply
   * @cppc_req_cached: cached performance request hints
- * @highest_perf: the maximum performance an individual processor may reach,
- *		  assuming ideal conditions
- *		  For platforms that do not support the preferred core feature, the
- *		  highest_pef may be configured with 166 or 255, to avoid max frequency
- *		  calculated wrongly. we take the fixed value as the highest_perf.
- * @nominal_perf: the maximum sustained performance level of the processor,
- *		  assuming ideal operating conditions
- * @lowest_nonlinear_perf: the lowest performance level at which nonlinear power
- *			   savings are achieved
- * @lowest_perf: the absolute lowest performance level of the processor
+ * @perf: cached performance-related data
   * @prefcore_ranking: the preferred core ranking, the higher value indicates a higher
   * 		  priority.
- * @min_limit_perf: Cached value of the performance corresponding to policy->min
- * @max_limit_perf: Cached value of the performance corresponding to policy->max
   * @min_limit_freq: Cached value of policy->min (in khz)
   * @max_limit_freq: Cached value of policy->max (in khz)
   * @nominal_freq: the frequency (in khz) that mapped to nominal_perf

@@ -68,13 +85,9 @@ struct amd_cpudata {
  	struct	freq_qos_request req[2];
  	u64	cppc_req_cached;
 
-	u8	highest_perf;
-	u8	nominal_perf;
-	u8	lowest_nonlinear_perf;
-	u8	lowest_perf;
+	union perf_cached perf;
+
  	u8	prefcore_ranking;
-	u8	min_limit_perf;
-	u8	max_limit_perf;
  	u32	min_limit_freq;
  	u32	max_limit_freq;
  	u32	nominal_freq;

Quoted 26 lines · Dhananjay Ugwekar

Quoted 25 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

By storing perf values in a union all the writes and reads can be done atomically, removing the need for some concurrency protections.

While making this change, also drop the cached frequency values, using inline helpers to calculate them on demand from perf value.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
v3:
  * Pick up tag
v2:
  * cache perf variable in unit tests
  * Drop unnecessary check from amd_pstate_update_min_max_limit()
  * Consistency with READ_ONCE()
  * Drop unneeded policy checks
  * add kdoc
---
  drivers/cpufreq/amd-pstate-ut.c |  18 +-- drivers/cpufreq/amd-pstate.c    | 195 ++++++++++++++++++-------------- drivers/cpufreq/amd-pstate.h    |  49 +++++---

  3 files changed, 151 insertions(+), 111 deletions(-)

Quoted 21 lines

Quoted 9 lines

+ *          calculated wrongly. we take the fixed value as the highest_perf.
+ * @nominal_perf: the maximum sustained performance level of the processor,
+ *          assuming ideal operating conditions
+ * @lowest_nonlinear_perf: the lowest performance level at which nonlinear power
+ *               savings are achieved
+ * @lowest_perf: the absolute lowest performance level of the processor
+ * @min_limit_perf: Cached value of the performance corresponding to policy->min
+ * @max_limit_perf: Cached value of the performance corresponding to policy->max
+ */

Quoted 8 lines

+union perf_cached {
+    struct {
+        u8    highest_perf;
+        u8    nominal_perf;
+        u8    lowest_nonlinear_perf;
+        u8    lowest_perf;
+        u8    min_limit_perf;
+        u8    max_limit_perf;

Just a thought, how about adding the "u8 desired_perf" (last requested) and "u8 prefcore_ranking" in this. We can pursue it as a separate patch if you want.

Quoted 3 lines

I think there is value in adding desired_perf atleast, so that when we are caching the min, max limits in the perf_cached variable, desired perf level is also atomically updated into the shared cpudata structure.

Can you see if there is any performance advantage to caching these? If there is, can you please do a follow up to my v5 series?

It's going to mean another write in amd_pstate_update() potentially.

Quoted 4 lines

Thanks,
Dhananjay

From: Mario Limonciello <[email protected]>

If a CPU is missing a policy or one has been offlined then the unit test is skipped for the rest of the CPUs on the system.

Instead; iterate online CPUs and skip any missing policies to allow continuing to test the rest of them.

Signed-off-by Mario Limonciello <[email protected]>

---
v3:
* Only check online CPUs
* Update commit message

---
 drivers/cpufreq/amd-pstate-ut.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/cpufreq/amd-pstate-ut.c +4 -4

index 028527a0019ca..3a541780f374f 100644
@@ -116,12 +116,12 @@ static int amd_pstate_ut_check_perf(u32 index)
 	struct amd_cpudata *cpudata = NULL;
 	union perf_cached cur_perf;
 
-	for_each_possible_cpu(cpu) {
+	for_each_online_cpu(cpu) {
 		struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
 
 		policy = cpufreq_cpu_get(cpu);
 		if (!policy)
-			break;
+			continue;
 		cpudata = policy->driver_data;
 
 		if (get_shared_mem()) {

@@ -188,12 +188,12 @@ static int amd_pstate_ut_check_freq(u32 index)
 	int cpu = 0;
 	struct amd_cpudata *cpudata = NULL;
 
-	for_each_possible_cpu(cpu) {
+	for_each_online_cpu(cpu) {
 		struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
 
 		policy = cpufreq_cpu_get(cpu);
 		if (!policy)
-			break;
+			continue;
 		cpudata = policy->driver_data;
 
 		if (!((policy->cpuinfo.max_freq >= cpudata->nominal_freq) &&

Signature · 1 line

2.43.0

From: Mario Limonciello <[email protected]>

The cpudata variable is only needed in the scope of the for loop. Move it there.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
 drivers/cpufreq/amd-pstate-ut.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/amd-pstate-ut.c +0 -0

index 3a541780f374f..6b04b5b54b3b5 100644

drivers/cpufreq/amd-pstate-ut.c +0 -0

 		policy = cpufreq_cpu_get(cpu);
 		if (!policy)

From: Mario Limonciello <[email protected]>

The CPPC enable register is configured as "write once". That is any future writes don't actually do anything.

Because of this, all the cleanup paths that currently exist for CPPC disable are non-effective.

Rework CPPC enable to only enable after all the CAP registers have been read to avoid enabling CPPC on CPUs with invalid _CPC or unpopulated MSRs.

As the register is write once, remove all cleanup paths as well.

Signed-off-by Mario Limonciello <[email protected]>

---
v3:
* Fixup for suspend/resume issue
---

drivers/cpufreq/amd-pstate.c +6 -4

-static int shmem_cppc_enable(bool enable)
+static int shmem_cppc_enable(struct cpufreq_policy *policy)
 {
-	int cpu, ret = 0;
+	struct amd_cpudata *cpudata = policy->driver_data;
 	struct cppc_perf_ctrls perf_ctrls;
+	int ret;
 
-	if (enable == cppc_enabled)
-		return 0;
+	ret = cppc_set_enable(cpudata->cpu, 1);
+	if (ret)
+		return ret;

drivers/cpufreq/amd-pstate.c +0 -11

-
-		/* Enable autonomous mode for EPP */
-		if (cppc_state == AMD_PSTATE_ACTIVE) {
-			/* Set desired perf as zero to allow EPP firmware control */
-			perf_ctrls.desired_perf = 0;
-			ret = cppc_set_perf(cpu, &perf_ctrls);
-			if (ret)
-				return ret;
-		}
 	}
 
-	cppc_enabled = enable;
 	return ret;
 }
 
 DEFINE_STATIC_CALL(amd_pstate_cppc_enable, msr_cppc_enable);
 
-static inline int amd_pstate_cppc_enable(bool enable)

drivers/cpufreq/amd-pstate.c +2 -10

-static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
+static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
 	union perf_cached perf = READ_ONCE(cpudata->perf);
 	int ret;
 
-	ret = amd_pstate_cppc_enable(true);
-	if (ret)
-		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
-
-
-	return amd_pstate_epp_update_limit(policy);
-}
+	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
 
-static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
-{

Quoted 19 lines · Mario Limonciello

From: Mario Limonciello <[email protected]>

The CPPC enable register is configured as "write once". That is any future writes don't actually do anything.

Because of this, all the cleanup paths that currently exist for CPPC disable are non-effective.

Rework CPPC enable to only enable after all the CAP registers have been read to avoid enabling CPPC on CPUs with invalid _CPC or unpopulated MSRs.

As the register is write once, remove all cleanup paths as well.

Signed-off-by Mario Limonciello <[email protected]>

---
v3:
* Fixup for suspend/resume issue
---

[..snip..]

Quoted 14 lines

drivers/cpufreq/amd-pstate.c +6 -4

-static int shmem_cppc_enable(bool enable)
+static int shmem_cppc_enable(struct cpufreq_policy *policy)
  {
-	int cpu, ret = 0;
+	struct amd_cpudata *cpudata = policy->driver_data;
  	struct cppc_perf_ctrls perf_ctrls;
+	int ret;
 
-	if (enable == cppc_enabled)
-		return 0;
+	ret = cppc_set_enable(cpudata->cpu, 1);
+	if (ret)
+		return ret;

drivers/cpufreq/amd-pstate.c static int shmem_set_epp(struct cpufreq_policy *policy, u8 epp) · line 396

Quoted 10 lines

drivers/cpufreq/amd-pstate.c +5 -2

 
-	for_each_present_cpu(cpu) {
-		ret = cppc_set_enable(cpu, enable);
+	/* Enable autonomous mode for EPP */
+	if (cppc_state == AMD_PSTATE_ACTIVE) {
+		/* Set desired perf as zero to allow EPP firmware control */
+		perf_ctrls.desired_perf = 0;
+		ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
  		if (ret)
  			return ret;

We don't need the if condition here. There is nothing following this inside the if block and the function return "ret" soon after coming out of this if block.

Quoted 18 lines

drivers/cpufreq/amd-pstate.c +0 -11

-
-		/* Enable autonomous mode for EPP */
-		if (cppc_state == AMD_PSTATE_ACTIVE) {
-			/* Set desired perf as zero to allow EPP firmware control */
-			perf_ctrls.desired_perf = 0;
-			ret = cppc_set_perf(cpu, &perf_ctrls);
-			if (ret)
-				return ret;
-		}
  	}
 
-	cppc_enabled = enable;
  	return ret;
  }
 
  DEFINE_STATIC_CALL(amd_pstate_cppc_enable, msr_cppc_enable);
 
-static inline int amd_pstate_cppc_enable(bool enable)

[..snip..]

Quoted 19 lines

drivers/cpufreq/amd-pstate.c +2 -10

-static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
+static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
  {
  	struct amd_cpudata *cpudata = policy->driver_data;
  	union perf_cached perf = READ_ONCE(cpudata->perf);
  	int ret;
 
-	ret = amd_pstate_cppc_enable(true);
-	if (ret)
-		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
-
-
-	return amd_pstate_epp_update_limit(policy);
-}
+	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
 
-static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
-{

drivers/cpufreq/amd-pstate.c static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy) · line 1615

Quoted 10 lines

drivers/cpufreq/amd-pstate.c +4 -4

-	struct amd_cpudata *cpudata = policy->driver_data;
-	int ret;
+	ret = amd_pstate_cppc_enable(policy);
+	if (ret)
+		return ret;
 
-	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
 
-	ret = amd_pstate_epp_reenable(policy);
+	ret = amd_pstate_update_perf(policy, 0, 0, perf.highest_perf, cpudata->epp_cached, false);

Previously, when a CPU came online, the callpath would be amd_pstate_epp_cpu_online(policy)
--> amd_pstate_epp_reenable(policy)

      --> amd_pstate_epp_update_limit(policy)
           --> amd_pstate_epp_update_limit(policy)

which reevaluates the min_perf_limit and max_perf_limit based on policy->min and policy->max and then calls

amd_pstate_update_perf(policy, min_limit_perf, 0, max_limit_perf, epp, false)

With this patch, we call

       amd_pstate_update_perf(policy, 0, 0, perf.highest_perf, cpudata->epp_cached, false);

which would set CPPC.min_perf to 0.

I guess this should be ok since cpufreq_online() would eventually call amd_pstate_verify() and amd_pstate_epp_set_policy() which should re-initialize the the min_limit_perf and max_limit_perf. Though I haven't verified if the behaviour changes with this patch when the CPU is offlined and brought back online.

From: Mario Limonciello <[email protected]>

EPP values are cached in the cpudata structure per CPU. This is needless though because they are also cached in the CPPC request variable.

Drop the separate cache for EPP values and always reference the CPPC request variable when needed.

Reviewed-by Dhananjay Ugwekar <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 27 ++++++++++++++-------------
 drivers/cpufreq/amd-pstate.h |  1 -
 2 files changed, 14 insertions(+), 14 deletions(-)

drivers/cpufreq/amd-pstate.c +14 -13

index 4660dd3f04796..48ec5e6527c68 100644
@@ -268,8 +268,6 @@ static int msr_update_perf(struct cpufreq_policy *policy, u8 min_perf,
 	}
 
 	WRITE_ONCE(cpudata->cppc_req_cached, value);
-	if (epp != cpudata->epp_cached)
-		WRITE_ONCE(cpudata->epp_cached, epp);
 
 	return 0;
 }

@@ -318,7 +316,6 @@ static int msr_set_epp(struct cpufreq_policy *policy, u8 epp)
 	}
 
 	/* update both so that msr_update_perf() can effectively check */
-	WRITE_ONCE(cpudata->epp_cached, epp);
 	WRITE_ONCE(cpudata->cppc_req_cached, value);
 
 	return ret;

@@ -335,9 +332,12 @@ static int shmem_set_epp(struct cpufreq_policy *policy, u8 epp)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
 	struct cppc_perf_ctrls perf_ctrls;
+	u8 epp_cached;
 	u64 value;
 	int ret;
 
+
+	epp_cached = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached);
 	if (trace_amd_pstate_epp_perf_enabled()) {
 		union perf_cached perf = cpudata->perf;
 

@@ -348,10 +348,10 @@ static int shmem_set_epp(struct cpufreq_policy *policy, u8 epp)
 					  FIELD_GET(AMD_CPPC_MAX_PERF_MASK,
 						    cpudata->cppc_req_cached),
 					  policy->boost_enabled,
-					  epp != cpudata->epp_cached);
+					  epp != epp_cached);
 	}
 
-	if (epp == cpudata->epp_cached)
+	if (epp == epp_cached)
 		return 0;
 
 	perf_ctrls.energy_perf = epp;

@@ -360,7 +360,6 @@ static int shmem_set_epp(struct cpufreq_policy *policy, u8 epp)
 		pr_debug("failed to set energy perf value (%d)\n", ret);
 		return ret;
 	}
-	WRITE_ONCE(cpudata->epp_cached, epp);
 
 	value = READ_ONCE(cpudata->cppc_req_cached);
 	value &= ~AMD_CPPC_EPP_PERF_MASK;

@@ -1203,9 +1202,11 @@ static ssize_t show_energy_performance_preference(
 				struct cpufreq_policy *policy, char *buf)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	u8 preference;
+	u8 preference, epp;
+
+	epp = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached);
 
-	switch (cpudata->epp_cached) {
+	switch (epp) {
 	case AMD_CPPC_EPP_PERFORMANCE:
 		preference = EPP_INDEX_PERFORMANCE;
 		break;

@@ -1568,7 +1569,7 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
 		epp = 0;
 	else
-		epp = READ_ONCE(cpudata->epp_cached);
+		epp = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached);
 
 	perf = READ_ONCE(cpudata->perf);
 

@@ -1604,22 +1605,22 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
 	struct amd_cpudata *cpudata = policy->driver_data;
 	union perf_cached perf = READ_ONCE(cpudata->perf);
 	int ret;
+	u8 epp;
+
+	epp = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached);
 
 	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
 
 	ret = amd_pstate_cppc_enable(policy);
 	if (ret)
 		return ret;
-
-
-	ret = amd_pstate_update_perf(policy, 0, 0, perf.highest_perf, cpudata->epp_cached, false);
+	ret = amd_pstate_update_perf(policy, 0, 0, perf.highest_perf, epp, false);
 	if (ret)
 		return ret;
 
 	cpudata->suspended = false;
 
 	return 0;
-
 }
 
 static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)

drivers/cpufreq/amd-pstate.h +0 -1

index 1a52582dbac9d..13918853f0a82 100644
@@ -100,7 +100,6 @@ struct amd_cpudata {
 	bool	hw_prefcore;
 
 	/* EPP feature related attributes*/
-	u8	epp_cached;
 	u32	policy;
 	bool	suspended;
 	u8	epp_default;

Signature · 1 line

2.43.0