RE: [PATCH v2 05/21] cpufreq: amd: introduce a new amd pstate driver to support future processors

From: "Huang, Ray" <Ray.Huang@amd.com>
To: "Fontenot, Nathan" <Nathan.Fontenot@amd.com>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Shuah Khan <skhan@linuxfoundation.org>,
	Borislav Petkov <bp@suse.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>
Cc: "Sharma, Deepak" <Deepak.Sharma@amd.com>,
	"Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"Limonciello, Mario" <Mario.Limonciello@amd.com>,
	"Su, Jinzhou (Joe)" <Jinzhou.Su@amd.com>,
	"Du, Xiaojian" <Xiaojian.Du@amd.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>
Subject: RE: [PATCH v2 05/21] cpufreq: amd: introduce a new amd pstate driver to support future processors
Date: Thu, 14 Oct 2021 10:14:56 +0000	[thread overview]
Message-ID: <CY4PR1201MB0246C879817ACF81CA4326A5ECB89@CY4PR1201MB0246.namprd12.prod.outlook.com> (raw)
In-Reply-To: <5b467a49-c65e-50ee-5e78-755578250647@amd.com>

[AMD Official Use Only]

> -----Original Message-----
> From: Fontenot, Nathan <Nathan.Fontenot@amd.com>
> Sent: Wednesday, September 29, 2021 4:41 AM
> To: Huang, Ray <Ray.Huang@amd.com>; Rafael J . Wysocki
> <rafael.j.wysocki@intel.com>; Viresh Kumar <viresh.kumar@linaro.org>;
> Shuah Khan <skhan@linuxfoundation.org>; Borislav Petkov <bp@suse.de>;
> Peter Zijlstra <peterz@infradead.org>; Ingo Molnar <mingo@kernel.org>;
> linux-pm@vger.kernel.org
> Cc: Sharma, Deepak <Deepak.Sharma@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Su, Jinzhou (Joe) <Jinzhou.Su@amd.com>;
> Du, Xiaojian <Xiaojian.Du@amd.com>; linux-kernel@vger.kernel.org;
> x86@kernel.org
> Subject: Re: [PATCH v2 05/21] cpufreq: amd: introduce a new amd pstate
> driver to support future processors
> 
> On 9/26/2021 4:05 AM, Huang Rui wrote:
> > amd-pstate is the AMD CPU performance scaling driver that introduces a
> > new CPU frequency control mechanism on AMD Zen based CPU series in
> Linux
> > kernel. The new mechanism is based on Collaborative processor
> > performance control (CPPC) which is finer grain frequency management
> > than legacy ACPI hardware P-States. Current AMD CPU platforms are using
> > the ACPI P-states driver to manage CPU frequency and clocks with
> > switching only in 3 P-states. AMD P-States is to replace the ACPI
> > P-states controls, allows a flexible, low-latency interface for the
> > Linux kernel to directly communicate the performance hints to hardware.
> >
> > "amd-pstate" leverages the Linux kernel governors such as *schedutil*,
> > *ondemand*, etc. to manage the performance hints which are provided by
> CPPC
> > hardware functionality. The first version for amd-pstate is to support one
> > of the Zen3 processors, and we will support more in future after we verify
> > the hardware and SBIOS functionalities.
> >
> > There are two types of hardware implementations for amd-pstate: one is
> full
> > MSR support and another is shared memory support. It can use
> > X86_FEATURE_AMD_CPPC_EXT feature flag to distinguish the different
> types.
> >
> > Using the new AMD P-States method + kernel governors (*schedutil*,
> > *ondemand*, ...) to manage the frequency update is the most appropriate
> > bridge between AMD Zen based hardware processor and Linux kernel, the
> > processor is able to ajust to the most efficiency frequency according to
> > the kernel scheduler loading.
> >
> > Performance Per Watt (PPW) Caculation:
> >
> > The PPW caculation is referred by below paper:
> >
> https://software.intel.com/content/dam/develop/external/us/en/document
> s/performance-per-what-paper.pdf
> >
> > Below formula is referred from below spec to measure the PPW:
> >
> > (F / t) / P = F * t / (t * E) = F / E,
> >
> > "F" is the number of frames per second.
> > "P" is power measurd in watts.
> > "E" is energy measured in joules.
> >
> > We use the RAPL interface with "perf" tool to get the energy data of the
> > package power.
> >
> > The data comparsions between amd-pstate and acpi-freq module are
> tested on
> > AMD Cezanne processor:
> >
> > 1) TBench CPU benchmark:
> >
> > +---------------------------------------------------------------------+
> > |                                                                     |
> > |               TBench (Performance Per Watt)                         |
> > |                                                    Higher is better |
> > +-------------------+------------------------+------------------------+
> > |                   |  Performance Per Watt  |  Performance Per Watt  |
> > |   Kernel Module   |       (Schedutil)      |       (Ondemand)       |
> > |                   |  Unit: MB / (s * J)    |  Unit: MB / (s * J)    |
> > +-------------------+------------------------+------------------------+
> > |                   |                        |                        |
> > |    acpi-cpufreq   |         3.022          |        2.969           |
> > |                   |                        |                        |
> > +-------------------+------------------------+------------------------+
> > |                   |                        |                        |
> > |     amd-pstate    |         3.131          |        3.284           |
> > |                   |                        |                        |
> > +-------------------+------------------------+------------------------+
> >
> > 2) Gitsource CPU benchmark:
> >
> > +---------------------------------------------------------------------+
> > |                                                                     |
> > |               Gitsource (Performance Per Watt)                      |
> > |                                                    Higher is better |
> > +-------------------+------------------------+------------------------+
> > |                   |  Performance Per Watt  |  Performance Per Watt  |
> > |   Kernel Module   |       (Schedutil)      |       (Ondemand)       |
> > |                   |  Unit: 1 / (s * J)     |  Unit: 1 / (s * J)     |
> > +-------------------+------------------------+------------------------+
> > |                   |                        |                        |
> > |    acpi-cpufreq   |     3.42172E-07        |     2.74508E-07        |
> > |                   |                        |                        |
> > +-------------------+------------------------+------------------------+
> > |                   |                        |                        |
> > |     amd-pstate    |     4.09141E-07        |     3.47610E-07        |
> > |                   |                        |                        |
> > +-------------------+------------------------+------------------------+
> >
> > 3) Speedometer 2.0 CPU benchmark:
> >
> > +---------------------------------------------------------------------+
> > |                                                                     |
> > |               Speedometer 2.0 (Performance Per Watt)                |
> > |                                                    Higher is better |
> > +-------------------+------------------------+------------------------+
> > |                   |  Performance Per Watt  |  Performance Per Watt  |
> > |   Kernel Module   |       (Schedutil)      |       (Ondemand)       |
> > |                   |  Unit: 1 / (s * J)     |  Unit: 1 / (s * J)     |
> > +-------------------+------------------------+------------------------+
> > |                   |                        |                        |
> > |    acpi-cpufreq   |      0.116111767       |      0.110321664       |
> > |                   |                        |                        |
> > +-------------------+------------------------+------------------------+
> > |                   |                        |                        |
> > |     amd-pstate    |      0.115825281       |      0.122024299       |
> > |                   |                        |                        |
> > +-------------------+------------------------+------------------------+
> >
> > According to above average data, we can see this solution has shown
> better
> > performance per watt scaling on mobile CPU benchmarks in most of cases.
> >
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >  drivers/cpufreq/Kconfig.x86  |  13 +
> >  drivers/cpufreq/Makefile     |   1 +
> >  drivers/cpufreq/amd-pstate.c | 446
> +++++++++++++++++++++++++++++++++++
> >  3 files changed, 460 insertions(+)
> >  create mode 100644 drivers/cpufreq/amd-pstate.c
> >
> > diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
> > index 92701a18bdd9..9cd7e338bdcd 100644
> > --- a/drivers/cpufreq/Kconfig.x86
> > +++ b/drivers/cpufreq/Kconfig.x86
> > @@ -34,6 +34,19 @@ config X86_PCC_CPUFREQ
> >
> >  	  If in doubt, say N.
> >
> > +config X86_AMD_PSTATE
> > +	tristate "AMD Processor P-State driver"
> > +	depends on X86
> > +	select ACPI_PROCESSOR if ACPI
> > +	select ACPI_CPPC_LIB if X86_64 && ACPI && SCHED_MC_PRIO
> > +	select CPU_FREQ_GOV_SCHEDUTIL if SMP
> > +	help
> > +	  This driver adds a CPUFreq driver which utilizes a fine grain
> > +	  processor performance freqency control range instead of legacy
> > +	  performance levels. This driver also supports newer AMD CPUs.
> 
> Go ahead and call out that this is a CPPC driver in the help message, that
> is what the driver is.
> 
> The reference to "also supports newer AMD CPUs" seems vague, can you
> elaborate?
> 

