mbox series

[v7,00/13] Implement AMD Pstate EPP Driver

Message ID 20221208111852.386731-1-perry.yuan@amd.com
Headers show
Series Implement AMD Pstate EPP Driver | expand

Message

Yuan, Perry Dec. 8, 2022, 11:18 a.m. UTC
Hi all,

This patchset implements one new AMD CPU frequency driver
`amd-pstate-epp` instance for better performance and power control.
CPPC has a parameter called energy preference performance (EPP).
The EPP is used in the CCLK DPM controller to drive the frequency that a core
is going to operate during short periods of activity.
EPP values will be utilized for different OS profiles (balanced, performance, power savings).

AMD Energy Performance Preference (EPP) provides a hint to the hardware
if software wants to bias toward performance (0x0) or energy efficiency (0xff)
The lowlevel power firmware will calculate the runtime frequency according to the EPP preference 
value. So the EPP hint will impact the CPU cores frequency responsiveness.

We use the RAPL interface with "perf" tool to get the energy data of the package power.
Performance Per Watt (PPW) Calculation:

The PPW calculation is referred by below paper:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsoftware.intel.com%2Fcontent%2Fdam%2Fdevelop%2Fexternal%2Fus%2Fen%2Fdocuments%2Fperformance-per-what-paper.pdf&data=04%7C01%7CPerry.Yuan%40amd.com%7Cac66e8ce98044e9b062708d9ab47c8d8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637729147708574423%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=TPOvCE%2Frbb0ptBreWNxHqOi9YnVhcHGKG88vviDLb00%3D&reserved=0

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 measured in watts.
"E" is energy measured in joules.

Gitsouce Benchmark Data on ROME Server CPU
+------------------------------+------------------------------+------------+------------------+
| Kernel Module                | PPW (1 / s * J)              |Energy(J) | PPW Improvement (%)|
+==============================+==============================+============+==================+
| acpi-cpufreq:schedutil       | 5.85658E-05                  | 17074.8    | base             |
+------------------------------+------------------------------+------------+------------------+
| acpi-cpufreq:ondemand        | 5.03079E-05                  | 19877.6    | -14.10%          |
+------------------------------+------------------------------+------------+------------------+
| acpi-cpufreq:performance     | 5.88132E-05                  | 17003      | 0.42%            |
+------------------------------+------------------------------+------------+------------------+
| amd-pstate:ondemand          | 4.60295E-05                  | 21725.2    | -21.41%          |
+------------------------------+------------------------------+------------+------------------+
| amd-pstate:schedutil         | 4.70026E-05                  | 21275.4    | -19.7%           |
+------------------------------+------------------------------+------------+------------------+
| amd-pstate:performance       | 5.80094E-05                  | 17238.6    | -0.95%           |
+------------------------------+------------------------------+------------+------------------+
| EPP:performance              | 5.8292E-05                   | 17155      | -0.47%           |
+------------------------------+------------------------------+------------+------------------+
| EPP: balance performance:    | 6.71709E-05                  | 14887.4    | 14.69%           |
+------------------------------+------------------------------+------------+------------------+
| EPP:power                    | 6.66951E-05                  | 4993.6     | 13.88%           |
+------------------------------+------------------------------+------------+------------------+

Tbench Benchmark Data on ROME Server CPU
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| Kernel Module                               | PPW MB / (s * J)  |Throughput(MB/s)| Energy (J)|PPW Improvement(%)|
+=============================================+===================+==============+=============+==================+
| acpi_cpufreq: schedutil                     | 46.39             | 17191        | 37057.3     | base             |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| acpi_cpufreq: ondemand                      | 51.51             | 19269.5      | 37406.5     | 11.04 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| acpi_cpufreq: performance                   | 45.96             | 17063.7      | 37123.7     | -0.74 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| EPP:powersave: performance(0)               | 54.46             | 20263.1      | 37205       | 17.87 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| EPP:powersave: balance performance          | 55.03             | 20481.9      | 37221.5     | 19.14 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| EPP:powersave: balance_power                | 54.43             | 20245.9      | 37194.2     | 17.77 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| EPP:powersave: power(255)                   | 54.26             | 20181.7      | 37197.4     | 17.40 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| amd-pstate: schedutil                       | 48.22             | 17844.9      | 37006.6     | 3.80 %           |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| amd-pstate: ondemand                        | 61.30             | 22988        | 37503.4     | 33.72 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+
| amd-pstate: performance                     | 54.52             | 20252.6      | 37147.8     | 17.81 %          |
+---------------------------------------------+-------------------+--------------+-------------+------------------+

