RE: [PATCH v6 2/6] cpufreq: amd-pstate: initialize new core precision boost state

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Gautham,


> -----Original Message-----
> From: Shenoy, Gautham Ranjal <gautham.shenoy@xxxxxxx>
> Sent: Wednesday, March 20, 2024 7:32 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>; oleksandr@xxxxxxxxxxxxxx; Du, Xiaojian
> <Xiaojian.Du@xxxxxxx>; Meng, Li (Jassmine) <Li.Meng@xxxxxxx>; linux-
> pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 2/6] cpufreq: amd-pstate: initialize new core precision
> boost state
>
> Hello Perry,
>
> On Mon, Mar 18, 2024 at 06:11:09PM +0800, Perry Yuan wrote:
> > From: Perry Yuan <Perry.Yuan@xxxxxxx>
> >
> > Add gloal global_params to represent current CPU Performance
> > Boost(cpb)
>       ^^^^^^
>       global ?

Typo will be fixed in v7.


>
> > state for cpu frequency scaling, both active and passive modes all can
> > support CPU cores frequency boosting control which is based on the
> > BIOS setting, while BIOS turn on the "Core Performance Boost", it will
> > allow OS control each core highest perf limitation from OS side.
>
> Could we reword this portion along the lines of the following:
>
> "The active, guided and passive modes of the amd-pstate driver can support
> frequency boost control when the "Core Performance Boost"
> (CPB) feature is enabled in the BIOS.  When enabled in BIOS, the user has an
> option at runtime to allow/disallow the cores from operating in the boost
> frequency range.
>
> Add an amd_pstate_global_params object to record whether CPB is enabled in
> BIOS, and if it has been activated by the user."

Sure, will revise the description in v7.

>
> >
> > If core performance boost is disabled while a core is in a boosted
> > P-state, the core transitions to the highest performance non-boosted
> > P-state, that is the same as the nominal frequency limit.
>
> >
> > Reported-by: Artem S. Tashkinov" <aros@xxxxxxx>
> > Cc: Oleksandr Natalenko <oleksandr@xxxxxxxxxxxxxx>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217931
> > Signed-off-by: Perry Yuan <Perry.Yuan@xxxxxxx>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++++++++++---------
> >  include/linux/amd-pstate.h   | 13 ++++++++++++
> >  2 files changed, 42 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index 59a2db225d98..81787f83c906
> 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -68,6 +68,8 @@ static int cppc_state = AMD_PSTATE_UNDEFINED;
> > static bool cppc_enabled;  static bool amd_pstate_prefcore = true;
> > static struct quirk_entry *quirks;
> > +struct amd_pstate_global_params amd_pstate_global_params;
> > +EXPORT_SYMBOL_GPL(amd_pstate_global_params);
> >
> >  /*
> >   * AMD Energy Preference Performance (EPP)
>
> [..snip..]
>
> > diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> > index 6b832153a126..c5e41de65f70 100644
> > --- a/include/linux/amd-pstate.h
> > +++ b/include/linux/amd-pstate.h
> > @@ -134,4 +134,17 @@ struct quirk_entry {
> >     u32 lowest_freq;
> >  };
> >
> > +/**
> > + * struct amd_pstate_global_params - Global parameters, mostly tunable via
> sysfs.
> > + * @cpb_boost:             Whether or not to use boost CPU P-states.
> > + * @cpb_supported: Whether or not CPU boost P-states are available
> > + *                 based on the MSR_K7_HWCR bit[25] state
> > + */
> > +struct amd_pstate_global_params {
> > +   bool cpb_boost;
> > +   bool cpb_supported;
> > +};
> > +
> > +extern struct amd_pstate_global_params amd_pstate_global_params;
>
> Will this be used in multiple files ? If no, it is better to define this in amd-pstate.c

Yes, the var will be used by amd-pstate-ut.c which can detect the CPB state for unit testing.
So we need to expose this


>
> Otherwise, I have no other issues with the patch.

Thanks for the review,

Perry.

>
> --
> Thanks and Regards
> gautham.