Actually, the detail introduction is in the RST documentation, but I can describe more information here at V3.

> > +
> > +	  If in doubt, say N.
> > +
> >  config X86_ACPI_CPUFREQ
> >  	tristate "ACPI Processor P-States driver"
> >  	depends on ACPI_PROCESSOR
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index 27d3bd7ea9d4..5c9a2a1ee8dc 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -25,6 +25,7 @@ obj-$(CONFIG_CPUFREQ_DT_PLATDEV)	+= cpufreq-
> dt-platdev.o
> >  # speedstep-* is preferred over p4-clockmod.
> >
> >  obj-$(CONFIG_X86_ACPI_CPUFREQ)		+= acpi-cpufreq.o
> > +obj-$(CONFIG_X86_AMD_PSTATE)		+= amd-pstate.o
> >  obj-$(CONFIG_X86_POWERNOW_K8)		+= powernow-k8.o
> >  obj-$(CONFIG_X86_PCC_CPUFREQ)		+= pcc-cpufreq.o
> >  obj-$(CONFIG_X86_POWERNOW_K6)		+= powernow-k6.o
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > new file mode 100644
> > index 000000000000..693d796eae55
> > --- /dev/null
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -0,0 +1,446 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * amd-pstate.c - AMD Processor P-state Frequency Driver
> > + *
> > + * Copyright (C) 2021 Advanced Micro Devices, Inc. All Rights Reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> along with
> > + * this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-
> 1301, USA.
> > + *
> 
> You've included the SPDX identifier, you shouldn't need to include the license
> information text also.
> 

