[permalink] [raw]

Subject: Re: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization

2024-05-07 21:04:03

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 01/11] cpufreq: amd-pstate: optimiza the initial frequency values verification

Hi Perry,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge next-20240507]
[cannot apply to tip/x86/core linus/master v6.9-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Perry-Yuan/cpufreq-amd-pstate-optimiza-the-initial-frequency-values-verification/20240507-151930
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/0049ad44052b051cf57d1059bf71b7ce227a5f21.1715065568.git.perry.yuan%40amd.com
patch subject: [PATCH 01/11] cpufreq: amd-pstate: optimiza the initial frequency values verification
config: x86_64-randconfig-102-20240507 (https://download.01.org/0day-ci/archive/20240508/[email protected]/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

drivers/cpufreq/amd-pstate.c: In function 'amd_pstate_cpu_init':
>> drivers/cpufreq/amd-pstate.c:899:33: warning: variable 'nominal_freq' set but not used [-Wunused-but-set-variable]
899 | int min_freq, max_freq, nominal_freq, ret;
| ^~~~~~~~~~~~
drivers/cpufreq/amd-pstate.c: In function 'amd_pstate_epp_cpu_init':
drivers/cpufreq/amd-pstate.c:1350:33: warning: variable 'nominal_freq' set but not used [-Wunused-but-set-variable]
1350 | int min_freq, max_freq, nominal_freq, ret;
| ^~~~~~~~~~~~

vim +/nominal_freq +899 drivers/cpufreq/amd-pstate.c

5547c0ebfc2efd Perry Yuan 2024-04-25 896
ec437d71db77a1 Huang Rui 2021-12-24 897 static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
ec437d71db77a1 Huang Rui 2021-12-24 898 {
5c3fd1edaa8b4c Perry Yuan 2024-04-30 @899 int min_freq, max_freq, nominal_freq, ret;
ec437d71db77a1 Huang Rui 2021-12-24 900 struct device *dev;
ec437d71db77a1 Huang Rui 2021-12-24 901 struct amd_cpudata *cpudata;
ec437d71db77a1 Huang Rui 2021-12-24 902
919f4557696939 Wyes Karny 2022-11-17 903 /*
919f4557696939 Wyes Karny 2022-11-17 904 * Resetting PERF_CTL_MSR will put the CPU in P0 frequency,
919f4557696939 Wyes Karny 2022-11-17 905 * which is ideal for initialization process.
919f4557696939 Wyes Karny 2022-11-17 906 */
919f4557696939 Wyes Karny 2022-11-17 907 amd_perf_ctl_reset(policy->cpu);
ec437d71db77a1 Huang Rui 2021-12-24 908 dev = get_cpu_device(policy->cpu);
ec437d71db77a1 Huang Rui 2021-12-24 909 if (!dev)
ec437d71db77a1 Huang Rui 2021-12-24 910 return -ENODEV;
ec437d71db77a1 Huang Rui 2021-12-24 911
ec437d71db77a1 Huang Rui 2021-12-24 912 cpudata = kzalloc(sizeof(*cpudata), GFP_KERNEL);
ec437d71db77a1 Huang Rui 2021-12-24 913 if (!cpudata)
ec437d71db77a1 Huang Rui 2021-12-24 914 return -ENOMEM;
ec437d71db77a1 Huang Rui 2021-12-24 915
ec437d71db77a1 Huang Rui 2021-12-24 916 cpudata->cpu = policy->cpu;
ec437d71db77a1 Huang Rui 2021-12-24 917
f3a052391822b7 Meng Li 2024-01-19 918 amd_pstate_init_prefcore(cpudata);
f3a052391822b7 Meng Li 2024-01-19 919
ec437d71db77a1 Huang Rui 2021-12-24 920 ret = amd_pstate_init_perf(cpudata);
ec437d71db77a1 Huang Rui 2021-12-24 921 if (ret)
41271016dfa4a0 Huang Rui 2021-12-24 922 goto free_cpudata1;
ec437d71db77a1 Huang Rui 2021-12-24 923
5547c0ebfc2efd Perry Yuan 2024-04-25 924 ret = amd_pstate_init_freq(cpudata);
5547c0ebfc2efd Perry Yuan 2024-04-25 925 if (ret)
5547c0ebfc2efd Perry Yuan 2024-04-25 926 goto free_cpudata1;
5547c0ebfc2efd Perry Yuan 2024-04-25 927
3cbbe8871a2fb8 Gautham R. Shenoy 2024-04-25 928 min_freq = READ_ONCE(cpudata->min_freq);
3cbbe8871a2fb8 Gautham R. Shenoy 2024-04-25 929 max_freq = READ_ONCE(cpudata->max_freq);
3cbbe8871a2fb8 Gautham R. Shenoy 2024-04-25 930 nominal_freq = READ_ONCE(cpudata->nominal_freq);
ec437d71db77a1 Huang Rui 2021-12-24 931
069a2bb8c48c43 Perry Yuan 2024-04-25 932 policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
069a2bb8c48c43 Perry Yuan 2024-04-25 933 policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
ec437d71db77a1 Huang Rui 2021-12-24 934
ec437d71db77a1 Huang Rui 2021-12-24 935 policy->min = min_freq;
ec437d71db77a1 Huang Rui 2021-12-24 936 policy->max = max_freq;
ec437d71db77a1 Huang Rui 2021-12-24 937
ec437d71db77a1 Huang Rui 2021-12-24 938 policy->cpuinfo.min_freq = min_freq;
ec437d71db77a1 Huang Rui 2021-12-24 939 policy->cpuinfo.max_freq = max_freq;
ec437d71db77a1 Huang Rui 2021-12-24 940
ec437d71db77a1 Huang Rui 2021-12-24 941 /* It will be updated by governor */
ec437d71db77a1 Huang Rui 2021-12-24 942 policy->cur = policy->cpuinfo.min_freq;
ec437d71db77a1 Huang Rui 2021-12-24 943
e059c184da47e9 Huang Rui 2021-12-24 944 if (boot_cpu_has(X86_FEATURE_CPPC))
1d215f0319c206 Huang Rui 2021-12-24 945 policy->fast_switch_possible = true;
1d215f0319c206 Huang Rui 2021-12-24 946
41271016dfa4a0 Huang Rui 2021-12-24 947 ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
41271016dfa4a0 Huang Rui 2021-12-24 948 FREQ_QOS_MIN, policy->cpuinfo.min_freq);
41271016dfa4a0 Huang Rui 2021-12-24 949 if (ret < 0) {
41271016dfa4a0 Huang Rui 2021-12-24 950 dev_err(dev, "Failed to add min-freq constraint (%d)\n", ret);
41271016dfa4a0 Huang Rui 2021-12-24 951 goto free_cpudata1;
41271016dfa4a0 Huang Rui 2021-12-24 952 }
41271016dfa4a0 Huang Rui 2021-12-24 953
41271016dfa4a0 Huang Rui 2021-12-24 954 ret = freq_qos_add_request(&policy->constraints, &cpudata->req[1],
41271016dfa4a0 Huang Rui 2021-12-24 955 FREQ_QOS_MAX, policy->cpuinfo.max_freq);
41271016dfa4a0 Huang Rui 2021-12-24 956 if (ret < 0) {
41271016dfa4a0 Huang Rui 2021-12-24 957 dev_err(dev, "Failed to add max-freq constraint (%d)\n", ret);
41271016dfa4a0 Huang Rui 2021-12-24 958 goto free_cpudata2;
41271016dfa4a0 Huang Rui 2021-12-24 959 }
41271016dfa4a0 Huang Rui 2021-12-24 960
febab20caebac9 Wyes Karny 2023-11-17 961 cpudata->max_limit_freq = max_freq;
febab20caebac9 Wyes Karny 2023-11-17 962 cpudata->min_limit_freq = min_freq;
ec437d71db77a1 Huang Rui 2021-12-24 963
ec437d71db77a1 Huang Rui 2021-12-24 964 policy->driver_data = cpudata;
ec437d71db77a1 Huang Rui 2021-12-24 965
41271016dfa4a0 Huang Rui 2021-12-24 966 amd_pstate_boost_init(cpudata);
abd61c08ef349a Perry Yuan 2023-01-31 967 if (!current_pstate_driver->adjust_perf)
abd61c08ef349a Perry Yuan 2023-01-31 968 current_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
41271016dfa4a0 Huang Rui 2021-12-24 969
ec437d71db77a1 Huang Rui 2021-12-24 970 return 0;
ec437d71db77a1 Huang Rui 2021-12-24 971
41271016dfa4a0 Huang Rui 2021-12-24 972 free_cpudata2:
41271016dfa4a0 Huang Rui 2021-12-24 973 freq_qos_remove_request(&cpudata->req[0]);
41271016dfa4a0 Huang Rui 2021-12-24 974 free_cpudata1:
ec437d71db77a1 Huang Rui 2021-12-24 975 kfree(cpudata);
ec437d71db77a1 Huang Rui 2021-12-24 976 return ret;
ec437d71db77a1 Huang Rui 2021-12-24 977 }
ec437d71db77a1 Huang Rui 2021-12-24 978

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2024-05-08 15:21:04

[AMD Official Use Only - General]

Hi Mario

> -----Original Message-----
> From: Limonciello, Mario <[email protected]>
> Sent: Tuesday, May 7, 2024 11:19 PM
> To: Yuan, Perry <[email protected]>; [email protected];
> [email protected]; Huang, Ray <[email protected]>; Shenoy,
> Gautham Ranjal <[email protected]>; Petkov, Borislav
> <[email protected]>
> Cc: Deucher, Alexander <[email protected]>; Huang, Shimmer
> <[email protected]>; Du, Xiaojian <[email protected]>; Meng,
> Li (Jassmine) <[email protected]>; [email protected]; linux-
> [email protected]
> 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 <[email protected]>

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 */