| Message ID | c46eb6d79bd8e7068955cf993ffc6b726d86eb92.1718262992.git.perry.yuan@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mario Limonciello |
| Headers | show |
| Series | AMD Pstate Driver Core Performance Boost | expand |
On 6/13/2024 02:25, Perry Yuan wrote: > Add support for the set_boost callback in the active mode driver to > enable boost control via the cpufreq core. This ensures a consistent > boost control interface across all pstate modes, including passive > mode, guided mode, and active mode. > > With this addition, all three pstate modes can support the same boost > control interface with unique interface and global CPB control. Each > CPU also supports individual boost control, allowing global CPB to > change all cores' boost states simultaneously. Specific CPUs can > update their boost states separately, ensuring all cores' boost > states are synchronized. > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index fe7c9d3562c5..d07f09dd7eab 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -701,20 +701,11 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) > pr_err("Boost mode is not supported by this processor or SBIOS\n"); > return -ENOTSUPP; > } > + mutex_lock(&amd_pstate_driver_lock); > + ret = amd_pstate_cpu_boost(policy->cpu, state); > + mutex_unlock(&amd_pstate_driver_lock); > > - if (state) > - policy->cpuinfo.max_freq = cpudata->max_freq; > - else > - policy->cpuinfo.max_freq = cpudata->nominal_freq * 1000; > - > - policy->max = policy->cpuinfo.max_freq; > - > - ret = freq_qos_update_request(&cpudata->req[1], > - policy->cpuinfo.max_freq); > - if (ret < 0) > - return ret; > - > - return 0; > + return ret < 0 ? ret : 0; What's wrong with just doing this instead: return ret; > } > > static int amd_pstate_boost_set(struct amd_cpudata *cpudata) > @@ -1875,6 +1866,7 @@ static struct cpufreq_driver amd_pstate_epp_driver = { > .resume = amd_pstate_epp_resume, > .update_limits = amd_pstate_update_limits, > .init_boost = amd_pstate_init_boost, > + .set_boost = amd_pstate_set_boost, > .name = "amd-pstate-epp", > .attr = amd_pstate_epp_attr, > };
[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Limonciello, Mario <Mario.Limonciello@amd.com> > Sent: Friday, June 14, 2024 1:53 AM > To: Yuan, Perry <Perry.Yuan@amd.com>; Shenoy, Gautham Ranjal > <gautham.shenoy@amd.com>; Petkov, Borislav <Borislav.Petkov@amd.com> > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Deucher, Alexander > <Alexander.Deucher@amd.com>; Huang, Shimmer > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH v11 6/9] cpufreq: amd-pstate: Add set_boost callback for > active mode > > On 6/13/2024 02:25, Perry Yuan wrote: > > Add support for the set_boost callback in the active mode driver to > > enable boost control via the cpufreq core. This ensures a consistent > > boost control interface across all pstate modes, including passive > > mode, guided mode, and active mode. > > > > With this addition, all three pstate modes can support the same boost > > control interface with unique interface and global CPB control. Each > > CPU also supports individual boost control, allowing global CPB to > > change all cores' boost states simultaneously. Specific CPUs can > > update their boost states separately, ensuring all cores' boost states > > are synchronized. > > > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > --- > > drivers/cpufreq/amd-pstate.c | 18 +++++------------- > > 1 file changed, 5 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/cpufreq/amd-pstate.c > > b/drivers/cpufreq/amd-pstate.c index fe7c9d3562c5..d07f09dd7eab > 100644 > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -701,20 +701,11 @@ static int amd_pstate_set_boost(struct > cpufreq_policy *policy, int state) > > pr_err("Boost mode is not supported by this processor or > SBIOS\n"); > > return -ENOTSUPP; > > } > > + mutex_lock(&amd_pstate_driver_lock); > > + ret = amd_pstate_cpu_boost(policy->cpu, state); > > + mutex_unlock(&amd_pstate_driver_lock); > > > > - if (state) > > - policy->cpuinfo.max_freq = cpudata->max_freq; > > - else > > - policy->cpuinfo.max_freq = cpudata->nominal_freq * 1000; > > - > > - policy->max = policy->cpuinfo.max_freq; > > - > > - ret = freq_qos_update_request(&cpudata->req[1], > > - policy->cpuinfo.max_freq); > > - if (ret < 0) > > - return ret; > > - > > - return 0; > > + return ret < 0 ? ret : 0; > > What's wrong with just doing this instead: > > return ret; /** * freq_qos_update_request - Modify existing frequency QoS request. * @req: Request to modify. * @new_value: New request value. * * Update an existing frequency QoS request along with the effective constraint * value for the list of requests it belongs to. * * Return 1 if the effective constraint value has changed, 0 if the effective * constraint value has not changed, or a negative error code on failures. */ int freq_qos_update_request(struct freq_qos_request *req, s32 new_value) The effective return value from freq_qos_update_request will be "1", we expect the return value to be -1 or 0 in the code path, 0 is the successful return value passed to caller. Perry. > > > } > > > > static int amd_pstate_boost_set(struct amd_cpudata *cpudata) @@ > > -1875,6 +1866,7 @@ static struct cpufreq_driver amd_pstate_epp_driver = > { > > .resume = amd_pstate_epp_resume, > > .update_limits = amd_pstate_update_limits, > > .init_boost = amd_pstate_init_boost, > > + .set_boost = amd_pstate_set_boost, > > .name = "amd-pstate-epp", > > .attr = amd_pstate_epp_attr, > > };
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index fe7c9d3562c5..d07f09dd7eab 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -701,20 +701,11 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) pr_err("Boost mode is not supported by this processor or SBIOS\n"); return -ENOTSUPP; } + mutex_lock(&amd_pstate_driver_lock); + ret = amd_pstate_cpu_boost(policy->cpu, state); + mutex_unlock(&amd_pstate_driver_lock); - if (state) - policy->cpuinfo.max_freq = cpudata->max_freq; - else - policy->cpuinfo.max_freq = cpudata->nominal_freq * 1000; - - policy->max = policy->cpuinfo.max_freq; - - ret = freq_qos_update_request(&cpudata->req[1], - policy->cpuinfo.max_freq); - if (ret < 0) - return ret; - - return 0; + return ret < 0 ? ret : 0; } static int amd_pstate_boost_set(struct amd_cpudata *cpudata) @@ -1875,6 +1866,7 @@ static struct cpufreq_driver amd_pstate_epp_driver = { .resume = amd_pstate_epp_resume, .update_limits = amd_pstate_update_limits, .init_boost = amd_pstate_init_boost, + .set_boost = amd_pstate_set_boost, .name = "amd-pstate-epp", .attr = amd_pstate_epp_attr, };
Add support for the set_boost callback in the active mode driver to enable boost control via the cpufreq core. This ensures a consistent boost control interface across all pstate modes, including passive mode, guided mode, and active mode. With this addition, all three pstate modes can support the same boost control interface with unique interface and global CPB control. Each CPU also supports individual boost control, allowing global CPB to change all cores' boost states simultaneously. Specific CPUs can update their boost states separately, ensuring all cores' boost states are synchronized. Signed-off-by: Perry Yuan <perry.yuan@amd.com> --- drivers/cpufreq/amd-pstate.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)