Thanks to point it out. I will remove the first line to use the AMD header.

> > + * Author: Huang Rui <ray.huang@amd.com>
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/smp.h>
> > +#include <linux/sched.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/compiler.h>
> > +#include <linux/dmi.h>
> > +#include <linux/slab.h>
> > +#include <linux/acpi.h>
> > +#include <linux/io.h>
> > +#include <linux/delay.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/static_call.h>
> > +
> > +#include <acpi/processor.h>
> > +#include <acpi/cppc_acpi.h>
> > +
> > +#include <asm/msr.h>
> > +#include <asm/processor.h>
> > +#include <asm/cpufeature.h>
> > +#include <asm/cpu_device_id.h>
> > +
> > +#define AMD_PSTATE_TRANSITION_LATENCY	0x20000
> > +#define AMD_PSTATE_TRANSITION_DELAY	500
> > +
> > +static struct cpufreq_driver amd_pstate_driver;
> > +
> > +struct amd_cpudata {
> > +	int	cpu;
> > +
> > +	struct freq_qos_request req[2];
> > +	struct cpufreq_policy *policy;
> 
> You include a pointer back to the policy, it's is set in amd_pstate_cpu_init()
> but isn't used anywhere. This could be dropped from the struct.
> 

Dropped.

> > +
> > +	u64	cppc_req_cached;
> > +
> > +	u32	highest_perf;
> > +	u32	nominal_perf;
> > +	u32	lowest_nonlinear_perf;
> > +	u32	lowest_perf;
> 
> The lowest_perf value is saved but never referenced, should this be dropped?
> 
> It looks like it is used in a later patch to report the lowest_perf value in
> sysfs. Do we need to cache it for that? Could just read the value when
> requested.
> 
> > +
> > +	u32	max_freq;
> > +	u32	min_freq;
> > +	u32	nominal_freq;
> 
> You're saving the nominal freq value here but I don't see that it is used
> anywhere. It looks like you grab the current nominal freq value via
> cppc_get_perf_caps() instead. This could be dropped from the struct.
> 
> > +	u32	lowest_nonlinear_freq;
> 
> The same goes for lowest_nonlinear_freq.

The performance and frequency level values won't be changed after the system boots.
We stored them in the data structure, that is to avoid kernel querying or accessing MSR or other ACPI IO operations every time.   

> 
> > +};
> > +
> > +static inline int pstate_enable(bool enable)
> > +{
> > +	return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable ? 1 : 0);
> > +}
> > +
> > +DEFINE_STATIC_CALL(amd_pstate_enable, pstate_enable);
> > +
> > +static inline int amd_pstate_enable(bool enable)
> > +{
> > +	return static_call(amd_pstate_enable)(enable);
> > +}
> > +
> > +static int pstate_init_perf(struct amd_cpudata *cpudata)
> > +{
> > +	u64 cap1;
> > +
> > +	int ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
> > +				     &cap1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * TODO: Introduce AMD specific power feature.
> > +	 *
> > +	 * CPPC entry doesn't indicate the highest performance in some
> ASICs.
> > +	 */
> > +	WRITE_ONCE(cpudata->highest_perf, amd_get_highest_perf());
> > +
> > +	WRITE_ONCE(cpudata->nominal_perf, CAP1_NOMINAL_PERF(cap1));
> > +	WRITE_ONCE(cpudata->lowest_nonlinear_perf,
> CAP1_LOWNONLIN_PERF(cap1));
> > +	WRITE_ONCE(cpudata->lowest_perf, CAP1_LOWEST_PERF(cap1));
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_STATIC_CALL(amd_pstate_init_perf, pstate_init_perf);
> > +
> > +static inline int amd_pstate_init_perf(struct amd_cpudata *cpudata)
> > +{
> > +	return static_call(amd_pstate_init_perf)(cpudata);
> > +}
> > +
> > +static void pstate_update_perf(struct amd_cpudata *cpudata, u32
> min_perf,
> > +			       u32 des_perf, u32 max_perf,
> > +			       bool fast_switch)
> > +{
> > +	if (fast_switch)
> > +		wrmsrl(MSR_AMD_CPPC_REQ, READ_ONCE(cpudata-
> >cppc_req_cached));
> > +	else
> > +		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
> > +			      READ_ONCE(cpudata->cppc_req_cached));
> > +}
> > +
> > +DEFINE_STATIC_CALL(amd_pstate_update_perf, pstate_update_perf);
> > +
> > +static inline void
> > +amd_pstate_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
> 
> Please be consistant on function definition, this should all be on
> one line (here and elsewhere);
> 

