* [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-19 21:09 ` Limonciello, Mario
2022-12-23 2:16 ` Huang Rui
2022-12-19 6:40 ` [PATCH v8 02/13] Documentation: amd-pstate: add EPP profiles introduction Perry Yuan
` (12 subsequent siblings)
13 siblings, 2 replies; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
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_epp_perf to enable the EPP function firstly.
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
drivers/acpi/cppc_acpi.c | 76 +++++++++++++++++++++++++++++++++++++---
include/acpi/cppc_acpi.h | 12 +++++++
2 files changed, 83 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 093675b1a1ff..81081eb899ea 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;
}
/**
@@ -1153,6 +1152,19 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
}
+/**
+ * cppc_get_epp_perf - Get the epp register value.
+ * @cpunum: CPU from which to get epp preference value.
+ * @epp_perf: Return address.
+ *
+ * Return: 0 for success, -EIO otherwise.
+ */
+int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
+{
+ return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
+}
+EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
+
/**
* cppc_get_perf_caps - Get a CPU's performance capabilities.
* @cpunum: CPU from which to get capabilities info.
@@ -1365,6 +1377,60 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
}
EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
+/*
+ * 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..6b487a5bd638 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_perf(int cpunum, u64 *epp_perf);
+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_perf(int cpunum, u64 *epp_perf)
+{
+ return -ENOTSUPP;
+}
#endif /* !CONFIG_ACPI_CPPC_LIB */
#endif /* _CPPC_ACPI_H*/
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control
2022-12-19 6:40 ` [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control Perry Yuan
@ 2022-12-19 21:09 ` Limonciello, Mario
2022-12-23 2:16 ` Huang Rui
1 sibling, 0 replies; 45+ messages in thread
From: Limonciello, Mario @ 2022-12-19 21:09 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
For this patch there's a few small nits you should fix for v9.
With those fixed:
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
On 12/19/2022 00:40, Perry Yuan 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
Missing a period here at end of the sentence.
>
> 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_epp_perf to enable the EPP function firstly.
>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
> drivers/acpi/cppc_acpi.c | 76 +++++++++++++++++++++++++++++++++++++---
> include/acpi/cppc_acpi.h | 12 +++++++
> 2 files changed, 83 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 093675b1a1ff..81081eb899ea 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;
> }
>
> /**
> @@ -1153,6 +1152,19 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
> return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
> }
>
> +/**
> + * cppc_get_epp_perf - Get the epp register value.
> + * @cpunum: CPU from which to get epp preference value.
> + * @epp_perf: Return address.
> + *
> + * Return: 0 for success, -EIO otherwise.
> + */
> +int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
> +{
> + return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
> +}
> +EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
> +
> /**
> * cppc_get_perf_caps - Get a CPU's performance capabilities.
> * @cpunum: CPU from which to get capabilities info.
> @@ -1365,6 +1377,60 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> }
> EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
>
> +/*
> + * 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;
AFAICT, you don't actually ever return -EINVAL in this function.
I think you should just avoid initializing this variable.
> +
> + 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");
Would it perhaps be useful to note the cpu # for this message?
> + 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..6b487a5bd638 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_perf(int cpunum, u64 *epp_perf);
> +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_perf(int cpunum, u64 *epp_perf)
> +{
> + return -ENOTSUPP;
> +}
> #endif /* !CONFIG_ACPI_CPPC_LIB */
>
> #endif /* _CPPC_ACPI_H*/
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control
2022-12-19 6:40 ` [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control Perry Yuan
2022-12-19 21:09 ` Limonciello, Mario
@ 2022-12-23 2:16 ` Huang Rui
2022-12-23 3:19 ` Yuan, Perry
1 sibling, 1 reply; 45+ messages in thread
From: Huang Rui @ 2022-12-23 2:16 UTC (permalink / raw)
To: Yuan, Perry
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On Mon, Dec 19, 2022 at 02:40:30PM +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_epp_perf to enable the EPP function firstly.
>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
> drivers/acpi/cppc_acpi.c | 76 +++++++++++++++++++++++++++++++++++++---
> include/acpi/cppc_acpi.h | 12 +++++++
> 2 files changed, 83 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 093675b1a1ff..81081eb899ea 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;
> -
Do you have any specific reason to move this piece out of if-condition?
> 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;
> }
>
> /**
> @@ -1153,6 +1152,19 @@ int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf)
> return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf);
> }
>
> +/**
> + * cppc_get_epp_perf - Get the epp register value.
> + * @cpunum: CPU from which to get epp preference value.
> + * @epp_perf: Return address.
> + *
> + * Return: 0 for success, -EIO otherwise.
> + */
> +int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
> +{
> + return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf);
> +}
> +EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
> +
This function is enough to get the epp value.
Thanks,
Ray
> /**
> * cppc_get_perf_caps - Get a CPU's performance capabilities.
> * @cpunum: CPU from which to get capabilities info.
> @@ -1365,6 +1377,60 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> }
> EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
>
> +/*
> + * 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..6b487a5bd638 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_perf(int cpunum, u64 *epp_perf);
> +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_perf(int cpunum, u64 *epp_perf)
> +{
> + return -ENOTSUPP;
> +}
> #endif /* !CONFIG_ACPI_CPPC_LIB */
>
> #endif /* _CPPC_ACPI_H*/
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control
2022-12-23 2:16 ` Huang Rui
@ 2022-12-23 3:19 ` Yuan, Perry
2022-12-23 9:06 ` Huang Rui
0 siblings, 1 reply; 45+ messages in thread
From: Yuan, Perry @ 2022-12-23 3:19 UTC (permalink / raw)
To: Huang, Ray
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
[AMD Official Use Only - General]
> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Friday, December 23, 2022 10:16 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 v8 01/13] ACPI: CPPC: Add AMD pstate energy
> performance preference cppc control
>
> On Mon, Dec 19, 2022 at 02:40:30PM +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_epp_perf to enable the EPP function firstly.
> >
> > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > ---
> > drivers/acpi/cppc_acpi.c | 76
> > +++++++++++++++++++++++++++++++++++++---
> > include/acpi/cppc_acpi.h | 12 +++++++
> > 2 files changed, 83 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index
> > 093675b1a1ff..81081eb899ea 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;
> > -
>
> Do you have any specific reason to move this piece out of if-condition?
Move the declaration ahead of the If conditions like other functions did.
It looks more reasonable and no functions impact.
Perry.
>
> > 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;
> > }
> >
> > /**
> > @@ -1153,6 +1152,19 @@ int cppc_get_nominal_perf(int cpunum, u64
> *nominal_perf)
> > return cppc_get_perf(cpunum, NOMINAL_PERF, nominal_perf); }
> >
> > +/**
> > + * cppc_get_epp_perf - Get the epp register value.
> > + * @cpunum: CPU from which to get epp preference value.
> > + * @epp_perf: Return address.
> > + *
> > + * Return: 0 for success, -EIO otherwise.
> > + */
> > +int cppc_get_epp_perf(int cpunum, u64 *epp_perf) {
> > + return cppc_get_perf(cpunum, ENERGY_PERF, epp_perf); }
> > +EXPORT_SYMBOL_GPL(cppc_get_epp_perf);
> > +
>
> This function is enough to get the epp value.
>
> Thanks,
> Ray
>
> > /**
> > * cppc_get_perf_caps - Get a CPU's performance capabilities.
> > * @cpunum: CPU from which to get capabilities info.
> > @@ -1365,6 +1377,60 @@ int cppc_get_perf_ctrs(int cpunum, struct
> > cppc_perf_fb_ctrs *perf_fb_ctrs) }
> > EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
> >
> > +/*
> > + * 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..6b487a5bd638 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_perf(int cpunum, u64 *epp_perf); 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_perf(int cpunum, u64 *epp_perf) {
> > + return -ENOTSUPP;
> > +}
> > #endif /* !CONFIG_ACPI_CPPC_LIB */
> >
> > #endif /* _CPPC_ACPI_H*/
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control
2022-12-23 3:19 ` Yuan, Perry
@ 2022-12-23 9:06 ` Huang Rui
2022-12-25 16:41 ` Yuan, Perry
0 siblings, 1 reply; 45+ messages in thread
From: Huang Rui @ 2022-12-23 9:06 UTC (permalink / raw)
To: Yuan, Perry
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On Fri, Dec 23, 2022 at 11:19:57AM +0800, Yuan, Perry wrote:
> [AMD Official Use Only - General]
>
>
>
> > -----Original Message-----
> > From: Huang, Ray <Ray.Huang@amd.com>
> > Sent: Friday, December 23, 2022 10:16 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 v8 01/13] ACPI: CPPC: Add AMD pstate energy
> > performance preference cppc control
> >
> > On Mon, Dec 19, 2022 at 02:40:30PM +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_epp_perf to enable the EPP function firstly.
> > >
> > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > ---
> > > drivers/acpi/cppc_acpi.c | 76
> > > +++++++++++++++++++++++++++++++++++++---
> > > include/acpi/cppc_acpi.h | 12 +++++++
> > > 2 files changed, 83 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index
> > > 093675b1a1ff..81081eb899ea 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;
> > > -
> >
> > Do you have any specific reason to move this piece out of if-condition?
>
> Move the declaration ahead of the If conditions like other functions did.
> It looks more reasonable and no functions impact.
>
If one platform doesn't have any CPC registers, it even won't need define
these variables like pcc_ss_id, pcc_ss_data, and it will save more time.
Thanks,
Ray
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control
2022-12-23 9:06 ` Huang Rui
@ 2022-12-25 16:41 ` Yuan, Perry
0 siblings, 0 replies; 45+ messages in thread
From: Yuan, Perry @ 2022-12-25 16:41 UTC (permalink / raw)
To: Huang, Ray
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
[AMD Official Use Only - General]
> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Friday, December 23, 2022 5:06 PM
> 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 v8 01/13] ACPI: CPPC: Add AMD pstate energy
> performance preference cppc control
>
> On Fri, Dec 23, 2022 at 11:19:57AM +0800, Yuan, Perry wrote:
> > [AMD Official Use Only - General]
> >
> >
> >
> > > -----Original Message-----
> > > From: Huang, Ray <Ray.Huang@amd.com>
> > > Sent: Friday, December 23, 2022 10:16 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 v8 01/13] ACPI: CPPC: Add AMD pstate energy
> > > performance preference cppc control
> > >
> > > On Mon, Dec 19, 2022 at 02:40:30PM +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_epp_perf to enable the EPP
> function firstly.
> > > >
> > > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > > ---
> > > > drivers/acpi/cppc_acpi.c | 76
> > > > +++++++++++++++++++++++++++++++++++++---
> > > > include/acpi/cppc_acpi.h | 12 +++++++
> > > > 2 files changed, 83 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > > > index 093675b1a1ff..81081eb899ea 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;
> > > > -
> > >
> > > Do you have any specific reason to move this piece out of if-condition?
> >
> > Move the declaration ahead of the If conditions like other functions did.
> > It looks more reasonable and no functions impact.
> >
>
> If one platform doesn't have any CPC registers, it even won't need define
> these variables like pcc_ss_id, pcc_ss_data, and it will save more time.
>
> Thanks,
> Ray
Make sense , get it fixed in V9
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 02/13] Documentation: amd-pstate: add EPP profiles introduction
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
2022-12-19 6:40 ` [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-19 22:37 ` Limonciello, Mario
2022-12-19 6:40 ` [PATCH v8 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP) Perry Yuan
` (11 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
From: Perry Yuan <Perry.Yuan@amd.com>
The patch add AMD pstate EPP feature introduction and what EPP
preference supported for AMD processors.
User can get supported list from
energy_performance_available_preferences attribute file, or update
current profile to energy_performance_preference file
1) See all EPP profiles
$ sudo cat /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_available_preferences
default performance balance_performance balance_power power
2) Check current EPP profile
$ sudo cat /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference
performance
3) Set new EPP profile
$ sudo bash -c "echo power > /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference"
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
Documentation/admin-guide/pm/amd-pstate.rst | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
index 06e23538f79c..33ab8ec8fc2f 100644
--- a/Documentation/admin-guide/pm/amd-pstate.rst
+++ b/Documentation/admin-guide/pm/amd-pstate.rst
@@ -262,6 +262,25 @@ lowest non-linear performance in `AMD CPPC Performance Capability
<perf_cap_>`_.)
This attribute is read-only.
+``energy_performance_available_preferences``
+
+A list of all the supported EPP preferences that could be used for
+``energy_performance_preference`` on this system.
+These profiles represent different hints that are provided
+to the low-level firmware about the user's desired energy vs efficiency
+tradeoff. ``default`` represents the epp value is set by platform
+firmware. This attribute is read-only.
+
+``energy_performance_preference``
+
+The current energy performance preference can be read from this attribute.
+and user can change current preference according to energy or performance needs
+Please get all support profiles list from
+``energy_performance_available_preferences`` attribute, all the profiles are
+integer values defined between 0 to 255 when EPP feature is enabled by platform
+firmware, if EPP feature is disabled, driver will ignore the written value
+This attribute is read-write.
+
Other performance and frequency values can be read back from
``/sys/devices/system/cpu/cpuX/acpi_cppc/``, see :ref:`cppc_sysfs`.
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 02/13] Documentation: amd-pstate: add EPP profiles introduction
2022-12-19 6:40 ` [PATCH v8 02/13] Documentation: amd-pstate: add EPP profiles introduction Perry Yuan
@ 2022-12-19 22:37 ` Limonciello, Mario
0 siblings, 0 replies; 45+ messages in thread
From: Limonciello, Mario @ 2022-12-19 22:37 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/2022 00:40, Perry Yuan wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
>
> The patch add AMD pstate EPP feature introduction and what EPP
> preference supported for AMD processors.
Don't use "the patch" or "this patch" in the commit message.
I would propose something like this:
>
> User can get supported list from
> energy_performance_available_preferences attribute file, or update
> current profile to energy_performance_preference file
The amd-pstate driver supports a feature called energy performance
preference (EPP). Add information to the documentation to explain
how users can interact with the sysfs files for this feature.
>
> 1) See all EPP profiles
> $ sudo cat /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_available_preferences
> default performance balance_performance balance_power power
>
> 2) Check current EPP profile
> $ sudo cat /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference
> performance
>
> 3) Set new EPP profile
> $ sudo bash -c "echo power > /sys/devices/system/cpu/cpu0/cpufreq/energy_performance_preference"
>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
> Documentation/admin-guide/pm/amd-pstate.rst | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index 06e23538f79c..33ab8ec8fc2f 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -262,6 +262,25 @@ lowest non-linear performance in `AMD CPPC Performance Capability
> <perf_cap_>`_.)
> This attribute is read-only.
>
> +``energy_performance_available_preferences``
> +
> +A list of all the supported EPP preferences that could be used for
> +``energy_performance_preference`` on this system.
> +These profiles represent different hints that are provided
> +to the low-level firmware about the user's desired energy vs efficiency
> +tradeoff. ``default`` represents the epp value is set by platform
> +firmware. This attribute is read-only.
> +
> +``energy_performance_preference``
> +
> +The current energy performance preference can be read from this attribute.
> +and user can change current preference according to energy or performance needs
> +Please get all support profiles list from
> +``energy_performance_available_preferences`` attribute, all the profiles are
> +integer values defined between 0 to 255 when EPP feature is enabled by platform
> +firmware, if EPP feature is disabled, driver will ignore the written value
> +This attribute is read-write.
> +
> Other performance and frequency values can be read back from
> ``/sys/devices/system/cpu/cpuX/acpi_cppc/``, see :ref:`cppc_sysfs`.
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP)
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
2022-12-19 6:40 ` [PATCH v8 01/13] ACPI: CPPC: Add AMD pstate energy performance preference cppc control Perry Yuan
2022-12-19 6:40 ` [PATCH v8 02/13] Documentation: amd-pstate: add EPP profiles introduction Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-20 2:53 ` Mario Limonciello
2022-12-23 3:10 ` Huang Rui
2022-12-19 6:40 ` [PATCH v8 04/13] cpufreq: amd-pstate: fix kernel hang issue while amd-pstate unregistering Perry Yuan
` (10 subsequent siblings)
13 siblings, 2 replies; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
make the energy preference performance strings and profiles using one
common header for intel_pstate driver, then the amd_pstate epp driver can
use the common header as well. This will simpify the intel_pstate and
amd_pstate driver.
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
drivers/cpufreq/intel_pstate.c | 13 +++----------
include/linux/cpufreq.h | 11 +++++++++++
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index ad9be31753b6..93a60fdac0fc 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -640,15 +640,7 @@ static int intel_pstate_set_epb(int cpu, s16 pref)
* 4 power
*/
-enum energy_perf_value_index {
- EPP_INDEX_DEFAULT = 0,
- EPP_INDEX_PERFORMANCE,
- EPP_INDEX_BALANCE_PERFORMANCE,
- EPP_INDEX_BALANCE_POWERSAVE,
- EPP_INDEX_POWERSAVE,
-};
-
-static const char * const energy_perf_strings[] = {
+const char * const energy_perf_strings[] = {
[EPP_INDEX_DEFAULT] = "default",
[EPP_INDEX_PERFORMANCE] = "performance",
[EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
@@ -656,7 +648,8 @@ static const char * const energy_perf_strings[] = {
[EPP_INDEX_POWERSAVE] = "power",
NULL
};
-static unsigned int epp_values[] = {
+
+unsigned int epp_values[] = {
[EPP_INDEX_DEFAULT] = 0, /* Unused index */
[EPP_INDEX_PERFORMANCE] = HWP_EPP_PERFORMANCE,
[EPP_INDEX_BALANCE_PERFORMANCE] = HWP_EPP_BALANCE_PERFORMANCE,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d5595d57f4e5..e63309d497fe 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -20,6 +20,7 @@
#include <linux/pm_qos.h>
#include <linux/spinlock.h>
#include <linux/sysfs.h>
+#include <asm/msr.h>
/*********************************************************************
* CPUFREQ INTERFACE *
@@ -185,6 +186,16 @@ struct cpufreq_freqs {
u8 flags; /* flags of cpufreq_driver, see below. */
};
+enum energy_perf_value_index {
+ EPP_INDEX_DEFAULT = 0,
+ EPP_INDEX_PERFORMANCE,
+ EPP_INDEX_BALANCE_PERFORMANCE,
+ EPP_INDEX_BALANCE_POWERSAVE,
+ EPP_INDEX_POWERSAVE,
+};
+extern const char * const energy_perf_strings[];
+extern unsigned int epp_values[];
+
/* Only for ACPI */
#define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
#define CPUFREQ_SHARED_TYPE_HW (1) /* HW does needed coordination */
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP)
2022-12-19 6:40 ` [PATCH v8 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP) Perry Yuan
@ 2022-12-20 2:53 ` Mario Limonciello
2022-12-23 3:10 ` Huang Rui
1 sibling, 0 replies; 45+ messages in thread
From: Mario Limonciello @ 2022-12-20 2:53 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/22 00:40, Perry Yuan wrote:
> make the energy preference performance strings and profiles using one
> common header for intel_pstate driver, then the amd_pstate epp driver can
> use the common header as well. This will simpify the intel_pstate and
> amd_pstate driver.
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
> drivers/cpufreq/intel_pstate.c | 13 +++----------
> include/linux/cpufreq.h | 11 +++++++++++
> 2 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index ad9be31753b6..93a60fdac0fc 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -640,15 +640,7 @@ static int intel_pstate_set_epb(int cpu, s16 pref)
> * 4 power
> */
>
> -enum energy_perf_value_index {
> - EPP_INDEX_DEFAULT = 0,
> - EPP_INDEX_PERFORMANCE,
> - EPP_INDEX_BALANCE_PERFORMANCE,
> - EPP_INDEX_BALANCE_POWERSAVE,
> - EPP_INDEX_POWERSAVE,
> -};
> -
> -static const char * const energy_perf_strings[] = {
> +const char * const energy_perf_strings[] = {
> [EPP_INDEX_DEFAULT] = "default",
> [EPP_INDEX_PERFORMANCE] = "performance",
> [EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
> @@ -656,7 +648,8 @@ static const char * const energy_perf_strings[] = {
> [EPP_INDEX_POWERSAVE] = "power",
> NULL
> };
> -static unsigned int epp_values[] = {
> +
> +unsigned int epp_values[] = {
> [EPP_INDEX_DEFAULT] = 0, /* Unused index */
> [EPP_INDEX_PERFORMANCE] = HWP_EPP_PERFORMANCE,
> [EPP_INDEX_BALANCE_PERFORMANCE] = HWP_EPP_BALANCE_PERFORMANCE,
I think this is going to make CONFIG_AMD_PSTATE depend on
CONFIG_INTEL_PSTATE. What you'll want to do is put these symbols in a
"common" C file used by both. How about in the cppc lib stuff?
Please make sure that you test compile/link of v9 both with
CONFIG_AMD_PSTATE/CONFIG_INTEL_PSTATE set and either or set.
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index d5595d57f4e5..e63309d497fe 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -20,6 +20,7 @@
> #include <linux/pm_qos.h>
> #include <linux/spinlock.h>
> #include <linux/sysfs.h>
> +#include <asm/msr.h>
>
> /*********************************************************************
> * CPUFREQ INTERFACE *
> @@ -185,6 +186,16 @@ struct cpufreq_freqs {
> u8 flags; /* flags of cpufreq_driver, see below. */
> };
>
> +enum energy_perf_value_index {
> + EPP_INDEX_DEFAULT = 0,
> + EPP_INDEX_PERFORMANCE,
> + EPP_INDEX_BALANCE_PERFORMANCE,
> + EPP_INDEX_BALANCE_POWERSAVE,
> + EPP_INDEX_POWERSAVE,
> +};
> +extern const char * const energy_perf_strings[];
> +extern unsigned int epp_values[];
> +
> /* Only for ACPI */
> #define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
> #define CPUFREQ_SHARED_TYPE_HW (1) /* HW does needed coordination */
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP)
2022-12-19 6:40 ` [PATCH v8 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP) Perry Yuan
2022-12-20 2:53 ` Mario Limonciello
@ 2022-12-23 3:10 ` Huang Rui
2022-12-23 3:12 ` Yuan, Perry
1 sibling, 1 reply; 45+ messages in thread
From: Huang Rui @ 2022-12-23 3:10 UTC (permalink / raw)
To: Yuan, Perry
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On Mon, Dec 19, 2022 at 02:40:32PM +0800, Yuan, Perry wrote:
> make the energy preference performance strings and profiles using one
> common header for intel_pstate driver, then the amd_pstate epp driver can
> use the common header as well. This will simpify the intel_pstate and
> amd_pstate driver.
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
> drivers/cpufreq/intel_pstate.c | 13 +++----------
> include/linux/cpufreq.h | 11 +++++++++++
> 2 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index ad9be31753b6..93a60fdac0fc 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -640,15 +640,7 @@ static int intel_pstate_set_epb(int cpu, s16 pref)
> * 4 power
> */
>
> -enum energy_perf_value_index {
> - EPP_INDEX_DEFAULT = 0,
> - EPP_INDEX_PERFORMANCE,
> - EPP_INDEX_BALANCE_PERFORMANCE,
> - EPP_INDEX_BALANCE_POWERSAVE,
> - EPP_INDEX_POWERSAVE,
> -};
> -
> -static const char * const energy_perf_strings[] = {
> +const char * const energy_perf_strings[] = {
> [EPP_INDEX_DEFAULT] = "default",
> [EPP_INDEX_PERFORMANCE] = "performance",
> [EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
> @@ -656,7 +648,8 @@ static const char * const energy_perf_strings[] = {
> [EPP_INDEX_POWERSAVE] = "power",
> NULL
> };
> -static unsigned int epp_values[] = {
> +
> +unsigned int epp_values[] = {
> [EPP_INDEX_DEFAULT] = 0, /* Unused index */
> [EPP_INDEX_PERFORMANCE] = HWP_EPP_PERFORMANCE,
> [EPP_INDEX_BALANCE_PERFORMANCE] = HWP_EPP_BALANCE_PERFORMANCE,
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index d5595d57f4e5..e63309d497fe 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -20,6 +20,7 @@
> #include <linux/pm_qos.h>
> #include <linux/spinlock.h>
> #include <linux/sysfs.h>
> +#include <asm/msr.h>
Please don't include msr header in cpufreq common file, we already include
it in amd-pstate.c, that's fairly enough.
Thanks,
Ray
>
> /*********************************************************************
> * CPUFREQ INTERFACE *
> @@ -185,6 +186,16 @@ struct cpufreq_freqs {
> u8 flags; /* flags of cpufreq_driver, see below. */
> };
>
> +enum energy_perf_value_index {
> + EPP_INDEX_DEFAULT = 0,
> + EPP_INDEX_PERFORMANCE,
> + EPP_INDEX_BALANCE_PERFORMANCE,
> + EPP_INDEX_BALANCE_POWERSAVE,
> + EPP_INDEX_POWERSAVE,
> +};
> +extern const char * const energy_perf_strings[];
> +extern unsigned int epp_values[];
> +
> /* Only for ACPI */
> #define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
> #define CPUFREQ_SHARED_TYPE_HW (1) /* HW does needed coordination */
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v8 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP)
2022-12-23 3:10 ` Huang Rui
@ 2022-12-23 3:12 ` Yuan, Perry
0 siblings, 0 replies; 45+ messages in thread
From: Yuan, Perry @ 2022-12-23 3:12 UTC (permalink / raw)
To: Huang, Ray
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
[AMD Official Use Only - General]
> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Friday, December 23, 2022 11:10 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 v8 03/13] cpufreq: intel_pstate: use common macro
> definition for Energy Preference Performance(EPP)
>
> On Mon, Dec 19, 2022 at 02:40:32PM +0800, Yuan, Perry wrote:
> > make the energy preference performance strings and profiles using one
> > common header for intel_pstate driver, then the amd_pstate epp driver
> > can use the common header as well. This will simpify the intel_pstate
> > and amd_pstate driver.
> >
> > Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> > ---
> > drivers/cpufreq/intel_pstate.c | 13 +++----------
> > include/linux/cpufreq.h | 11 +++++++++++
> > 2 files changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/cpufreq/intel_pstate.c
> > b/drivers/cpufreq/intel_pstate.c index ad9be31753b6..93a60fdac0fc
> > 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -640,15 +640,7 @@ static int intel_pstate_set_epb(int cpu, s16 pref)
> > * 4 power
> > */
> >
> > -enum energy_perf_value_index {
> > - EPP_INDEX_DEFAULT = 0,
> > - EPP_INDEX_PERFORMANCE,
> > - EPP_INDEX_BALANCE_PERFORMANCE,
> > - EPP_INDEX_BALANCE_POWERSAVE,
> > - EPP_INDEX_POWERSAVE,
> > -};
> > -
> > -static const char * const energy_perf_strings[] = {
> > +const char * const energy_perf_strings[] = {
> > [EPP_INDEX_DEFAULT] = "default",
> > [EPP_INDEX_PERFORMANCE] = "performance",
> > [EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
> @@ -656,7
> > +648,8 @@ static const char * const energy_perf_strings[] = {
> > [EPP_INDEX_POWERSAVE] = "power",
> > NULL
> > };
> > -static unsigned int epp_values[] = {
> > +
> > +unsigned int epp_values[] = {
> > [EPP_INDEX_DEFAULT] = 0, /* Unused index */
> > [EPP_INDEX_PERFORMANCE] = HWP_EPP_PERFORMANCE,
> > [EPP_INDEX_BALANCE_PERFORMANCE] =
> HWP_EPP_BALANCE_PERFORMANCE, diff
> > --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index
> > d5595d57f4e5..e63309d497fe 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -20,6 +20,7 @@
> > #include <linux/pm_qos.h>
> > #include <linux/spinlock.h>
> > #include <linux/sysfs.h>
> > +#include <asm/msr.h>
>
> Please don't include msr header in cpufreq common file, we already include
> it in amd-pstate.c, that's fairly enough.
>
> Thanks,
> Ray
Good , will remove the msr.h from this file.
Thank you.
Perry.
>
> >
> >
> /**********************************************************
> ***********
> > * CPUFREQ INTERFACE *
> > @@ -185,6 +186,16 @@ struct cpufreq_freqs {
> > u8 flags; /* flags of cpufreq_driver, see below. */
> > };
> >
> > +enum energy_perf_value_index {
> > + EPP_INDEX_DEFAULT = 0,
> > + EPP_INDEX_PERFORMANCE,
> > + EPP_INDEX_BALANCE_PERFORMANCE,
> > + EPP_INDEX_BALANCE_POWERSAVE,
> > + EPP_INDEX_POWERSAVE,
> > +};
> > +extern const char * const energy_perf_strings[]; extern unsigned int
> > +epp_values[];
> > +
> > /* Only for ACPI */
> > #define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
> > #define CPUFREQ_SHARED_TYPE_HW (1) /* HW does needed
> coordination */
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 04/13] cpufreq: amd-pstate: fix kernel hang issue while amd-pstate unregistering
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (2 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP) Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-19 22:55 ` Limonciello, Mario
2022-12-19 6:40 ` [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param() Perry Yuan
` (9 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
In the amd_pstate_adjust_perf(), there is one cpufreq_cpu_get() call to
increase increments the kobject reference count of policy and make it as
busy. Therefore, a corresponding call to cpufreq_cpu_put() is needed to
decrement the kobject reference count back, it will resolve the kernel
hang issue when unregistering the amd-pstate driver and register the
`amd_pstate_epp` driver instance.
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
Acked-by: Huang Rui <ray.huang@amd.com>
---
drivers/cpufreq/amd-pstate.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 204e39006dda..c17bd845f5fc 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -307,6 +307,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
max_perf = min_perf;
amd_pstate_update(cpudata, min_perf, des_perf, max_perf, true);
+ cpufreq_cpu_put(policy);
}
static int amd_get_min_freq(struct amd_cpudata *cpudata)
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 04/13] cpufreq: amd-pstate: fix kernel hang issue while amd-pstate unregistering
2022-12-19 6:40 ` [PATCH v8 04/13] cpufreq: amd-pstate: fix kernel hang issue while amd-pstate unregistering Perry Yuan
@ 2022-12-19 22:55 ` Limonciello, Mario
0 siblings, 0 replies; 45+ messages in thread
From: Limonciello, Mario @ 2022-12-19 22:55 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/2022 00:40, Perry Yuan wrote:
> In the amd_pstate_adjust_perf(), there is one cpufreq_cpu_get() call to
> increase increments the kobject reference count of policy and make it as
> busy. Therefore, a corresponding call to cpufreq_cpu_put() is needed to
> decrement the kobject reference count back, it will resolve the kernel
> hang issue when unregistering the amd-pstate driver and register the
> `amd_pstate_epp` driver instance.
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> Acked-by: Huang Rui <ray.huang@amd.com>
Even if this patch comes into mainline through the series, this
particular patch should go to stable too to fix users unloading
amd-pstate with the existing passive mode.
Cc: stable@vger.kernel.org
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 204e39006dda..c17bd845f5fc 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -307,6 +307,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
> max_perf = min_perf;
>
> amd_pstate_update(cpudata, min_perf, des_perf, max_perf, true);
> + cpufreq_cpu_put(policy);
> }
>
> static int amd_get_min_freq(struct amd_cpudata *cpudata)
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param()
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (3 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 04/13] cpufreq: amd-pstate: fix kernel hang issue while amd-pstate unregistering Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-19 23:01 ` Limonciello, Mario
` (2 more replies)
2022-12-19 6:40 ` [PATCH v8 06/13] cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors Perry Yuan
` (8 subsequent siblings)
13 siblings, 3 replies; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
There are some other amd pstate driver working mode to be supported.
Here we use cppc_state var to indicate which mode is enabled.
This change will help to simplify the the amd_pstate_param() to choose
which mode used for the following driver registeration.
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
Signed-off-by: Wyes Karny <wyes.karny@amd.com>
---
drivers/cpufreq/amd-pstate.c | 35 +++++++++++++++++++++++++++--------
include/linux/amd-pstate.h | 29 +++++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 8 deletions(-)
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index c17bd845f5fc..861a905f9324 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -60,7 +60,18 @@
* module parameter to be able to enable it manually for debugging.
*/
static struct cpufreq_driver amd_pstate_driver;
-static int cppc_load __initdata;
+static int cppc_state = AMD_PSTATE_DISABLE;
+
+static inline int get_mode_idx_from_str(const char *str, size_t size)
+{
+ int i;
+
+ for (i=0; i < AMD_PSTATE_MAX; i++) {
+ if (!strncmp(str, amd_pstate_mode_string[i], size))
+ return i;
+ }
+ return -EINVAL;
+}
static inline int pstate_enable(bool enable)
{
@@ -628,7 +639,7 @@ static int __init amd_pstate_init(void)
* enable the amd_pstate passive mode driver explicitly
* with amd_pstate=passive in kernel command line
*/
- if (!cppc_load) {
+ if (cppc_state == AMD_PSTATE_DISABLE) {
pr_debug("driver load is disabled, boot with amd_pstate=passive to enable this\n");
return -ENODEV;
}
@@ -671,16 +682,24 @@ device_initcall(amd_pstate_init);
static int __init amd_pstate_param(char *str)
{
+ size_t size;
+ int mode_idx;
+
if (!str)
return -EINVAL;
- if (!strcmp(str, "disable")) {
- cppc_load = 0;
- pr_info("driver is explicitly disabled\n");
- } else if (!strcmp(str, "passive"))
- cppc_load = 1;
+ size = strlen(str);
+ mode_idx = get_mode_idx_from_str(str, size);
- return 0;
+ if (mode_idx >= AMD_PSTATE_DISABLE && mode_idx < AMD_PSTATE_MAX) {
+ cppc_state = mode_idx;
+ if (cppc_state == AMD_PSTATE_DISABLE)
+ pr_info("driver is explicitly disabled\n");
+
+ return 0;
+ }
+
+ return -EINVAL;
}
early_param("amd_pstate", amd_pstate_param);
diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
index 1c4b8659f171..922d05a13902 100644
--- a/include/linux/amd-pstate.h
+++ b/include/linux/amd-pstate.h
@@ -74,4 +74,33 @@ struct amd_cpudata {
bool boost_supported;
};
+/**
+ * enum amd_pstate_mode - driver working mode of amd pstate
+ */
+
+enum amd_pstate_mode {
+ /** @AMD_PSTATE_DISABLE: Driver mode is disabled */
+ AMD_PSTATE_DISABLE = 0,
+
+ /** @AMD_PSTATE_PASSIVE: Drier mode is passive mode */
+ AMD_PSTATE_PASSIVE = 1,
+
+ /** @AMD_PSTATE_ACTIVE: Driver mode is active mode */
+ AMD_PSTATE_ACTIVE = 2,
+
+ /** @AMD_PSTATE_GUIDE: Driver mode is guided mode */
+ AMD_PSTATE_GUIDE = 3,
+
+ /** @AMD_PSTATE_MAX */
+ AMD_PSTATE_MAX = 4,
+};
+
+static const char * const amd_pstate_mode_string[] = {
+ [AMD_PSTATE_DISABLE] = "disable",
+ [AMD_PSTATE_PASSIVE] = "passive",
+ [AMD_PSTATE_ACTIVE] = "active",
+ [AMD_PSTATE_GUIDE] = "guide",
+ NULL,
+};
+
#endif /* _LINUX_AMD_PSTATE_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param()
2022-12-19 6:40 ` [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param() Perry Yuan
@ 2022-12-19 23:01 ` Limonciello, Mario
2022-12-23 4:23 ` Huang Rui
2022-12-23 9:45 ` Wyes Karny
2 siblings, 0 replies; 45+ messages in thread
From: Limonciello, Mario @ 2022-12-19 23:01 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/2022 00:40, Perry Yuan wrote:
> There are some other amd pstate driver working mode to be supported.
Perhaps:
The amd-pstate driver may support multiple working modes. Introduce a
variable to keep track of which mode is currently enabled.
> Here we use cppc_state var to indicate which mode is enabled.
> This change will help to simplify the the amd_pstate_param() to choose
> which mode used for the following driver registeration.
s/registeration/registration/
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> Signed-off-by: Wyes Karny <wyes.karny@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 35 +++++++++++++++++++++++++++--------
> include/linux/amd-pstate.h | 29 +++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index c17bd845f5fc..861a905f9324 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -60,7 +60,18 @@
> * module parameter to be able to enable it manually for debugging.
> */
> static struct cpufreq_driver amd_pstate_driver;
> -static int cppc_load __initdata;
> +static int cppc_state = AMD_PSTATE_DISABLE;
> +
> +static inline int get_mode_idx_from_str(const char *str, size_t size)
> +{
> + int i;
> +
> + for (i=0; i < AMD_PSTATE_MAX; i++) {
> + if (!strncmp(str, amd_pstate_mode_string[i], size))
> + return i;
> + }
> + return -EINVAL;
> +}
>
> static inline int pstate_enable(bool enable)
> {
> @@ -628,7 +639,7 @@ static int __init amd_pstate_init(void)
> * enable the amd_pstate passive mode driver explicitly
> * with amd_pstate=passive in kernel command line
> */
> - if (!cppc_load) {
> + if (cppc_state == AMD_PSTATE_DISABLE) {
> pr_debug("driver load is disabled, boot with amd_pstate=passive to enable this\n");
Presumably this message needs to be changed since there are different
modes that can be used to enable it now.
> return -ENODEV;
> }
> @@ -671,16 +682,24 @@ device_initcall(amd_pstate_init);
>
> static int __init amd_pstate_param(char *str)
> {
> + size_t size;
> + int mode_idx;
> +
> if (!str)
> return -EINVAL;
>
> - if (!strcmp(str, "disable")) {
> - cppc_load = 0;
> - pr_info("driver is explicitly disabled\n");
> - } else if (!strcmp(str, "passive"))
> - cppc_load = 1;
> + size = strlen(str);
> + mode_idx = get_mode_idx_from_str(str, size);
>
> - return 0;
> + if (mode_idx >= AMD_PSTATE_DISABLE && mode_idx < AMD_PSTATE_MAX) {
> + cppc_state = mode_idx;
> + if (cppc_state == AMD_PSTATE_DISABLE)
> + pr_info("driver is explicitly disabled\n");
> +
> + return 0;
> + }
> +
> + return -EINVAL;
> }
> early_param("amd_pstate", amd_pstate_param);
>
> diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> index 1c4b8659f171..922d05a13902 100644
> --- a/include/linux/amd-pstate.h
> +++ b/include/linux/amd-pstate.h
> @@ -74,4 +74,33 @@ struct amd_cpudata {
> bool boost_supported;
> };
>
> +/**
> + * enum amd_pstate_mode - driver working mode of amd pstate
> + */
> +
> +enum amd_pstate_mode {
> + /** @AMD_PSTATE_DISABLE: Driver mode is disabled */
> + AMD_PSTATE_DISABLE = 0,
> +
> + /** @AMD_PSTATE_PASSIVE: Drier mode is passive mode */
s/Drier/Driver/
> + AMD_PSTATE_PASSIVE = 1,
> +
> + /** @AMD_PSTATE_ACTIVE: Driver mode is active mode */
> + AMD_PSTATE_ACTIVE = 2,
> +
> + /** @AMD_PSTATE_GUIDE: Driver mode is guided mode */
> + AMD_PSTATE_GUIDE = 3,
> +
> + /** @AMD_PSTATE_MAX */
> + AMD_PSTATE_MAX = 4,
> +};
> +
> +static const char * const amd_pstate_mode_string[] = {
> + [AMD_PSTATE_DISABLE] = "disable",
> + [AMD_PSTATE_PASSIVE] = "passive",
> + [AMD_PSTATE_ACTIVE] = "active",
> + [AMD_PSTATE_GUIDE] = "guide",
I get the intent here, but guided mode isn't actually part of the v8
series under review.
I think it should either be folded into the series or this patch
should be split into two across the two patch-sets:
1) introduce the state machine (part of the EPP patch-set)
2) add guided mode support (part of the guided mode patch-set).
> + NULL,
> +};
> +
> #endif /* _LINUX_AMD_PSTATE_H */
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param()
2022-12-19 6:40 ` [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param() Perry Yuan
2022-12-19 23:01 ` Limonciello, Mario
@ 2022-12-23 4:23 ` Huang Rui
2022-12-23 9:45 ` Wyes Karny
2 siblings, 0 replies; 45+ messages in thread
From: Huang Rui @ 2022-12-23 4:23 UTC (permalink / raw)
To: Yuan, Perry
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On Mon, Dec 19, 2022 at 02:40:34PM +0800, Yuan, Perry wrote:
> There are some other amd pstate driver working mode to be supported.
> Here we use cppc_state var to indicate which mode is enabled.
> This change will help to simplify the the amd_pstate_param() to choose
> which mode used for the following driver registeration.
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> Signed-off-by: Wyes Karny <wyes.karny@amd.com>
The most of codes for this patch should be inherited by below patch:
https://lore.kernel.org/lkml/20221207154648.233759-2-wyes.karny@amd.com/
So I believe Wyes should be the author of this patch, please use git commit
--amend --author="Wyes Karny <wyes.karny@amd.com>" to update.
Thanks,
Ray
> ---
> drivers/cpufreq/amd-pstate.c | 35 +++++++++++++++++++++++++++--------
> include/linux/amd-pstate.h | 29 +++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index c17bd845f5fc..861a905f9324 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -60,7 +60,18 @@
> * module parameter to be able to enable it manually for debugging.
> */
> static struct cpufreq_driver amd_pstate_driver;
> -static int cppc_load __initdata;
> +static int cppc_state = AMD_PSTATE_DISABLE;
> +
> +static inline int get_mode_idx_from_str(const char *str, size_t size)
> +{
> + int i;
> +
> + for (i=0; i < AMD_PSTATE_MAX; i++) {
> + if (!strncmp(str, amd_pstate_mode_string[i], size))
> + return i;
> + }
> + return -EINVAL;
> +}
>
> static inline int pstate_enable(bool enable)
> {
> @@ -628,7 +639,7 @@ static int __init amd_pstate_init(void)
> * enable the amd_pstate passive mode driver explicitly
> * with amd_pstate=passive in kernel command line
> */
> - if (!cppc_load) {
> + if (cppc_state == AMD_PSTATE_DISABLE) {
> pr_debug("driver load is disabled, boot with amd_pstate=passive to enable this\n");
> return -ENODEV;
> }
> @@ -671,16 +682,24 @@ device_initcall(amd_pstate_init);
>
> static int __init amd_pstate_param(char *str)
> {
> + size_t size;
> + int mode_idx;
> +
> if (!str)
> return -EINVAL;
>
> - if (!strcmp(str, "disable")) {
> - cppc_load = 0;
> - pr_info("driver is explicitly disabled\n");
> - } else if (!strcmp(str, "passive"))
> - cppc_load = 1;
> + size = strlen(str);
> + mode_idx = get_mode_idx_from_str(str, size);
>
> - return 0;
> + if (mode_idx >= AMD_PSTATE_DISABLE && mode_idx < AMD_PSTATE_MAX) {
> + cppc_state = mode_idx;
> + if (cppc_state == AMD_PSTATE_DISABLE)
> + pr_info("driver is explicitly disabled\n");
> +
> + return 0;
> + }
> +
> + return -EINVAL;
> }
> early_param("amd_pstate", amd_pstate_param);
>
> diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> index 1c4b8659f171..922d05a13902 100644
> --- a/include/linux/amd-pstate.h
> +++ b/include/linux/amd-pstate.h
> @@ -74,4 +74,33 @@ struct amd_cpudata {
> bool boost_supported;
> };
>
> +/**
> + * enum amd_pstate_mode - driver working mode of amd pstate
> + */
> +
> +enum amd_pstate_mode {
> + /** @AMD_PSTATE_DISABLE: Driver mode is disabled */
> + AMD_PSTATE_DISABLE = 0,
> +
> + /** @AMD_PSTATE_PASSIVE: Drier mode is passive mode */
> + AMD_PSTATE_PASSIVE = 1,
> +
> + /** @AMD_PSTATE_ACTIVE: Driver mode is active mode */
> + AMD_PSTATE_ACTIVE = 2,
> +
> + /** @AMD_PSTATE_GUIDE: Driver mode is guided mode */
> + AMD_PSTATE_GUIDE = 3,
> +
> + /** @AMD_PSTATE_MAX */
> + AMD_PSTATE_MAX = 4,
> +};
> +
> +static const char * const amd_pstate_mode_string[] = {
> + [AMD_PSTATE_DISABLE] = "disable",
> + [AMD_PSTATE_PASSIVE] = "passive",
> + [AMD_PSTATE_ACTIVE] = "active",
> + [AMD_PSTATE_GUIDE] = "guide",
> + NULL,
> +};
> +
> #endif /* _LINUX_AMD_PSTATE_H */
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param()
2022-12-19 6:40 ` [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param() Perry Yuan
2022-12-19 23:01 ` Limonciello, Mario
2022-12-23 4:23 ` Huang Rui
@ 2022-12-23 9:45 ` Wyes Karny
2022-12-25 16:41 ` Yuan, Perry
2 siblings, 1 reply; 45+ messages in thread
From: Wyes Karny @ 2022-12-23 9:45 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, Mario.Limonciello, ray.huang,
viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, linux-pm, linux-kernel
On 12/19/2022 12:10 PM, Perry Yuan wrote:
--------------------->8-----------------------------
>
> +/**
> + * enum amd_pstate_mode - driver working mode of amd pstate
> + */
> +
> +enum amd_pstate_mode {
> + /** @AMD_PSTATE_DISABLE: Driver mode is disabled */
> + AMD_PSTATE_DISABLE = 0,
> +
> + /** @AMD_PSTATE_PASSIVE: Drier mode is passive mode */
> + AMD_PSTATE_PASSIVE = 1,
> +
> + /** @AMD_PSTATE_ACTIVE: Driver mode is active mode */
> + AMD_PSTATE_ACTIVE = 2,
> +
> + /** @AMD_PSTATE_GUIDE: Driver mode is guided mode */
> + AMD_PSTATE_GUIDE = 3,
> +
> + /** @AMD_PSTATE_MAX */
> + AMD_PSTATE_MAX = 4,
> +};
IMO the above enum is self explanatory we don't need to annotate.
what about below?
/**
* enum amd_pstate_mode - driver working mode
* All supported modes are explained in kernel-parameters.txt
*/
enum amd_pstate_mode {
AMD_PSTATE_DISABLE = 0,
AMD_PSTATE_PASSIVE,
AMD_PSTATE_ACTIVE,
AMD_PSTATE_MAX,
};
Plz remove GUIDED mode here because it allows user to pass "amd_pstate=guided"
in kernel cmdline. Therefore it breaks the driver flow without guided patches.
I can update the enum in my guided patch.
> +
> +static const char * const amd_pstate_mode_string[] = {
> + [AMD_PSTATE_DISABLE] = "disable",
> + [AMD_PSTATE_PASSIVE] = "passive",
> + [AMD_PSTATE_ACTIVE] = "active",
> + [AMD_PSTATE_GUIDE] = "guide",
> + NULL,
> +};
> +
> #endif /* _LINUX_AMD_PSTATE_H */
--
Thanks & Regards,
Wyes
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param()
2022-12-23 9:45 ` Wyes Karny
@ 2022-12-25 16:41 ` Yuan, Perry
0 siblings, 0 replies; 45+ messages in thread
From: Yuan, Perry @ 2022-12-25 16:41 UTC (permalink / raw)
To: Karny, Wyes, rafael.j.wysocki@intel.com, Limonciello, Mario,
Huang, Ray, viresh.kumar@linaro.org
Cc: Sharma, Deepak, Fontenot, Nathan, Deucher, Alexander,
Huang, Shimmer, Du, Xiaojian, Meng, Li (Jassmine),
linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
[AMD Official Use Only - General]
> -----Original Message-----
> From: Karny, Wyes <Wyes.Karny@amd.com>
> Sent: Friday, December 23, 2022 5:45 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> Limonciello, Mario <Mario.Limonciello@amd.com>; Huang, Ray
> <Ray.Huang@amd.com>; viresh.kumar@linaro.org
> Cc: 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>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working
> mode selection in amd_pstate_param()
>
>
>
> On 12/19/2022 12:10 PM, Perry Yuan wrote:
> --------------------->8-----------------------------
> >
> > +/**
> > + * enum amd_pstate_mode - driver working mode of amd pstate */
> > +
> > +enum amd_pstate_mode {
> > + /** @AMD_PSTATE_DISABLE: Driver mode is disabled */
> > + AMD_PSTATE_DISABLE = 0,
> > +
> > + /** @AMD_PSTATE_PASSIVE: Drier mode is passive mode */
> > + AMD_PSTATE_PASSIVE = 1,
> > +
> > + /** @AMD_PSTATE_ACTIVE: Driver mode is active mode */
> > + AMD_PSTATE_ACTIVE = 2,
> > +
> > + /** @AMD_PSTATE_GUIDE: Driver mode is guided mode */
> > + AMD_PSTATE_GUIDE = 3,
> > +
> > + /** @AMD_PSTATE_MAX */
> > + AMD_PSTATE_MAX = 4,
> > +};
>
> IMO the above enum is self explanatory we don't need to annotate.
>
> what about below?
>
> /**
> * enum amd_pstate_mode - driver working mode
> * All supported modes are explained in kernel-parameters.txt */ enum
> amd_pstate_mode {
> AMD_PSTATE_DISABLE = 0,
> AMD_PSTATE_PASSIVE,
> AMD_PSTATE_ACTIVE,
> AMD_PSTATE_MAX,
> };
Sure , changed it in v9 like you suggested.
>
> Plz remove GUIDED mode here because it allows user to pass
> "amd_pstate=guided"
> in kernel cmdline. Therefore it breaks the driver flow without guided patches.
> I can update the enum in my guided patch.
>
Removed
> > +
> > +static const char * const amd_pstate_mode_string[] = {
> > + [AMD_PSTATE_DISABLE] = "disable",
> > + [AMD_PSTATE_PASSIVE] = "passive",
> > + [AMD_PSTATE_ACTIVE] = "active",
> > + [AMD_PSTATE_GUIDE] = "guide",
> > + NULL,
> > +};
> > +
> > #endif /* _LINUX_AMD_PSTATE_H */
>
> --
> Thanks & Regards,
> Wyes
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 06/13] cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (4 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param() Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-20 3:09 ` Mario Limonciello
2022-12-23 7:43 ` Huang Rui
2022-12-19 6:40 ` [PATCH v8 07/13] cpufreq: amd-pstate: implement amd pstate cpu online and offline callback Perry Yuan
` (7 subsequent siblings)
13 siblings, 2 replies; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
From: Perry Yuan <Perry.Yuan@amd.com>
Add EPP driver support for AMD SoCs which support a dedicated MSR for
CPPC. EPP is used by the DPM controller to configure the frequency that
a core operates at during short periods of activity.
The SoC EPP targets are configured on a scale from 0 to 255 where 0
represents maximum performance and 255 represents maximum efficiency.
The amd-pstate driver exports profile string names to userspace that are
tied to specific EPP values.
The balance_performance string (0x80) provides the best balance for
efficiency versus power on most systems, but users can choose other
strings to meet their needs as well.
$ cat /sys/devices/system/cpu/cpufreq/policy0/energy_performance_available_preferences
default performance balance_performance balance_power power
$ cat /sys/devices/system/cpu/cpufreq/policy0/energy_performance_preference
balance_performance
To enable the driver,it needs to add `amd_pstate=active` to kernel
command line and kernel will load the active mode epp driver
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
drivers/cpufreq/amd-pstate.c | 447 ++++++++++++++++++++++++++++++++++-
include/linux/amd-pstate.h | 10 +
2 files changed, 451 insertions(+), 6 deletions(-)
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 861a905f9324..66b39457a312 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -59,7 +59,10 @@
* we disable it by default to go acpi-cpufreq on these processors and add a
* module parameter to be able to enable it manually for debugging.
*/
+static struct cpufreq_driver *default_pstate_driver;
static struct cpufreq_driver amd_pstate_driver;
+static struct cpufreq_driver amd_pstate_epp_driver;
+static struct amd_cpudata **all_cpu_data;
static int cppc_state = AMD_PSTATE_DISABLE;
static inline int get_mode_idx_from_str(const char *str, size_t size)
@@ -70,9 +73,128 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
if (!strncmp(str, amd_pstate_mode_string[i], size))
return i;
}
+
return -EINVAL;
}
+/**
+ * struct amd_pstate_params - global parameters for the performance control
+ * @ cppc_boost_disabled wheher the core performance boost disabled
+ */
+struct amd_pstate_params {
+ bool cppc_boost_disabled;
+};
+
+static struct amd_pstate_params global_params;
+
+static DEFINE_MUTEX(amd_pstate_limits_lock);
+static DEFINE_MUTEX(amd_pstate_driver_lock);
+
+static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
+{
+ u64 epp;
+ int ret;
+
+ if (boot_cpu_has(X86_FEATURE_CPPC)) {
+ if (!cppc_req_cached) {
+ epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
+ &cppc_req_cached);
+ if (epp)
+ return epp;
+ }
+ epp = (cppc_req_cached >> 24) & 0xFF;
+ } else {
+ ret = cppc_get_epp_perf(cpudata->cpu, &epp);
+ if (ret < 0) {
+ pr_debug("Could not retrieve energy perf value (%d)\n", ret);
+ return -EIO;
+ }
+ }
+
+ return (s16)(epp & 0xff);
+}
+
+static int amd_pstate_get_energy_pref_index(struct amd_cpudata *cpudata)
+{
+ s16 epp;
+ int index = -EINVAL;
+
+ epp = amd_pstate_get_epp(cpudata, 0);
+ if (epp < 0)
+ return epp;
+
+ switch (epp) {
+ case HWP_EPP_PERFORMANCE:
+ index = EPP_INDEX_PERFORMANCE;
+ break;
+ case HWP_EPP_BALANCE_PERFORMANCE:
+ index = EPP_INDEX_BALANCE_PERFORMANCE;
+ break;
+ case HWP_EPP_BALANCE_POWERSAVE:
+ index = EPP_INDEX_BALANCE_POWERSAVE;
+ break;
+ case HWP_EPP_POWERSAVE:
+ index = EPP_INDEX_POWERSAVE;
+ break;
+ default:
+ break;
+ }
+
+ return index;
+}
+
+static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
+{
+ int ret;
+ struct cppc_perf_ctrls perf_ctrls;
+
+ if (boot_cpu_has(X86_FEATURE_CPPC)) {
+ u64 value = READ_ONCE(cpudata->cppc_req_cached);
+
+ value &= ~GENMASK_ULL(31, 24);
+ value |= (u64)epp << 24;
+ WRITE_ONCE(cpudata->cppc_req_cached, value);
+
+ ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+ if (!ret)
+ cpudata->epp_cached = epp;
+ } else {
+ perf_ctrls.energy_perf = epp;
+ ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
+ if (ret) {
+ pr_debug("failed to set energy perf value (%d)\n", ret);
+ return ret;
+ }
+ cpudata->epp_cached = epp;
+ }
+
+ return ret;
+}
+
+static int amd_pstate_set_energy_pref_index(struct amd_cpudata *cpudata,
+ int pref_index)
+{
+ int epp = -EINVAL;
+ int ret;
+
+ if (!pref_index) {
+ pr_debug("EPP pref_index is invalid\n");
+ return -EINVAL;
+ }
+
+ if (epp == -EINVAL)
+ epp = epp_values[pref_index];
+
+ if (epp > 0 && cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
+ pr_debug("EPP cannot be set under performance policy\n");
+ return -EBUSY;
+ }
+
+ ret = amd_pstate_set_epp(cpudata, epp);
+
+ return ret;
+}
+
static inline int pstate_enable(bool enable)
{
return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable);
@@ -81,11 +203,21 @@ static inline int pstate_enable(bool enable)
static int cppc_enable(bool enable)
{
int cpu, ret = 0;
+ struct cppc_perf_ctrls perf_ctrls;
for_each_present_cpu(cpu) {
ret = cppc_set_enable(cpu, enable);
if (ret)
return ret;
+
+ /* Enable autonomous mode for EPP */
+ if (cppc_state == AMD_PSTATE_ACTIVE) {
+ /* Set desired perf as zero to allow EPP firmware control */
+ perf_ctrls.desired_perf = 0;
+ ret = cppc_set_perf(cpu, &perf_ctrls);
+ if (ret)
+ return ret;
+ }
}
return ret;
@@ -429,7 +561,7 @@ static void amd_pstate_boost_init(struct amd_cpudata *cpudata)
return;
cpudata->boost_supported = true;
- amd_pstate_driver.boost_enabled = true;
+ default_pstate_driver->boost_enabled = true;
}
static void amd_perf_ctl_reset(unsigned int cpu)
@@ -603,10 +735,61 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
return sprintf(&buf[0], "%u\n", perf);
}
+static ssize_t show_energy_performance_available_preferences(
+ struct cpufreq_policy *policy, char *buf)
+{
+ int i = 0;
+ int offset = 0;
+
+ while (energy_perf_strings[i] != NULL)
+ offset += sysfs_emit_at(buf, offset, "%s ", energy_perf_strings[i++]);
+
+ sysfs_emit_at(buf, offset, "\n");
+
+ return offset;
+}
+
+static ssize_t store_energy_performance_preference(
+ struct cpufreq_policy *policy, const char *buf, size_t count)
+{
+ struct amd_cpudata *cpudata = policy->driver_data;
+ char str_preference[21];
+ ssize_t ret;
+
+ ret = sscanf(buf, "%20s", str_preference);
+ if (ret != 1)
+ return -EINVAL;
+
+ ret = match_string(energy_perf_strings, -1, str_preference);
+ if (ret < 0)
+ return -EINVAL;
+
+ mutex_lock(&amd_pstate_limits_lock);
+ ret = amd_pstate_set_energy_pref_index(cpudata, ret);
+ mutex_unlock(&amd_pstate_limits_lock);
+
+ return ret ?: count;
+}
+
+static ssize_t show_energy_performance_preference(
+ struct cpufreq_policy *policy, char *buf)
+{
+ struct amd_cpudata *cpudata = policy->driver_data;
+ int preference;
+
+ preference = amd_pstate_get_energy_pref_index(cpudata);
+ if (preference < 0)
+ return preference;
+
+ return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
+}
+
cpufreq_freq_attr_ro(amd_pstate_max_freq);
cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
cpufreq_freq_attr_ro(amd_pstate_highest_perf);
+cpufreq_freq_attr_rw(energy_performance_preference);
+cpufreq_freq_attr_ro(energy_performance_available_preferences);
static struct freq_attr *amd_pstate_attr[] = {
&amd_pstate_max_freq,
@@ -615,6 +798,235 @@ static struct freq_attr *amd_pstate_attr[] = {
NULL,
};
+static struct freq_attr *amd_pstate_epp_attr[] = {
+ &amd_pstate_max_freq,
+ &amd_pstate_lowest_nonlinear_freq,
+ &amd_pstate_highest_perf,
+ &energy_performance_preference,
+ &energy_performance_available_preferences,
+ NULL,
+};
+
+static inline void update_boost_state(void)
+{
+ u64 misc_en;
+ struct amd_cpudata *cpudata;
+
+ cpudata = all_cpu_data[0];
+ rdmsrl(MSR_K7_HWCR, misc_en);
+ global_params.cppc_boost_disabled = misc_en & BIT_ULL(25);
+}
+
+static int amd_pstate_init_cpu(unsigned int cpunum)
+{
+ struct amd_cpudata *cpudata;
+
+ cpudata = all_cpu_data[cpunum];
+ if (!cpudata) {
+ cpudata = kzalloc(sizeof(*cpudata), GFP_KERNEL);
+ if (!cpudata)
+ return -ENOMEM;
+ WRITE_ONCE(all_cpu_data[cpunum], cpudata);
+
+ cpudata->cpu = cpunum;
+ }
+
+ cpudata->epp_policy = 0;
+ pr_debug("controlling: cpu %d\n", cpunum);
+ return 0;
+}
+
+static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
+{
+ int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret;
+ struct amd_cpudata *cpudata;
+ struct device *dev;
+ int rc;
+ u64 value;
+
+ rc = amd_pstate_init_cpu(policy->cpu);
+ if (rc)
+ return rc;
+
+ cpudata = all_cpu_data[policy->cpu];
+
+ dev = get_cpu_device(policy->cpu);
+ if (!dev)
+ goto free_cpudata1;
+
+ rc = amd_pstate_init_perf(cpudata);
+ if (rc)
+ goto free_cpudata1;
+
+ min_freq = amd_get_min_freq(cpudata);
+ max_freq = amd_get_max_freq(cpudata);
+ nominal_freq = amd_get_nominal_freq(cpudata);
+ lowest_nonlinear_freq = amd_get_lowest_nonlinear_freq(cpudata);
+ if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) {
+ dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n",
+ min_freq, max_freq);
+ ret = -EINVAL;
+ goto free_cpudata1;
+ }
+
+ policy->min = min_freq;
+ policy->max = max_freq;
+
+ policy->cpuinfo.min_freq = min_freq;
+ policy->cpuinfo.max_freq = max_freq;
+ /* It will be updated by governor */
+ policy->cur = policy->cpuinfo.min_freq;
+
+ /* Initial processor data capability frequencies */
+ cpudata->max_freq = max_freq;
+ cpudata->min_freq = min_freq;
+ cpudata->nominal_freq = nominal_freq;
+ cpudata->lowest_nonlinear_freq = lowest_nonlinear_freq;
+
+ policy->driver_data = cpudata;
+
+ cpudata->epp_cached = amd_pstate_get_epp(cpudata, value);
+
+ policy->min = policy->cpuinfo.min_freq;
+ policy->max = policy->cpuinfo.max_freq;
+
+ /*
+ * Set the policy to powersave to provide a valid fallback value in case
+ * the default cpufreq governor is neither powersave nor performance.
+ */
+ policy->policy = CPUFREQ_POLICY_POWERSAVE;
+
+ if (boot_cpu_has(X86_FEATURE_CPPC)) {
+ policy->fast_switch_possible = true;
+ ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value);
+ if (ret)
+ return ret;
+ WRITE_ONCE(cpudata->cppc_req_cached, value);
+
+ ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1, &value);
+ if (ret)
+ return ret;
+ WRITE_ONCE(cpudata->cppc_cap1_cached, value);
+ }
+ amd_pstate_boost_init(cpudata);
+
+ return 0;
+
+free_cpudata1:
+ kfree(cpudata);
+ return ret;
+}
+
+static int amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
+{
+ pr_debug("CPU %d exiting\n", policy->cpu);
+ policy->fast_switch_possible = false;
+ return 0;
+}
+
+static void amd_pstate_update_max_freq(unsigned int cpu)
+{
+ struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+
+ if (!policy)
+ return;
+
+ refresh_frequency_limits(policy);
+ cpufreq_cpu_put(policy);
+}
+
+static void amd_pstate_epp_update_limits(unsigned int cpu)
+{
+ mutex_lock(&amd_pstate_driver_lock);
+ update_boost_state();
+ if (global_params.cppc_boost_disabled) {
+ for_each_possible_cpu(cpu)
+ amd_pstate_update_max_freq(cpu);
+ } else {
+ cpufreq_update_policy(cpu);
+ }
+ mutex_unlock(&amd_pstate_driver_lock);
+}
+
+static void amd_pstate_epp_init(unsigned int cpu)
+{
+ struct amd_cpudata *cpudata = all_cpu_data[cpu];
+ u32 max_perf, min_perf;
+ u64 value;
+ s16 epp;
+
+ max_perf = READ_ONCE(cpudata->highest_perf);
+ min_perf = READ_ONCE(cpudata->lowest_perf);
+
+ value = READ_ONCE(cpudata->cppc_req_cached);
+
+ if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
+ min_perf = max_perf;
+
+ /* Initial min/max values for CPPC Performance Controls Register */
+ value &= ~AMD_CPPC_MIN_PERF(~0L);
+ value |= AMD_CPPC_MIN_PERF(min_perf);
+
+ value &= ~AMD_CPPC_MAX_PERF(~0L);
+ value |= AMD_CPPC_MAX_PERF(max_perf);
+
+ /* CPPC EPP feature require to set zero to the desire perf bit */
+ value &= ~AMD_CPPC_DES_PERF(~0L);
+ value |= AMD_CPPC_DES_PERF(0);
+
+ if (cpudata->epp_policy == cpudata->policy)
+ goto skip_epp;
+
+ cpudata->epp_policy = cpudata->policy;
+
+ if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
+ epp = amd_pstate_get_epp(cpudata, value);
+ if (epp < 0)
+ goto skip_epp;
+ /* force the epp value to be zero for performance policy */
+ epp = 0;
+ } else {
+ /* Get BIOS pre-defined epp value */
+ epp = amd_pstate_get_epp(cpudata, value);
+ if (epp)
+ goto skip_epp;
+ }
+ /* Set initial EPP value */
+ if (boot_cpu_has(X86_FEATURE_CPPC)) {
+ value &= ~GENMASK_ULL(31, 24);
+ value |= (u64)epp << 24;
+ }
+
+skip_epp:
+ WRITE_ONCE(cpudata->cppc_req_cached, value);
+ amd_pstate_set_epp(cpudata, epp);
+}
+
+static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
+{
+ struct amd_cpudata *cpudata;
+
+ if (!policy->cpuinfo.max_freq)
+ return -ENODEV;
+
+ pr_debug("set_policy: cpuinfo.max %u policy->max %u\n",
+ policy->cpuinfo.max_freq, policy->max);
+
+ cpudata = all_cpu_data[policy->cpu];
+ cpudata->policy = policy->policy;
+
+ amd_pstate_epp_init(policy->cpu);
+
+ return 0;
+}
+
+static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
+{
+ cpufreq_verify_within_cpu_limits(policy);
+ pr_debug("policy_max =%d, policy_min=%d\n", policy->max, policy->min);
+ return 0;
+}
+
static struct cpufreq_driver amd_pstate_driver = {
.flags = CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
.verify = amd_pstate_verify,
@@ -628,8 +1040,20 @@ static struct cpufreq_driver amd_pstate_driver = {
.attr = amd_pstate_attr,
};
+static struct cpufreq_driver amd_pstate_epp_driver = {
+ .flags = CPUFREQ_CONST_LOOPS,
+ .verify = amd_pstate_epp_verify_policy,
+ .setpolicy = amd_pstate_epp_set_policy,
+ .init = amd_pstate_epp_cpu_init,
+ .exit = amd_pstate_epp_cpu_exit,
+ .update_limits = amd_pstate_epp_update_limits,
+ .name = "amd_pstate_epp",
+ .attr = amd_pstate_epp_attr,
+};
+
static int __init amd_pstate_init(void)
{
+ static struct amd_cpudata **cpudata;
int ret;
if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
@@ -656,7 +1080,8 @@ static int __init amd_pstate_init(void)
/* capability check */
if (boot_cpu_has(X86_FEATURE_CPPC)) {
pr_debug("AMD CPPC MSR based functionality is supported\n");
- amd_pstate_driver.adjust_perf = amd_pstate_adjust_perf;
+ if (cppc_state == AMD_PSTATE_PASSIVE)
+ default_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
} else {
pr_debug("AMD CPPC shared memory based functionality is supported\n");
static_call_update(amd_pstate_enable, cppc_enable);
@@ -664,17 +1089,21 @@ static int __init amd_pstate_init(void)
static_call_update(amd_pstate_update_perf, cppc_update_perf);
}
+ cpudata = vzalloc(array_size(sizeof(void *), num_possible_cpus()));
+ if (!cpudata)
+ return -ENOMEM;
+ WRITE_ONCE(all_cpu_data, cpudata);
+
/* enable amd pstate feature */
ret = amd_pstate_enable(true);
if (ret) {
- pr_err("failed to enable amd-pstate with return %d\n", ret);
+ pr_err("failed to enable with return %d\n", ret);
return ret;
}
- ret = cpufreq_register_driver(&amd_pstate_driver);
+ ret = cpufreq_register_driver(default_pstate_driver);
if (ret)
- pr_err("failed to register amd_pstate_driver with return %d\n",
- ret);
+ pr_err("failed to register with return %d\n", ret);
return ret;
}
@@ -696,6 +1125,12 @@ static int __init amd_pstate_param(char *str)
if (cppc_state == AMD_PSTATE_DISABLE)
pr_info("driver is explicitly disabled\n");
+ if (cppc_state == AMD_PSTATE_ACTIVE)
+ default_pstate_driver = &amd_pstate_epp_driver;
+
+ if (cppc_state == AMD_PSTATE_PASSIVE)
+ default_pstate_driver = &amd_pstate_driver;
+
return 0;
}
diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
index 922d05a13902..fe1aef743c09 100644
--- a/include/linux/amd-pstate.h
+++ b/include/linux/amd-pstate.h
@@ -47,6 +47,10 @@ struct amd_aperf_mperf {
* @prev: Last Aperf/Mperf/tsc count value read from register
* @freq: current cpu frequency value
* @boost_supported: check whether the Processor or SBIOS supports boost mode
+ * @epp_policy: Last saved policy used to set energy-performance preference
+ * @epp_cached: Cached CPPC energy-performance preference value
+ * @policy: Cpufreq policy value
+ * @cppc_cap1_cached Cached MSR_AMD_CPPC_CAP1 register value
*
* The amd_cpudata is key private data for each CPU thread in AMD P-State, and
* represents all the attributes and goals that AMD P-State requests at runtime.
@@ -72,6 +76,12 @@ struct amd_cpudata {
u64 freq;
bool boost_supported;
+
+ /* EPP feature related attributes*/
+ s16 epp_policy;
+ s16 epp_cached;
+ u32 policy;
+ u64 cppc_cap1_cached;
};
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 06/13] cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors
2022-12-19 6:40 ` [PATCH v8 06/13] cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors Perry Yuan
@ 2022-12-20 3:09 ` Mario Limonciello
2022-12-23 7:43 ` Huang Rui
1 sibling, 0 replies; 45+ messages in thread
From: Mario Limonciello @ 2022-12-20 3:09 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/22 00:40, Perry Yuan wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
>
> Add EPP driver support for AMD SoCs which support a dedicated MSR for
> CPPC. EPP is used by the DPM controller to configure the frequency that
> a core operates at during short periods of activity.
>
> The SoC EPP targets are configured on a scale from 0 to 255 where 0
> represents maximum performance and 255 represents maximum efficiency.
>
> The amd-pstate driver exports profile string names to userspace that are
> tied to specific EPP values.
>
> The balance_performance string (0x80) provides the best balance for
> efficiency versus power on most systems, but users can choose other
> strings to meet their needs as well.
>
> $ cat /sys/devices/system/cpu/cpufreq/policy0/energy_performance_available_preferences
> default performance balance_performance balance_power power
>
> $ cat /sys/devices/system/cpu/cpufreq/policy0/energy_performance_preference
> balance_performance
>
> To enable the driver,it needs to add `amd_pstate=active` to kernel
> command line and kernel will load the active mode epp driver
>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 447 ++++++++++++++++++++++++++++++++++-
> include/linux/amd-pstate.h | 10 +
> 2 files changed, 451 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 861a905f9324..66b39457a312 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -59,7 +59,10 @@
> * we disable it by default to go acpi-cpufreq on these processors and add a
> * module parameter to be able to enable it manually for debugging.
> */
> +static struct cpufreq_driver *default_pstate_driver;
> static struct cpufreq_driver amd_pstate_driver;
> +static struct cpufreq_driver amd_pstate_epp_driver;
> +static struct amd_cpudata **all_cpu_data;
> static int cppc_state = AMD_PSTATE_DISABLE;
>
> static inline int get_mode_idx_from_str(const char *str, size_t size)
> @@ -70,9 +73,128 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
> if (!strncmp(str, amd_pstate_mode_string[i], size))
> return i;
> }
> +
Unrelated whitespace change.
> return -EINVAL;
> }
>
> +/**
> + * struct amd_pstate_params - global parameters for the performance control
> + * @ cppc_boost_disabled wheher the core performance boost disabled
> + */
> +struct amd_pstate_params {
> + bool cppc_boost_disabled;
> +};
> +
> +static struct amd_pstate_params global_params;
> +
> +static DEFINE_MUTEX(amd_pstate_limits_lock);
> +static DEFINE_MUTEX(amd_pstate_driver_lock);
> +
> +static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
> +{
> + u64 epp;
> + int ret;
> +
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + if (!cppc_req_cached) {
> + epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
> + &cppc_req_cached);
> + if (epp)
> + return epp;
> + }
> + epp = (cppc_req_cached >> 24) & 0xFF;
> + } else {
> + ret = cppc_get_epp_perf(cpudata->cpu, &epp);
> + if (ret < 0) {
> + pr_debug("Could not retrieve energy perf value (%d)\n", ret);
> + return -EIO;
> + }
> + }
> +
> + return (s16)(epp & 0xff); > +}
> +
> +static int amd_pstate_get_energy_pref_index(struct amd_cpudata *cpudata)
> +{
> + s16 epp;
> + int index = -EINVAL;
> +
> + epp = amd_pstate_get_epp(cpudata, 0);
> + if (epp < 0)
> + return epp;
> +
> + switch (epp) {
> + case HWP_EPP_PERFORMANCE:
> + index = EPP_INDEX_PERFORMANCE;
> + break;
> + case HWP_EPP_BALANCE_PERFORMANCE:
> + index = EPP_INDEX_BALANCE_PERFORMANCE;
> + break;
> + case HWP_EPP_BALANCE_POWERSAVE:
> + index = EPP_INDEX_BALANCE_POWERSAVE;
> + break;
> + case HWP_EPP_POWERSAVE:
> + index = EPP_INDEX_POWERSAVE;
> + break;
> + default:
> + break;
Extra tab here
> + }
> +
> + return index;
> +}
> +
> +static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +{
> + int ret;
> + struct cppc_perf_ctrls perf_ctrls;
> +
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + u64 value = READ_ONCE(cpudata->cppc_req_cached);
> +
> + value &= ~GENMASK_ULL(31, 24);
> + value |= (u64)epp << 24;
> + WRITE_ONCE(cpudata->cppc_req_cached, value);
> +
> + ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> + if (!ret)
> + cpudata->epp_cached = epp;
> + } else {
> + perf_ctrls.energy_perf = epp;
> + ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
> + if (ret) {
> + pr_debug("failed to set energy perf value (%d)\n", ret);
> + return ret;
> + }
> + cpudata->epp_cached = epp;
> + }
> +
> + return ret;
> +}
> +
> +static int amd_pstate_set_energy_pref_index(struct amd_cpudata *cpudata,
> + int pref_index)
> +{
> + int epp = -EINVAL;
> + int ret;
> +
> + if (!pref_index) {
> + pr_debug("EPP pref_index is invalid\n");
> + return -EINVAL > + }
> +
> + if (epp == -EINVAL)
> + epp = epp_values[pref_index];
Didn't you just hardcode epp to -EINVAL at the beginning of function?
> +
> + if (epp > 0 && cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
> + pr_debug("EPP cannot be set under performance policy\n");
> + return -EBUSY;
> + }
> +
> + ret = amd_pstate_set_epp(cpudata, epp);
> +
> + return ret;
> +}
> +
> static inline int pstate_enable(bool enable)
> {
> return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable);
> @@ -81,11 +203,21 @@ static inline int pstate_enable(bool enable)
> static int cppc_enable(bool enable)
> {
> int cpu, ret = 0;
> + struct cppc_perf_ctrls perf_ctrls;
>
> for_each_present_cpu(cpu) {
> ret = cppc_set_enable(cpu, enable);
> if (ret)
> return ret;
> +
> + /* Enable autonomous mode for EPP */
> + if (cppc_state == AMD_PSTATE_ACTIVE) {
> + /* Set desired perf as zero to allow EPP firmware control */
> + perf_ctrls.desired_perf = 0;
> + ret = cppc_set_perf(cpu, &perf_ctrls);
> + if (ret)
> + return ret;
> + }
> }
>
> return ret;
> @@ -429,7 +561,7 @@ static void amd_pstate_boost_init(struct amd_cpudata *cpudata)
> return;
>
> cpudata->boost_supported = true;
> - amd_pstate_driver.boost_enabled = true;
> + default_pstate_driver->boost_enabled = true;
> }
>
> static void amd_perf_ctl_reset(unsigned int cpu)
> @@ -603,10 +735,61 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
> return sprintf(&buf[0], "%u\n", perf);
> }
>
> +static ssize_t show_energy_performance_available_preferences(
> + struct cpufreq_policy *policy, char *buf)
> +{
> + int i = 0;
> + int offset = 0;
> +
> + while (energy_perf_strings[i] != NULL)
> + offset += sysfs_emit_at(buf, offset, "%s ", energy_perf_strings[i++]);
> +
> + sysfs_emit_at(buf, offset, "\n");
> +
> + return offset;
> +}
> +
> +static ssize_t store_energy_performance_preference(
> + struct cpufreq_policy *policy, const char *buf, size_t count)
> +{
> + struct amd_cpudata *cpudata = policy->driver_data;
> + char str_preference[21];
> + ssize_t ret;
> +
> + ret = sscanf(buf, "%20s", str_preference);
> + if (ret != 1)
> + return -EINVAL;
> +
> + ret = match_string(energy_perf_strings, -1, str_preference);
> + if (ret < 0)
> + return -EINVAL;
> +
> + mutex_lock(&amd_pstate_limits_lock);
> + ret = amd_pstate_set_energy_pref_index(cpudata, ret);
> + mutex_unlock(&amd_pstate_limits_lock);
> +
> + return ret ?: count;
> +}
> +
> +static ssize_t show_energy_performance_preference(
> + struct cpufreq_policy *policy, char *buf)
> +{
> + struct amd_cpudata *cpudata = policy->driver_data;
> + int preference;
> +
> + preference = amd_pstate_get_energy_pref_index(cpudata);
> + if (preference < 0)
> + return preference;
> +
> + return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
> +}
> +
> cpufreq_freq_attr_ro(amd_pstate_max_freq);
> cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
>
> cpufreq_freq_attr_ro(amd_pstate_highest_perf);
> +cpufreq_freq_attr_rw(energy_performance_preference);
> +cpufreq_freq_attr_ro(energy_performance_available_preferences);
>
> static struct freq_attr *amd_pstate_attr[] = {
> &amd_pstate_max_freq,
> @@ -615,6 +798,235 @@ static struct freq_attr *amd_pstate_attr[] = {
> NULL,
> };
>
> +static struct freq_attr *amd_pstate_epp_attr[] = {
> + &amd_pstate_max_freq,
> + &amd_pstate_lowest_nonlinear_freq,
> + &amd_pstate_highest_perf,
> + &energy_performance_preference,
> + &energy_performance_available_preferences,
> + NULL,
> +};
> +
> +static inline void update_boost_state(void)
> +{
> + u64 misc_en;
> + struct amd_cpudata *cpudata;
> +
> + cpudata = all_cpu_data[0];
> + rdmsrl(MSR_K7_HWCR, misc_en);
> + global_params.cppc_boost_disabled = misc_en & BIT_ULL(25);
> +}
> +
> +static int amd_pstate_init_cpu(unsigned int cpunum)
> +{
> + struct amd_cpudata *cpudata;
> +
> + cpudata = all_cpu_data[cpunum];
> + if (!cpudata) {
> + cpudata = kzalloc(sizeof(*cpudata), GFP_KERNEL);
> + if (!cpudata)
> + return -ENOMEM;
> + WRITE_ONCE(all_cpu_data[cpunum], cpudata);
> +
> + cpudata->cpu = cpunum;
> + }
> +
> + cpudata->epp_policy = 0;
> + pr_debug("controlling: cpu %d\n", cpunum);
> + return 0;
> +}
> +
> +static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
> +{
> + int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret;
> + struct amd_cpudata *cpudata;
> + struct device *dev;
> + int rc;
> + u64 value;
> +
> + rc = amd_pstate_init_cpu(policy->cpu);
> + if (rc)
> + return rc;
> +
> + cpudata = all_cpu_data[policy->cpu];
> +
> + dev = get_cpu_device(policy->cpu);
> + if (!dev)
> + goto free_cpudata1;
> +
> + rc = amd_pstate_init_perf(cpudata);
> + if (rc)
> + goto free_cpudata1;
> +
> + min_freq = amd_get_min_freq(cpudata);
> + max_freq = amd_get_max_freq(cpudata);
> + nominal_freq = amd_get_nominal_freq(cpudata);
> + lowest_nonlinear_freq = amd_get_lowest_nonlinear_freq(cpudata);
> + if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) {
> + dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n",
> + min_freq, max_freq);
> + ret = -EINVAL;
> + goto free_cpudata1;
> + }
> +
> + policy->min = min_freq;
> + policy->max = max_freq;
> +
> + policy->cpuinfo.min_freq = min_freq;
> + policy->cpuinfo.max_freq = max_freq;
> + /* It will be updated by governor */
> + policy->cur = policy->cpuinfo.min_freq;
> +
> + /* Initial processor data capability frequencies */
> + cpudata->max_freq = max_freq;
> + cpudata->min_freq = min_freq;
> + cpudata->nominal_freq = nominal_freq;
> + cpudata->lowest_nonlinear_freq = lowest_nonlinear_freq;
> +
> + policy->driver_data = cpudata;
> +
> + cpudata->epp_cached = amd_pstate_get_epp(cpudata, value);
> +
> + policy->min = policy->cpuinfo.min_freq;
> + policy->max = policy->cpuinfo.max_freq;
> +
> + /*
> + * Set the policy to powersave to provide a valid fallback value in case
> + * the default cpufreq governor is neither powersave nor performance.
> + */
> + policy->policy = CPUFREQ_POLICY_POWERSAVE;
> +
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + policy->fast_switch_possible = true;
> + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value);
> + if (ret)
> + return ret;
> + WRITE_ONCE(cpudata->cppc_req_cached, value);
> +
> + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1, &value);
> + if (ret)
> + return ret;
> + WRITE_ONCE(cpudata->cppc_cap1_cached, value);
> + }
> + amd_pstate_boost_init(cpudata);
> +
> + return 0;
> +
> +free_cpudata1:
> + kfree(cpudata);
> + return ret;
> +}
> +
> +static int amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
> +{
> + pr_debug("CPU %d exiting\n", policy->cpu);
> + policy->fast_switch_possible = false;
> + return 0;
> +}
> +
> +static void amd_pstate_update_max_freq(unsigned int cpu)
> +{
> + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +
> + if (!policy)
> + return;
> +
> + refresh_frequency_limits(policy);
> + cpufreq_cpu_put(policy);
> +}
> +
> +static void amd_pstate_epp_update_limits(unsigned int cpu)
> +{
> + mutex_lock(&amd_pstate_driver_lock);
> + update_boost_state();
> + if (global_params.cppc_boost_disabled) {
> + for_each_possible_cpu(cpu)
> + amd_pstate_update_max_freq(cpu);
> + } else {
> + cpufreq_update_policy(cpu);
> + }
> + mutex_unlock(&amd_pstate_driver_lock);
> +}
> +
> +static void amd_pstate_epp_init(unsigned int cpu)
> +{
> + struct amd_cpudata *cpudata = all_cpu_data[cpu];
> + u32 max_perf, min_perf;
> + u64 value;
> + s16 epp;
> +
> + max_perf = READ_ONCE(cpudata->highest_perf);
> + min_perf = READ_ONCE(cpudata->lowest_perf);
> +
> + value = READ_ONCE(cpudata->cppc_req_cached);
> +
> + if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
> + min_perf = max_perf;
> +
> + /* Initial min/max values for CPPC Performance Controls Register */
> + value &= ~AMD_CPPC_MIN_PERF(~0L);
> + value |= AMD_CPPC_MIN_PERF(min_perf);
> +
> + value &= ~AMD_CPPC_MAX_PERF(~0L);
> + value |= AMD_CPPC_MAX_PERF(max_perf);
> +
> + /* CPPC EPP feature require to set zero to the desire perf bit */
> + value &= ~AMD_CPPC_DES_PERF(~0L);
> + value |= AMD_CPPC_DES_PERF(0);
> +
> + if (cpudata->epp_policy == cpudata->policy)
> + goto skip_epp;
> +
> + cpudata->epp_policy = cpudata->policy;
> +
> + if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
> + epp = amd_pstate_get_epp(cpudata, value);
> + if (epp < 0)
> + goto skip_epp;
> + /* force the epp value to be zero for performance policy */
> + epp = 0;
> + } else {
> + /* Get BIOS pre-defined epp value */
> + epp = amd_pstate_get_epp(cpudata, value);
> + if (epp)
> + goto skip_epp;
> + }
> + /* Set initial EPP value */
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + value &= ~GENMASK_ULL(31, 24);
> + value |= (u64)epp << 24;
> + }
> +
> +skip_epp:
> + WRITE_ONCE(cpudata->cppc_req_cached, value);
> + amd_pstate_set_epp(cpudata, epp);
> +}
> +
> +static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
> +{
> + struct amd_cpudata *cpudata;
> +
> + if (!policy->cpuinfo.max_freq)
> + return -ENODEV;
> +
> + pr_debug("set_policy: cpuinfo.max %u policy->max %u\n",
> + policy->cpuinfo.max_freq, policy->max);
> +
> + cpudata = all_cpu_data[policy->cpu];
> + cpudata->policy = policy->policy;
> +
> + amd_pstate_epp_init(policy->cpu);
> +
> + return 0;
> +}
> +
> +static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
> +{
> + cpufreq_verify_within_cpu_limits(policy);
> + pr_debug("policy_max =%d, policy_min=%d\n", policy->max, policy->min);
> + return 0;
> +}
> +
> static struct cpufreq_driver amd_pstate_driver = {
> .flags = CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
> .verify = amd_pstate_verify,
> @@ -628,8 +1040,20 @@ static struct cpufreq_driver amd_pstate_driver = {
> .attr = amd_pstate_attr,
> };
>
> +static struct cpufreq_driver amd_pstate_epp_driver = {
> + .flags = CPUFREQ_CONST_LOOPS,
> + .verify = amd_pstate_epp_verify_policy,
> + .setpolicy = amd_pstate_epp_set_policy,
> + .init = amd_pstate_epp_cpu_init,
> + .exit = amd_pstate_epp_cpu_exit,
> + .update_limits = amd_pstate_epp_update_limits,
> + .name = "amd_pstate_epp",
> + .attr = amd_pstate_epp_attr,
> +};
> +
> static int __init amd_pstate_init(void)
> {
> + static struct amd_cpudata **cpudata;
> int ret;
>
> if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> @@ -656,7 +1080,8 @@ static int __init amd_pstate_init(void)
> /* capability check */
> if (boot_cpu_has(X86_FEATURE_CPPC)) {
> pr_debug("AMD CPPC MSR based functionality is supported\n");
> - amd_pstate_driver.adjust_perf = amd_pstate_adjust_perf;
> + if (cppc_state == AMD_PSTATE_PASSIVE)
> + default_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
> } else {
> pr_debug("AMD CPPC shared memory based functionality is supported\n");
> static_call_update(amd_pstate_enable, cppc_enable);
> @@ -664,17 +1089,21 @@ static int __init amd_pstate_init(void)
> static_call_update(amd_pstate_update_perf, cppc_update_perf);
> }
>
> + cpudata = vzalloc(array_size(sizeof(void *), num_possible_cpus()));
> + if (!cpudata)
> + return -ENOMEM;
> + WRITE_ONCE(all_cpu_data, cpudata);
> +
> /* enable amd pstate feature */
> ret = amd_pstate_enable(true);
> if (ret) {
> - pr_err("failed to enable amd-pstate with return %d\n", ret);
> + pr_err("failed to enable with return %d\n", ret);
> return ret;
> }
>
> - ret = cpufreq_register_driver(&amd_pstate_driver);
> + ret = cpufreq_register_driver(default_pstate_driver);
> if (ret)
> - pr_err("failed to register amd_pstate_driver with return %d\n",
> - ret);
> + pr_err("failed to register with return %d\n", ret);
>
> return ret;
> }
> @@ -696,6 +1125,12 @@ static int __init amd_pstate_param(char *str)
> if (cppc_state == AMD_PSTATE_DISABLE)
> pr_info("driver is explicitly disabled\n");
>
> + if (cppc_state == AMD_PSTATE_ACTIVE)
> + default_pstate_driver = &amd_pstate_epp_driver;
> +
> + if (cppc_state == AMD_PSTATE_PASSIVE)
> + default_pstate_driver = &amd_pstate_driver;
> +
> return 0;
> }
>
> diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> index 922d05a13902..fe1aef743c09 100644
> --- a/include/linux/amd-pstate.h
> +++ b/include/linux/amd-pstate.h
> @@ -47,6 +47,10 @@ struct amd_aperf_mperf {
> * @prev: Last Aperf/Mperf/tsc count value read from register
> * @freq: current cpu frequency value
> * @boost_supported: check whether the Processor or SBIOS supports boost mode
> + * @epp_policy: Last saved policy used to set energy-performance preference
> + * @epp_cached: Cached CPPC energy-performance preference value
> + * @policy: Cpufreq policy value
> + * @cppc_cap1_cached Cached MSR_AMD_CPPC_CAP1 register value
> *
> * The amd_cpudata is key private data for each CPU thread in AMD P-State, and
> * represents all the attributes and goals that AMD P-State requests at runtime.
> @@ -72,6 +76,12 @@ struct amd_cpudata {
>
> u64 freq;
> bool boost_supported;
> +
> + /* EPP feature related attributes*/
> + s16 epp_policy;
> + s16 epp_cached;
> + u32 policy;
> + u64 cppc_cap1_cached;
> };
>
> /**
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 06/13] cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors
2022-12-19 6:40 ` [PATCH v8 06/13] cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors Perry Yuan
2022-12-20 3:09 ` Mario Limonciello
@ 2022-12-23 7:43 ` Huang Rui
2022-12-23 7:52 ` Yuan, Perry
1 sibling, 1 reply; 45+ messages in thread
From: Huang Rui @ 2022-12-23 7:43 UTC (permalink / raw)
To: Yuan, Perry
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On Mon, Dec 19, 2022 at 02:40:35PM +0800, Yuan, Perry wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
>
> Add EPP driver support for AMD SoCs which support a dedicated MSR for
> CPPC. EPP is used by the DPM controller to configure the frequency that
> a core operates at during short periods of activity.
>
> The SoC EPP targets are configured on a scale from 0 to 255 where 0
> represents maximum performance and 255 represents maximum efficiency.
>
> The amd-pstate driver exports profile string names to userspace that are
> tied to specific EPP values.
>
> The balance_performance string (0x80) provides the best balance for
> efficiency versus power on most systems, but users can choose other
> strings to meet their needs as well.
>
> $ cat /sys/devices/system/cpu/cpufreq/policy0/energy_performance_available_preferences
> default performance balance_performance balance_power power
>
> $ cat /sys/devices/system/cpu/cpufreq/policy0/energy_performance_preference
> balance_performance
>
> To enable the driver,it needs to add `amd_pstate=active` to kernel
> command line and kernel will load the active mode epp driver
>
Please check the comments in V7's reply:
https://lore.kernel.org/lkml/Y6VVr+WYqwWb6XV0@amd.com/
I think the static call is not hard required at this moment.
But the boost/refresh_freq_limits stuff and cpudata may still need some
enhancement. Others, looks good for me right now.
Thanks,
Ray
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 447 ++++++++++++++++++++++++++++++++++-
> include/linux/amd-pstate.h | 10 +
> 2 files changed, 451 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 861a905f9324..66b39457a312 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -59,7 +59,10 @@
> * we disable it by default to go acpi-cpufreq on these processors and add a
> * module parameter to be able to enable it manually for debugging.
> */
> +static struct cpufreq_driver *default_pstate_driver;
> static struct cpufreq_driver amd_pstate_driver;
> +static struct cpufreq_driver amd_pstate_epp_driver;
> +static struct amd_cpudata **all_cpu_data;
> static int cppc_state = AMD_PSTATE_DISABLE;
>
> static inline int get_mode_idx_from_str(const char *str, size_t size)
> @@ -70,9 +73,128 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
> if (!strncmp(str, amd_pstate_mode_string[i], size))
> return i;
> }
> +
> return -EINVAL;
> }
>
> +/**
> + * struct amd_pstate_params - global parameters for the performance control
> + * @ cppc_boost_disabled wheher the core performance boost disabled
> + */
> +struct amd_pstate_params {
> + bool cppc_boost_disabled;
> +};
> +
> +static struct amd_pstate_params global_params;
> +
> +static DEFINE_MUTEX(amd_pstate_limits_lock);
> +static DEFINE_MUTEX(amd_pstate_driver_lock);
> +
> +static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
> +{
> + u64 epp;
> + int ret;
> +
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + if (!cppc_req_cached) {
> + epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
> + &cppc_req_cached);
> + if (epp)
> + return epp;
> + }
> + epp = (cppc_req_cached >> 24) & 0xFF;
> + } else {
> + ret = cppc_get_epp_perf(cpudata->cpu, &epp);
> + if (ret < 0) {
> + pr_debug("Could not retrieve energy perf value (%d)\n", ret);
> + return -EIO;
> + }
> + }
> +
> + return (s16)(epp & 0xff);
> +}
> +
> +static int amd_pstate_get_energy_pref_index(struct amd_cpudata *cpudata)
> +{
> + s16 epp;
> + int index = -EINVAL;
> +
> + epp = amd_pstate_get_epp(cpudata, 0);
> + if (epp < 0)
> + return epp;
> +
> + switch (epp) {
> + case HWP_EPP_PERFORMANCE:
> + index = EPP_INDEX_PERFORMANCE;
> + break;
> + case HWP_EPP_BALANCE_PERFORMANCE:
> + index = EPP_INDEX_BALANCE_PERFORMANCE;
> + break;
> + case HWP_EPP_BALANCE_POWERSAVE:
> + index = EPP_INDEX_BALANCE_POWERSAVE;
> + break;
> + case HWP_EPP_POWERSAVE:
> + index = EPP_INDEX_POWERSAVE;
> + break;
> + default:
> + break;
> + }
> +
> + return index;
> +}
> +
> +static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +{
> + int ret;
> + struct cppc_perf_ctrls perf_ctrls;
> +
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + u64 value = READ_ONCE(cpudata->cppc_req_cached);
> +
> + value &= ~GENMASK_ULL(31, 24);
> + value |= (u64)epp << 24;
> + WRITE_ONCE(cpudata->cppc_req_cached, value);
> +
> + ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> + if (!ret)
> + cpudata->epp_cached = epp;
> + } else {
> + perf_ctrls.energy_perf = epp;
> + ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
> + if (ret) {
> + pr_debug("failed to set energy perf value (%d)\n", ret);
> + return ret;
> + }
> + cpudata->epp_cached = epp;
> + }
> +
> + return ret;
> +}
> +
> +static int amd_pstate_set_energy_pref_index(struct amd_cpudata *cpudata,
> + int pref_index)
> +{
> + int epp = -EINVAL;
> + int ret;
> +
> + if (!pref_index) {
> + pr_debug("EPP pref_index is invalid\n");
> + return -EINVAL;
> + }
> +
> + if (epp == -EINVAL)
> + epp = epp_values[pref_index];
> +
> + if (epp > 0 && cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
> + pr_debug("EPP cannot be set under performance policy\n");
> + return -EBUSY;
> + }
> +
> + ret = amd_pstate_set_epp(cpudata, epp);
> +
> + return ret;
> +}
> +
> static inline int pstate_enable(bool enable)
> {
> return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable);
> @@ -81,11 +203,21 @@ static inline int pstate_enable(bool enable)
> static int cppc_enable(bool enable)
> {
> int cpu, ret = 0;
> + struct cppc_perf_ctrls perf_ctrls;
>
> for_each_present_cpu(cpu) {
> ret = cppc_set_enable(cpu, enable);
> if (ret)
> return ret;
> +
> + /* Enable autonomous mode for EPP */
> + if (cppc_state == AMD_PSTATE_ACTIVE) {
> + /* Set desired perf as zero to allow EPP firmware control */
> + perf_ctrls.desired_perf = 0;
> + ret = cppc_set_perf(cpu, &perf_ctrls);
> + if (ret)
> + return ret;
> + }
> }
>
> return ret;
> @@ -429,7 +561,7 @@ static void amd_pstate_boost_init(struct amd_cpudata *cpudata)
> return;
>
> cpudata->boost_supported = true;
> - amd_pstate_driver.boost_enabled = true;
> + default_pstate_driver->boost_enabled = true;
> }
>
> static void amd_perf_ctl_reset(unsigned int cpu)
> @@ -603,10 +735,61 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
> return sprintf(&buf[0], "%u\n", perf);
> }
>
> +static ssize_t show_energy_performance_available_preferences(
> + struct cpufreq_policy *policy, char *buf)
> +{
> + int i = 0;
> + int offset = 0;
> +
> + while (energy_perf_strings[i] != NULL)
> + offset += sysfs_emit_at(buf, offset, "%s ", energy_perf_strings[i++]);
> +
> + sysfs_emit_at(buf, offset, "\n");
> +
> + return offset;
> +}
> +
> +static ssize_t store_energy_performance_preference(
> + struct cpufreq_policy *policy, const char *buf, size_t count)
> +{
> + struct amd_cpudata *cpudata = policy->driver_data;
> + char str_preference[21];
> + ssize_t ret;
> +
> + ret = sscanf(buf, "%20s", str_preference);
> + if (ret != 1)
> + return -EINVAL;
> +
> + ret = match_string(energy_perf_strings, -1, str_preference);
> + if (ret < 0)
> + return -EINVAL;
> +
> + mutex_lock(&amd_pstate_limits_lock);
> + ret = amd_pstate_set_energy_pref_index(cpudata, ret);
> + mutex_unlock(&amd_pstate_limits_lock);
> +
> + return ret ?: count;
> +}
> +
> +static ssize_t show_energy_performance_preference(
> + struct cpufreq_policy *policy, char *buf)
> +{
> + struct amd_cpudata *cpudata = policy->driver_data;
> + int preference;
> +
> + preference = amd_pstate_get_energy_pref_index(cpudata);
> + if (preference < 0)
> + return preference;
> +
> + return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
> +}
> +
> cpufreq_freq_attr_ro(amd_pstate_max_freq);
> cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
>
> cpufreq_freq_attr_ro(amd_pstate_highest_perf);
> +cpufreq_freq_attr_rw(energy_performance_preference);
> +cpufreq_freq_attr_ro(energy_performance_available_preferences);
>
> static struct freq_attr *amd_pstate_attr[] = {
> &amd_pstate_max_freq,
> @@ -615,6 +798,235 @@ static struct freq_attr *amd_pstate_attr[] = {
> NULL,
> };
>
> +static struct freq_attr *amd_pstate_epp_attr[] = {
> + &amd_pstate_max_freq,
> + &amd_pstate_lowest_nonlinear_freq,
> + &amd_pstate_highest_perf,
> + &energy_performance_preference,
> + &energy_performance_available_preferences,
> + NULL,
> +};
> +
> +static inline void update_boost_state(void)
> +{
> + u64 misc_en;
> + struct amd_cpudata *cpudata;
> +
> + cpudata = all_cpu_data[0];
> + rdmsrl(MSR_K7_HWCR, misc_en);
> + global_params.cppc_boost_disabled = misc_en & BIT_ULL(25);
> +}
> +
> +static int amd_pstate_init_cpu(unsigned int cpunum)
> +{
> + struct amd_cpudata *cpudata;
> +
> + cpudata = all_cpu_data[cpunum];
> + if (!cpudata) {
> + cpudata = kzalloc(sizeof(*cpudata), GFP_KERNEL);
> + if (!cpudata)
> + return -ENOMEM;
> + WRITE_ONCE(all_cpu_data[cpunum], cpudata);
> +
> + cpudata->cpu = cpunum;
> + }
> +
> + cpudata->epp_policy = 0;
> + pr_debug("controlling: cpu %d\n", cpunum);
> + return 0;
> +}
> +
> +static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
> +{
> + int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret;
> + struct amd_cpudata *cpudata;
> + struct device *dev;
> + int rc;
> + u64 value;
> +
> + rc = amd_pstate_init_cpu(policy->cpu);
> + if (rc)
> + return rc;
> +
> + cpudata = all_cpu_data[policy->cpu];
> +
> + dev = get_cpu_device(policy->cpu);
> + if (!dev)
> + goto free_cpudata1;
> +
> + rc = amd_pstate_init_perf(cpudata);
> + if (rc)
> + goto free_cpudata1;
> +
> + min_freq = amd_get_min_freq(cpudata);
> + max_freq = amd_get_max_freq(cpudata);
> + nominal_freq = amd_get_nominal_freq(cpudata);
> + lowest_nonlinear_freq = amd_get_lowest_nonlinear_freq(cpudata);
> + if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) {
> + dev_err(dev, "min_freq(%d) or max_freq(%d) value is incorrect\n",
> + min_freq, max_freq);
> + ret = -EINVAL;
> + goto free_cpudata1;
> + }
> +
> + policy->min = min_freq;
> + policy->max = max_freq;
> +
> + policy->cpuinfo.min_freq = min_freq;
> + policy->cpuinfo.max_freq = max_freq;
> + /* It will be updated by governor */
> + policy->cur = policy->cpuinfo.min_freq;
> +
> + /* Initial processor data capability frequencies */
> + cpudata->max_freq = max_freq;
> + cpudata->min_freq = min_freq;
> + cpudata->nominal_freq = nominal_freq;
> + cpudata->lowest_nonlinear_freq = lowest_nonlinear_freq;
> +
> + policy->driver_data = cpudata;
> +
> + cpudata->epp_cached = amd_pstate_get_epp(cpudata, value);
> +
> + policy->min = policy->cpuinfo.min_freq;
> + policy->max = policy->cpuinfo.max_freq;
> +
> + /*
> + * Set the policy to powersave to provide a valid fallback value in case
> + * the default cpufreq governor is neither powersave nor performance.
> + */
> + policy->policy = CPUFREQ_POLICY_POWERSAVE;
> +
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + policy->fast_switch_possible = true;
> + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value);
> + if (ret)
> + return ret;
> + WRITE_ONCE(cpudata->cppc_req_cached, value);
> +
> + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1, &value);
> + if (ret)
> + return ret;
> + WRITE_ONCE(cpudata->cppc_cap1_cached, value);
> + }
> + amd_pstate_boost_init(cpudata);
> +
> + return 0;
> +
> +free_cpudata1:
> + kfree(cpudata);
> + return ret;
> +}
> +
> +static int amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
> +{
> + pr_debug("CPU %d exiting\n", policy->cpu);
> + policy->fast_switch_possible = false;
> + return 0;
> +}
> +
> +static void amd_pstate_update_max_freq(unsigned int cpu)
> +{
> + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +
> + if (!policy)
> + return;
> +
> + refresh_frequency_limits(policy);
> + cpufreq_cpu_put(policy);
> +}
> +
> +static void amd_pstate_epp_update_limits(unsigned int cpu)
> +{
> + mutex_lock(&amd_pstate_driver_lock);
> + update_boost_state();
> + if (global_params.cppc_boost_disabled) {
> + for_each_possible_cpu(cpu)
> + amd_pstate_update_max_freq(cpu);
> + } else {
> + cpufreq_update_policy(cpu);
> + }
> + mutex_unlock(&amd_pstate_driver_lock);
> +}
> +
> +static void amd_pstate_epp_init(unsigned int cpu)
> +{
> + struct amd_cpudata *cpudata = all_cpu_data[cpu];
> + u32 max_perf, min_perf;
> + u64 value;
> + s16 epp;
> +
> + max_perf = READ_ONCE(cpudata->highest_perf);
> + min_perf = READ_ONCE(cpudata->lowest_perf);
> +
> + value = READ_ONCE(cpudata->cppc_req_cached);
> +
> + if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
> + min_perf = max_perf;
> +
> + /* Initial min/max values for CPPC Performance Controls Register */
> + value &= ~AMD_CPPC_MIN_PERF(~0L);
> + value |= AMD_CPPC_MIN_PERF(min_perf);
> +
> + value &= ~AMD_CPPC_MAX_PERF(~0L);
> + value |= AMD_CPPC_MAX_PERF(max_perf);
> +
> + /* CPPC EPP feature require to set zero to the desire perf bit */
> + value &= ~AMD_CPPC_DES_PERF(~0L);
> + value |= AMD_CPPC_DES_PERF(0);
> +
> + if (cpudata->epp_policy == cpudata->policy)
> + goto skip_epp;
> +
> + cpudata->epp_policy = cpudata->policy;
> +
> + if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
> + epp = amd_pstate_get_epp(cpudata, value);
> + if (epp < 0)
> + goto skip_epp;
> + /* force the epp value to be zero for performance policy */
> + epp = 0;
> + } else {
> + /* Get BIOS pre-defined epp value */
> + epp = amd_pstate_get_epp(cpudata, value);
> + if (epp)
> + goto skip_epp;
> + }
> + /* Set initial EPP value */
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + value &= ~GENMASK_ULL(31, 24);
> + value |= (u64)epp << 24;
> + }
> +
> +skip_epp:
> + WRITE_ONCE(cpudata->cppc_req_cached, value);
> + amd_pstate_set_epp(cpudata, epp);
> +}
> +
> +static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
> +{
> + struct amd_cpudata *cpudata;
> +
> + if (!policy->cpuinfo.max_freq)
> + return -ENODEV;
> +
> + pr_debug("set_policy: cpuinfo.max %u policy->max %u\n",
> + policy->cpuinfo.max_freq, policy->max);
> +
> + cpudata = all_cpu_data[policy->cpu];
> + cpudata->policy = policy->policy;
> +
> + amd_pstate_epp_init(policy->cpu);
> +
> + return 0;
> +}
> +
> +static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
> +{
> + cpufreq_verify_within_cpu_limits(policy);
> + pr_debug("policy_max =%d, policy_min=%d\n", policy->max, policy->min);
> + return 0;
> +}
> +
> static struct cpufreq_driver amd_pstate_driver = {
> .flags = CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
> .verify = amd_pstate_verify,
> @@ -628,8 +1040,20 @@ static struct cpufreq_driver amd_pstate_driver = {
> .attr = amd_pstate_attr,
> };
>
> +static struct cpufreq_driver amd_pstate_epp_driver = {
> + .flags = CPUFREQ_CONST_LOOPS,
> + .verify = amd_pstate_epp_verify_policy,
> + .setpolicy = amd_pstate_epp_set_policy,
> + .init = amd_pstate_epp_cpu_init,
> + .exit = amd_pstate_epp_cpu_exit,
> + .update_limits = amd_pstate_epp_update_limits,
> + .name = "amd_pstate_epp",
> + .attr = amd_pstate_epp_attr,
> +};
> +
> static int __init amd_pstate_init(void)
> {
> + static struct amd_cpudata **cpudata;
> int ret;
>
> if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> @@ -656,7 +1080,8 @@ static int __init amd_pstate_init(void)
> /* capability check */
> if (boot_cpu_has(X86_FEATURE_CPPC)) {
> pr_debug("AMD CPPC MSR based functionality is supported\n");
> - amd_pstate_driver.adjust_perf = amd_pstate_adjust_perf;
> + if (cppc_state == AMD_PSTATE_PASSIVE)
> + default_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
> } else {
> pr_debug("AMD CPPC shared memory based functionality is supported\n");
> static_call_update(amd_pstate_enable, cppc_enable);
> @@ -664,17 +1089,21 @@ static int __init amd_pstate_init(void)
> static_call_update(amd_pstate_update_perf, cppc_update_perf);
> }
>
> + cpudata = vzalloc(array_size(sizeof(void *), num_possible_cpus()));
> + if (!cpudata)
> + return -ENOMEM;
> + WRITE_ONCE(all_cpu_data, cpudata);
> +
> /* enable amd pstate feature */
> ret = amd_pstate_enable(true);
> if (ret) {
> - pr_err("failed to enable amd-pstate with return %d\n", ret);
> + pr_err("failed to enable with return %d\n", ret);
> return ret;
> }
>
> - ret = cpufreq_register_driver(&amd_pstate_driver);
> + ret = cpufreq_register_driver(default_pstate_driver);
> if (ret)
> - pr_err("failed to register amd_pstate_driver with return %d\n",
> - ret);
> + pr_err("failed to register with return %d\n", ret);
>
> return ret;
> }
> @@ -696,6 +1125,12 @@ static int __init amd_pstate_param(char *str)
> if (cppc_state == AMD_PSTATE_DISABLE)
> pr_info("driver is explicitly disabled\n");
>
> + if (cppc_state == AMD_PSTATE_ACTIVE)
> + default_pstate_driver = &amd_pstate_epp_driver;
> +
> + if (cppc_state == AMD_PSTATE_PASSIVE)
> + default_pstate_driver = &amd_pstate_driver;
> +
> return 0;
> }
>
> diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> index 922d05a13902..fe1aef743c09 100644
> --- a/include/linux/amd-pstate.h
> +++ b/include/linux/amd-pstate.h
> @@ -47,6 +47,10 @@ struct amd_aperf_mperf {
> * @prev: Last Aperf/Mperf/tsc count value read from register
> * @freq: current cpu frequency value
> * @boost_supported: check whether the Processor or SBIOS supports boost mode
> + * @epp_policy: Last saved policy used to set energy-performance preference
> + * @epp_cached: Cached CPPC energy-performance preference value
> + * @policy: Cpufreq policy value
> + * @cppc_cap1_cached Cached MSR_AMD_CPPC_CAP1 register value
> *
> * The amd_cpudata is key private data for each CPU thread in AMD P-State, and
> * represents all the attributes and goals that AMD P-State requests at runtime.
> @@ -72,6 +76,12 @@ struct amd_cpudata {
>
> u64 freq;
> bool boost_supported;
> +
> + /* EPP feature related attributes*/
> + s16 epp_policy;
> + s16 epp_cached;
> + u32 policy;
> + u64 cppc_cap1_cached;
> };
>
> /**
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v8 06/13] cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors
2022-12-23 7:43 ` Huang Rui
@ 2022-12-23 7:52 ` Yuan, Perry
0 siblings, 0 replies; 45+ messages in thread
From: Yuan, Perry @ 2022-12-23 7:52 UTC (permalink / raw)
To: Huang, Ray
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
[AMD Official Use Only - General]
Hi Ray.
> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Friday, December 23, 2022 3:43 PM
> 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 v8 06/13] cpufreq: amd-pstate: implement Pstate EPP
> support for the AMD processors
>
> On Mon, Dec 19, 2022 at 02:40:35PM +0800, Yuan, Perry wrote:
> > From: Perry Yuan <Perry.Yuan@amd.com>
> >
> > Add EPP driver support for AMD SoCs which support a dedicated MSR for
> > CPPC. EPP is used by the DPM controller to configure the frequency
> > that a core operates at during short periods of activity.
> >
> > The SoC EPP targets are configured on a scale from 0 to 255 where 0
> > represents maximum performance and 255 represents maximum
> efficiency.
> >
> > The amd-pstate driver exports profile string names to userspace that
> > are tied to specific EPP values.
> >
> > The balance_performance string (0x80) provides the best balance for
> > efficiency versus power on most systems, but users can choose other
> > strings to meet their needs as well.
> >
> > $ cat
> >
> /sys/devices/system/cpu/cpufreq/policy0/energy_performance_available_
> p
> > references default performance balance_performance balance_power
> power
> >
> > $ cat
> >
> /sys/devices/system/cpu/cpufreq/policy0/energy_performance_preferenc
> e
> > balance_performance
> >
> > To enable the driver,it needs to add `amd_pstate=active` to kernel
> > command line and kernel will load the active mode epp driver
> >
>
> Please check the comments in V7's reply:
>
> https://lore.kernel.org/lkml/Y6VVr+WYqwWb6XV0@amd.com/
>
> I think the static call is not hard required at this moment.
>
> But the boost/refresh_freq_limits stuff and cpudata may still need some
> enhancement. Others, looks good for me right now.
>
> Thanks,
> Ray
Thanks for your quick review at this hard time.
I will rework the patch as your suggestion in V9.
Perry.
>
> > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > ---
> > drivers/cpufreq/amd-pstate.c | 447
> ++++++++++++++++++++++++++++++++++-
> > include/linux/amd-pstate.h | 10 +
> > 2 files changed, 451 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index 861a905f9324..66b39457a312 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -59,7 +59,10 @@
> > * we disable it by default to go acpi-cpufreq on these processors and add
> a
> > * module parameter to be able to enable it manually for debugging.
> > */
> > +static struct cpufreq_driver *default_pstate_driver;
> > static struct cpufreq_driver amd_pstate_driver;
> > +static struct cpufreq_driver amd_pstate_epp_driver; static struct
> > +amd_cpudata **all_cpu_data;
> > static int cppc_state = AMD_PSTATE_DISABLE;
> >
> > static inline int get_mode_idx_from_str(const char *str, size_t size)
> > @@ -70,9 +73,128 @@ static inline int get_mode_idx_from_str(const char
> *str, size_t size)
> > if (!strncmp(str, amd_pstate_mode_string[i], size))
> > return i;
> > }
> > +
> > return -EINVAL;
> > }
> >
> > +/**
> > + * struct amd_pstate_params - global parameters for the performance
> > +control
> > + * @ cppc_boost_disabled wheher the core performance boost disabled
> > +*/ struct amd_pstate_params {
> > + bool cppc_boost_disabled;
> > +};
> > +
> > +static struct amd_pstate_params global_params;
> > +
> > +static DEFINE_MUTEX(amd_pstate_limits_lock);
> > +static DEFINE_MUTEX(amd_pstate_driver_lock);
> > +
> > +static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64
> > +cppc_req_cached) {
> > + u64 epp;
> > + int ret;
> > +
> > + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> > + if (!cppc_req_cached) {
> > + epp = rdmsrl_on_cpu(cpudata->cpu,
> MSR_AMD_CPPC_REQ,
> > + &cppc_req_cached);
> > + if (epp)
> > + return epp;
> > + }
> > + epp = (cppc_req_cached >> 24) & 0xFF;
> > + } else {
> > + ret = cppc_get_epp_perf(cpudata->cpu, &epp);
> > + if (ret < 0) {
> > + pr_debug("Could not retrieve energy perf value
> (%d)\n", ret);
> > + return -EIO;
> > + }
> > + }
> > +
> > + return (s16)(epp & 0xff);
> > +}
> > +
> > +static int amd_pstate_get_energy_pref_index(struct amd_cpudata
> > +*cpudata) {
> > + s16 epp;
> > + int index = -EINVAL;
> > +
> > + epp = amd_pstate_get_epp(cpudata, 0);
> > + if (epp < 0)
> > + return epp;
> > +
> > + switch (epp) {
> > + case HWP_EPP_PERFORMANCE:
> > + index = EPP_INDEX_PERFORMANCE;
> > + break;
> > + case HWP_EPP_BALANCE_PERFORMANCE:
> > + index = EPP_INDEX_BALANCE_PERFORMANCE;
> > + break;
> > + case HWP_EPP_BALANCE_POWERSAVE:
> > + index = EPP_INDEX_BALANCE_POWERSAVE;
> > + break;
> > + case HWP_EPP_POWERSAVE:
> > + index = EPP_INDEX_POWERSAVE;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return index;
> > +}
> > +
> > +static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp) {
> > + int ret;
> > + struct cppc_perf_ctrls perf_ctrls;
> > +
> > + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> > + u64 value = READ_ONCE(cpudata->cppc_req_cached);
> > +
> > + value &= ~GENMASK_ULL(31, 24);
> > + value |= (u64)epp << 24;
> > + WRITE_ONCE(cpudata->cppc_req_cached, value);
> > +
> > + ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
> value);
> > + if (!ret)
> > + cpudata->epp_cached = epp;
> > + } else {
> > + perf_ctrls.energy_perf = epp;
> > + ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
> > + if (ret) {
> > + pr_debug("failed to set energy perf value (%d)\n",
> ret);
> > + return ret;
> > + }
> > + cpudata->epp_cached = epp;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int amd_pstate_set_energy_pref_index(struct amd_cpudata
> *cpudata,
> > + int pref_index)
> > +{
> > + int epp = -EINVAL;
> > + int ret;
> > +
> > + if (!pref_index) {
> > + pr_debug("EPP pref_index is invalid\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (epp == -EINVAL)
> > + epp = epp_values[pref_index];
> > +
> > + if (epp > 0 && cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
> {
> > + pr_debug("EPP cannot be set under performance policy\n");
> > + return -EBUSY;
> > + }
> > +
> > + ret = amd_pstate_set_epp(cpudata, epp);
> > +
> > + return ret;
> > +}
> > +
> > static inline int pstate_enable(bool enable) {
> > return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable); @@ -81,11
> +203,21
> > @@ static inline int pstate_enable(bool enable) static int
> > cppc_enable(bool enable) {
> > int cpu, ret = 0;
> > + struct cppc_perf_ctrls perf_ctrls;
> >
> > for_each_present_cpu(cpu) {
> > ret = cppc_set_enable(cpu, enable);
> > if (ret)
> > return ret;
> > +
> > + /* Enable autonomous mode for EPP */
> > + if (cppc_state == AMD_PSTATE_ACTIVE) {
> > + /* Set desired perf as zero to allow EPP firmware
> control */
> > + perf_ctrls.desired_perf = 0;
> > + ret = cppc_set_perf(cpu, &perf_ctrls);
> > + if (ret)
> > + return ret;
> > + }
> > }
> >
> > return ret;
> > @@ -429,7 +561,7 @@ static void amd_pstate_boost_init(struct
> amd_cpudata *cpudata)
> > return;
> >
> > cpudata->boost_supported = true;
> > - amd_pstate_driver.boost_enabled = true;
> > + default_pstate_driver->boost_enabled = true;
> > }
> >
> > static void amd_perf_ctl_reset(unsigned int cpu) @@ -603,10 +735,61
> > @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy
> *policy,
> > return sprintf(&buf[0], "%u\n", perf); }
> >
> > +static ssize_t show_energy_performance_available_preferences(
> > + struct cpufreq_policy *policy, char *buf) {
> > + int i = 0;
> > + int offset = 0;
> > +
> > + while (energy_perf_strings[i] != NULL)
> > + offset += sysfs_emit_at(buf, offset, "%s ",
> > +energy_perf_strings[i++]);
> > +
> > + sysfs_emit_at(buf, offset, "\n");
> > +
> > + return offset;
> > +}
> > +
> > +static ssize_t store_energy_performance_preference(
> > + struct cpufreq_policy *policy, const char *buf, size_t count) {
> > + struct amd_cpudata *cpudata = policy->driver_data;
> > + char str_preference[21];
> > + ssize_t ret;
> > +
> > + ret = sscanf(buf, "%20s", str_preference);
> > + if (ret != 1)
> > + return -EINVAL;
> > +
> > + ret = match_string(energy_perf_strings, -1, str_preference);
> > + if (ret < 0)
> > + return -EINVAL;
> > +
> > + mutex_lock(&amd_pstate_limits_lock);
> > + ret = amd_pstate_set_energy_pref_index(cpudata, ret);
> > + mutex_unlock(&amd_pstate_limits_lock);
> > +
> > + return ret ?: count;
> > +}
> > +
> > +static ssize_t show_energy_performance_preference(
> > + struct cpufreq_policy *policy, char *buf) {
> > + struct amd_cpudata *cpudata = policy->driver_data;
> > + int preference;
> > +
> > + preference = amd_pstate_get_energy_pref_index(cpudata);
> > + if (preference < 0)
> > + return preference;
> > +
> > + return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]); }
> > +
> > cpufreq_freq_attr_ro(amd_pstate_max_freq);
> > cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
> >
> > cpufreq_freq_attr_ro(amd_pstate_highest_perf);
> > +cpufreq_freq_attr_rw(energy_performance_preference);
> > +cpufreq_freq_attr_ro(energy_performance_available_preferences);
> >
> > static struct freq_attr *amd_pstate_attr[] = {
> > &amd_pstate_max_freq,
> > @@ -615,6 +798,235 @@ static struct freq_attr *amd_pstate_attr[] = {
> > NULL,
> > };
> >
> > +static struct freq_attr *amd_pstate_epp_attr[] = {
> > + &amd_pstate_max_freq,
> > + &amd_pstate_lowest_nonlinear_freq,
> > + &amd_pstate_highest_perf,
> > + &energy_performance_preference,
> > + &energy_performance_available_preferences,
> > + NULL,
> > +};
> > +
> > +static inline void update_boost_state(void) {
> > + u64 misc_en;
> > + struct amd_cpudata *cpudata;
> > +
> > + cpudata = all_cpu_data[0];
> > + rdmsrl(MSR_K7_HWCR, misc_en);
> > + global_params.cppc_boost_disabled = misc_en & BIT_ULL(25); }
> > +
> > +static int amd_pstate_init_cpu(unsigned int cpunum) {
> > + struct amd_cpudata *cpudata;
> > +
> > + cpudata = all_cpu_data[cpunum];
> > + if (!cpudata) {
> > + cpudata = kzalloc(sizeof(*cpudata), GFP_KERNEL);
> > + if (!cpudata)
> > + return -ENOMEM;
> > + WRITE_ONCE(all_cpu_data[cpunum], cpudata);
> > +
> > + cpudata->cpu = cpunum;
> > + }
> > +
> > + cpudata->epp_policy = 0;
> > + pr_debug("controlling: cpu %d\n", cpunum);
> > + return 0;
> > +}
> > +
> > +static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) {
> > + int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret;
> > + struct amd_cpudata *cpudata;
> > + struct device *dev;
> > + int rc;
> > + u64 value;
> > +
> > + rc = amd_pstate_init_cpu(policy->cpu);
> > + if (rc)
> > + return rc;
> > +
> > + cpudata = all_cpu_data[policy->cpu];
> > +
> > + dev = get_cpu_device(policy->cpu);
> > + if (!dev)
> > + goto free_cpudata1;
> > +
> > + rc = amd_pstate_init_perf(cpudata);
> > + if (rc)
> > + goto free_cpudata1;
> > +
> > + min_freq = amd_get_min_freq(cpudata);
> > + max_freq = amd_get_max_freq(cpudata);
> > + nominal_freq = amd_get_nominal_freq(cpudata);
> > + lowest_nonlinear_freq = amd_get_lowest_nonlinear_freq(cpudata);
> > + if (min_freq < 0 || max_freq < 0 || min_freq > max_freq) {
> > + dev_err(dev, "min_freq(%d) or max_freq(%d) value is
> incorrect\n",
> > + min_freq, max_freq);
> > + ret = -EINVAL;
> > + goto free_cpudata1;
> > + }
> > +
> > + policy->min = min_freq;
> > + policy->max = max_freq;
> > +
> > + policy->cpuinfo.min_freq = min_freq;
> > + policy->cpuinfo.max_freq = max_freq;
> > + /* It will be updated by governor */
> > + policy->cur = policy->cpuinfo.min_freq;
> > +
> > + /* Initial processor data capability frequencies */
> > + cpudata->max_freq = max_freq;
> > + cpudata->min_freq = min_freq;
> > + cpudata->nominal_freq = nominal_freq;
> > + cpudata->lowest_nonlinear_freq = lowest_nonlinear_freq;
> > +
> > + policy->driver_data = cpudata;
> > +
> > + cpudata->epp_cached = amd_pstate_get_epp(cpudata, value);
> > +
> > + policy->min = policy->cpuinfo.min_freq;
> > + policy->max = policy->cpuinfo.max_freq;
> > +
> > + /*
> > + * Set the policy to powersave to provide a valid fallback value in case
> > + * the default cpufreq governor is neither powersave nor
> performance.
> > + */
> > + policy->policy = CPUFREQ_POLICY_POWERSAVE;
> > +
> > + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> > + policy->fast_switch_possible = true;
> > + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
> &value);
> > + if (ret)
> > + return ret;
> > + WRITE_ONCE(cpudata->cppc_req_cached, value);
> > +
> > + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
> &value);
> > + if (ret)
> > + return ret;
> > + WRITE_ONCE(cpudata->cppc_cap1_cached, value);
> > + }
> > + amd_pstate_boost_init(cpudata);
> > +
> > + return 0;
> > +
> > +free_cpudata1:
> > + kfree(cpudata);
> > + return ret;
> > +}
> > +
> > +static int amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy) {
> > + pr_debug("CPU %d exiting\n", policy->cpu);
> > + policy->fast_switch_possible = false;
> > + return 0;
> > +}
> > +
> > +static void amd_pstate_update_max_freq(unsigned int cpu) {
> > + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> > +
> > + if (!policy)
> > + return;
> > +
> > + refresh_frequency_limits(policy);
> > + cpufreq_cpu_put(policy);
> > +}
> > +
> > +static void amd_pstate_epp_update_limits(unsigned int cpu) {
> > + mutex_lock(&amd_pstate_driver_lock);
> > + update_boost_state();
> > + if (global_params.cppc_boost_disabled) {
> > + for_each_possible_cpu(cpu)
> > + amd_pstate_update_max_freq(cpu);
> > + } else {
> > + cpufreq_update_policy(cpu);
> > + }
> > + mutex_unlock(&amd_pstate_driver_lock);
> > +}
> > +
> > +static void amd_pstate_epp_init(unsigned int cpu) {
> > + struct amd_cpudata *cpudata = all_cpu_data[cpu];
> > + u32 max_perf, min_perf;
> > + u64 value;
> > + s16 epp;
> > +
> > + max_perf = READ_ONCE(cpudata->highest_perf);
> > + min_perf = READ_ONCE(cpudata->lowest_perf);
> > +
> > + value = READ_ONCE(cpudata->cppc_req_cached);
> > +
> > + if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
> > + min_perf = max_perf;
> > +
> > + /* Initial min/max values for CPPC Performance Controls Register */
> > + value &= ~AMD_CPPC_MIN_PERF(~0L);
> > + value |= AMD_CPPC_MIN_PERF(min_perf);
> > +
> > + value &= ~AMD_CPPC_MAX_PERF(~0L);
> > + value |= AMD_CPPC_MAX_PERF(max_perf);
> > +
> > + /* CPPC EPP feature require to set zero to the desire perf bit */
> > + value &= ~AMD_CPPC_DES_PERF(~0L);
> > + value |= AMD_CPPC_DES_PERF(0);
> > +
> > + if (cpudata->epp_policy == cpudata->policy)
> > + goto skip_epp;
> > +
> > + cpudata->epp_policy = cpudata->policy;
> > +
> > + if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
> > + epp = amd_pstate_get_epp(cpudata, value);
> > + if (epp < 0)
> > + goto skip_epp;
> > + /* force the epp value to be zero for performance policy */
> > + epp = 0;
> > + } else {
> > + /* Get BIOS pre-defined epp value */
> > + epp = amd_pstate_get_epp(cpudata, value);
> > + if (epp)
> > + goto skip_epp;
> > + }
> > + /* Set initial EPP value */
> > + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> > + value &= ~GENMASK_ULL(31, 24);
> > + value |= (u64)epp << 24;
> > + }
> > +
> > +skip_epp:
> > + WRITE_ONCE(cpudata->cppc_req_cached, value);
> > + amd_pstate_set_epp(cpudata, epp);
> > +}
> > +
> > +static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy) {
> > + struct amd_cpudata *cpudata;
> > +
> > + if (!policy->cpuinfo.max_freq)
> > + return -ENODEV;
> > +
> > + pr_debug("set_policy: cpuinfo.max %u policy->max %u\n",
> > + policy->cpuinfo.max_freq, policy->max);
> > +
> > + cpudata = all_cpu_data[policy->cpu];
> > + cpudata->policy = policy->policy;
> > +
> > + amd_pstate_epp_init(policy->cpu);
> > +
> > + return 0;
> > +}
> > +
> > +static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data
> > +*policy) {
> > + cpufreq_verify_within_cpu_limits(policy);
> > + pr_debug("policy_max =%d, policy_min=%d\n", policy->max, policy-
> >min);
> > + return 0;
> > +}
> > +
> > static struct cpufreq_driver amd_pstate_driver = {
> > .flags = CPUFREQ_CONST_LOOPS |
> CPUFREQ_NEED_UPDATE_LIMITS,
> > .verify = amd_pstate_verify,
> > @@ -628,8 +1040,20 @@ static struct cpufreq_driver amd_pstate_driver =
> {
> > .attr = amd_pstate_attr,
> > };
> >
> > +static struct cpufreq_driver amd_pstate_epp_driver = {
> > + .flags = CPUFREQ_CONST_LOOPS,
> > + .verify = amd_pstate_epp_verify_policy,
> > + .setpolicy = amd_pstate_epp_set_policy,
> > + .init = amd_pstate_epp_cpu_init,
> > + .exit = amd_pstate_epp_cpu_exit,
> > + .update_limits = amd_pstate_epp_update_limits,
> > + .name = "amd_pstate_epp",
> > + .attr = amd_pstate_epp_attr,
> > +};
> > +
> > static int __init amd_pstate_init(void) {
> > + static struct amd_cpudata **cpudata;
> > int ret;
> >
> > if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) @@ -656,7
> +1080,8 @@
> > static int __init amd_pstate_init(void)
> > /* capability check */
> > if (boot_cpu_has(X86_FEATURE_CPPC)) {
> > pr_debug("AMD CPPC MSR based functionality is
> supported\n");
> > - amd_pstate_driver.adjust_perf = amd_pstate_adjust_perf;
> > + if (cppc_state == AMD_PSTATE_PASSIVE)
> > + default_pstate_driver->adjust_perf =
> amd_pstate_adjust_perf;
> > } else {
> > pr_debug("AMD CPPC shared memory based functionality is
> supported\n");
> > static_call_update(amd_pstate_enable, cppc_enable); @@ -
> 664,17
> > +1089,21 @@ static int __init amd_pstate_init(void)
> > static_call_update(amd_pstate_update_perf,
> cppc_update_perf);
> > }
> >
> > + cpudata = vzalloc(array_size(sizeof(void *), num_possible_cpus()));
> > + if (!cpudata)
> > + return -ENOMEM;
> > + WRITE_ONCE(all_cpu_data, cpudata);
> > +
> > /* enable amd pstate feature */
> > ret = amd_pstate_enable(true);
> > if (ret) {
> > - pr_err("failed to enable amd-pstate with return %d\n", ret);
> > + pr_err("failed to enable with return %d\n", ret);
> > return ret;
> > }
> >
> > - ret = cpufreq_register_driver(&amd_pstate_driver);
> > + ret = cpufreq_register_driver(default_pstate_driver);
> > if (ret)
> > - pr_err("failed to register amd_pstate_driver with
> return %d\n",
> > - ret);
> > + pr_err("failed to register with return %d\n", ret);
> >
> > return ret;
> > }
> > @@ -696,6 +1125,12 @@ static int __init amd_pstate_param(char *str)
> > if (cppc_state == AMD_PSTATE_DISABLE)
> > pr_info("driver is explicitly disabled\n");
> >
> > + if (cppc_state == AMD_PSTATE_ACTIVE)
> > + default_pstate_driver = &amd_pstate_epp_driver;
> > +
> > + if (cppc_state == AMD_PSTATE_PASSIVE)
> > + default_pstate_driver = &amd_pstate_driver;
> > +
> > return 0;
> > }
> >
> > diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> > index 922d05a13902..fe1aef743c09 100644
> > --- a/include/linux/amd-pstate.h
> > +++ b/include/linux/amd-pstate.h
> > @@ -47,6 +47,10 @@ struct amd_aperf_mperf {
> > * @prev: Last Aperf/Mperf/tsc count value read from register
> > * @freq: current cpu frequency value
> > * @boost_supported: check whether the Processor or SBIOS supports
> > boost mode
> > + * @epp_policy: Last saved policy used to set energy-performance
> > + preference
> > + * @epp_cached: Cached CPPC energy-performance preference value
> > + * @policy: Cpufreq policy value
> > + * @cppc_cap1_cached Cached MSR_AMD_CPPC_CAP1 register value
> > *
> > * The amd_cpudata is key private data for each CPU thread in AMD P-
> State, and
> > * represents all the attributes and goals that AMD P-State requests at
> runtime.
> > @@ -72,6 +76,12 @@ struct amd_cpudata {
> >
> > u64 freq;
> > bool boost_supported;
> > +
> > + /* EPP feature related attributes*/
> > + s16 epp_policy;
> > + s16 epp_cached;
> > + u32 policy;
> > + u64 cppc_cap1_cached;
> > };
> >
> > /**
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 07/13] cpufreq: amd-pstate: implement amd pstate cpu online and offline callback
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (5 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 06/13] cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-23 8:11 ` Huang Rui
2022-12-19 6:40 ` [PATCH v8 08/13] cpufreq: amd-pstate: implement suspend and resume callbacks Perry Yuan
` (6 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
From: Perry Yuan <Perry.Yuan@amd.com>
Adds online and offline driver callback support to allow cpu cores go
offline and help to restore the previous working states when core goes
back online later for EPP driver mode.
Reviewed-by: Mario Limonciello <Mario.Limonciello@amd.com>
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
drivers/cpufreq/amd-pstate.c | 82 ++++++++++++++++++++++++++++++++++++
include/linux/amd-pstate.h | 1 +
2 files changed, 83 insertions(+)
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 66b39457a312..cb647f55a169 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1020,6 +1020,86 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
return 0;
}
+static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
+{
+ struct cppc_perf_ctrls perf_ctrls;
+ u64 value, max_perf;
+ int ret;
+
+ ret = amd_pstate_enable(true);
+ if (ret)
+ pr_err("failed to enable amd pstate during resume, return %d\n", ret);
+
+ value = READ_ONCE(cpudata->cppc_req_cached);
+ max_perf = READ_ONCE(cpudata->highest_perf);
+
+ if (boot_cpu_has(X86_FEATURE_CPPC)) {
+ wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+ } else {
+ perf_ctrls.max_perf = max_perf;
+ perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(cpudata->epp_cached);
+ cppc_set_perf(cpudata->cpu, &perf_ctrls);
+ }
+}
+
+static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
+{
+ struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+
+ pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
+
+ if (cppc_state == AMD_PSTATE_ACTIVE) {
+ amd_pstate_epp_reenable(cpudata);
+ cpudata->suspended = false;
+ }
+
+ return 0;
+}
+
+static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
+{
+ struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+ struct cppc_perf_ctrls perf_ctrls;
+ int min_perf;
+ u64 value;
+
+ min_perf = READ_ONCE(cpudata->lowest_perf);
+ value = READ_ONCE(cpudata->cppc_req_cached);
+
+ mutex_lock(&amd_pstate_limits_lock);
+ if (boot_cpu_has(X86_FEATURE_CPPC)) {
+ cpudata->epp_policy = CPUFREQ_POLICY_UNKNOWN;
+
+ /* Set max perf same as min perf */
+ value &= ~AMD_CPPC_MAX_PERF(~0L);
+ value |= AMD_CPPC_MAX_PERF(min_perf);
+ value &= ~AMD_CPPC_MIN_PERF(~0L);
+ value |= AMD_CPPC_MIN_PERF(min_perf);
+ wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+ } else {
+ perf_ctrls.desired_perf = 0;
+ perf_ctrls.max_perf = min_perf;
+ perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(HWP_EPP_BALANCE_POWERSAVE);
+ cppc_set_perf(cpudata->cpu, &perf_ctrls);
+ }
+ mutex_unlock(&amd_pstate_limits_lock);
+}
+
+static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
+{
+ struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+
+ pr_debug("AMD CPU Core %d going offline\n", cpudata->cpu);
+
+ if (cpudata->suspended)
+ return 0;
+
+ if (cppc_state == AMD_PSTATE_ACTIVE)
+ amd_pstate_epp_offline(policy);
+
+ return 0;
+}
+
static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
{
cpufreq_verify_within_cpu_limits(policy);
@@ -1047,6 +1127,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
.init = amd_pstate_epp_cpu_init,
.exit = amd_pstate_epp_cpu_exit,
.update_limits = amd_pstate_epp_update_limits,
+ .offline = amd_pstate_epp_cpu_offline,
+ .online = amd_pstate_epp_cpu_online,
.name = "amd_pstate_epp",
.attr = amd_pstate_epp_attr,
};
diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
index fe1aef743c09..1424b09ef543 100644
--- a/include/linux/amd-pstate.h
+++ b/include/linux/amd-pstate.h
@@ -82,6 +82,7 @@ struct amd_cpudata {
s16 epp_cached;
u32 policy;
u64 cppc_cap1_cached;
+ bool suspended;
};
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 07/13] cpufreq: amd-pstate: implement amd pstate cpu online and offline callback
2022-12-19 6:40 ` [PATCH v8 07/13] cpufreq: amd-pstate: implement amd pstate cpu online and offline callback Perry Yuan
@ 2022-12-23 8:11 ` Huang Rui
0 siblings, 0 replies; 45+ messages in thread
From: Huang Rui @ 2022-12-23 8:11 UTC (permalink / raw)
To: Yuan, Perry
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On Mon, Dec 19, 2022 at 02:40:36PM +0800, Yuan, Perry wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
>
> Adds online and offline driver callback support to allow cpu cores go
> offline and help to restore the previous working states when core goes
> back online later for EPP driver mode.
>
> Reviewed-by: Mario Limonciello <Mario.Limonciello@amd.com>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
Acked-by: Huang Rui <ray.huang@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 82 ++++++++++++++++++++++++++++++++++++
> include/linux/amd-pstate.h | 1 +
> 2 files changed, 83 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 66b39457a312..cb647f55a169 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1020,6 +1020,86 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
> return 0;
> }
>
> +static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
> +{
> + struct cppc_perf_ctrls perf_ctrls;
> + u64 value, max_perf;
> + int ret;
> +
> + ret = amd_pstate_enable(true);
> + if (ret)
> + pr_err("failed to enable amd pstate during resume, return %d\n", ret);
> +
> + value = READ_ONCE(cpudata->cppc_req_cached);
> + max_perf = READ_ONCE(cpudata->highest_perf);
> +
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> + } else {
> + perf_ctrls.max_perf = max_perf;
> + perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(cpudata->epp_cached);
> + cppc_set_perf(cpudata->cpu, &perf_ctrls);
> + }
> +}
> +
> +static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
> +{
> + struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
> +
> + pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
> +
> + if (cppc_state == AMD_PSTATE_ACTIVE) {
> + amd_pstate_epp_reenable(cpudata);
> + cpudata->suspended = false;
> + }
> +
> + return 0;
> +}
> +
> +static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
> +{
> + struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
> + struct cppc_perf_ctrls perf_ctrls;
> + int min_perf;
> + u64 value;
> +
> + min_perf = READ_ONCE(cpudata->lowest_perf);
> + value = READ_ONCE(cpudata->cppc_req_cached);
> +
> + mutex_lock(&amd_pstate_limits_lock);
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + cpudata->epp_policy = CPUFREQ_POLICY_UNKNOWN;
> +
> + /* Set max perf same as min perf */
> + value &= ~AMD_CPPC_MAX_PERF(~0L);
> + value |= AMD_CPPC_MAX_PERF(min_perf);
> + value &= ~AMD_CPPC_MIN_PERF(~0L);
> + value |= AMD_CPPC_MIN_PERF(min_perf);
> + wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> + } else {
> + perf_ctrls.desired_perf = 0;
> + perf_ctrls.max_perf = min_perf;
> + perf_ctrls.energy_perf = AMD_CPPC_ENERGY_PERF_PREF(HWP_EPP_BALANCE_POWERSAVE);
> + cppc_set_perf(cpudata->cpu, &perf_ctrls);
> + }
> + mutex_unlock(&amd_pstate_limits_lock);
> +}
> +
> +static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
> +{
> + struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
> +
> + pr_debug("AMD CPU Core %d going offline\n", cpudata->cpu);
> +
> + if (cpudata->suspended)
> + return 0;
> +
> + if (cppc_state == AMD_PSTATE_ACTIVE)
> + amd_pstate_epp_offline(policy);
> +
> + return 0;
> +}
> +
> static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
> {
> cpufreq_verify_within_cpu_limits(policy);
> @@ -1047,6 +1127,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
> .init = amd_pstate_epp_cpu_init,
> .exit = amd_pstate_epp_cpu_exit,
> .update_limits = amd_pstate_epp_update_limits,
> + .offline = amd_pstate_epp_cpu_offline,
> + .online = amd_pstate_epp_cpu_online,
> .name = "amd_pstate_epp",
> .attr = amd_pstate_epp_attr,
> };
> diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> index fe1aef743c09..1424b09ef543 100644
> --- a/include/linux/amd-pstate.h
> +++ b/include/linux/amd-pstate.h
> @@ -82,6 +82,7 @@ struct amd_cpudata {
> s16 epp_cached;
> u32 policy;
> u64 cppc_cap1_cached;
> + bool suspended;
> };
>
> /**
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 08/13] cpufreq: amd-pstate: implement suspend and resume callbacks
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (6 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 07/13] cpufreq: amd-pstate: implement amd pstate cpu online and offline callback Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-23 8:12 ` Huang Rui
2022-12-19 6:40 ` [PATCH v8 09/13] cpufreq: amd-pstate: add driver working mode switch support Perry Yuan
` (5 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
From: Perry Yuan <Perry.Yuan@amd.com>
add suspend and resume support for the AMD processors by amd_pstate_epp
driver instance.
When the CPPC is suspended, EPP driver will set EPP profile to 'power'
profile and set max/min perf to lowest perf value.
When resume happens, it will restore the MSR registers with
previous cached value.
Reviewed-by: Mario Limonciello <Mario.Limonciello@amd.com>
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
drivers/cpufreq/amd-pstate.c | 41 ++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index cb647f55a169..fc12d35bc7bd 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1107,6 +1107,45 @@ static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
return 0;
}
+static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
+{
+ struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+ int ret;
+
+ /* avoid suspending when EPP is not enabled */
+ if (cppc_state != AMD_PSTATE_ACTIVE)
+ return 0;
+
+ /* set this flag to avoid setting core offline*/
+ cpudata->suspended = true;
+
+ /* disable CPPC in lowlevel firmware */
+ ret = amd_pstate_enable(false);
+ if (ret)
+ pr_err("failed to suspend, return %d\n", ret);
+
+ return 0;
+}
+
+static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
+{
+ struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
+
+ if (cpudata->suspended) {
+ mutex_lock(&amd_pstate_limits_lock);
+
+ /* enable amd pstate from suspend state*/
+ amd_pstate_epp_reenable(cpudata);
+
+ mutex_unlock(&amd_pstate_limits_lock);
+
+ cpudata->suspended = false;
+ }
+
+ return 0;
+}
+
+
static struct cpufreq_driver amd_pstate_driver = {
.flags = CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
.verify = amd_pstate_verify,
@@ -1129,6 +1168,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
.update_limits = amd_pstate_epp_update_limits,
.offline = amd_pstate_epp_cpu_offline,
.online = amd_pstate_epp_cpu_online,
+ .suspend = amd_pstate_epp_suspend,
+ .resume = amd_pstate_epp_resume,
.name = "amd_pstate_epp",
.attr = amd_pstate_epp_attr,
};
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 08/13] cpufreq: amd-pstate: implement suspend and resume callbacks
2022-12-19 6:40 ` [PATCH v8 08/13] cpufreq: amd-pstate: implement suspend and resume callbacks Perry Yuan
@ 2022-12-23 8:12 ` Huang Rui
0 siblings, 0 replies; 45+ messages in thread
From: Huang Rui @ 2022-12-23 8:12 UTC (permalink / raw)
To: Yuan, Perry
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On Mon, Dec 19, 2022 at 02:40:37PM +0800, Yuan, Perry wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
>
> add suspend and resume support for the AMD processors by amd_pstate_epp
> driver instance.
>
> When the CPPC is suspended, EPP driver will set EPP profile to 'power'
> profile and set max/min perf to lowest perf value.
> When resume happens, it will restore the MSR registers with
> previous cached value.
>
> Reviewed-by: Mario Limonciello <Mario.Limonciello@amd.com>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
Acked-by: Huang Rui <ray.huang@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 41 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index cb647f55a169..fc12d35bc7bd 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1107,6 +1107,45 @@ static int amd_pstate_epp_verify_policy(struct cpufreq_policy_data *policy)
> return 0;
> }
>
> +static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
> +{
> + struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
> + int ret;
> +
> + /* avoid suspending when EPP is not enabled */
> + if (cppc_state != AMD_PSTATE_ACTIVE)
> + return 0;
> +
> + /* set this flag to avoid setting core offline*/
> + cpudata->suspended = true;
> +
> + /* disable CPPC in lowlevel firmware */
> + ret = amd_pstate_enable(false);
> + if (ret)
> + pr_err("failed to suspend, return %d\n", ret);
> +
> + return 0;
> +}
> +
> +static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
> +{
> + struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
> +
> + if (cpudata->suspended) {
> + mutex_lock(&amd_pstate_limits_lock);
> +
> + /* enable amd pstate from suspend state*/
> + amd_pstate_epp_reenable(cpudata);
> +
> + mutex_unlock(&amd_pstate_limits_lock);
> +
> + cpudata->suspended = false;
> + }
> +
> + return 0;
> +}
> +
> +
> static struct cpufreq_driver amd_pstate_driver = {
> .flags = CPUFREQ_CONST_LOOPS | CPUFREQ_NEED_UPDATE_LIMITS,
> .verify = amd_pstate_verify,
> @@ -1129,6 +1168,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
> .update_limits = amd_pstate_epp_update_limits,
> .offline = amd_pstate_epp_cpu_offline,
> .online = amd_pstate_epp_cpu_online,
> + .suspend = amd_pstate_epp_suspend,
> + .resume = amd_pstate_epp_resume,
> .name = "amd_pstate_epp",
> .attr = amd_pstate_epp_attr,
> };
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 09/13] cpufreq: amd-pstate: add driver working mode switch support
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (7 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 08/13] cpufreq: amd-pstate: implement suspend and resume callbacks Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-19 23:38 ` Limonciello, Mario
2022-12-23 8:56 ` Huang Rui
2022-12-19 6:40 ` [PATCH v8 10/13] Documentation: amd-pstate: add amd pstate driver mode introduction Perry Yuan
` (4 subsequent siblings)
13 siblings, 2 replies; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
From: Perry Yuan <Perry.Yuan@amd.com>
While amd-pstate driver was loaded with specific driver mode, it will
need to check which mode is enabled for the pstate driver,add this sysfs
entry to show the current status
$ cat /sys/devices/system/cpu/amd-pstate/status
active
Meanwhile, user can switch the pstate driver mode with writing mode
string to sysfs entry as below.
Enable passive mode:
$ sudo bash -c "echo passive > /sys/devices/system/cpu/amd-pstate/status"
Enable active mode (EPP driver mode):
$ sudo bash -c "echo active > /sys/devices/system/cpu/amd-pstate/status"
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
drivers/cpufreq/amd-pstate.c | 128 +++++++++++++++++++++++++++++++++++
1 file changed, 128 insertions(+)
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index fc12d35bc7bd..e8996e937e63 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -64,6 +64,7 @@ static struct cpufreq_driver amd_pstate_driver;
static struct cpufreq_driver amd_pstate_epp_driver;
static struct amd_cpudata **all_cpu_data;
static int cppc_state = AMD_PSTATE_DISABLE;
+struct kobject *amd_pstate_kobj;
static inline int get_mode_idx_from_str(const char *str, size_t size)
{
@@ -90,6 +91,8 @@ static struct amd_pstate_params global_params;
static DEFINE_MUTEX(amd_pstate_limits_lock);
static DEFINE_MUTEX(amd_pstate_driver_lock);
+static DEFINE_SPINLOCK(cppc_notify_lock);
+
static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
{
u64 epp;
@@ -644,6 +647,8 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
policy->driver_data = cpudata;
amd_pstate_boost_init(cpudata);
+ if (!default_pstate_driver->adjust_perf)
+ default_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
return 0;
@@ -784,12 +789,106 @@ static ssize_t show_energy_performance_preference(
return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
}
+static ssize_t amd_pstate_show_status(char *buf)
+{
+ if (!default_pstate_driver)
+ return sysfs_emit(buf, "off\n");
+
+ return sysfs_emit(buf, "%s\n", default_pstate_driver == &amd_pstate_epp_driver ?
+ "active" : "passive");
+}
+
+static void amd_pstate_driver_cleanup(void)
+{
+ unsigned int cpu;
+
+ cpus_read_lock();
+ for_each_online_cpu(cpu) {
+ if (all_cpu_data[cpu]) {
+ spin_lock(&cppc_notify_lock);
+ kfree(all_cpu_data[cpu]);
+ WRITE_ONCE(all_cpu_data[cpu], NULL);
+ spin_unlock(&cppc_notify_lock);
+ }
+ }
+ cpus_read_unlock();
+
+ default_pstate_driver = NULL;
+}
+
+static int amd_pstate_update_status(const char *buf, size_t size)
+{
+ if (size == 3 && !strncmp(buf, "off", size)) {
+ if (!default_pstate_driver)
+ return -EINVAL;
+
+ if (cppc_state == AMD_PSTATE_ACTIVE)
+ return -EBUSY;
+
+ cpufreq_unregister_driver(default_pstate_driver);
+ amd_pstate_driver_cleanup();
+ return 0;
+ }
+
+ if (size == 6 && !strncmp(buf, "active", size)) {
+ if (default_pstate_driver) {
+ if (default_pstate_driver == &amd_pstate_epp_driver)
+ return 0;
+ cpufreq_unregister_driver(default_pstate_driver);
+ default_pstate_driver = &amd_pstate_epp_driver;
+ cppc_state = AMD_PSTATE_ACTIVE;
+ }
+
+ return cpufreq_register_driver(default_pstate_driver);
+ }
+
+ if (size == 7 && !strncmp(buf, "passive", size)) {
+ if (default_pstate_driver) {
+ if (default_pstate_driver == &amd_pstate_driver)
+ return 0;
+ cpufreq_unregister_driver(default_pstate_driver);
+ cppc_state = AMD_PSTATE_PASSIVE;
+ default_pstate_driver = &amd_pstate_driver;
+ }
+
+ return cpufreq_register_driver(default_pstate_driver);
+ }
+
+ return -EINVAL;
+}
+
+static ssize_t show_status(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ ssize_t ret;
+
+ mutex_lock(&amd_pstate_driver_lock);
+ ret = amd_pstate_show_status(buf);
+ mutex_unlock(&amd_pstate_driver_lock);
+
+ return ret;
+}
+
+static ssize_t store_status(struct kobject *a, struct kobj_attribute *b,
+ const char *buf, size_t count)
+{
+ char *p = memchr(buf, '\n', count);
+ int ret;
+
+ mutex_lock(&amd_pstate_driver_lock);
+ ret = amd_pstate_update_status(buf, p ? p - buf : count);
+ mutex_unlock(&amd_pstate_driver_lock);
+
+ return ret < 0 ? ret : count;
+}
+
cpufreq_freq_attr_ro(amd_pstate_max_freq);
cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
cpufreq_freq_attr_ro(amd_pstate_highest_perf);
cpufreq_freq_attr_rw(energy_performance_preference);
cpufreq_freq_attr_ro(energy_performance_available_preferences);
+define_one_global_rw(status);
static struct freq_attr *amd_pstate_attr[] = {
&amd_pstate_max_freq,
@@ -807,6 +906,15 @@ static struct freq_attr *amd_pstate_epp_attr[] = {
NULL,
};
+static struct attribute *pstate_global_attributes[] = {
+ &status.attr,
+ NULL
+};
+
+static const struct attribute_group amd_pstate_global_attr_group = {
+ .attrs = pstate_global_attributes,
+};
+
static inline void update_boost_state(void)
{
u64 misc_en;
@@ -1228,6 +1336,26 @@ static int __init amd_pstate_init(void)
if (ret)
pr_err("failed to register with return %d\n", ret);
+ amd_pstate_kobj = kobject_create_and_add("amd-pstate", &cpu_subsys.dev_root->kobj);
+ if (!amd_pstate_kobj) {
+ ret = -EINVAL;
+ pr_err("global sysfs registration failed.\n");
+ goto kobject_free;
+ }
+
+ ret = sysfs_create_group(amd_pstate_kobj, &amd_pstate_global_attr_group);
+ if (ret) {
+ pr_err("sysfs attribute export failed with error %d.\n", ret);
+ goto global_attr_free;
+ }
+
+ return ret;
+
+global_attr_free:
+ kobject_put(amd_pstate_kobj);
+kobject_free:
+ kfree(cpudata);
+ cpufreq_unregister_driver(default_pstate_driver);
return ret;
}
device_initcall(amd_pstate_init);
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 09/13] cpufreq: amd-pstate: add driver working mode switch support
2022-12-19 6:40 ` [PATCH v8 09/13] cpufreq: amd-pstate: add driver working mode switch support Perry Yuan
@ 2022-12-19 23:38 ` Limonciello, Mario
2022-12-23 8:56 ` Huang Rui
1 sibling, 0 replies; 45+ messages in thread
From: Limonciello, Mario @ 2022-12-19 23:38 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/2022 00:40, Perry Yuan wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
>
> While amd-pstate driver was loaded with specific driver mode, it will
> need to check which mode is enabled for the pstate driver,add this sysfs
> entry to show the current status
>
> $ cat /sys/devices/system/cpu/amd-pstate/status
> active
>
> Meanwhile, user can switch the pstate driver mode with writing mode
> string to sysfs entry as below.
>
> Enable passive mode:
> $ sudo bash -c "echo passive > /sys/devices/system/cpu/amd-pstate/status"
>
> Enable active mode (EPP driver mode):
> $ sudo bash -c "echo active > /sys/devices/system/cpu/amd-pstate/status"
>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 128 +++++++++++++++++++++++++++++++++++
> 1 file changed, 128 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index fc12d35bc7bd..e8996e937e63 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -64,6 +64,7 @@ static struct cpufreq_driver amd_pstate_driver;
> static struct cpufreq_driver amd_pstate_epp_driver;
> static struct amd_cpudata **all_cpu_data;
> static int cppc_state = AMD_PSTATE_DISABLE;
> +struct kobject *amd_pstate_kobj;
>
> static inline int get_mode_idx_from_str(const char *str, size_t size)
> {
> @@ -90,6 +91,8 @@ static struct amd_pstate_params global_params;
> static DEFINE_MUTEX(amd_pstate_limits_lock);
> static DEFINE_MUTEX(amd_pstate_driver_lock);
>
> +static DEFINE_SPINLOCK(cppc_notify_lock);
> +
> static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
> {
> u64 epp;
> @@ -644,6 +647,8 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
> policy->driver_data = cpudata;
>
> amd_pstate_boost_init(cpudata);
> + if (!default_pstate_driver->adjust_perf)
> + default_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
>
> return 0;
>
> @@ -784,12 +789,106 @@ static ssize_t show_energy_performance_preference(
> return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
> }
>
> +static ssize_t amd_pstate_show_status(char *buf)
> +{
> + if (!default_pstate_driver)
> + return sysfs_emit(buf, "off\n");
> +
> + return sysfs_emit(buf, "%s\n", default_pstate_driver == &amd_pstate_epp_driver ?
> + "active" : "passive");
Didn't you introduce const strings for this in an earlier patch from the
series?
How about using the indices from that rather than hardcoding /another/
string.
Also keep in mind guided mode will be coming so
"case ? true_val : false_val"
So perhaps you should just have have some if cases so you don't have to
switch the whole thing out.
> +}
> +
> +static void amd_pstate_driver_cleanup(void)
> +{
> + unsigned int cpu;
> +
> + cpus_read_lock();
> + for_each_online_cpu(cpu) {
> + if (all_cpu_data[cpu]) {
> + spin_lock(&cppc_notify_lock);
> + kfree(all_cpu_data[cpu]);
> + WRITE_ONCE(all_cpu_data[cpu], NULL);
> + spin_unlock(&cppc_notify_lock);
> + }
> + }
> + cpus_read_unlock();
> +
> + default_pstate_driver = NULL;
> +}
> +
> +static int amd_pstate_update_status(const char *buf, size_t size)
> +{
Rather than looking at each size for each case, how about you just look
at the boundaries once? Something like this:
if (size > 7 || size < 3)
return -EINVAL;
Then you should be able to run strncmp for each of your cases.
> + if (size == 3 && !strncmp(buf, "off", size)) {
This seems like another case that would be good to use the const strings
you already introduced for state machine checking.
> + if (!default_pstate_driver)
> + return -EINVAL;
> +
> + if (cppc_state == AMD_PSTATE_ACTIVE)
> + return -EBUSY;
> +
> + cpufreq_unregister_driver(default_pstate_driver);
> + amd_pstate_driver_cleanup();
> + return 0;
> + }
> +
> + if (size == 6 && !strncmp(buf, "active", size)) {
This seems like another case that would be good to use the const strings
you already introduced for state machine checking.
> + if (default_pstate_driver) {
> + if (default_pstate_driver == &amd_pstate_epp_driver)
> + return 0;
> + cpufreq_unregister_driver(default_pstate_driver);
> + default_pstate_driver = &amd_pstate_epp_driver;
> + cppc_state = AMD_PSTATE_ACTIVE;
> + }
> +
> + return cpufreq_register_driver(default_pstate_driver);
> + }
> +
> + if (size == 7 && !strncmp(buf, "passive", size)) {
This seems like another case that would be good to use the const strings
you already introduced for state machine checking.
> + if (default_pstate_driver) {
> + if (default_pstate_driver == &amd_pstate_driver)
> + return 0;
> + cpufreq_unregister_driver(default_pstate_driver);
> + cppc_state = AMD_PSTATE_PASSIVE;
> + default_pstate_driver = &amd_pstate_driver;
> + }
> +
> + return cpufreq_register_driver(default_pstate_driver);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static ssize_t show_status(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + ssize_t ret;
> +
> + mutex_lock(&amd_pstate_driver_lock);
> + ret = amd_pstate_show_status(buf);
> + mutex_unlock(&amd_pstate_driver_lock);
> +
> + return ret;
> +}
> +
> +static ssize_t store_status(struct kobject *a, struct kobj_attribute *b,
> + const char *buf, size_t count)
> +{
> + char *p = memchr(buf, '\n', count);
> + int ret;
> +
> + mutex_lock(&amd_pstate_driver_lock);
> + ret = amd_pstate_update_status(buf, p ? p - buf : count);
> + mutex_unlock(&amd_pstate_driver_lock);
> +
> + return ret < 0 ? ret : count;
> +}
> +
> cpufreq_freq_attr_ro(amd_pstate_max_freq);
> cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
>
> cpufreq_freq_attr_ro(amd_pstate_highest_perf);
> cpufreq_freq_attr_rw(energy_performance_preference);
> cpufreq_freq_attr_ro(energy_performance_available_preferences);
> +define_one_global_rw(status);
>
> static struct freq_attr *amd_pstate_attr[] = {
> &amd_pstate_max_freq,
> @@ -807,6 +906,15 @@ static struct freq_attr *amd_pstate_epp_attr[] = {
> NULL,
> };
>
> +static struct attribute *pstate_global_attributes[] = {
> + &status.attr,
> + NULL
> +};
> +
> +static const struct attribute_group amd_pstate_global_attr_group = {
> + .attrs = pstate_global_attributes,
> +};
> +
> static inline void update_boost_state(void)
> {
> u64 misc_en;
> @@ -1228,6 +1336,26 @@ static int __init amd_pstate_init(void)
> if (ret)
> pr_err("failed to register with return %d\n", ret);
>
> + amd_pstate_kobj = kobject_create_and_add("amd-pstate", &cpu_subsys.dev_root->kobj);
> + if (!amd_pstate_kobj) {
> + ret = -EINVAL;
> + pr_err("global sysfs registration failed.\n");
> + goto kobject_free;
> + }
> +
> + ret = sysfs_create_group(amd_pstate_kobj, &amd_pstate_global_attr_group);
> + if (ret) {
> + pr_err("sysfs attribute export failed with error %d.\n", ret);
> + goto global_attr_free;
> + }
> +
> + return ret;
> +
> +global_attr_free:
> + kobject_put(amd_pstate_kobj);
> +kobject_free:
> + kfree(cpudata);
> + cpufreq_unregister_driver(default_pstate_driver);
> return ret;
> }
> device_initcall(amd_pstate_init);
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 09/13] cpufreq: amd-pstate: add driver working mode switch support
2022-12-19 6:40 ` [PATCH v8 09/13] cpufreq: amd-pstate: add driver working mode switch support Perry Yuan
2022-12-19 23:38 ` Limonciello, Mario
@ 2022-12-23 8:56 ` Huang Rui
2022-12-25 16:41 ` Yuan, Perry
1 sibling, 1 reply; 45+ messages in thread
From: Huang Rui @ 2022-12-23 8:56 UTC (permalink / raw)
To: Yuan, Perry
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On Mon, Dec 19, 2022 at 02:40:38PM +0800, Yuan, Perry wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
>
> While amd-pstate driver was loaded with specific driver mode, it will
> need to check which mode is enabled for the pstate driver,add this sysfs
> entry to show the current status
>
> $ cat /sys/devices/system/cpu/amd-pstate/status
> active
>
> Meanwhile, user can switch the pstate driver mode with writing mode
> string to sysfs entry as below.
>
> Enable passive mode:
> $ sudo bash -c "echo passive > /sys/devices/system/cpu/amd-pstate/status"
>
> Enable active mode (EPP driver mode):
> $ sudo bash -c "echo active > /sys/devices/system/cpu/amd-pstate/status"
>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 128 +++++++++++++++++++++++++++++++++++
> 1 file changed, 128 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index fc12d35bc7bd..e8996e937e63 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -64,6 +64,7 @@ static struct cpufreq_driver amd_pstate_driver;
> static struct cpufreq_driver amd_pstate_epp_driver;
> static struct amd_cpudata **all_cpu_data;
> static int cppc_state = AMD_PSTATE_DISABLE;
> +struct kobject *amd_pstate_kobj;
>
> static inline int get_mode_idx_from_str(const char *str, size_t size)
> {
> @@ -90,6 +91,8 @@ static struct amd_pstate_params global_params;
> static DEFINE_MUTEX(amd_pstate_limits_lock);
> static DEFINE_MUTEX(amd_pstate_driver_lock);
>
> +static DEFINE_SPINLOCK(cppc_notify_lock);
> +
> static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
> {
> u64 epp;
> @@ -644,6 +647,8 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
> policy->driver_data = cpudata;
>
> amd_pstate_boost_init(cpudata);
> + if (!default_pstate_driver->adjust_perf)
> + default_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
If default_driver is passive driver, it will be programed as
amd_pstate_adjust_perf. If not, the epp driver won't need adjust_perf.
Thanks,
Ray
>
> return 0;
>
> @@ -784,12 +789,106 @@ static ssize_t show_energy_performance_preference(
> return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
> }
>
> +static ssize_t amd_pstate_show_status(char *buf)
> +{
> + if (!default_pstate_driver)
> + return sysfs_emit(buf, "off\n");
> +
> + return sysfs_emit(buf, "%s\n", default_pstate_driver == &amd_pstate_epp_driver ?
> + "active" : "passive");
> +}
> +
> +static void amd_pstate_driver_cleanup(void)
> +{
> + unsigned int cpu;
> +
> + cpus_read_lock();
> + for_each_online_cpu(cpu) {
> + if (all_cpu_data[cpu]) {
> + spin_lock(&cppc_notify_lock);
> + kfree(all_cpu_data[cpu]);
> + WRITE_ONCE(all_cpu_data[cpu], NULL);
> + spin_unlock(&cppc_notify_lock);
> + }
> + }
> + cpus_read_unlock();
> +
> + default_pstate_driver = NULL;
> +}
> +
> +static int amd_pstate_update_status(const char *buf, size_t size)
> +{
> + if (size == 3 && !strncmp(buf, "off", size)) {
> + if (!default_pstate_driver)
> + return -EINVAL;
> +
> + if (cppc_state == AMD_PSTATE_ACTIVE)
> + return -EBUSY;
> +
> + cpufreq_unregister_driver(default_pstate_driver);
> + amd_pstate_driver_cleanup();
> + return 0;
> + }
> +
> + if (size == 6 && !strncmp(buf, "active", size)) {
> + if (default_pstate_driver) {
> + if (default_pstate_driver == &amd_pstate_epp_driver)
> + return 0;
> + cpufreq_unregister_driver(default_pstate_driver);
> + default_pstate_driver = &amd_pstate_epp_driver;
> + cppc_state = AMD_PSTATE_ACTIVE;
> + }
> +
> + return cpufreq_register_driver(default_pstate_driver);
> + }
> +
> + if (size == 7 && !strncmp(buf, "passive", size)) {
> + if (default_pstate_driver) {
> + if (default_pstate_driver == &amd_pstate_driver)
> + return 0;
> + cpufreq_unregister_driver(default_pstate_driver);
> + cppc_state = AMD_PSTATE_PASSIVE;
> + default_pstate_driver = &amd_pstate_driver;
> + }
> +
> + return cpufreq_register_driver(default_pstate_driver);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static ssize_t show_status(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + ssize_t ret;
> +
> + mutex_lock(&amd_pstate_driver_lock);
> + ret = amd_pstate_show_status(buf);
> + mutex_unlock(&amd_pstate_driver_lock);
> +
> + return ret;
> +}
> +
> +static ssize_t store_status(struct kobject *a, struct kobj_attribute *b,
> + const char *buf, size_t count)
> +{
> + char *p = memchr(buf, '\n', count);
> + int ret;
> +
> + mutex_lock(&amd_pstate_driver_lock);
> + ret = amd_pstate_update_status(buf, p ? p - buf : count);
> + mutex_unlock(&amd_pstate_driver_lock);
> +
> + return ret < 0 ? ret : count;
> +}
> +
> cpufreq_freq_attr_ro(amd_pstate_max_freq);
> cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
>
> cpufreq_freq_attr_ro(amd_pstate_highest_perf);
> cpufreq_freq_attr_rw(energy_performance_preference);
> cpufreq_freq_attr_ro(energy_performance_available_preferences);
> +define_one_global_rw(status);
>
> static struct freq_attr *amd_pstate_attr[] = {
> &amd_pstate_max_freq,
> @@ -807,6 +906,15 @@ static struct freq_attr *amd_pstate_epp_attr[] = {
> NULL,
> };
>
> +static struct attribute *pstate_global_attributes[] = {
> + &status.attr,
> + NULL
> +};
> +
> +static const struct attribute_group amd_pstate_global_attr_group = {
> + .attrs = pstate_global_attributes,
> +};
> +
> static inline void update_boost_state(void)
> {
> u64 misc_en;
> @@ -1228,6 +1336,26 @@ static int __init amd_pstate_init(void)
> if (ret)
> pr_err("failed to register with return %d\n", ret);
>
> + amd_pstate_kobj = kobject_create_and_add("amd-pstate", &cpu_subsys.dev_root->kobj);
> + if (!amd_pstate_kobj) {
> + ret = -EINVAL;
> + pr_err("global sysfs registration failed.\n");
> + goto kobject_free;
> + }
> +
> + ret = sysfs_create_group(amd_pstate_kobj, &amd_pstate_global_attr_group);
> + if (ret) {
> + pr_err("sysfs attribute export failed with error %d.\n", ret);
> + goto global_attr_free;
> + }
> +
> + return ret;
> +
> +global_attr_free:
> + kobject_put(amd_pstate_kobj);
> +kobject_free:
> + kfree(cpudata);
> + cpufreq_unregister_driver(default_pstate_driver);
> return ret;
> }
> device_initcall(amd_pstate_init);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v8 09/13] cpufreq: amd-pstate: add driver working mode switch support
2022-12-23 8:56 ` Huang Rui
@ 2022-12-25 16:41 ` Yuan, Perry
0 siblings, 0 replies; 45+ messages in thread
From: Yuan, Perry @ 2022-12-25 16:41 UTC (permalink / raw)
To: Huang, Ray
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
[AMD Official Use Only - General]
> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Friday, December 23, 2022 4:56 PM
> 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 v8 09/13] cpufreq: amd-pstate: add driver working mode
> switch support
>
> On Mon, Dec 19, 2022 at 02:40:38PM +0800, Yuan, Perry wrote:
> > From: Perry Yuan <Perry.Yuan@amd.com>
> >
> > While amd-pstate driver was loaded with specific driver mode, it will
> > need to check which mode is enabled for the pstate driver,add this
> > sysfs entry to show the current status
> >
> > $ cat /sys/devices/system/cpu/amd-pstate/status
> > active
> >
> > Meanwhile, user can switch the pstate driver mode with writing mode
> > string to sysfs entry as below.
> >
> > Enable passive mode:
> > $ sudo bash -c "echo passive > /sys/devices/system/cpu/amd-
> pstate/status"
> >
> > Enable active mode (EPP driver mode):
> > $ sudo bash -c "echo active > /sys/devices/system/cpu/amd-pstate/status"
> >
> > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > ---
> > drivers/cpufreq/amd-pstate.c | 128
> > +++++++++++++++++++++++++++++++++++
> > 1 file changed, 128 insertions(+)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index fc12d35bc7bd..e8996e937e63
> 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -64,6 +64,7 @@ static struct cpufreq_driver amd_pstate_driver;
> > static struct cpufreq_driver amd_pstate_epp_driver; static struct
> > amd_cpudata **all_cpu_data; static int cppc_state =
> > AMD_PSTATE_DISABLE;
> > +struct kobject *amd_pstate_kobj;
> >
> > static inline int get_mode_idx_from_str(const char *str, size_t size)
> > { @@ -90,6 +91,8 @@ static struct amd_pstate_params global_params;
> > static DEFINE_MUTEX(amd_pstate_limits_lock);
> > static DEFINE_MUTEX(amd_pstate_driver_lock);
> >
> > +static DEFINE_SPINLOCK(cppc_notify_lock);
> > +
> > static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64
> > cppc_req_cached) {
> > u64 epp;
> > @@ -644,6 +647,8 @@ static int amd_pstate_cpu_init(struct
> cpufreq_policy *policy)
> > policy->driver_data = cpudata;
> >
> > amd_pstate_boost_init(cpudata);
> > + if (!default_pstate_driver->adjust_perf)
> > + default_pstate_driver->adjust_perf =
> amd_pstate_adjust_perf;
>
> If default_driver is passive driver, it will be programed as
> amd_pstate_adjust_perf. If not, the epp driver won't need adjust_perf.
>
> Thanks,
> Ray
The adjust_perf() should be set again when switching from epp to cppc driver.
The above amd_pstate_cpu_init () is called when amd-pstate driver loading, epp driver will not load it.
>
> >
> > return 0;
> >
> > @@ -784,12 +789,106 @@ static ssize_t
> show_energy_performance_preference(
> > return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]); }
> >
> > +static ssize_t amd_pstate_show_status(char *buf) {
> > + if (!default_pstate_driver)
> > + return sysfs_emit(buf, "off\n");
> > +
> > + return sysfs_emit(buf, "%s\n", default_pstate_driver ==
> &amd_pstate_epp_driver ?
> > + "active" : "passive");
> > +}
> > +
> > +static void amd_pstate_driver_cleanup(void) {
> > + unsigned int cpu;
> > +
> > + cpus_read_lock();
> > + for_each_online_cpu(cpu) {
> > + if (all_cpu_data[cpu]) {
> > + spin_lock(&cppc_notify_lock);
> > + kfree(all_cpu_data[cpu]);
> > + WRITE_ONCE(all_cpu_data[cpu], NULL);
> > + spin_unlock(&cppc_notify_lock);
> > + }
> > + }
> > + cpus_read_unlock();
> > +
> > + default_pstate_driver = NULL;
> > +}
> > +
> > +static int amd_pstate_update_status(const char *buf, size_t size) {
> > + if (size == 3 && !strncmp(buf, "off", size)) {
> > + if (!default_pstate_driver)
> > + return -EINVAL;
> > +
> > + if (cppc_state == AMD_PSTATE_ACTIVE)
> > + return -EBUSY;
> > +
> > + cpufreq_unregister_driver(default_pstate_driver);
> > + amd_pstate_driver_cleanup();
> > + return 0;
> > + }
> > +
> > + if (size == 6 && !strncmp(buf, "active", size)) {
> > + if (default_pstate_driver) {
> > + if (default_pstate_driver ==
> &amd_pstate_epp_driver)
> > + return 0;
> > + cpufreq_unregister_driver(default_pstate_driver);
> > + default_pstate_driver = &amd_pstate_epp_driver;
> > + cppc_state = AMD_PSTATE_ACTIVE;
> > + }
> > +
> > + return cpufreq_register_driver(default_pstate_driver);
> > + }
> > +
> > + if (size == 7 && !strncmp(buf, "passive", size)) {
> > + if (default_pstate_driver) {
> > + if (default_pstate_driver == &amd_pstate_driver)
> > + return 0;
> > + cpufreq_unregister_driver(default_pstate_driver);
> > + cppc_state = AMD_PSTATE_PASSIVE;
> > + default_pstate_driver = &amd_pstate_driver;
> > + }
> > +
> > + return cpufreq_register_driver(default_pstate_driver);
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static ssize_t show_status(struct kobject *kobj,
> > + struct kobj_attribute *attr, char *buf) {
> > + ssize_t ret;
> > +
> > + mutex_lock(&amd_pstate_driver_lock);
> > + ret = amd_pstate_show_status(buf);
> > + mutex_unlock(&amd_pstate_driver_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t store_status(struct kobject *a, struct kobj_attribute *b,
> > + const char *buf, size_t count) {
> > + char *p = memchr(buf, '\n', count);
> > + int ret;
> > +
> > + mutex_lock(&amd_pstate_driver_lock);
> > + ret = amd_pstate_update_status(buf, p ? p - buf : count);
> > + mutex_unlock(&amd_pstate_driver_lock);
> > +
> > + return ret < 0 ? ret : count;
> > +}
> > +
> > cpufreq_freq_attr_ro(amd_pstate_max_freq);
> > cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
> >
> > cpufreq_freq_attr_ro(amd_pstate_highest_perf);
> > cpufreq_freq_attr_rw(energy_performance_preference);
> > cpufreq_freq_attr_ro(energy_performance_available_preferences);
> > +define_one_global_rw(status);
> >
> > static struct freq_attr *amd_pstate_attr[] = {
> > &amd_pstate_max_freq,
> > @@ -807,6 +906,15 @@ static struct freq_attr *amd_pstate_epp_attr[] = {
> > NULL,
> > };
> >
> > +static struct attribute *pstate_global_attributes[] = {
> > + &status.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group amd_pstate_global_attr_group = {
> > + .attrs = pstate_global_attributes,
> > +};
> > +
> > static inline void update_boost_state(void) {
> > u64 misc_en;
> > @@ -1228,6 +1336,26 @@ static int __init amd_pstate_init(void)
> > if (ret)
> > pr_err("failed to register with return %d\n", ret);
> >
> > + amd_pstate_kobj = kobject_create_and_add("amd-pstate",
> &cpu_subsys.dev_root->kobj);
> > + if (!amd_pstate_kobj) {
> > + ret = -EINVAL;
> > + pr_err("global sysfs registration failed.\n");
> > + goto kobject_free;
> > + }
> > +
> > + ret = sysfs_create_group(amd_pstate_kobj,
> &amd_pstate_global_attr_group);
> > + if (ret) {
> > + pr_err("sysfs attribute export failed with error %d.\n", ret);
> > + goto global_attr_free;
> > + }
> > +
> > + return ret;
> > +
> > +global_attr_free:
> > + kobject_put(amd_pstate_kobj);
> > +kobject_free:
> > + kfree(cpudata);
> > + cpufreq_unregister_driver(default_pstate_driver);
> > return ret;
> > }
> > device_initcall(amd_pstate_init);
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 10/13] Documentation: amd-pstate: add amd pstate driver mode introduction
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (8 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 09/13] cpufreq: amd-pstate: add driver working mode switch support Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-19 23:28 ` Limonciello, Mario
2022-12-19 6:40 ` [PATCH v8 11/13] Documentation: introduce amd pstate active mode kernel command line options Perry Yuan
` (3 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
From: Perry Yuan <Perry.Yuan@amd.com>
Introduce ``amd-pstate`` CPPC has two operation modes:
* CPPC Autonomous (active) mode
* CPPC non-autonomous (passive) mode.
active mode and passive mode can be chosen by different kernel parameters.
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
Documentation/admin-guide/pm/amd-pstate.rst | 26 +++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
index 33ab8ec8fc2f..62744dae3c5f 100644
--- a/Documentation/admin-guide/pm/amd-pstate.rst
+++ b/Documentation/admin-guide/pm/amd-pstate.rst
@@ -299,8 +299,30 @@ module which supports the new AMD P-States mechanism on most of the future AMD
platforms. The AMD P-States mechanism is the more performance and energy
efficiency frequency management method on AMD processors.
-Kernel Module Options for ``amd-pstate``
-=========================================
+
+AMD Pstate Driver Operation Modes
+=================================
+
+``amd_pstate`` CPPC has two operation modes: CPPC Autonomous(active) mode and
+CPPC non-autonomous(passive) mode.
+active mode and passive mode can be chosen by different kernel parameters.
+When in Autonomous mode, CPPC ignores requests done in the Desired Performance
+Target register and takes into account only the values set to the Minimum requested
+performance, Maximum requested performance, and Energy Performance Preference
+registers. When Autonomous is disabled, it only considers the Desired Performance Target.
+
+Active Mode
+------------
+
+``amd_pstate=active``
+
+This is the low-level firmware control mode which is implemented by ``amd_pstate_epp``
+driver with ``amd_pstate=active`` passed to the kernel in the command line.
+In this mode, ``amd_pstate_epp`` driver provides a hint to the hardware if software
+wants to bias toward performance (0x0) or energy efficiency (0xff) to the CPPC firmware.
+then CPPC power algorithm will calculate the runtime workload and adjust the realtime
+cores frequency according to the power supply and thermal, core voltage and some other
+hardware conditions.
Passive Mode
------------
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 10/13] Documentation: amd-pstate: add amd pstate driver mode introduction
2022-12-19 6:40 ` [PATCH v8 10/13] Documentation: amd-pstate: add amd pstate driver mode introduction Perry Yuan
@ 2022-12-19 23:28 ` Limonciello, Mario
0 siblings, 0 replies; 45+ messages in thread
From: Limonciello, Mario @ 2022-12-19 23:28 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/2022 00:40, Perry Yuan wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
>
> Introduce ``amd-pstate`` CPPC has two operation modes:
Drop the word Introduce.
> * CPPC Autonomous (active) mode
> * CPPC non-autonomous (passive) mode.
> active mode and passive mode can be chosen by different kernel parameters.
>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
With nit fixed:
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Documentation/admin-guide/pm/amd-pstate.rst | 26 +++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index 33ab8ec8fc2f..62744dae3c5f 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -299,8 +299,30 @@ module which supports the new AMD P-States mechanism on most of the future AMD
> platforms. The AMD P-States mechanism is the more performance and energy
> efficiency frequency management method on AMD processors.
>
> -Kernel Module Options for ``amd-pstate``
> -=========================================
> +
> +AMD Pstate Driver Operation Modes
> +=================================
> +
> +``amd_pstate`` CPPC has two operation modes: CPPC Autonomous(active) mode and
> +CPPC non-autonomous(passive) mode.
> +active mode and passive mode can be chosen by different kernel parameters.
> +When in Autonomous mode, CPPC ignores requests done in the Desired Performance
> +Target register and takes into account only the values set to the Minimum requested
> +performance, Maximum requested performance, and Energy Performance Preference
> +registers. When Autonomous is disabled, it only considers the Desired Performance Target.
> +
> +Active Mode
> +------------
> +
> +``amd_pstate=active``
> +
> +This is the low-level firmware control mode which is implemented by ``amd_pstate_epp``
> +driver with ``amd_pstate=active`` passed to the kernel in the command line.
> +In this mode, ``amd_pstate_epp`` driver provides a hint to the hardware if software
> +wants to bias toward performance (0x0) or energy efficiency (0xff) to the CPPC firmware.
> +then CPPC power algorithm will calculate the runtime workload and adjust the realtime
> +cores frequency according to the power supply and thermal, core voltage and some other
> +hardware conditions.
>
> Passive Mode
> ------------
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 11/13] Documentation: introduce amd pstate active mode kernel command line options
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (9 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 10/13] Documentation: amd-pstate: add amd pstate driver mode introduction Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-19 23:29 ` Limonciello, Mario
2022-12-19 6:40 ` [PATCH v8 12/13] cpufreq: amd-pstate: convert sprintf with sysfs_emit() Perry Yuan
` (2 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
AMD Pstate driver support another firmware based autonomous mode
with "amd_pstate=active" added to the kernel command line.
In autonomous mode SMU firmware decides frequencies at 1 ms timescale
based on workload utilization, usage in other IPs, infrastructure
limits such as power, thermals and so on.
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 42af9ca0127e..73a02816f6f8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6970,3 +6970,10 @@
management firmware translates the requests into actual
hardware states (core frequency, data fabric and memory
clocks etc.)
+ active
+ Use amd_pstate_epp driver instance as the scaling driver,
+ driver provides a hint to the hardware if software wants
+ to bias toward performance (0x0) or energy efficiency (0xff)
+ to the CPPC firmware. then CPPC power algorithm will
+ calculate the runtime workload and adjust the realtime cores
+ frequency.
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 11/13] Documentation: introduce amd pstate active mode kernel command line options
2022-12-19 6:40 ` [PATCH v8 11/13] Documentation: introduce amd pstate active mode kernel command line options Perry Yuan
@ 2022-12-19 23:29 ` Limonciello, Mario
0 siblings, 0 replies; 45+ messages in thread
From: Limonciello, Mario @ 2022-12-19 23:29 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/2022 00:40, Perry Yuan wrote:
> AMD Pstate driver support another firmware based autonomous mode
> with "amd_pstate=active" added to the kernel command line.
> In autonomous mode SMU firmware decides frequencies at 1 ms timescale
This might be the case right now, but I don't know that it will always
be this timescale. Suggest to drop drop "at 1ms timescale".
> based on workload utilization, usage in other IPs, infrastructure
> limits such as power, thermals and so on.
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
With nit fixed:
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 42af9ca0127e..73a02816f6f8 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6970,3 +6970,10 @@
> management firmware translates the requests into actual
> hardware states (core frequency, data fabric and memory
> clocks etc.)
> + active
> + Use amd_pstate_epp driver instance as the scaling driver,
> + driver provides a hint to the hardware if software wants
> + to bias toward performance (0x0) or energy efficiency (0xff)
> + to the CPPC firmware. then CPPC power algorithm will
> + calculate the runtime workload and adjust the realtime cores
> + frequency.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 12/13] cpufreq: amd-pstate: convert sprintf with sysfs_emit()
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (10 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 11/13] Documentation: introduce amd pstate active mode kernel command line options Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-19 23:31 ` Limonciello, Mario
2022-12-23 7:45 ` Huang Rui
2022-12-19 6:40 ` [PATCH v8 13/13] Documentation: amd-pstate: introduce new global sysfs attributes Perry Yuan
2022-12-20 18:13 ` [PATCH v8 00/13] Implement AMD Pstate EPP Driver Tor Vic
13 siblings, 2 replies; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
replace the sprintf with a more generic sysfs_emit function
No potential function impact
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
drivers/cpufreq/amd-pstate.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index e8996e937e63..d8b182614c18 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -709,7 +709,7 @@ static ssize_t show_amd_pstate_max_freq(struct cpufreq_policy *policy,
if (max_freq < 0)
return max_freq;
- return sprintf(&buf[0], "%u\n", max_freq);
+ return sysfs_emit(buf, "%u\n", max_freq);
}
static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *policy,
@@ -722,7 +722,7 @@ static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *poli
if (freq < 0)
return freq;
- return sprintf(&buf[0], "%u\n", freq);
+ return sysfs_emit(buf, "%u\n", freq);
}
/*
@@ -737,7 +737,7 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
perf = READ_ONCE(cpudata->highest_perf);
- return sprintf(&buf[0], "%u\n", perf);
+ return sysfs_emit(buf, "%u\n", perf);
}
static ssize_t show_energy_performance_available_preferences(
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 12/13] cpufreq: amd-pstate: convert sprintf with sysfs_emit()
2022-12-19 6:40 ` [PATCH v8 12/13] cpufreq: amd-pstate: convert sprintf with sysfs_emit() Perry Yuan
@ 2022-12-19 23:31 ` Limonciello, Mario
2022-12-25 16:42 ` Yuan, Perry
2022-12-23 7:45 ` Huang Rui
1 sibling, 1 reply; 45+ messages in thread
From: Limonciello, Mario @ 2022-12-19 23:31 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/2022 00:40, Perry Yuan wrote:
> replace the sprintf with a more generic sysfs_emit function
>
> No potential function impact
Anything is possible! "No intended functional impact." I believe is
what you meant =)
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
With commit message fixed:
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index e8996e937e63..d8b182614c18 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -709,7 +709,7 @@ static ssize_t show_amd_pstate_max_freq(struct cpufreq_policy *policy,
> if (max_freq < 0)
> return max_freq;
>
> - return sprintf(&buf[0], "%u\n", max_freq);
> + return sysfs_emit(buf, "%u\n", max_freq);
> }
>
> static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *policy,
> @@ -722,7 +722,7 @@ static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *poli
> if (freq < 0)
> return freq;
>
> - return sprintf(&buf[0], "%u\n", freq);
> + return sysfs_emit(buf, "%u\n", freq);
> }
>
> /*
> @@ -737,7 +737,7 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
>
> perf = READ_ONCE(cpudata->highest_perf);
>
> - return sprintf(&buf[0], "%u\n", perf);
> + return sysfs_emit(buf, "%u\n", perf);
> }
>
> static ssize_t show_energy_performance_available_preferences(
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v8 12/13] cpufreq: amd-pstate: convert sprintf with sysfs_emit()
2022-12-19 23:31 ` Limonciello, Mario
@ 2022-12-25 16:42 ` Yuan, Perry
0 siblings, 0 replies; 45+ messages in thread
From: Yuan, Perry @ 2022-12-25 16:42 UTC (permalink / raw)
To: Limonciello, Mario, rafael.j.wysocki@intel.com, Huang, Ray,
viresh.kumar@linaro.org
Cc: Sharma, Deepak, Fontenot, Nathan, Deucher, Alexander,
Huang, Shimmer, Du, Xiaojian, Meng, Li (Jassmine), Karny, Wyes,
linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
[AMD Official Use Only - General]
Hi Mario.
> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Tuesday, December 20, 2022 7:31 AM
> To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com; Huang,
> Ray <Ray.Huang@amd.com>; viresh.kumar@linaro.org
> Cc: 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 v8 12/13] cpufreq: amd-pstate: convert sprintf with
> sysfs_emit()
>
> On 12/19/2022 00:40, Perry Yuan wrote:
> > replace the sprintf with a more generic sysfs_emit function
> >
> > No potential function impact
>
> Anything is possible! "No intended functional impact." I believe is what you
> meant =)
>
> >
> > Signed-off-by: Perry Yuan <perry.yuan@amd.com>
>
> With commit message fixed:
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Thanks for your review , all the R-B flags have been picked up in v9 .
And other feedbacks from you have been changed in v9 as well.
Perry.
>
> > ---
> > drivers/cpufreq/amd-pstate.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index e8996e937e63..d8b182614c18
> 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -709,7 +709,7 @@ static ssize_t show_amd_pstate_max_freq(struct
> cpufreq_policy *policy,
> > if (max_freq < 0)
> > return max_freq;
> >
> > - return sprintf(&buf[0], "%u\n", max_freq);
> > + return sysfs_emit(buf, "%u\n", max_freq);
> > }
> >
> > static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct
> > cpufreq_policy *policy, @@ -722,7 +722,7 @@ static ssize_t
> show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *poli
> > if (freq < 0)
> > return freq;
> >
> > - return sprintf(&buf[0], "%u\n", freq);
> > + return sysfs_emit(buf, "%u\n", freq);
> > }
> >
> > /*
> > @@ -737,7 +737,7 @@ static ssize_t
> show_amd_pstate_highest_perf(struct
> > cpufreq_policy *policy,
> >
> > perf = READ_ONCE(cpudata->highest_perf);
> >
> > - return sprintf(&buf[0], "%u\n", perf);
> > + return sysfs_emit(buf, "%u\n", perf);
> > }
> >
> > static ssize_t show_energy_performance_available_preferences(
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 12/13] cpufreq: amd-pstate: convert sprintf with sysfs_emit()
2022-12-19 6:40 ` [PATCH v8 12/13] cpufreq: amd-pstate: convert sprintf with sysfs_emit() Perry Yuan
2022-12-19 23:31 ` Limonciello, Mario
@ 2022-12-23 7:45 ` Huang Rui
1 sibling, 0 replies; 45+ messages in thread
From: Huang Rui @ 2022-12-23 7:45 UTC (permalink / raw)
To: Yuan, Perry
Cc: rafael.j.wysocki@intel.com, Limonciello, Mario,
viresh.kumar@linaro.org, Sharma, Deepak, Fontenot, Nathan,
Deucher, Alexander, Huang, Shimmer, Du, Xiaojian,
Meng, Li (Jassmine), Karny, Wyes, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On Mon, Dec 19, 2022 at 02:40:41PM +0800, Yuan, Perry wrote:
> replace the sprintf with a more generic sysfs_emit function
>
> No potential function impact
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
Acked-by: Huang Rui <ray.huang@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index e8996e937e63..d8b182614c18 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -709,7 +709,7 @@ static ssize_t show_amd_pstate_max_freq(struct cpufreq_policy *policy,
> if (max_freq < 0)
> return max_freq;
>
> - return sprintf(&buf[0], "%u\n", max_freq);
> + return sysfs_emit(buf, "%u\n", max_freq);
> }
>
> static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *policy,
> @@ -722,7 +722,7 @@ static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *poli
> if (freq < 0)
> return freq;
>
> - return sprintf(&buf[0], "%u\n", freq);
> + return sysfs_emit(buf, "%u\n", freq);
> }
>
> /*
> @@ -737,7 +737,7 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
>
> perf = READ_ONCE(cpudata->highest_perf);
>
> - return sprintf(&buf[0], "%u\n", perf);
> + return sysfs_emit(buf, "%u\n", perf);
> }
>
> static ssize_t show_energy_performance_available_preferences(
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 13/13] Documentation: amd-pstate: introduce new global sysfs attributes
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (11 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 12/13] cpufreq: amd-pstate: convert sprintf with sysfs_emit() Perry Yuan
@ 2022-12-19 6:40 ` Perry Yuan
2022-12-19 22:35 ` Limonciello, Mario
2022-12-20 18:13 ` [PATCH v8 00/13] Implement AMD Pstate EPP Driver Tor Vic
13 siblings, 1 reply; 45+ messages in thread
From: Perry Yuan @ 2022-12-19 6:40 UTC (permalink / raw)
To: rafael.j.wysocki, Mario.Limonciello, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
The new amd-pstate driver support to switch the driver working mode and
use can switch the driver mode within the sysfs attributes in the below
path and check current mode
$ cd /sys/devices/system/cpu/amd-pstate
check driver mode:
$ cat /sys/devices/system/cpu/amd-pstate/status
switch mode:
$ sudo bash -c "echo passive > /sys/devices/system/cpu/amd-pstate/status"
or
$ sudo bash -c "echo active > /sys/devices/system/cpu/amd-pstate/status"
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
Documentation/admin-guide/pm/amd-pstate.rst | 28 +++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
index 62744dae3c5f..f3a8f8a66783 100644
--- a/Documentation/admin-guide/pm/amd-pstate.rst
+++ b/Documentation/admin-guide/pm/amd-pstate.rst
@@ -339,6 +339,34 @@ processor must provide at least nominal performance requested and go higher if c
operating conditions allow.
+User Space Interface in ``sysfs``
+=================================
+
+Global Attributes
+-----------------
+
+``amd-pstate`` exposes several global attributes (files) in ``sysfs`` to
+control its functionality at the system level. They are located in the
+``/sys/devices/system/cpu/amd-pstate/`` directory and affect all CPUs.
+
+``status``
+ Operation mode of the driver: "active", "passive" or "off".
+
+ "active"
+ The driver is functional and in the ``active mode``
+
+ "passive"
+ The driver is functional and in the ``passive mode``
+
+ "off"
+ The driver is unregistered and not functional now.
+
+ This attribute can be written to in order to change the driver's
+ operation mode or to unregister it. The string written to it must be
+ one of the possible values of it and, if successful, the write will
+ cause the driver to switch over to the operation mode represented by
+ that string - or to be unregistered in the "off" case.
+
``cpupower`` tool support for ``amd-pstate``
===============================================
--
2.34.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v8 13/13] Documentation: amd-pstate: introduce new global sysfs attributes
2022-12-19 6:40 ` [PATCH v8 13/13] Documentation: amd-pstate: introduce new global sysfs attributes Perry Yuan
@ 2022-12-19 22:35 ` Limonciello, Mario
0 siblings, 0 replies; 45+ messages in thread
From: Limonciello, Mario @ 2022-12-19 22:35 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, ray.huang, viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 12/19/2022 00:40, Perry Yuan wrote:
> The new amd-pstate driver support to switch the driver working mode and
Words like "new" don't age well.
> use can switch the driver mode within the sysfs attributes in the below
> path and check current mode
Perhaps "The amd-pstate driver supports switching working modes at runtime.
Users can view and change modes by interacting with the "status" sysfs
attribute.
>
> $ cd /sys/devices/system/cpu/amd-pstate
>
No need to change directories; you're demonstrating below using full paths.
> check driver mode:
> $ cat /sys/devices/system/cpu/amd-pstate/status
>
> switch mode:
> $ sudo bash -c "echo passive > /sys/devices/system/cpu/amd-pstate/status"
> or
> $ sudo bash -c "echo active > /sys/devices/system/cpu/amd-pstate/status"
Another way you could suggest this:
# echo "passive" | sudo tee /sys/devices/system/cpu/amd-pstate/status
or
# echo "active" | sudo tee /sys/devices/system/cpu/amd-pstate/status
I don't feel strongly which way to suggest though.
>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
> Documentation/admin-guide/pm/amd-pstate.rst | 28 +++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index 62744dae3c5f..f3a8f8a66783 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -339,6 +339,34 @@ processor must provide at least nominal performance requested and go higher if c
> operating conditions allow.
>
>
> +User Space Interface in ``sysfs``
> +=================================
> +
> +Global Attributes
> +-----------------
> +
> +``amd-pstate`` exposes several global attributes (files) in ``sysfs`` to
> +control its functionality at the system level. They are located in the
> +``/sys/devices/system/cpu/amd-pstate/`` directory and affect all CPUs.
> +
> +``status``
> + Operation mode of the driver: "active", "passive" or "off".
> +
> + "active"
> + The driver is functional and in the ``active mode``
> +
> + "passive"
> + The driver is functional and in the ``passive mode``
> +
> + "off"
> + The driver is unregistered and not functional now.
> +
> + This attribute can be written to in order to change the driver's
> + operation mode or to unregister it. The string written to it must be
> + one of the possible values of it and, if successful, the write will
I think this is implied that you wrote a possible value and if it
returns success something happens. That's how all sysfs files work and
it's needlessly wordy.
> + cause the driver to switch over to the operation mode represented by
> + that string - or to be unregistered in the "off" case.
Considering the implication of my above comment I think you can reword
this as:
"Writing one of these values to the sysfs file will cause the driver to..."
> +
> ``cpupower`` tool support for ``amd-pstate``
> ===============================================
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 00/13] Implement AMD Pstate EPP Driver
2022-12-19 6:40 [PATCH v8 00/13] Implement AMD Pstate EPP Driver Perry Yuan
` (12 preceding siblings ...)
2022-12-19 6:40 ` [PATCH v8 13/13] Documentation: amd-pstate: introduce new global sysfs attributes Perry Yuan
@ 2022-12-20 18:13 ` Tor Vic
2022-12-20 18:52 ` Tor Vic
13 siblings, 1 reply; 45+ messages in thread
From: Tor Vic @ 2022-12-20 18:13 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, Mario.Limonciello, ray.huang,
viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 19.12.22 06:40, Perry Yuan 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).
>
Using v8 and clang-15 on 6.1 I get:
---
ld.lld: error: undefined symbol: energy_perf_strings
>>> referenced by amd-pstate.c:789
(/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c:789)
>>> vmlinux.o:(show_energy_performance_preference)
>>> referenced by amd-pstate.c:768
(/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c:768)
>>> vmlinux.o:(store_energy_performance_preference)
>>> referenced by amd-pstate.c:749
(/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c:749)
>>> vmlinux.o:(show_energy_performance_available_preferences)
>>> referenced 1 more times
>>> did you mean: energy_perf_strings
>>> defined in: vmlinux.o
ld.lld: error: undefined symbol: epp_values
>>> referenced by amd-pstate.c:189
(/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c:189)
>>> vmlinux.o:(store_energy_performance_preference)
---
and a few warnings:
---
drivers/cpufreq/amd-pstate.c:966:6: warning: variable 'ret' is used
uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (rc)
^~
drivers/cpufreq/amd-pstate.c:1025:9: note: uninitialized use occurs here
return ret;
^~~
drivers/cpufreq/amd-pstate.c:966:2: note: remove the 'if' if its
condition is always false
if (rc)
^~~~~~~
drivers/cpufreq/amd-pstate.c:962:6: warning: variable 'ret' is used
uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!dev)
^~~~
drivers/cpufreq/amd-pstate.c:1025:9: note: uninitialized use occurs here
return ret;
^~~
drivers/cpufreq/amd-pstate.c:962:2: note: remove the 'if' if its
condition is always false
if (!dev)
^~~~~~~~~
drivers/cpufreq/amd-pstate.c:949:66: note: initialize the variable 'ret'
to silence this warning
int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret;
^
= 0
drivers/cpufreq/amd-pstate.c:996:52: warning: variable 'value' is
uninitialized when used here [-Wuninitialized]
cpudata->epp_cached = amd_pstate_get_epp(cpudata, value);
^~~~~
drivers/cpufreq/amd-pstate.c:953:11: note: initialize the variable
'value' to silence this warning
u64 value;
^
= 0
drivers/cpufreq/amd-pstate.c:1085:6: warning: variable 'epp' is used
uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (cpudata->epp_policy == cpudata->policy)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/cpufreq/amd-pstate.c:1110:30: note: uninitialized use occurs here
amd_pstate_set_epp(cpudata, epp);
^~~
drivers/cpufreq/amd-pstate.c:1085:2: note: remove the 'if' if its
condition is always false
if (cpudata->epp_policy == cpudata->policy)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/cpufreq/amd-pstate.c:1064:9: note: initialize the variable 'epp'
to silence this warning
s16 epp;
^
= 0
---
Cheers,
Tor Vic
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v8 00/13] Implement AMD Pstate EPP Driver
2022-12-20 18:13 ` [PATCH v8 00/13] Implement AMD Pstate EPP Driver Tor Vic
@ 2022-12-20 18:52 ` Tor Vic
2022-12-21 3:08 ` Yuan, Perry
0 siblings, 1 reply; 45+ messages in thread
From: Tor Vic @ 2022-12-20 18:52 UTC (permalink / raw)
To: Perry Yuan, rafael.j.wysocki, Mario.Limonciello, ray.huang,
viresh.kumar
Cc: Deepak.Sharma, Nathan.Fontenot, Alexander.Deucher, Shimmer.Huang,
Xiaojian.Du, Li.Meng, wyes.karny, linux-pm, linux-kernel
On 20.12.22 18:13, Tor Vic wrote:
>
> On 19.12.22 06:40, Perry Yuan 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).
>>
>
> Using v8 and clang-15 on 6.1 I get:
>
Got it.
Mario was right. INTEL_PSTATE must be selected, it has become a dependency.
That doesn't seem correct.
With it selected, it builds just fine. Not tested though.
> ---
> ld.lld: error: undefined symbol: energy_perf_strings
> >>> referenced by amd-pstate.c:789
> (/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c:789)
> >>> vmlinux.o:(show_energy_performance_preference)
> >>> referenced by amd-pstate.c:768
> (/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c:768)
> >>> vmlinux.o:(store_energy_performance_preference)
> >>> referenced by amd-pstate.c:749
> (/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c:749)
> >>>
> vmlinux.o:(show_energy_performance_available_preferences)
> >>> referenced 1 more times
> >>> did you mean: energy_perf_strings
> >>> defined in: vmlinux.o
>
> ld.lld: error: undefined symbol: epp_values
> >>> referenced by amd-pstate.c:189
> (/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c:189)
> >>> vmlinux.o:(store_energy_performance_preference)
> ---
>
> and a few warnings:
>
> ---
> drivers/cpufreq/amd-pstate.c:966:6: warning: variable 'ret' is used
> uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> if (rc)
> ^~
> drivers/cpufreq/amd-pstate.c:1025:9: note: uninitialized use occurs here
> return ret;
> ^~~
> drivers/cpufreq/amd-pstate.c:966:2: note: remove the 'if' if its
> condition is always false
> if (rc)
> ^~~~~~~
> drivers/cpufreq/amd-pstate.c:962:6: warning: variable 'ret' is used
> uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> if (!dev)
> ^~~~
> drivers/cpufreq/amd-pstate.c:1025:9: note: uninitialized use occurs here
> return ret;
> ^~~
> drivers/cpufreq/amd-pstate.c:962:2: note: remove the 'if' if its
> condition is always false
> if (!dev)
> ^~~~~~~~~
> drivers/cpufreq/amd-pstate.c:949:66: note: initialize the variable 'ret'
> to silence this warning
> int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret;
> ^
>
> = 0
> drivers/cpufreq/amd-pstate.c:996:52: warning: variable 'value' is
> uninitialized when used here [-Wuninitialized]
> cpudata->epp_cached = amd_pstate_get_epp(cpudata, value);
> ^~~~~
> drivers/cpufreq/amd-pstate.c:953:11: note: initialize the variable
> 'value' to silence this warning
> u64 value;
> ^
> = 0
> drivers/cpufreq/amd-pstate.c:1085:6: warning: variable 'epp' is used
> uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> if (cpudata->epp_policy == cpudata->policy)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/cpufreq/amd-pstate.c:1110:30: note: uninitialized use occurs here
> amd_pstate_set_epp(cpudata, epp);
> ^~~
> drivers/cpufreq/amd-pstate.c:1085:2: note: remove the 'if' if its
> condition is always false
> if (cpudata->epp_policy == cpudata->policy)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/cpufreq/amd-pstate.c:1064:9: note: initialize the variable 'epp'
> to silence this warning
> s16 epp;
> ^
> = 0
> ---
>
> Cheers,
>
> Tor Vic
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v8 00/13] Implement AMD Pstate EPP Driver
2022-12-20 18:52 ` Tor Vic
@ 2022-12-21 3:08 ` Yuan, Perry
0 siblings, 0 replies; 45+ messages in thread
From: Yuan, Perry @ 2022-12-21 3:08 UTC (permalink / raw)
To: Tor Vic, rafael.j.wysocki@intel.com, Limonciello, Mario,
Huang, Ray, viresh.kumar@linaro.org
Cc: Sharma, Deepak, Fontenot, Nathan, Deucher, Alexander,
Huang, Shimmer, Du, Xiaojian, Meng, Li (Jassmine), Karny, Wyes,
linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
[AMD Official Use Only - General]
> -----Original Message-----
> From: Tor Vic <torvic9@mailbox.org>
> Sent: Wednesday, December 21, 2022 2:53 AM
> To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> Limonciello, Mario <Mario.Limonciello@amd.com>; Huang, Ray
> <Ray.Huang@amd.com>; viresh.kumar@linaro.org
> Cc: 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 v8 00/13] Implement AMD Pstate EPP Driver
>
>
> On 20.12.22 18:13, Tor Vic wrote:
> >
> > On 19.12.22 06:40, Perry Yuan 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).
> >>
> >
> > Using v8 and clang-15 on 6.1 I get:
> >
>
> Got it.
> Mario was right. INTEL_PSTATE must be selected, it has become a
> dependency.
>
> That doesn't seem correct.
>
> With it selected, it builds just fine. Not tested though.
Yeah, I will make it in v9.
Thanks for your feedback!
>
> > ---
> > ld.lld: error: undefined symbol: energy_perf_strings >>> referenced
> > by amd-pstate.c:789
> > (/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c
> > :789) >>>
> > vmlinux.o:(show_energy_performance_preference)
> > >>> referenced by amd-pstate.c:768
> > (/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c
> > :768) >>>
> > vmlinux.o:(store_energy_performance_preference)
> > >>> referenced by amd-pstate.c:749
> > (/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-
> pstate.c:749)
> > >>>
> > vmlinux.o:(show_energy_performance_available_preferences)
> > >>> referenced 1 more times
> > >>> did you mean: energy_perf_strings >>> defined in: vmlinux.o
> >
> > ld.lld: error: undefined symbol: epp_values >>> referenced by
> > amd-pstate.c:189
> > (/tmp/makepkg/linux61-vd/src/linux-stable/drivers/cpufreq/amd-pstate.c
> > :189) >>>
> > vmlinux.o:(store_energy_performance_preference)
> > ---
> >
> > and a few warnings:
> >
> > ---
> > drivers/cpufreq/amd-pstate.c:966:6: warning: variable 'ret' is used
> > uninitialized whenever 'if' condition is true
> > [-Wsometimes-uninitialized]
> > if (rc)
> > ^~
> > drivers/cpufreq/amd-pstate.c:1025:9: note: uninitialized use occurs
> > here
> > return ret;
> > ^~~
> > drivers/cpufreq/amd-pstate.c:966:2: note: remove the 'if' if its
> > condition is always false
> > if (rc)
> > ^~~~~~~
> > drivers/cpufreq/amd-pstate.c:962:6: warning: variable 'ret' is used
> > uninitialized whenever 'if' condition is true
> > [-Wsometimes-uninitialized]
> > if (!dev)
> > ^~~~
> > drivers/cpufreq/amd-pstate.c:1025:9: note: uninitialized use occurs
> > here
> > return ret;
> > ^~~
> > drivers/cpufreq/amd-pstate.c:962:2: note: remove the 'if' if its
> > condition is always false
> > if (!dev)
> > ^~~~~~~~~
> > drivers/cpufreq/amd-pstate.c:949:66: note: initialize the variable 'ret'
> > to silence this warning
> > int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq,
> > ret;
> >
> > ^
> >
> > = 0
> > drivers/cpufreq/amd-pstate.c:996:52: warning: variable 'value' is
> > uninitialized when used here [-Wuninitialized]
> > cpudata->epp_cached = amd_pstate_get_epp(cpudata, value);
> > ^~~~~
> > drivers/cpufreq/amd-pstate.c:953:11: note: initialize the variable
> > 'value' to silence this warning
> > u64 value;
> > ^
> > = 0
> > drivers/cpufreq/amd-pstate.c:1085:6: warning: variable 'epp' is used
> > uninitialized whenever 'if' condition is true
> > [-Wsometimes-uninitialized]
> > if (cpudata->epp_policy == cpudata->policy)
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/cpufreq/amd-pstate.c:1110:30: note: uninitialized use occurs
> > here
> > amd_pstate_set_epp(cpudata, epp);
> > ^~~
> > drivers/cpufreq/amd-pstate.c:1085:2: note: remove the 'if' if its
> > condition is always false
> > if (cpudata->epp_policy == cpudata->policy)
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/cpufreq/amd-pstate.c:1064:9: note: initialize the variable 'epp'
> > to silence this warning
> > s16 epp;
> > ^
> > = 0
> > ---
> >
> > Cheers,
> >
> > Tor Vic
> >
^ permalink raw reply [flat|nested] 45+ messages in thread