From: "Yuan, Perry" <Perry.Yuan@amd.com>
To: "Limonciello, Mario" <Mario.Limonciello@amd.com>,
"rafael.j.wysocki@intel.com" <rafael.j.wysocki@intel.com>,
"viresh.kumar@linaro.org" <viresh.kumar@linaro.org>,
"Huang, Ray" <Ray.Huang@amd.com>,
"Shenoy, Gautham Ranjal" <gautham.shenoy@amd.com>,
"Petkov, Borislav" <Borislav.Petkov@amd.com>
Cc: "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-pm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization
Date: Thu, 9 May 2024 16:36:58 +0000 [thread overview]
Message-ID: <CYYPR12MB86556FDDBC0843970569177B9CE62@CYYPR12MB8655.namprd12.prod.outlook.com> (raw)
In-Reply-To: <9255af0c-4574-49fe-922f-56885ae2d55b@amd.com>
[AMD Official Use Only - General]
Hi Mario
> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Tuesday, May 7, 2024 11:19 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> viresh.kumar@linaro.org; Huang, Ray <Ray.Huang@amd.com>; Shenoy,
> Gautham Ranjal <gautham.shenoy@amd.com>; Petkov, Borislav
> <Borislav.Petkov@amd.com>
> Cc: 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 09/11] cpufreq: amd-pstate: implement heterogeneous
> core topology for highest performance initialization
>
> On 5/7/2024 02:15, Perry Yuan wrote:
> > Introduces an optimization to the AMD-Pstate driver by implementing a
> > heterogeneous core topology for the initialization of the highest
> > performance value while driver loading.
> > There are two type cores designed including performance core and
> > efficiency Core. each core type has different highest performance
> > value
>
> Capitalize the "E" in each.
>
> > and highest frequency initialized by power firmware, `amd_pstate`
> > driver
>
> Three things:
>
> 1. Rather than "power firmware" you should just say "platform".
> 2. I would use "configured" instead of "initialized"
> 3. This is a run on sentence.
>
> Here's my proposed change.
>
> Each core type has different highest performance and frequency values
> configured by the platform. The `amd_pstate` driver needs to identify the
> type of core to correctly set an appropriate highest perf value.
>
> > need to identify the core types and set correct highest perf value.
> >
> > X86_FEATURE_HETERO_CORE_TOPOLOGY is used to identify whether the
> > processor support heterogeneous core type by reading CPUID leaf
> > Fn_0x80000026_EAX and bit 30. if the bit is set as one, then
> > amd_pstate driver will check EBX 30:28 bits to get the core type.
> >
> > Value Description:
> > 0h Performance Core.
> > 1h Efficiency Core.
> >
> > https://www.amd.com/content/dam/amd/en/documents/processor-
> tech-docs/p
> > rogrammer-references/24593.pdf
>
> Use the word "Link:" to prefix this link.
>
> > PDF p274
> > Signed-off-by: Perry Yuan <perry.yuan@amd.com>
Above comments have be addressed in V2.
> > ---
> > arch/x86/include/asm/processor.h | 2 ++
> > arch/x86/kernel/cpu/amd.c | 19 ++++++++++
> > drivers/cpufreq/amd-pstate.c | 62 ++++++++++++++++++++++++-------
> -
> > include/linux/amd-pstate.h | 8 +++++
> > 4 files changed, 77 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/processor.h
> > b/arch/x86/include/asm/processor.h
> > index 811548f131f4..30d1900bb7e0 100644
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -683,10 +683,12 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu)
> > extern u32 amd_get_highest_perf(void);
> > extern void amd_clear_divider(void);
> > extern void amd_check_microcode(void);
> > +extern int amd_get_this_core_type(void);
> > #else
> > static inline u32 amd_get_highest_perf(void) { return 0; }
> > static inline void amd_clear_divider(void) { }
> > static inline void amd_check_microcode(void) { }
> > +static inline int amd_get_this_core_type(void) { }
> > #endif
> >
> > extern unsigned long arch_align_stack(unsigned long sp); diff --git
> > a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index
> > 307302af0aee..67966bdcde65 100644
> > --- a/arch/x86/kernel/cpu/amd.c
> > +++ b/arch/x86/kernel/cpu/amd.c
> > @@ -1219,3 +1219,22 @@ void noinstr amd_clear_divider(void)
> > :: "a" (0), "d" (0), "r" (1));
> > }
> > EXPORT_SYMBOL_GPL(amd_clear_divider);
> > +
> > +#define X86_CPU_TYPE_ID_SHIFT 28
> > +
> > +/**
> > + * amd_get_this_core_type - Get the type of this heterogeneous CPU
> > + *
> > + * Returns the CPU type [31:28] (i.e., performance or efficient) of
> > + * a CPU in the processor.
> > + * If the processor has no core type support, returns -1.
> > + */
> > +
> > +int amd_get_this_core_type(void)
> > +{
> > + if
> (!cpu_feature_enabled(X86_FEATURE_HETERO_CORE_TOPOLOGY))
> > + return -1;
> > +
> > + return cpuid_ebx(0x80000026) >> X86_CPU_TYPE_ID_SHIFT; }
> > +EXPORT_SYMBOL_GPL(amd_get_this_core_type);
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index 7145248b38ec..7fe8a8fc6227 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -50,7 +50,9 @@
> >
> > #define AMD_PSTATE_TRANSITION_LATENCY 20000
> > #define AMD_PSTATE_TRANSITION_DELAY 1000
> > -#define AMD_PSTATE_PREFCORE_THRESHOLD 166
> > +#define CPPC_HIGHEST_PERF_EFFICIENT 132
> > +#define CPPC_HIGHEST_PERF_PERFORMANCE 196
> > +#define CPPC_HIGHEST_PERF_DEFAULT 166
> >
> > /*
> > * TODO: We need more time to fine tune processors with shared
> > memory solution @@ -326,6 +328,49 @@ static inline int
> amd_pstate_enable(bool enable)
> > return static_call(amd_pstate_enable)(enable);
> > }
> >
> > +static void get_this_core_type(void *data) {
> > + int *cpu_type = data;
> > +
> > + *cpu_type = amd_get_this_core_type(); }
>
> Does this really need a static function calling a static function just to set a
> variable?
>
> I would think that you can simplify it something like this:
>
> int amd_get_core_type(void *data)
> {
> int *type = data;
>
> if (!type)
> return -EINVAL;
> if
> (!cpu_feature_enabled(X86_FEATURE_HETERO_CORE_TOPOLOGY)) {
> *type = -1;
> return -ENODEV;
> }
> *type = cpuid_ebx(0x80000026) >> X86_CPU_TYPE_ID_SHIFT; }
>
> And then amd_pstate_get_cpu_type() can call:
>
> smp_call_function_single(cpu, amd_get_core_type, &cpu_type, 1);
amd_get_this_core_type() cannot be merged, it will be called by some other components, for example qemu.
It is better to user separate function to make codes clear and simple to maintain.
Perry.
>
> > +
> > +static int amd_pstate_get_cpu_type(int cpu) {
> > + int cpu_type = 0;
> > +
> > + smp_call_function_single(cpu, get_this_core_type, &cpu_type, 1);
> > +
> > + return cpu_type;
> > +}
> > +
> > +static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata) {
> > + u32 highest_perf;
> > + int core_type;
> > +
> > + core_type = amd_pstate_get_cpu_type(cpudata->cpu);
> > + pr_debug("core_type %d found\n", core_type);
> > +
> > + switch (core_type) {
> > + case CPU_CORE_TYPE_NO_HETERO_SUP:
> > + highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
> > + break;
> > + case CPU_CORE_TYPE_PERFORMANCE:
> > + highest_perf = CPPC_HIGHEST_PERF_PERFORMANCE;
> > + break;
> > + case CPU_CORE_TYPE_EFFICIENCY:
> > + highest_perf = CPPC_HIGHEST_PERF_EFFICIENT;
> > + break;
> > + default:
> > + highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
> > + WARN_ONCE(true, "WARNING: Undefined core type
> found");
> > + break;
> > + }
> > +
> > + return highest_perf;
> > +}
> > +
> > static int pstate_init_perf(struct amd_cpudata *cpudata)
> > {
> > u64 cap1;
> > @@ -336,15 +381,7 @@ static int pstate_init_perf(struct amd_cpudata
> *cpudata)
> > 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
> > - * the default max perf.
> > - */
> > - if (cpudata->hw_prefcore)
> > - highest_perf = AMD_PSTATE_PREFCORE_THRESHOLD;
> > - else
> > - highest_perf = AMD_CPPC_HIGHEST_PERF(cap1);
> > + highest_perf = amd_pstate_highest_perf_set(cpudata);
>
> I think it would be a good idea to follow up later on validate the selections of
> this logic from amd-pstate-ut.c.
Yes, you are right.
pstate ut codes will be modified later
perry.
>
> >
> > WRITE_ONCE(cpudata->highest_perf, highest_perf);
> > WRITE_ONCE(cpudata->max_limit_perf, highest_perf); @@ -365,10
> > +402,7 @@ static int cppc_init_perf(struct amd_cpudata *cpudata)
> > if (ret)
> > return ret;
> >
> > - if (cpudata->hw_prefcore)
> > - highest_perf = AMD_PSTATE_PREFCORE_THRESHOLD;
> > - else
> > - highest_perf = cppc_perf.highest_perf;
> > + highest_perf = amd_pstate_highest_perf_set(cpudata);
> >
> > WRITE_ONCE(cpudata->highest_perf, highest_perf);
> > WRITE_ONCE(cpudata->max_limit_perf, highest_perf); diff --git
> > a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h index
> > d58fc022ec46..869d916003f1 100644
> > --- a/include/linux/amd-pstate.h
> > +++ b/include/linux/amd-pstate.h
> > @@ -134,4 +134,12 @@ struct quirk_entry {
> > u32 lowest_freq;
> > };
> >
> > +/* defined by CPUID_Fn80000026_EBX BIT [31:28] */ enum
> amd_core_type
> > +{
> > + CPU_CORE_TYPE_NO_HETERO_SUP = -1,
> > + CPU_CORE_TYPE_PERFORMANCE = 0,
> > + CPU_CORE_TYPE_EFFICIENCY = 1,
> > + CPU_CORE_TYPE_UNDEFINED = 2,
> > +};
> > +
> > #endif /* _LINUX_AMD_PSTATE_H */
next prev parent reply other threads:[~2024-05-09 16:37 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 7:15 [PATCH 00/11] AMD Pstate Driver Fixes and Improvements Perry Yuan
2024-05-07 7:15 ` [PATCH 01/11] cpufreq: amd-pstate: optimiza the initial frequency values verification Perry Yuan
2024-05-07 14:44 ` Mario Limonciello
2024-05-07 21:02 ` kernel test robot
2024-05-07 7:15 ` [PATCH 02/11] cpufreq: amd-pstate: show CPPC debug message if CPPC is not supported Perry Yuan
2024-05-07 14:45 ` Mario Limonciello
2024-05-07 7:15 ` [PATCH 03/11] cpufreq: amd-pstate: add debug message while CPPC is supported and disabled by SBIOS Perry Yuan
2024-05-07 14:55 ` Mario Limonciello
2024-05-07 7:15 ` [PATCH 04/11] Documentation: PM: amd-pstate: introducing recommended reboot requirement during driver switch Perry Yuan
2024-05-07 7:15 ` [PATCH 05/11] Documentation: PM: amd-pstate: add debugging section for driver loading failure Perry Yuan
2024-05-07 15:01 ` Mario Limonciello
2024-05-07 7:15 ` [PATCH 06/11] Documentation: PM: amd-pstate: add guide mode to the Operation mode Perry Yuan
2024-05-07 15:02 ` Mario Limonciello
2024-05-07 7:15 ` [PATCH 07/11] cpufreq: amd-pstate: switch boot_cpu_has() to cpu_feature_enabled() Perry Yuan
2024-05-07 15:03 ` Mario Limonciello
2024-05-07 7:15 ` [PATCH 08/11] x86/cpufeatures: Add feature bits for AMD heterogeneous processor Perry Yuan
2024-05-07 7:15 ` [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization Perry Yuan
2024-05-07 15:19 ` Mario Limonciello
2024-05-09 16:36 ` Yuan, Perry [this message]
2024-05-07 18:14 ` kernel test robot
2024-05-07 19:40 ` kernel test robot
2024-05-07 7:15 ` [PATCH 10/11] cpufreq: amd-pstate: fix the highest frequency issue which limit performance Perry Yuan
2024-05-07 15:23 ` Mario Limonciello
2024-05-07 7:15 ` [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate driver by default Perry Yuan
2024-05-07 14:41 ` Mario Limonciello
2024-05-08 15:20 ` Oleksandr Natalenko
2024-05-08 15:24 ` Yuan, Perry
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CYYPR12MB86556FDDBC0843970569177B9CE62@CYYPR12MB8655.namprd12.prod.outlook.com \
--to=perry.yuan@amd.com \
--cc=Alexander.Deucher@amd.com \
--cc=Borislav.Petkov@amd.com \
--cc=Li.Meng@amd.com \
--cc=Mario.Limonciello@amd.com \
--cc=Ray.Huang@amd.com \
--cc=Shimmer.Huang@amd.com \
--cc=Xiaojian.Du@amd.com \
--cc=gautham.shenoy@amd.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rafael.j.wysocki@intel.com \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).