RE: [PATCH v7 6/6] cpufreq:amd-pstate: initialize capabilities in amd_pstate_init_perf

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Gautham,

> -----Original Message-----
> From: Shenoy, Gautham Ranjal <gautham.shenoy@xxxxxxx>
> Sent: Thursday, March 14, 2024 2:39 PM
> To: Yuan, Perry <Perry.Yuan@xxxxxxx>
> Cc: rafael.j.wysocki@xxxxxxxxx; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>; viresh.kumar@xxxxxxxxxx; Huang, Ray
> <Ray.Huang@xxxxxxx>; Petkov, Borislav <Borislav.Petkov@xxxxxxx>;
> Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Huang, Shimmer
> <Shimmer.Huang@xxxxxxx>; Du, Xiaojian <Xiaojian.Du@xxxxxxx>; Meng,
> Li (Jassmine) <Li.Meng@xxxxxxx>; linux-pm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 6/6] cpufreq:amd-pstate: initialize capabilities in
> amd_pstate_init_perf
>
> Hello Perry,
>
> On Wed, Mar 13, 2024 at 05:59:18PM +0800, Perry Yuan wrote:
> > Moved the initialization of some perf and frequency values related to
> > cpudata to the amd_pstate_init_perf and cppc_init_perf functions.
> > It can avoid duplicate calls to cppc_get_perf_caps function.
>
> Does it make sense to fold this into Patch 2 where you are caching the nominal
> frequency for later use ?
>
> Otherwise, this patch looks good to me.

That nominal perf change is reviewed by Mario,
This patch can be reviewed separately and the whole changes can be applied after that without function impact.
It will be simpler to look what we changed in this one. 😊

Thanks for your review efforts!

Perry.

