| Message ID | 688d8e1ce875655c344ac3b29876da5dc0456739.1718262992.git.perry.yuan@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AMD Pstate Driver Core Performance Boost | expand |
[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Limonciello, Mario <Mario.Limonciello@amd.com> > Sent: Friday, June 14, 2024 1:55 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 3/9] cpufreq: introduce init_boost callback to > initialize boost state for pstate drivers > > On 6/13/2024 02:25, Perry Yuan wrote: > > Introduce a new init_boost callback in cpufreq to initialize the boost > > state for specific pstate drivers. This initialization is required > > before calling the set_boost interface for each CPU. > > > > The init_boost callback will set up and synchronize each CPU's current > > boost state before invoking the set_boost function. Without this step, > > the boost state may be inconsistent across CPUs. > > > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > --- > > drivers/cpufreq/cpufreq.c | 12 ++++++++++-- > > include/linux/cpufreq.h | 2 ++ > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 1fdabb660231..e1a4730f4f8c 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -1429,8 +1429,16 @@ static int cpufreq_online(unsigned int cpu) > > goto out_free_policy; > > } > > > > - /* Let the per-policy boost flag mirror the cpufreq_driver > boost during init */ > > - policy->boost_enabled = cpufreq_boost_enabled() && > policy_has_boost_freq(policy); > > + /* init boost state to prepare set_boost callback for each CPU > */ > > + if (cpufreq_driver->init_boost) { > > + ret = cpufreq_driver->init_boost(policy); > > + if (ret) > > + pr_debug("%s: %d: initialization boost > failed\n", __func__, > > + __LINE__); > > The message should have the subject at the beginning. IE: > > "boost initialization failed\n" > > Also, isn't this fatal if init failed? IE shouldn't failing have a: Firstly, I also add the " goto out_free_policy", and removed later, because I think it is a little risky to fail the whole online process if driver init boost callback failed. If driver init boost failed, it just let boost control function lost, but online initialize failed and go to free the policy, it is a big problem. I am ok to add the error handling if maintainer agree to see the potential online function aborting. 😊 Perry. > > goto out_free_policy; > > > + } else { > > + /* Let the per-policy boost flag mirror the > cpufreq_driver boost during init */ > > + policy->boost_enabled = cpufreq_boost_enabled() > && policy_has_boost_freq(policy); > > + } > > > > /* > > * The initialization has succeeded and the policy is online. > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index > > 20f7e98ee8af..0698c0292d8f 100644 > > --- a/include/linux/cpufreq.h > > +++ b/include/linux/cpufreq.h > > @@ -409,6 +409,8 @@ struct cpufreq_driver { > > bool boost_enabled; > > int (*set_boost)(struct cpufreq_policy *policy, int state); > > > > + /* initialize boost state to be consistent before calling set_boost */ > > + int (*init_boost)(struct cpufreq_policy *policy); > > /* > > * Set by drivers that want to register with the energy model after the > > * policy is properly initialized, but before the governor is started.
[Public] Hi Rafael > -----Original Message----- > From: Yuan, Perry <Perry.Yuan@amd.com> > Sent: Friday, June 14, 2024 10:43 AM > To: Limonciello, Mario <Mario.Limonciello@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 3/9] cpufreq: introduce init_boost callback to > initialize boost state for pstate drivers > > [AMD Official Use Only - AMD Internal Distribution Only] > > [AMD Official Use Only - AMD Internal Distribution Only] > > > -----Original Message----- > > From: Limonciello, Mario <Mario.Limonciello@amd.com> > > Sent: Friday, June 14, 2024 1:55 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 3/9] cpufreq: introduce init_boost callback to > > initialize boost state for pstate drivers > > > > On 6/13/2024 02:25, Perry Yuan wrote: > > > Introduce a new init_boost callback in cpufreq to initialize the > > > boost state for specific pstate drivers. This initialization is > > > required before calling the set_boost interface for each CPU. > > > > > > The init_boost callback will set up and synchronize each CPU's > > > current boost state before invoking the set_boost function. Without > > > this step, the boost state may be inconsistent across CPUs. > > > > > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > > --- > > > drivers/cpufreq/cpufreq.c | 12 ++++++++++-- > > > include/linux/cpufreq.h | 2 ++ > > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > > index 1fdabb660231..e1a4730f4f8c 100644 > > > --- a/drivers/cpufreq/cpufreq.c > > > +++ b/drivers/cpufreq/cpufreq.c > > > @@ -1429,8 +1429,16 @@ static int cpufreq_online(unsigned int cpu) > > > goto out_free_policy; > > > } > > > > > > - /* Let the per-policy boost flag mirror the cpufreq_driver > > boost during init */ > > > - policy->boost_enabled = cpufreq_boost_enabled() && > > policy_has_boost_freq(policy); > > > + /* init boost state to prepare set_boost callback for > > > + each CPU > > */ > > > + if (cpufreq_driver->init_boost) { > > > + ret = cpufreq_driver->init_boost(policy); > > > + if (ret) > > > + pr_debug("%s: %d: initialization boost > > failed\n", __func__, > > > + __LINE__); > > > > The message should have the subject at the beginning. IE: > > > > "boost initialization failed\n" > > > > Also, isn't this fatal if init failed? IE shouldn't failing have a: > > > Firstly, I also add the " goto out_free_policy", and removed later, because I > think it is a little risky to fail the whole online process if driver init boost > callback failed. > If driver init boost failed, it just let boost control function lost, but online > initialize failed and go to free the policy, it is a big problem. > I am ok to add the error handling if maintainer agree to see the potential > online function aborting. 😊 > > Perry. > > > > > goto out_free_policy; > > > > > + } else { > > > + /* Let the per-policy boost flag mirror the > > cpufreq_driver boost during init */ > > > + policy->boost_enabled = cpufreq_boost_enabled() > > && policy_has_boost_freq(policy); > > > + } > > > > > > /* > > > * The initialization has succeeded and the policy is online. > > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index > > > 20f7e98ee8af..0698c0292d8f 100644 > > > --- a/include/linux/cpufreq.h > > > +++ b/include/linux/cpufreq.h > > > @@ -409,6 +409,8 @@ struct cpufreq_driver { > > > bool boost_enabled; > > > int (*set_boost)(struct cpufreq_policy *policy, int state); > > > > > > + /* initialize boost state to be consistent before calling set_boost */ > > > + int (*init_boost)(struct cpufreq_policy *policy); > > > /* > > > * Set by drivers that want to register with the energy model after the > > > * policy is properly initialized, but before the governor is started. Could you please help to review this cpufreq change to add init_boost callback for the pstate driver? We need to add this to initialize boost_supported and boost_state according to SBIOS , then user could change the boost setting as preference. Perry.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 1fdabb660231..e1a4730f4f8c 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1429,8 +1429,16 @@ static int cpufreq_online(unsigned int cpu) goto out_free_policy; } - /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ - policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); + /* init boost state to prepare set_boost callback for each CPU */ + if (cpufreq_driver->init_boost) { + ret = cpufreq_driver->init_boost(policy); + if (ret) + pr_debug("%s: %d: initialization boost failed\n", __func__, + __LINE__); + } else { + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ + policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); + } /* * The initialization has succeeded and the policy is online. diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 20f7e98ee8af..0698c0292d8f 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -409,6 +409,8 @@ struct cpufreq_driver { bool boost_enabled; int (*set_boost)(struct cpufreq_policy *policy, int state); + /* initialize boost state to be consistent before calling set_boost */ + int (*init_boost)(struct cpufreq_policy *policy); /* * Set by drivers that want to register with the energy model after the * policy is properly initialized, but before the governor is started.
Introduce a new init_boost callback in cpufreq to initialize the boost state for specific pstate drivers. This initialization is required before calling the set_boost interface for each CPU. The init_boost callback will set up and synchronize each CPU's current boost state before invoking the set_boost function. Without this step, the boost state may be inconsistent across CPUs. Signed-off-by: Perry Yuan <perry.yuan@amd.com> --- drivers/cpufreq/cpufreq.c | 12 ++++++++++-- include/linux/cpufreq.h | 2 ++ 2 files changed, 12 insertions(+), 2 deletions(-)