diff mbox series

[v3,01/18] cpufreq/amd-pstate: Invalidate cppc_req_cached during suspend

Message ID 20250217220707.1468365-2-superm1@kernel.org (mailing list archive)
State Superseded, archived
Delegated to: Mario Limonciello
Headers show
Series amd-pstate cleanups | expand

Commit Message

Mario Limonciello Feb. 17, 2025, 10:06 p.m. UTC 
From: Mario Limonciello <mario.limonciello@amd.com>

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 <miroslav@pavleski.net>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217931
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Gautham R. Shenoy Feb. 19, 2025, 5:24 a.m. UTC | #1 
Hello Mario,


On Mon, Feb 17, 2025 at 04:06:50PM -0600, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> 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 <miroslav@pavleski.net>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217931
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index f425fb7ec77d7..12fb63169a24c 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -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 <gautham.shenoy@amd.com>
Dhananjay Ugwekar Feb. 19, 2025, 6:12 a.m. UTC | #2 
On 2/18/2025 3:36 AM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> 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 <miroslav@pavleski.net>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217931
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index f425fb7ec77d7..12fb63169a24c 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1611,7 +1611,7 @@ static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
>  					  max_perf, policy->boost_enabled);
>  	}
>  
> -	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

>  }
>  
>  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;
>
Dhananjay Ugwekar Feb. 19, 2025, 6:37 a.m. UTC | #3 
On 2/19/2025 11:42 AM, Dhananjay Ugwekar wrote:
> On 2/18/2025 3:36 AM, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> 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 <miroslav@pavleski.net>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217931
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>  drivers/cpufreq/amd-pstate.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index f425fb7ec77d7..12fb63169a24c 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -1611,7 +1611,7 @@ static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
>>  					  max_perf, policy->boost_enabled);
>>  	}
>>  
>> -	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.

You can ignore this, I see that you have handled it in the 3rd patch.

Reviewed-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

> 
> Patch looks good to me otherwise.
> 
> Thanks,
> Dhananjay
> 
>>  }
>>  
>>  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;
>>  
>
Mario Limonciello Feb. 19, 2025, 5:21 p.m. UTC | #4 
On 2/18/2025 23:24, Gautham R. Shenoy wrote:
> Hello Mario,
> 
> 
> On Mon, Feb 17, 2025 at 04:06:50PM -0600, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> 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 <miroslav@pavleski.net>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217931
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/cpufreq/amd-pstate.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index f425fb7ec77d7..12fb63169a24c 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -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.

Yeah; the tracing code gets updated later in the series.
My plan is this commit is a minimal fix, and will go to 6.14, the rest 
will be in 6.15.

> 
> The patch looks good to me otherwise.
> 
> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
> 

Thanks!
diff mbox series

Patch

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index f425fb7ec77d7..12fb63169a24c 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1611,7 +1611,7 @@  static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
 					  max_perf, policy->boost_enabled);
 	}
 
-	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;