>
> --
> Thanks and Regards
> gautham.
>
> >
> > Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 43 ++++++++++++++----------------------
> >  include/linux/amd-pstate.h   |  1 +
> >  2 files changed, 18 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index 59bcdf829c93..3877d4ecb5d4
> 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -330,12 +330,18 @@ static int pstate_init_perf(struct amd_cpudata
> > *cpudata)  {
> >     u64 cap1;
> >     u32 highest_perf;
> > +   struct cppc_perf_caps cppc_perf;
> > +   int ret;
> >
> > -   int ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
> > +   ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
> >                                  &cap1);
> >     if (ret)
> >             return ret;
> >
> > +   ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> > +   if (ret)
> > +           return ret;
> > +
> >     /* For platforms that do not support the preferred core feature, the
> >      * highest_pef may be configured with 166 or 255, to avoid max
> frequency
> >      * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1)
> value
> > as @@ -353,6 +359,9 @@ static int pstate_init_perf(struct amd_cpudata
> *cpudata)
> >     WRITE_ONCE(cpudata->lowest_perf,
> AMD_CPPC_LOWEST_PERF(cap1));
> >     WRITE_ONCE(cpudata->prefcore_ranking,
> AMD_CPPC_HIGHEST_PERF(cap1));
> >     WRITE_ONCE(cpudata->min_limit_perf,
> AMD_CPPC_LOWEST_PERF(cap1));
> > +   WRITE_ONCE(cpudata->lowest_freq, cppc_perf.lowest_freq);
> > +   WRITE_ONCE(cpudata->nominal_freq, cppc_perf.nominal_freq);
> > +
> >     return 0;
> >  }
> >
> > @@ -360,8 +369,9 @@ static int cppc_init_perf(struct amd_cpudata
> > *cpudata)  {
> >     struct cppc_perf_caps cppc_perf;
> >     u32 highest_perf;
> > +   int ret;
> >
> > -   int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> > +   ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> >     if (ret)
> >             return ret;
> >
> > @@ -378,6 +388,8 @@ static int cppc_init_perf(struct amd_cpudata
> *cpudata)
> >     WRITE_ONCE(cpudata->lowest_perf, cppc_perf.lowest_perf);
> >     WRITE_ONCE(cpudata->prefcore_ranking, cppc_perf.highest_perf);
> >     WRITE_ONCE(cpudata->min_limit_perf, cppc_perf.lowest_perf);
> > +   WRITE_ONCE(cpudata->lowest_freq, cppc_perf.lowest_freq);
> > +   WRITE_ONCE(cpudata->nominal_freq, cppc_perf.nominal_freq);
> >
> >     if (cppc_state == AMD_PSTATE_ACTIVE)
> >             return 0;
> > @@ -642,17 +654,12 @@ static void amd_pstate_adjust_perf(unsigned int
> > cpu,
> >
> >  static int amd_get_min_freq(struct amd_cpudata *cpudata)  {
> > -   struct cppc_perf_caps cppc_perf;
> >     u32 lowest_freq;
> >
> > -   int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> > -   if (ret)
> > -           return ret;
> > -
> >     if (quirks && quirks->lowest_freq)
> >             lowest_freq = quirks->lowest_freq;
> >     else
> > -           lowest_freq = cppc_perf.lowest_freq;
> > +           lowest_freq = READ_ONCE(cpudata->lowest_freq);
> >
> >     /* Switch to khz */
> >     return lowest_freq * 1000;
> > @@ -660,14 +667,9 @@ static int amd_get_min_freq(struct amd_cpudata
> > *cpudata)
> >
> >  static int amd_get_max_freq(struct amd_cpudata *cpudata)  {
> > -   struct cppc_perf_caps cppc_perf;
> >     u32 max_perf, max_freq, nominal_freq, nominal_perf;
> >     u64 boost_ratio;
> >
> > -   int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> > -   if (ret)
> > -           return ret;
> > -
> >     nominal_freq = READ_ONCE(cpudata->nominal_freq);
> >     nominal_perf = READ_ONCE(cpudata->nominal_perf);
> >     max_perf = READ_ONCE(cpudata->highest_perf); @@ -683,36
> +685,25 @@
> > static int amd_get_max_freq(struct amd_cpudata *cpudata)
> >
> >  static int amd_get_nominal_freq(struct amd_cpudata *cpudata)  {
> > -   struct cppc_perf_caps cppc_perf;
> >     u32 nominal_freq;
> >
> > -   int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> > -   if (ret)
> > -           return ret;
> > -
> >     if (quirks && quirks->nominal_freq)
> >             nominal_freq = quirks->nominal_freq;
> >     else
> > -           nominal_freq = cppc_perf.nominal_freq;
> > +           nominal_freq = READ_ONCE(cpudata->nominal_freq);
> >
> >     return nominal_freq;
> >  }
> >
> >  static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
> > {
> > -   struct cppc_perf_caps cppc_perf;
> >     u32 lowest_nonlinear_freq, lowest_nonlinear_perf,
> >         nominal_freq, nominal_perf;
> >     u64 lowest_nonlinear_ratio;
> >
> > -   int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> > -   if (ret)
> > -           return ret;
> > -
> >     nominal_freq = READ_ONCE(cpudata->nominal_freq);
> >     nominal_perf = READ_ONCE(cpudata->nominal_perf);
> > -
> > -   lowest_nonlinear_perf = cppc_perf.lowest_nonlinear_perf;
> > +   lowest_nonlinear_perf = READ_ONCE(cpudata-
> >lowest_nonlinear_perf);
> >
> >     lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf <<
> SCHED_CAPACITY_SHIFT,
> >                                      nominal_perf);
> > diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> > index 7b2cbb892fd9..1fbbe75c3dcc 100644
> > --- a/include/linux/amd-pstate.h
> > +++ b/include/linux/amd-pstate.h
> > @@ -88,6 +88,7 @@ struct amd_cpudata {
> >     u32     min_freq;
> >     u32     nominal_freq;
> >     u32     lowest_nonlinear_freq;
> > +   u32     lowest_freq;
> >
> >     struct amd_aperf_mperf cur;
> >     struct amd_aperf_mperf prev;
> > --
> > 2.34.1
> >