changes from v6:
 * fix one legacy kernel hang issue when amd-pstate driver unregistering
 * add new documentation to introduce new global sysfs attributes
 * use sysfs_emit_at() to print epp profiles array
 * update commit info for patch v6 patch 1/11 as Mario sugguested.
 * trying to add the EPP profiles into cpufreq.h, but it will cause lots
   of build failues,continue to keep cpufreq_common.h used in v7
 * update commit info using amd-pstate as prefix same as before.
 * remove CONFIG_ACPI for the header as Ray suggested.
 * move amd_pstate_kobj to where it is used in patch "add frequency dynamic boost sysfs control"
 * drive feedback removing X86_FEATURE_CPPC check for the epp init from Huang Ray 
 * drive feedback from Mario
 
change from v5:
 * add one common header `cpufreq_commoncpufreq_common` to extract EPP profiles 
   definition for amd and intel pstate driver.
 * remove the epp_off value to avoid confusion.
 * convert some other sysfs sprintf() function with sysfs_emit() and add onew new patch
 * add acpi pm server priofile detection to enable dynamic boost control
 * fix some code format with checkpatch script
 * move the EPP profile declaration into common header file `cpufreq_common.h`
 * fix commit typos

changes from v4:
 * rebase driver based on the v6.1-rc7
 * remove the builtin changes patch because pstate driver has been
   changed to builtin type by another thread patch
 * update Documentation: amd-pstate: add amd pstate driver mode introduction 
 * replace sprintf with sysfs_emit() instead.
 * fix typo for cppc_set_epp_perf() in cppc_acpi.h header

changes from v3:
 * add one more document update patch for the active and passive mode
   introducion.
 * drive most of the feedbacks from Mario
 * drive feedback from Rafael for the cppc_acpi driver.
 * remove the epp raw data set/get function
 * set the amd-pstate drive by passing kernel parameter
 * set amd-pstate driver disabled by default if no kernel parameter
   input from booting
 * get cppc_set_auto_epp and cppc_set_epp_perf combined
 * pick up reviewed by flag from Mario

changes from v2:
 * change pstate driver as builtin type from module
 * drop patch "export cpufreq cpu release and acquire"
 * squash patch of shared mem into single patch of epp implementation
 * add one new patch to support frequency boost control
 * add patch to expose driver working status checking
 * rebase driver into v6.1-rc4 kernel release
 * move some declaration to amd-pstate.h
 * drive feedback from Mario for the online/offline patch
 * drive feedback from Mario for the suspend/resume patch
 * drive feedback from Ray for the cppc_acpi and some other patches
 * drive feedback from Nathan for the epp patch

changes from v1:
 * rebased to v6.0
 * drive feedbacks from Mario for the suspend/resume patch
 * drive feedbacks from Nathan for the EPP support on msr type
 * fix some typos and code style indent problems
 * update commit comments for patch 4/7
 * change the `epp_enabled` module param name to `epp`
 * set the default epp mode to be false
 * add testing for the x86_energy_perf_policy utility patchset(will
   send that utility patchset with another thread)

v6: https://lore.kernel.org/lkml/20221202074719.623673-1-perry.yuan@amd.com/
v5: https://lore.kernel.org/lkml/20221128170314.2276636-1-perry.yuan@amd.com/
v4: https://lore.kernel.org/lkml/20221110175847.3098728-1-Perry.Yuan@amd.com/
v3: https://lore.kernel.org/all/20221107175705.2207842-1-Perry.Yuan@amd.com/
v2: https://lore.kernel.org/all/20221010162248.348141-1-Perry.Yuan@amd.com/
v1: https://lore.kernel.org/all/20221009071033.21170-1-Perry.Yuan@amd.com/