No problem.

> > +		       u32 des_perf, u32 max_perf, bool fast_switch)
> > +{
> > +	static_call(amd_pstate_update_perf)(cpudata, min_perf, des_perf,
> > +					    max_perf, fast_switch);
> > +}
> > +
> > +static void
> > +amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
> > +		  u32 des_perf, u32 max_perf, bool fast_switch)
> > +{
> > +	u64 prev = READ_ONCE(cpudata->cppc_req_cached);
> > +	u64 value = prev;
> > +
> > +	value &= ~REQ_MIN_PERF(~0L);
> > +	value |= REQ_MIN_PERF(min_perf);
> > +
> > +	value &= ~REQ_DES_PERF(~0L);
> > +	value |= REQ_DES_PERF(des_perf);
> > +
> > +	value &= ~REQ_MAX_PERF(~0L);
> > +	value |= REQ_MAX_PERF(max_perf);
> > +
> > +	if (value == prev)
> > +		return;
> > +
> > +	WRITE_ONCE(cpudata->cppc_req_cached, value);
> > +
> > +	amd_pstate_update_perf(cpudata, min_perf, des_perf,
> > +			       max_perf, fast_switch);
> > +}
> > +
> > +static int amd_pstate_verify(struct cpufreq_policy_data *policy)
> > +{
> > +	cpufreq_verify_within_cpu_limits(policy);
> > +
> > +	return 0;
> > +}
> > +
> > +static int amd_pstate_target(struct cpufreq_policy *policy,
> > +			     unsigned int target_freq,
> > +			     unsigned int relation)
> > +{
> > +	struct cpufreq_freqs freqs;
> > +	struct amd_cpudata *cpudata = policy->driver_data;
> > +	unsigned long amd_max_perf, amd_min_perf, amd_des_perf,
> > +		      amd_cap_perf;
> > +
> > +	if (!cpudata->max_freq)
> > +		return -ENODEV;
> > +
> > +	amd_cap_perf = READ_ONCE(cpudata->highest_perf);
> > +	amd_min_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> 
> can you help me understand why you use the cached value for lowest
> nonlinear perf here but use the value returned from cppc_get_perf_caps()
> in amd_get_lowest_nonlinear_freq()?
> 
> Should we be using the value from cppc_get_perf_caps() in both cases?
> 

There are two mainly reasons:

1. In some processors which has "full MSR support", the related performance values are read back from MSR directly.
2. In some processors, the performance value which read back from cppc helper is not expected. For example, please check below bug, the highest perf is obviously not the correct one that will report the processor frequency over 7 GHz.
https://bugzilla.kernel.org/show_bug.cgi?id=211791

> > +	amd_max_perf = amd_cap_perf;
> > +
> > +	freqs.old = policy->cur;
> > +	freqs.new = target_freq;
> > +
> > +	amd_des_perf = DIV_ROUND_CLOSEST(target_freq * amd_cap_perf,
> > +					 cpudata->max_freq);
> > +
> > +	cpufreq_freq_transition_begin(policy, &freqs);
> > +	amd_pstate_update(cpudata, amd_min_perf, amd_des_perf,
> > +			  amd_max_perf, false);
> > +	cpufreq_freq_transition_end(policy, &freqs, false);
> > +
> > +	return 0;
> > +}
> > +
> > +static int amd_get_min_freq(struct amd_cpudata *cpudata)
> > +{
> > +	struct cppc_perf_caps cppc_perf;
> > +
> > +	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Switch to khz */
> > +	return cppc_perf.lowest_freq * 1000;
> > +}
> > +
> > +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 = cppc_perf.nominal_freq;
> > +	nominal_perf = READ_ONCE(cpudata->nominal_perf);
> > +	max_perf = READ_ONCE(cpudata->highest_perf);
> > +
> > +	boost_ratio = div_u64(max_perf << SCHED_CAPACITY_SHIFT,
> > +			      nominal_perf);
> > +
> > +	max_freq = nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT;
> > +
> > +	/* Switch to khz */
> > +	return max_freq * 1000;
> > +}
> > +
> > +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;
> > +
> > +	nominal_freq = cppc_perf.nominal_freq;
> > +
> > +	/* Switch to khz */
> > +	return nominal_freq * 1000;
> > +}
> > +
> > +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 = cppc_perf.nominal_freq;
> > +	nominal_perf = READ_ONCE(cpudata->nominal_perf);
> > +
> > +	lowest_nonlinear_perf = cppc_perf.lowest_nonlinear_perf;
> > +
> > +	lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf <<
> > +					 SCHED_CAPACITY_SHIFT,
> nominal_perf);
> > +
> > +	lowest_nonlinear_freq = nominal_freq * lowest_nonlinear_ratio >>
> SCHED_CAPACITY_SHIFT;
> > +
> > +	/* Switch to khz */
> > +	return lowest_nonlinear_freq * 1000;
> > +}
> > +
> > +static int amd_pstate_init_freqs_in_cpudata(struct amd_cpudata
> *cpudata,
> > +					    u32 max_freq, u32 min_freq,
> > +					    u32 nominal_freq,
> > +					    u32 lowest_nonlinear_freq)
> 
> This is only called from one place (below in amd_pstate_cpu_init()),
> this could just be inline below
> 

Will updated in V3.

Thanks,
Ray