Re: [PATCH v3 18/18] cpufreq/amd-pstate: Stop caching EPP

[Gautham R. Shenoy]

On Mon, Feb 17, 2025 at 04:07:07PM -0600, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@xxxxxxx>
> 
> 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 <dhananjay.ugwekar@xxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>

LGTM

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@xxxxxxx>


-- 
Thanks and Regards
gautham.

> ---
>  drivers/cpufreq/amd-pstate.c | 27 ++++++++++++++-------------
>  drivers/cpufreq/amd-pstate.h |  1 -
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 4660dd3f04796..48ec5e6527c68 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -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)
> diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
> index 1a52582dbac9d..13918853f0a82 100644
> --- a/drivers/cpufreq/amd-pstate.h
> +++ b/drivers/cpufreq/amd-pstate.h
> @@ -100,7 +100,6 @@ struct amd_cpudata {
>  	bool	hw_prefcore;
>  
>  	/* EPP feature related attributes*/
> -	u8	epp_cached;
>  	u32	policy;
>  	bool	suspended;
>  	u8	epp_default;
> -- 
> 2.43.0
>