diff mbox series

[v6,3/6] cpufreq:amd-pstate: initialize nominal_freq of each cpudata

Message ID b4fcf3b4c2b5abcf79bbaf9780fe67b4ec697dee.1707363758.git.perry.yuan@amd.com
State New
Headers
Series AMD Pstate Fixes And Enhancements |

Commit Message

Yuan, Perry Feb. 8, 2024, 3:46 a.m. UTC 
  Optimizes the process of retrieving the nominal frequency by utilizing
'cpudata->nominal_freq' instead of repeatedly accessing the cppc_acpi interface.

To enhance efficiency and reduce the CPU load, shifted to using
'cpudata->nominal_freq'. It allows for the nominal frequency to be accessed
directly from the cached data in 'cpudata' of each CPU.
It will also slightly reduce the frequency change latency while using pstate
driver passive mode.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Comments

Gautham R. Shenoy Feb. 8, 2024, 6:34 a.m. UTC | #1 
On Thu, Feb 08, 2024 at 11:46:30AM +0800, Perry Yuan wrote:
> Optimizes the process of retrieving the nominal frequency by utilizing
> 'cpudata->nominal_freq' instead of repeatedly accessing the cppc_acpi interface.
> 
> To enhance efficiency and reduce the CPU load, shifted to using
> 'cpudata->nominal_freq'. It allows for the nominal frequency to be accessed
> directly from the cached data in 'cpudata' of each CPU.
> It will also slightly reduce the frequency change latency while using pstate
> driver passive mode.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ac7faa98a450..ea8681ea3bad 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -619,7 +619,7 @@ static int amd_get_max_freq(struct amd_cpudata *cpudata)
>  	if (ret)
>  		return ret;
>  
> -	nominal_freq = cppc_perf.nominal_freq;
> +	nominal_freq = READ_ONCE(cpudata->nominal_freq);

Does it make sense to call cppc_get_perf_caps() in
amd_pstate_cpu_init() and amd_pstate_epp_cpu_init() and cache all the
relevant metrics such as highest_perf, lowest_perf,
lowest_nonlinear_perf, nominal_perf, nominal_freq, lowest_freq so that
subsequent calls to amd_get_max/min/lowest_nonlinear/nominal_freq()
can use these cached values instead of calling cppc_get_perf_caps()
again ?

Because even with this patch, we continue to call cppc_get_perf_caps()
in amd_get_max_freq(), amd_get_min_freq() and
amd_get_lowest_nonlinear_freq().


>  	nominal_perf = READ_ONCE(cpudata->nominal_perf);
>  	max_perf = READ_ONCE(cpudata->highest_perf);
>  
> @@ -654,7 +654,7 @@ static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
>  	if (ret)
>  		return ret;
>  
> -	nominal_freq = cppc_perf.nominal_freq;
> +	nominal_freq = READ_ONCE(cpudata->nominal_freq);
>  	nominal_perf = READ_ONCE(cpudata->nominal_perf);
>  
>  	lowest_nonlinear_perf = cppc_perf.lowest_nonlinear_perf;
> @@ -848,13 +848,14 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>  		goto free_cpudata1;
>  
>  	min_freq = amd_get_min_freq(cpudata);
> -	max_freq = amd_get_max_freq(cpudata);
>  	nominal_freq = amd_get_nominal_freq(cpudata);
> +	cpudata->nominal_freq = nominal_freq;
> +	max_freq = amd_get_max_freq(cpudata);
>  	lowest_nonlinear_freq = amd_get_lowest_nonlinear_freq(cpudata);
>  
> -	if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) {
> -		dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n",
> -			min_freq, max_freq);
> +	if (min_freq < 0 || max_freq < 0 || min_freq > max_freq || nominal_freq == 0) {

+1 for the "nominal_freq == 0" check. On CPPCv2, nominal_freq and
lowest_freq are not advertised. So nominal_freq will be 0. It is
better to fail here than failing later at the time of governor
initialization.


> +		dev_err(dev, "min_freq(%d) or max_freq(%d) or nominal_freq(%d) is incorrect\n",
> +			min_freq, max_freq, nominal_freq);
>  		ret = -EINVAL;
>  		goto free_cpudata1;
>  	}
> @@ -893,7 +894,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>  	cpudata->min_freq = min_freq;
>  	cpudata->max_limit_freq = max_freq;
>  	cpudata->min_limit_freq = min_freq;
> -	cpudata->nominal_freq = nominal_freq;
>  	cpudata->lowest_nonlinear_freq = lowest_nonlinear_freq;
>  
>  	policy->driver_data = cpudata;
> @@ -1310,12 +1310,13 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  		goto free_cpudata1;
>  
>  	min_freq = amd_get_min_freq(cpudata);
> -	max_freq = amd_get_max_freq(cpudata);
>  	nominal_freq = amd_get_nominal_freq(cpudata);
> +	cpudata->nominal_freq = nominal_freq;
> +	max_freq = amd_get_max_freq(cpudata);
>  	lowest_nonlinear_freq = amd_get_lowest_nonlinear_freq(cpudata);
> -	if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) {
> -		dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n",
> -				min_freq, max_freq);
> +	if (min_freq < 0 || max_freq < 0 || min_freq > max_freq || nominal_freq == 0) {


Ditto.

> +		dev_err(dev, "min_freq(%d) or max_freq(%d) or nominal_freq(%d) is incorrect\n",
> +				min_freq, max_freq, nominal_freq);
>  		ret = -EINVAL;
>  		goto free_cpudata1;
>  	}
> @@ -1328,7 +1329,6 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	/* Initial processor data capability frequencies */
>  	cpudata->max_freq = max_freq;
>  	cpudata->min_freq = min_freq;
> -	cpudata->nominal_freq = nominal_freq;
>  	cpudata->lowest_nonlinear_freq = lowest_nonlinear_freq;