Perry Yuan (13):
  ACPI: CPPC: Add AMD pstate energy performance preference cppc control
  Documentation: amd-pstate: add EPP profiles introduction
  cpufreq: intel_pstate: use common macro definition for Energy
    Preference Performance(EPP)
  cpufreq: amd-pstate: fix kernel hang issue while amd-pstate
    unregistering
  cpufreq: amd-pstate: implement Pstate EPP support for the AMD
    processors
  cpufreq: amd-pstate: implement amd pstate cpu online and offline
    callback
  cpufreq: amd-pstate: implement suspend and resume callbacks
  cpufreq: amd-pstate: add frequency dynamic boost sysfs control
  cpufreq: amd-pstate: add driver working mode status sysfs entry
  Documentation: amd-pstate: add amd pstate driver mode introduction
  Documentation: introduce amd pstate active mode kernel command line
    options
  cpufreq: amd-pstate: convert sprintf with sysfs_emit()
  Documentation: amd-pstate: introduce new global sysfs attributes

 .../admin-guide/kernel-parameters.txt         |   7 +
 Documentation/admin-guide/pm/amd-pstate.rst   |  83 +-
 arch/x86/include/asm/msr-index.h              |   4 -
 drivers/acpi/cppc_acpi.c                      | 114 ++-
 drivers/cpufreq/amd-pstate.c                  | 933 +++++++++++++++++-
 drivers/cpufreq/intel_pstate.c                |  37 +-
 include/acpi/cppc_acpi.h                      |  12 +
 include/linux/amd-pstate.h                    |  36 +
 include/linux/cpufreq_common.h                |  53 +
 9 files changed, 1222 insertions(+), 57 deletions(-)
 create mode 100644 include/linux/cpufreq_common.h

Comments

Huang Rui Dec. 9, 2022, 7:55 a.m. UTC | #1
On Thu, Dec 08, 2022 at 07:18:40PM +0800, Yuan, Perry wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
> 
> Add support for setting and querying EPP preferences to the generic
> CPPC driver.  This enables downstream drivers such as amd-pstate to discover
> and use these values
> 
> Downstream drivers that want to use the new symbols cppc_get_epp_caps
> and cppc_set_epp_perf for querying and setting EPP preferences will need
> to call cppc_set_auto_epp to enable the EPP function first.
> 
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>

Acked-by: Huang Rui <ray.huang@amd.com>