--
Thanks and Regards
gautham.
diff mbox series

Patch

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index ac7faa98a450..ea8681ea3bad 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -619,7 +619,7 @@  static int amd_get_max_freq(struct amd_cpudata *cpudata)
 	if (ret)
 		return ret;
 
-	nominal_freq = cppc_perf.nominal_freq;
+	nominal_freq = READ_ONCE(cpudata->nominal_freq);
 	nominal_perf = READ_ONCE(cpudata->nominal_perf);
 	max_perf = READ_ONCE(cpudata->highest_perf);
 
@@ -654,7 +654,7 @@  static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
 	if (ret)
 		return ret;
 
-	nominal_freq = cppc_perf.nominal_freq;
+	nominal_freq = READ_ONCE(cpudata->nominal_freq);
 	nominal_perf = READ_ONCE(cpudata->nominal_perf);
 
 	lowest_nonlinear_perf = cppc_perf.lowest_nonlinear_perf;
@@ -848,13 +848,14 @@  static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 		goto free_cpudata1;
 
 	min_freq = amd_get_min_freq(cpudata);
-	max_freq = amd_get_max_freq(cpudata);
 	nominal_freq = amd_get_nominal_freq(cpudata);
+	cpudata->nominal_freq = nominal_freq;
+	max_freq = amd_get_max_freq(cpudata);
 	lowest_nonlinear_freq = amd_get_lowest_nonlinear_freq(cpudata);
 
-	if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) {
-		dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n",
-			min_freq, max_freq);
+	if (min_freq < 0 || max_freq < 0 || min_freq > max_freq || nominal_freq == 0) {
+		dev_err(dev, "min_freq(%d) or max_freq(%d) or nominal_freq(%d) is incorrect\n",
+			min_freq, max_freq, nominal_freq);
 		ret = -EINVAL;
 		goto free_cpudata1;
 	}
@@ -893,7 +894,6 @@  static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 	cpudata->min_freq = min_freq;
 	cpudata->max_limit_freq = max_freq;
 	cpudata->min_limit_freq = min_freq;
-	cpudata->nominal_freq = nominal_freq;
 	cpudata->lowest_nonlinear_freq = lowest_nonlinear_freq;
 
 	policy->driver_data = cpudata;
@@ -1310,12 +1310,13 @@  static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 		goto free_cpudata1;
 
 	min_freq = amd_get_min_freq(cpudata);
-	max_freq = amd_get_max_freq(cpudata);
 	nominal_freq = amd_get_nominal_freq(cpudata);
+	cpudata->nominal_freq = nominal_freq;
+	max_freq = amd_get_max_freq(cpudata);
 	lowest_nonlinear_freq = amd_get_lowest_nonlinear_freq(cpudata);
-	if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) {
-		dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n",
-				min_freq, max_freq);
+	if (min_freq < 0 || max_freq < 0 || min_freq > max_freq || nominal_freq == 0) {
+		dev_err(dev, "min_freq(%d) or max_freq(%d) or nominal_freq(%d) is incorrect\n",
+				min_freq, max_freq, nominal_freq);
 		ret = -EINVAL;
 		goto free_cpudata1;
 	}
@@ -1328,7 +1329,6 @@  static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	/* Initial processor data capability frequencies */
 	cpudata->max_freq = max_freq;
 	cpudata->min_freq = min_freq;
-	cpudata->nominal_freq = nominal_freq;
 	cpudata->lowest_nonlinear_freq = lowest_nonlinear_freq;
 
 	policy->driver_data = cpudata;