> ---
>  drivers/acpi/cppc_acpi.c | 114 +++++++++++++++++++++++++++++++++++++--
>  include/acpi/cppc_acpi.h |  12 +++++
>  2 files changed, 121 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 093675b1a1ff..37fa75f25f62 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1093,6 +1093,9 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>  {
>  	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>  	struct cpc_register_resource *reg;
> +	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> +	struct cppc_pcc_data *pcc_ss_data = NULL;
> +	int ret = -EINVAL;
>  
>  	if (!cpc_desc) {
>  		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> @@ -1102,10 +1105,6 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>  	reg = &cpc_desc->cpc_regs[reg_idx];
>  
>  	if (CPC_IN_PCC(reg)) {
> -		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> -		struct cppc_pcc_data *pcc_ss_data = NULL;
> -		int ret = 0;
> -
>  		if (pcc_ss_id < 0)
>  			return -EIO;
>  
> @@ -1125,7 +1124,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>  
>  	cpc_read(cpunum, reg, perf);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -1365,6 +1364,111 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
>  }
>  EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
>  
> +/**
> + * cppc_get_epp_caps - Get the energy preference register value.
> + * @cpunum: CPU from which to get epp preference level.
> + * @perf_caps: Return address.
> + *
> + * Return: 0 for success, -EIO otherwise.
> + */
> +int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> +{
> +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> +	struct cpc_register_resource *energy_perf_reg;
> +	u64 energy_perf;
> +
> +	if (!cpc_desc) {
> +		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> +		return -ENODEV;
> +	}
> +
> +	energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
> +
> +	if (!CPC_SUPPORTED(energy_perf_reg))
> +		pr_warn_once("energy perf reg update is unsupported!\n");
> +
> +	if (CPC_IN_PCC(energy_perf_reg)) {
> +		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> +		struct cppc_pcc_data *pcc_ss_data = NULL;
> +		int ret = 0;
> +
> +		if (pcc_ss_id < 0)
> +			return -ENODEV;
> +
> +		pcc_ss_data = pcc_data[pcc_ss_id];
> +
> +		down_write(&pcc_ss_data->pcc_lock);
> +
> +		if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0) {
> +			cpc_read(cpunum, energy_perf_reg, &energy_perf);
> +			perf_caps->energy_perf = energy_perf;
> +		} else {
> +			ret = -EIO;
> +		}
> +
> +		up_write(&pcc_ss_data->pcc_lock);
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cppc_get_epp_caps);
> +
> +/*
> + * Set Energy Performance Preference Register value through
> + * Performance Controls Interface
> + */
> +int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
> +{
> +	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> +	struct cpc_register_resource *epp_set_reg;
> +	struct cpc_register_resource *auto_sel_reg;
> +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> +	struct cppc_pcc_data *pcc_ss_data = NULL;
> +	int ret = -EINVAL;
> +
> +	if (!cpc_desc) {
> +		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> +		return -ENODEV;
> +	}
> +
> +	auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
> +	epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
> +
> +	if (CPC_IN_PCC(epp_set_reg) || CPC_IN_PCC(auto_sel_reg)) {
> +		if (pcc_ss_id < 0) {
> +			pr_debug("Invalid pcc_ss_id\n");
> +			return -ENODEV;
> +		}
> +
> +		if (CPC_SUPPORTED(auto_sel_reg)) {
> +			ret = cpc_write(cpu, auto_sel_reg, enable);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		if (CPC_SUPPORTED(epp_set_reg)) {
> +			ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		pcc_ss_data = pcc_data[pcc_ss_id];
> +
> +		down_write(&pcc_ss_data->pcc_lock);
> +		/* after writing CPC, transfer the ownership of PCC to platform */
> +		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> +		up_write(&pcc_ss_data->pcc_lock);
> +	} else {
> +		ret = -ENOTSUPP;
> +		pr_debug("_CPC in PCC is not supported\n");
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
> +
>  /**
>   * cppc_set_enable - Set to enable CPPC on the processor by writing the
>   * Continuous Performance Control package EnableRegister field.
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index c5614444031f..a45bb876a19c 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -108,12 +108,14 @@ struct cppc_perf_caps {
>  	u32 lowest_nonlinear_perf;
>  	u32 lowest_freq;
>  	u32 nominal_freq;
> +	u32 energy_perf;
>  };
>  
>  struct cppc_perf_ctrls {
>  	u32 max_perf;
>  	u32 min_perf;
>  	u32 desired_perf;
> +	u32 energy_perf;
>  };
>  
>  struct cppc_perf_fb_ctrs {
> @@ -149,6 +151,8 @@ extern bool cpc_ffh_supported(void);
>  extern bool cpc_supported_by_cpu(void);
>  extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
>  extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
> +extern int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps);
> +extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
>  #else /* !CONFIG_ACPI_CPPC_LIB */
>  static inline int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
>  {
> @@ -202,6 +206,14 @@ static inline int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
>  {
>  	return -ENOTSUPP;
>  }
> +static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
> +{
> +	return -ENOTSUPP;
> +}
> +static inline int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> +{
> +	return -ENOTSUPP;
> +}
>  #endif /* !CONFIG_ACPI_CPPC_LIB */
>  
>  #endif /* _CPPC_ACPI_H*/
> -- 
> 2.34.1
>
Yuan, Perry Dec. 12, 2022, 2:59 a.m. UTC | #2
[AMD Official Use Only - General]

Hi Rafael. 

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Thursday, December 8, 2022 7:36 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: rafael.j.wysocki@intel.com; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Huang, Ray <Ray.Huang@amd.com>;
> viresh.kumar@linaro.org; Sharma, Deepak <Deepak.Sharma@amd.com>;
> Fontenot, Nathan <Nathan.Fontenot@amd.com>; 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>; Karny, Wyes <Wyes.Karny@amd.com>;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7 00/13] Implement AMD Pstate EPP Driver
> 
> Hi,
> 
> On Thu, Dec 8, 2022 at 12:19 PM Perry Yuan <perry.yuan@amd.com> wrote:
> >
> > Hi all,
> >
> > This patchset implements one new AMD CPU frequency driver
> > `amd-pstate-epp` instance for better performance and power control.
> > CPPC has a parameter called energy preference performance (EPP).
> > The EPP is used in the CCLK DPM controller to drive the frequency that
> > a core is going to operate during short periods of activity.
> > EPP values will be utilized for different OS profiles (balanced, performance,
> power savings).
> 
> I honestly don't think that this work is ready for 6.2.
> 
> The number of patches in the series seems to change frequently and there
> are active discussions around specific patches.
> 
> Accordingly, I will not consider applying it until 6.2-rc1 is out.
> 
> Thanks!

Thanks for your feedback on this.  I add some issue fix  and some documents patches to the series which changes the patches numbers.  
I will drive the feedback and will get review and ack flags before you help to merge it. 

Perry.
Huang Rui Dec. 12, 2022, 3:29 a.m. UTC | #3
On Fri, Dec 09, 2022 at 03:55:28PM +0800, Huang Rui wrote:
> On Thu, Dec 08, 2022 at 07:18:40PM +0800, Yuan, Perry wrote:
> > From: Perry Yuan <Perry.Yuan@amd.com>
> > 
> > Add support for setting and querying EPP preferences to the generic
> > CPPC driver.  This enables downstream drivers such as amd-pstate to discover
> > and use these values
> > 
> > Downstream drivers that want to use the new symbols cppc_get_epp_caps
> > and cppc_set_epp_perf for querying and setting EPP preferences will need
> > to call cppc_set_auto_epp to enable the EPP function first.
> > 
> > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> 
> Acked-by: Huang Rui <ray.huang@amd.com>
> 
> > ---
> >  drivers/acpi/cppc_acpi.c | 114 +++++++++++++++++++++++++++++++++++++--
> >  include/acpi/cppc_acpi.h |  12 +++++
> >  2 files changed, 121 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > index 093675b1a1ff..37fa75f25f62 100644
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -1093,6 +1093,9 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
> >  {
> >  	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> >  	struct cpc_register_resource *reg;
> > +	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> > +	struct cppc_pcc_data *pcc_ss_data = NULL;
> > +	int ret = -EINVAL;
> >  
> >  	if (!cpc_desc) {
> >  		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> > @@ -1102,10 +1105,6 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
> >  	reg = &cpc_desc->cpc_regs[reg_idx];
> >  
> >  	if (CPC_IN_PCC(reg)) {
> > -		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> > -		struct cppc_pcc_data *pcc_ss_data = NULL;
> > -		int ret = 0;
> > -
> >  		if (pcc_ss_id < 0)
> >  			return -EIO;
> >  
> > @@ -1125,7 +1124,7 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
> >  
> >  	cpc_read(cpunum, reg, perf);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  /**
> > @@ -1365,6 +1364,111 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> >  }
> >  EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
> >  
> > +/**
> > + * cppc_get_epp_caps - Get the energy preference register value.
> > + * @cpunum: CPU from which to get epp preference level.
> > + * @perf_caps: Return address.
> > + *
> > + * Return: 0 for success, -EIO otherwise.
> > + */
> > +int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)

Take a look at the patch again, due to the energy_perf is actually one of
the members in struct cppc_perf_caps. It's better to modify the existing
cppc_get_perf_caps() to get the epp value as well.

Thanks,
Ray

> > +{
> > +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> > +	struct cpc_register_resource *energy_perf_reg;
> > +	u64 energy_perf;
> > +
> > +	if (!cpc_desc) {
> > +		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> > +		return -ENODEV;
> > +	}
> > +
> > +	energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
> > +
> > +	if (!CPC_SUPPORTED(energy_perf_reg))
> > +		pr_warn_once("energy perf reg update is unsupported!\n");
> > +
> > +	if (CPC_IN_PCC(energy_perf_reg)) {
> > +		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> > +		struct cppc_pcc_data *pcc_ss_data = NULL;
> > +		int ret = 0;
> > +
> > +		if (pcc_ss_id < 0)
> > +			return -ENODEV;
> > +
> > +		pcc_ss_data = pcc_data[pcc_ss_id];
> > +
> > +		down_write(&pcc_ss_data->pcc_lock);
> > +
> > +		if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0) {
> > +			cpc_read(cpunum, energy_perf_reg, &energy_perf);
> > +			perf_caps->energy_perf = energy_perf;
> > +		} else {
> > +			ret = -EIO;
> > +		}
> > +
> > +		up_write(&pcc_ss_data->pcc_lock);
> > +
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(cppc_get_epp_caps);
> > +
> > +/*
> > + * Set Energy Performance Preference Register value through
> > + * Performance Controls Interface
> > + */
> > +int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
> > +{
> > +	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> > +	struct cpc_register_resource *epp_set_reg;
> > +	struct cpc_register_resource *auto_sel_reg;
> > +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> > +	struct cppc_pcc_data *pcc_ss_data = NULL;
> > +	int ret = -EINVAL;
> > +
> > +	if (!cpc_desc) {
> > +		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> > +		return -ENODEV;
> > +	}
> > +
> > +	auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
> > +	epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
> > +
> > +	if (CPC_IN_PCC(epp_set_reg) || CPC_IN_PCC(auto_sel_reg)) {
> > +		if (pcc_ss_id < 0) {
> > +			pr_debug("Invalid pcc_ss_id\n");
> > +			return -ENODEV;
> > +		}
> > +
> > +		if (CPC_SUPPORTED(auto_sel_reg)) {
> > +			ret = cpc_write(cpu, auto_sel_reg, enable);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +
> > +		if (CPC_SUPPORTED(epp_set_reg)) {
> > +			ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +
> > +		pcc_ss_data = pcc_data[pcc_ss_id];
> > +
> > +		down_write(&pcc_ss_data->pcc_lock);
> > +		/* after writing CPC, transfer the ownership of PCC to platform */
> > +		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> > +		up_write(&pcc_ss_data->pcc_lock);
> > +	} else {
> > +		ret = -ENOTSUPP;
> > +		pr_debug("_CPC in PCC is not supported\n");
> > +	}
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
> > +
> >  /**
> >   * cppc_set_enable - Set to enable CPPC on the processor by writing the
> >   * Continuous Performance Control package EnableRegister field.
> > diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> > index c5614444031f..a45bb876a19c 100644
> > --- a/include/acpi/cppc_acpi.h
> > +++ b/include/acpi/cppc_acpi.h
> > @@ -108,12 +108,14 @@ struct cppc_perf_caps {
> >  	u32 lowest_nonlinear_perf;
> >  	u32 lowest_freq;
> >  	u32 nominal_freq;
> > +	u32 energy_perf;
> >  };
> >  
> >  struct cppc_perf_ctrls {
> >  	u32 max_perf;
> >  	u32 min_perf;
> >  	u32 desired_perf;
> > +	u32 energy_perf;
> >  };
> >  
> >  struct cppc_perf_fb_ctrs {
> > @@ -149,6 +151,8 @@ extern bool cpc_ffh_supported(void);
> >  extern bool cpc_supported_by_cpu(void);
> >  extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
> >  extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
> > +extern int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps);
> > +extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
> >  #else /* !CONFIG_ACPI_CPPC_LIB */
> >  static inline int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
> >  {
> > @@ -202,6 +206,14 @@ static inline int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
> >  {
> >  	return -ENOTSUPP;
> >  }
> > +static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
> > +{
> > +	return -ENOTSUPP;
> > +}
> > +static inline int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> > +{
> > +	return -ENOTSUPP;
> > +}
> >  #endif /* !CONFIG_ACPI_CPPC_LIB */
> >  
> >  #endif /* _CPPC_ACPI_H*/
> > -- 
> > 2.34.1
> >
Yuan, Perry Dec. 12, 2022, 9:17 a.m. UTC | #4
[AMD Official Use Only - General]

Hi Ray.

> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Monday, December 12, 2022 11:29 AM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: rafael.j.wysocki@intel.com; Limonciello, Mario
> <Mario.Limonciello@amd.com>; viresh.kumar@linaro.org; Sharma, Deepak
> <Deepak.Sharma@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; 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>; Karny, Wyes <Wyes.Karny@amd.com>;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7 01/13] ACPI: CPPC: Add AMD pstate energy
> performance preference cppc control
> 
> On Fri, Dec 09, 2022 at 03:55:28PM +0800, Huang Rui wrote:
> > On Thu, Dec 08, 2022 at 07:18:40PM +0800, Yuan, Perry wrote:
> > > From: Perry Yuan <Perry.Yuan@amd.com>
> > >
> > > Add support for setting and querying EPP preferences to the generic
> > > CPPC driver.  This enables downstream drivers such as amd-pstate to
> > > discover and use these values
> > >
> > > Downstream drivers that want to use the new symbols
> > > cppc_get_epp_caps and cppc_set_epp_perf for querying and setting EPP
> > > preferences will need to call cppc_set_auto_epp to enable the EPP
> function first.
> > >
> > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> >
> > Acked-by: Huang Rui <ray.huang@amd.com>
> >
> > > ---
> > >  drivers/acpi/cppc_acpi.c | 114
> > > +++++++++++++++++++++++++++++++++++++--
> > >  include/acpi/cppc_acpi.h |  12 +++++
> > >  2 files changed, 121 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > > index 093675b1a1ff..37fa75f25f62 100644
> > > --- a/drivers/acpi/cppc_acpi.c
> > > +++ b/drivers/acpi/cppc_acpi.c
> > > @@ -1093,6 +1093,9 @@ static int cppc_get_perf(int cpunum, enum
> > > cppc_regs reg_idx, u64 *perf)  {
> > >  	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> > >  	struct cpc_register_resource *reg;
> > > +	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> > > +	struct cppc_pcc_data *pcc_ss_data = NULL;
> > > +	int ret = -EINVAL;
> > >
> > >  	if (!cpc_desc) {
> > >  		pr_debug("No CPC descriptor for CPU:%d\n", cpunum); @@
> -1102,10
> > > +1105,6 @@ static int cppc_get_perf(int cpunum, enum cppc_regs
> reg_idx, u64 *perf)
> > >  	reg = &cpc_desc->cpc_regs[reg_idx];
> > >
> > >  	if (CPC_IN_PCC(reg)) {
> > > -		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> > > -		struct cppc_pcc_data *pcc_ss_data = NULL;
> > > -		int ret = 0;
> > > -
> > >  		if (pcc_ss_id < 0)
> > >  			return -EIO;
> > >
> > > @@ -1125,7 +1124,7 @@ static int cppc_get_perf(int cpunum, enum
> > > cppc_regs reg_idx, u64 *perf)
> > >
> > >  	cpc_read(cpunum, reg, perf);
> > >
> > > -	return 0;
> > > +	return ret;
> > >  }
> > >
> > >  /**
> > > @@ -1365,6 +1364,111 @@ int cppc_get_perf_ctrs(int cpunum, struct
> > > cppc_perf_fb_ctrs *perf_fb_ctrs)  }
> > > EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
> > >
> > > +/**
> > > + * cppc_get_epp_caps - Get the energy preference register value.
> > > + * @cpunum: CPU from which to get epp preference level.
> > > + * @perf_caps: Return address.
> > > + *
> > > + * Return: 0 for success, -EIO otherwise.
> > > + */
> > > +int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> 
> Take a look at the patch again, due to the energy_perf is actually one of the
> members in struct cppc_perf_caps. It's better to modify the existing
> cppc_get_perf_caps() to get the epp value as well.
> 
> Thanks,
> Ray

Makes sense, I will change it in the V8.

Perry.

> 
> > > +{
> > > +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> > > +	struct cpc_register_resource *energy_perf_reg;
> > > +	u64 energy_perf;
> > > +
> > > +	if (!cpc_desc) {
> > > +		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
> > > +
> > > +	if (!CPC_SUPPORTED(energy_perf_reg))
> > > +		pr_warn_once("energy perf reg update is unsupported!\n");
> > > +
> > > +	if (CPC_IN_PCC(energy_perf_reg)) {
> > > +		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> > > +		struct cppc_pcc_data *pcc_ss_data = NULL;
> > > +		int ret = 0;
> > > +
> > > +		if (pcc_ss_id < 0)
> > > +			return -ENODEV;
> > > +
> > > +		pcc_ss_data = pcc_data[pcc_ss_id];
> > > +
> > > +		down_write(&pcc_ss_data->pcc_lock);
> > > +
> > > +		if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0) {
> > > +			cpc_read(cpunum, energy_perf_reg, &energy_perf);
> > > +			perf_caps->energy_perf = energy_perf;
> > > +		} else {
> > > +			ret = -EIO;
> > > +		}
> > > +
> > > +		up_write(&pcc_ss_data->pcc_lock);
> > > +
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cppc_get_epp_caps);
> > > +
> > > +/*
> > > + * Set Energy Performance Preference Register value through
> > > + * Performance Controls Interface
> > > + */
> > > +int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls,
> > > +bool enable) {
> > > +	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> > > +	struct cpc_register_resource *epp_set_reg;
> > > +	struct cpc_register_resource *auto_sel_reg;
> > > +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> > > +	struct cppc_pcc_data *pcc_ss_data = NULL;
> > > +	int ret = -EINVAL;
> > > +
> > > +	if (!cpc_desc) {
> > > +		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
> > > +	epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
> > > +
> > > +	if (CPC_IN_PCC(epp_set_reg) || CPC_IN_PCC(auto_sel_reg)) {
> > > +		if (pcc_ss_id < 0) {
> > > +			pr_debug("Invalid pcc_ss_id\n");
> > > +			return -ENODEV;
> > > +		}
> > > +
> > > +		if (CPC_SUPPORTED(auto_sel_reg)) {
> > > +			ret = cpc_write(cpu, auto_sel_reg, enable);
> > > +			if (ret)
> > > +				return ret;
> > > +		}
> > > +
> > > +		if (CPC_SUPPORTED(epp_set_reg)) {
> > > +			ret = cpc_write(cpu, epp_set_reg, perf_ctrls-
> >energy_perf);
> > > +			if (ret)
> > > +				return ret;
> > > +		}
> > > +
> > > +		pcc_ss_data = pcc_data[pcc_ss_id];
> > > +
> > > +		down_write(&pcc_ss_data->pcc_lock);
> > > +		/* after writing CPC, transfer the ownership of PCC to
> platform */
> > > +		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> > > +		up_write(&pcc_ss_data->pcc_lock);
> > > +	} else {
> > > +		ret = -ENOTSUPP;
> > > +		pr_debug("_CPC in PCC is not supported\n");
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
> > > +
> > >  /**
> > >   * cppc_set_enable - Set to enable CPPC on the processor by writing the
> > >   * Continuous Performance Control package EnableRegister field.
> > > diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> > > index c5614444031f..a45bb876a19c 100644
> > > --- a/include/acpi/cppc_acpi.h
> > > +++ b/include/acpi/cppc_acpi.h
> > > @@ -108,12 +108,14 @@ struct cppc_perf_caps {
> > >  	u32 lowest_nonlinear_perf;
> > >  	u32 lowest_freq;
> > >  	u32 nominal_freq;
> > > +	u32 energy_perf;
> > >  };
> > >
> > >  struct cppc_perf_ctrls {
> > >  	u32 max_perf;
> > >  	u32 min_perf;
> > >  	u32 desired_perf;
> > > +	u32 energy_perf;
> > >  };
> > >
> > >  struct cppc_perf_fb_ctrs {
> > > @@ -149,6 +151,8 @@ extern bool cpc_ffh_supported(void);  extern
> > > bool cpc_supported_by_cpu(void);  extern int cpc_read_ffh(int
> > > cpunum, struct cpc_reg *reg, u64 *val);  extern int
> > > cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
> > > +extern int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps
> > > +*perf_caps); extern int cppc_set_epp_perf(int cpu, struct
> > > +cppc_perf_ctrls *perf_ctrls, bool enable);
> > >  #else /* !CONFIG_ACPI_CPPC_LIB */
> > >  static inline int cppc_get_desired_perf(int cpunum, u64
> > > *desired_perf)  { @@ -202,6 +206,14 @@ static inline int
> > > cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)  {
> > >  	return -ENOTSUPP;
> > >  }
> > > +static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls
> > > +*perf_ctrls, bool enable) {
> > > +	return -ENOTSUPP;
> > > +}
> > > +static inline int cppc_get_epp_caps(int cpunum, struct
> > > +cppc_perf_caps *perf_caps) {
> > > +	return -ENOTSUPP;
> > > +}
> > >  #endif /* !CONFIG_ACPI_CPPC_LIB */
> > >
> > >  #endif /* _CPPC_ACPI_H*/
> > > --
> > > 2.34.1
> > >