Skip to contentSkip to conversation

intel_pstate: play well with frequency limits set by acpi

RFC PATCH In review Konstantin Khlebnikov 4 participants 10 messages Jul 16, 2015 – Jul 21, 2015

Subsystems (3)
ACPI CPU frequency scaling framework Intel pstate driver
lore
Share & tools
lore

Konstantin Khlebnikov

author
#0 lore raw

RFC PATCH 1/1 intel_pstate: play well with frequency limits set by acpi

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
 drivers/acpi/processor_perflib.c |    3 +-
 drivers/cpufreq/intel_pstate.c   |   57 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +2 -1
index cfc8aba72f86..781e328c9d5f 100644
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb,
 
 	ppc = (unsigned int)pr->performance_platform_limit;
 
-	if (ppc >= pr->performance->state_count)
+	/* Ignore limit of top state: it lower than turbo boost frequency */
+	if (!ppc || ppc >= pr->performance->state_count)
 		goto out;
 
 	cpufreq_verify_within_limits(policy, 0,
drivers/cpufreq/intel_pstate.c +57 -0

112 lines not on this page · raw message

maintainer
#1 lore raw
Quoted 19 lines · Konstantin Khlebnikov

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
 drivers/acpi/processor_perflib.c |    3 +-
 drivers/cpufreq/intel_pstate.c   |   57 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +0 -0
index cfc8aba72f86..781e328c9d5f 100644

drivers/acpi/processor_perflib.c static int acpi_processor_ppc_notifier(struct notifier_block *nb, · line 102

Quoted 6 lines
drivers/acpi/processor_perflib.c +2 -1
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb,
 
 	ppc = (unsigned int)pr->performance_platform_limit;
 
-	if (ppc >= pr->performance->state_count)
+	/* Ignore limit of top state: it lower than turbo boost frequency */
+	if (!ppc || ppc >= pr->performance->state_count)

Why? Isn't the previous check enough?

Quoted 121 lines
drivers/acpi/processor_perflib.c +0 -0
 		goto out;
 
 	cpufreq_verify_within_limits(policy, 0,
drivers/cpufreq/intel_pstate.c +55 -0
index 15ada47bb720..4a34ddf4fa73 100644
@@ -26,6 +26,7 @@
 #include <linux/fs.h>
 #include <linux/debugfs.h>
 #include <linux/acpi.h>
+#include <acpi/processor.h>
 #include <linux/vmalloc.h>
 #include <trace/events/power.h>
 

@@ -113,6 +114,9 @@ struct cpudata {
 	u64	prev_mperf;
 	u64	prev_tsc;
 	struct sample sample;
+#ifdef CONFIG_ACPI_PROCESSOR
+	struct acpi_processor_performance acpi_data;
+#endif
 };
 
 static struct cpudata **all_cpu_data;

@@ -145,6 +149,7 @@ static int hwp_active;
 
 struct perf_limits {
 	int no_turbo;
+	int no_acpi;
 	int turbo_disabled;
 	int max_perf_pct;
 	int min_perf_pct;

@@ -158,6 +163,7 @@ struct perf_limits {
 
 static struct perf_limits limits = {
 	.no_turbo = 0,
+	.no_acpi = !IS_ENABLED(CONFIG_ACPI_PROCESSOR),
 	.turbo_disabled = 0,
 	.max_perf_pct = 100,
 	.max_perf = int_tofp(1),

@@ -449,6 +455,18 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
 	return count;
 }
 
+static ssize_t store_no_acpi(struct kobject *a, struct attribute *b,
+			     const char *buf, size_t count)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+	return kstrtouint(buf, 0, &limits.no_acpi) ?: count;
+#else
+	return -ENODEV;
+#endif
+}
+show_one(no_acpi, no_acpi);
+define_one_global_rw(no_acpi);
+
 show_one(max_perf_pct, max_perf_pct);
 show_one(min_perf_pct, min_perf_pct);
 

@@ -460,6 +478,7 @@ define_one_global_ro(num_pstates);
 
 static struct attribute *intel_pstate_attributes[] = {
 	&no_turbo.attr,
+	&no_acpi.attr,
 	&max_perf_pct.attr,
 	&min_perf_pct.attr,
 	&turbo_pct.attr,

@@ -1049,6 +1068,38 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
 	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 	cpumask_set_cpu(policy->cpu, policy->cpus);
 
+#ifdef CONFIG_ACPI_PROCESSOR
+	if (!limits.no_acpi) {
+		/*
+		 * Minimum necessary to get acpi_processor_ppc_notifier() and
+		 * acpi_processor_get_bios_limit() working.
+		 */
+		if (!zalloc_cpumask_var(&cpu->acpi_data.shared_cpu_map,
+					GFP_KERNEL))
+			rc = -ENOMEM;
+		else
+			rc = acpi_processor_register_performance(
+					&cpu->acpi_data, policy->cpu);
+		if (rc) {
+			pr_err("intel_pstate: acpi init failed: %d\n", rc);
+			free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+			limits.no_acpi = 1;
+		}
+	}
+#endif
+	return 0;
+}
+
+static int intel_pstate_cpu_exit(struct cpufreq_policy *policy)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+	struct cpudata *cpu = all_cpu_data[policy->cpu];
+
+	if (cpu->acpi_data.state_count)
+		acpi_processor_unregister_performance(&cpu->acpi_data,
+						      policy->cpu);
+	free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+#endif
 	return 0;
 }
 

@@ -1057,7 +1108,11 @@ static struct cpufreq_driver intel_pstate_driver = {
 	.verify		= intel_pstate_verify_policy,
 	.setpolicy	= intel_pstate_set_policy,
 	.get		= intel_pstate_get,
+#ifdef CONFIG_ACPI_PROCESSOR
+	.bios_limit	= acpi_processor_get_bios_limit,
+#endif
 	.init		= intel_pstate_cpu_init,
+	.exit		= intel_pstate_cpu_exit,
 	.stop_cpu	= intel_pstate_stop_cpu,
 	.name		= "intel_pstate",
 };

drivers/cpufreq/intel_pstate.c static int __init intel_pstate_setup(char *str) · line 1348

Quoted 8 lines
drivers/cpufreq/intel_pstate.c +2 -0
@@ -1286,6 +1341,8 @@ static int __init intel_pstate_setup(char *str)
 		force_load = 1;
 	if (!strcmp(str, "hwp_only"))
 		hwp_only = 1;
+	if (!strcmp(str, "no_acpi"))
+		limits.no_acpi = 1;
 	return 0;
 }
 early_param("intel_pstate", intel_pstate_setup);

_PPC is index into _PSS. Since intel P state doesn't follow _PSS, the states may not be 1:1 matching. So we have to harmonize them.

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
Quoted 21 lines · Srinivas Pandruvada
Quoted 20 lines · Konstantin Khlebnikov

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
 drivers/acpi/processor_perflib.c |    3 +-
 drivers/cpufreq/intel_pstate.c   |   57 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +0 -0
index cfc8aba72f86..781e328c9d5f 100644
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb,
Quoted 6 lines

drivers/acpi/processor_perflib.c static int acpi_processor_ppc_notifier(struct notifier_block *nb, · line 102

Quoted 5 lines
drivers/acpi/processor_perflib.c +2 -1
      ppc = (unsigned int)pr->performance_platform_limit;
 
-     if (ppc >= pr->performance->state_count)
+     /* Ignore limit of top state: it lower than turbo boost frequency */
+     if (!ppc || ppc >= pr->performance->state_count)

Why? Isn't the previous check enough?

Zero _PPC state must be top performance state but as I see frequency in _PSS is lower than maximum possible turbo frequency. So, in this case intel_pstate cannnot get "100%" for max bound even it there is no limit set.

For example: I saw _PSS[0] = 2601 Mhz, PSS[1] = 2600 Mhz while turbo state is 3400 Mhz.

Quoted 144 lines
Quoted 121 lines
drivers/acpi/processor_perflib.c +0 -0
              goto out;
 
      cpufreq_verify_within_limits(policy, 0,
drivers/cpufreq/intel_pstate.c +55 -0
index 15ada47bb720..4a34ddf4fa73 100644
@@ -26,6 +26,7 @@
 #include <linux/fs.h>
 #include <linux/debugfs.h>
 #include <linux/acpi.h>
+#include <acpi/processor.h>
 #include <linux/vmalloc.h>
 #include <trace/events/power.h>
 

@@ -113,6 +114,9 @@ struct cpudata {
      u64     prev_mperf;
      u64     prev_tsc;
      struct sample sample;
+#ifdef CONFIG_ACPI_PROCESSOR
+     struct acpi_processor_performance acpi_data;
+#endif
 };
 
 static struct cpudata **all_cpu_data;

@@ -145,6 +149,7 @@ static int hwp_active;
 
 struct perf_limits {
      int no_turbo;
+     int no_acpi;
      int turbo_disabled;
      int max_perf_pct;
      int min_perf_pct;

@@ -158,6 +163,7 @@ struct perf_limits {
 
 static struct perf_limits limits = {
      .no_turbo = 0,
+     .no_acpi = !IS_ENABLED(CONFIG_ACPI_PROCESSOR),
      .turbo_disabled = 0,
      .max_perf_pct = 100,
      .max_perf = int_tofp(1),

@@ -449,6 +455,18 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
      return count;
 }
 
+static ssize_t store_no_acpi(struct kobject *a, struct attribute *b,
+                          const char *buf, size_t count)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+     return kstrtouint(buf, 0, &limits.no_acpi) ?: count;
+#else
+     return -ENODEV;
+#endif
+}
+show_one(no_acpi, no_acpi);
+define_one_global_rw(no_acpi);
+
 show_one(max_perf_pct, max_perf_pct);
 show_one(min_perf_pct, min_perf_pct);
 

@@ -460,6 +478,7 @@ define_one_global_ro(num_pstates);
 
 static struct attribute *intel_pstate_attributes[] = {
      &no_turbo.attr,
+     &no_acpi.attr,
      &max_perf_pct.attr,
      &min_perf_pct.attr,
      &turbo_pct.attr,

@@ -1049,6 +1068,38 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
      policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
      cpumask_set_cpu(policy->cpu, policy->cpus);
 
+#ifdef CONFIG_ACPI_PROCESSOR
+     if (!limits.no_acpi) {
+             /*
+              * Minimum necessary to get acpi_processor_ppc_notifier() and
+              * acpi_processor_get_bios_limit() working.
+              */
+             if (!zalloc_cpumask_var(&cpu->acpi_data.shared_cpu_map,
+                                     GFP_KERNEL))
+                     rc = -ENOMEM;
+             else
+                     rc = acpi_processor_register_performance(
+                                     &cpu->acpi_data, policy->cpu);
+             if (rc) {
+                     pr_err("intel_pstate: acpi init failed: %d\n", rc);
+                     free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+                     limits.no_acpi = 1;
+             }
+     }
+#endif
+     return 0;
+}
+
+static int intel_pstate_cpu_exit(struct cpufreq_policy *policy)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+     struct cpudata *cpu = all_cpu_data[policy->cpu];
+
+     if (cpu->acpi_data.state_count)
+             acpi_processor_unregister_performance(&cpu->acpi_data,
+                                                   policy->cpu);
+     free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+#endif
      return 0;
 }
 

@@ -1057,7 +1108,11 @@ static struct cpufreq_driver intel_pstate_driver = {
      .verify         = intel_pstate_verify_policy,
      .setpolicy      = intel_pstate_set_policy,
      .get            = intel_pstate_get,
+#ifdef CONFIG_ACPI_PROCESSOR
+     .bios_limit     = acpi_processor_get_bios_limit,
+#endif
      .init           = intel_pstate_cpu_init,
+     .exit           = intel_pstate_cpu_exit,
      .stop_cpu       = intel_pstate_stop_cpu,
      .name           = "intel_pstate",
 };

drivers/cpufreq/intel_pstate.c static int __init intel_pstate_setup(char *str) · line 1348

Quoted 8 lines
drivers/cpufreq/intel_pstate.c +2 -0
@@ -1286,6 +1341,8 @@ static int __init intel_pstate_setup(char *str)
              force_load = 1;
      if (!strcmp(str, "hwp_only"))
              hwp_only = 1;
+     if (!strcmp(str, "no_acpi"))
+             limits.no_acpi = 1;
      return 0;
 }
 early_param("intel_pstate", intel_pstate_setup);

_PPC is index into _PSS. Since intel P state doesn't follow _PSS, the states may not be 1:1 matching. So we have to harmonize them.

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

maintainer
#3 lore raw
Quoted 30 lines · Konstantin Khlebnikov
Quoted 21 lines · Srinivas Pandruvada
Quoted 20 lines · Konstantin Khlebnikov

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
 drivers/acpi/processor_perflib.c |    3 +-
 drivers/cpufreq/intel_pstate.c   |   57 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +0 -0
index cfc8aba72f86..781e328c9d5f 100644
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb,
Quoted 6 lines

drivers/acpi/processor_perflib.c static int acpi_processor_ppc_notifier(struct notifier_block *nb, · line 102

Quoted 5 lines
drivers/acpi/processor_perflib.c +2 -1
      ppc = (unsigned int)pr->performance_platform_limit;
 
-     if (ppc >= pr->performance->state_count)
+     /* Ignore limit of top state: it lower than turbo boost frequency */
+     if (!ppc || ppc >= pr->performance->state_count)

Why? Isn't the previous check enough?

Quoted 7 lines

Zero _PPC state must be top performance state but as I see frequency in _PSS is lower than maximum possible turbo frequency. So, in this case intel_pstate cannnot get "100%" for max bound even it there is no limit set.

For example: I saw _PSS[0] = 2601 Mhz, PSS[1] = 2600 Mhz while turbo state is 3400 Mhz.

Have you tested dynamic _PPC modification with acpi cpufreq with this change (after boot)? Suppose _PPC is changed from 3 to 0, then cpufreq_verify_within_limits will not be called to change to new max turbo performance state.

Thanks,
Srinivas

Quoted 148 lines
Quoted 139 lines
Quoted 121 lines
drivers/acpi/processor_perflib.c +0 -0
              goto out;
 
      cpufreq_verify_within_limits(policy, 0,
drivers/cpufreq/intel_pstate.c +55 -0
index 15ada47bb720..4a34ddf4fa73 100644
@@ -26,6 +26,7 @@
 #include <linux/fs.h>
 #include <linux/debugfs.h>
 #include <linux/acpi.h>
+#include <acpi/processor.h>
 #include <linux/vmalloc.h>
 #include <trace/events/power.h>
 

@@ -113,6 +114,9 @@ struct cpudata {
      u64     prev_mperf;
      u64     prev_tsc;
      struct sample sample;
+#ifdef CONFIG_ACPI_PROCESSOR
+     struct acpi_processor_performance acpi_data;
+#endif
 };
 
 static struct cpudata **all_cpu_data;

@@ -145,6 +149,7 @@ static int hwp_active;
 
 struct perf_limits {
      int no_turbo;
+     int no_acpi;
      int turbo_disabled;
      int max_perf_pct;
      int min_perf_pct;

@@ -158,6 +163,7 @@ struct perf_limits {
 
 static struct perf_limits limits = {
      .no_turbo = 0,
+     .no_acpi = !IS_ENABLED(CONFIG_ACPI_PROCESSOR),
      .turbo_disabled = 0,
      .max_perf_pct = 100,
      .max_perf = int_tofp(1),

@@ -449,6 +455,18 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
      return count;
 }
 
+static ssize_t store_no_acpi(struct kobject *a, struct attribute *b,
+                          const char *buf, size_t count)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+     return kstrtouint(buf, 0, &limits.no_acpi) ?: count;
+#else
+     return -ENODEV;
+#endif
+}
+show_one(no_acpi, no_acpi);
+define_one_global_rw(no_acpi);
+
 show_one(max_perf_pct, max_perf_pct);
 show_one(min_perf_pct, min_perf_pct);
 

@@ -460,6 +478,7 @@ define_one_global_ro(num_pstates);
 
 static struct attribute *intel_pstate_attributes[] = {
      &no_turbo.attr,
+     &no_acpi.attr,
      &max_perf_pct.attr,
      &min_perf_pct.attr,
      &turbo_pct.attr,

@@ -1049,6 +1068,38 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
      policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
      cpumask_set_cpu(policy->cpu, policy->cpus);
 
+#ifdef CONFIG_ACPI_PROCESSOR
+     if (!limits.no_acpi) {
+             /*
+              * Minimum necessary to get acpi_processor_ppc_notifier() and
+              * acpi_processor_get_bios_limit() working.
+              */
+             if (!zalloc_cpumask_var(&cpu->acpi_data.shared_cpu_map,
+                                     GFP_KERNEL))
+                     rc = -ENOMEM;
+             else
+                     rc = acpi_processor_register_performance(
+                                     &cpu->acpi_data, policy->cpu);
+             if (rc) {
+                     pr_err("intel_pstate: acpi init failed: %d\n", rc);
+                     free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+                     limits.no_acpi = 1;
+             }
+     }
+#endif
+     return 0;
+}
+
+static int intel_pstate_cpu_exit(struct cpufreq_policy *policy)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+     struct cpudata *cpu = all_cpu_data[policy->cpu];
+
+     if (cpu->acpi_data.state_count)
+             acpi_processor_unregister_performance(&cpu->acpi_data,
+                                                   policy->cpu);
+     free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+#endif
      return 0;
 }
 

@@ -1057,7 +1108,11 @@ static struct cpufreq_driver intel_pstate_driver = {
      .verify         = intel_pstate_verify_policy,
      .setpolicy      = intel_pstate_set_policy,
      .get            = intel_pstate_get,
+#ifdef CONFIG_ACPI_PROCESSOR
+     .bios_limit     = acpi_processor_get_bios_limit,
+#endif
      .init           = intel_pstate_cpu_init,
+     .exit           = intel_pstate_cpu_exit,
      .stop_cpu       = intel_pstate_stop_cpu,
      .name           = "intel_pstate",
 };

drivers/cpufreq/intel_pstate.c static int __init intel_pstate_setup(char *str) · line 1348

Quoted 8 lines
drivers/cpufreq/intel_pstate.c +2 -0
@@ -1286,6 +1341,8 @@ static int __init intel_pstate_setup(char *str)
              force_load = 1;
      if (!strcmp(str, "hwp_only"))
              hwp_only = 1;
+     if (!strcmp(str, "no_acpi"))
+             limits.no_acpi = 1;
      return 0;
 }
 early_param("intel_pstate", intel_pstate_setup);

_PPC is index into _PSS. Since intel P state doesn't follow _PSS, the states may not be 1:1 matching. So we have to harmonize them.

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw
Quoted 35 lines · Srinivas Pandruvada
Quoted 34 lines · Konstantin Khlebnikov
Quoted 21 lines · Srinivas Pandruvada
Quoted 20 lines · Konstantin Khlebnikov

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
  drivers/acpi/processor_perflib.c |    3 +-
  drivers/cpufreq/intel_pstate.c   |   57 ++++++++++++++++++++++++++++++++++++++
  2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +0 -0
index cfc8aba72f86..781e328c9d5f 100644
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb,
Quoted 6 lines

drivers/acpi/processor_perflib.c static int acpi_processor_ppc_notifier(struct notifier_block *nb, · line 102

Quoted 5 lines
drivers/acpi/processor_perflib.c +2 -1
       ppc = (unsigned int)pr->performance_platform_limit;
 
-     if (ppc >= pr->performance->state_count)
+     /* Ignore limit of top state: it lower than turbo boost frequency */
+     if (!ppc || ppc >= pr->performance->state_count)

Why? Isn't the previous check enough?

Zero _PPC state must be top performance state but as I see frequency in _PSS is lower than maximum possible turbo frequency. So, in this case intel_pstate cannnot get "100%" for max bound even it there is no limit set.

Quoted 8 lines
Quoted 3 lines

For example: I saw _PSS[0] = 2601 Mhz, PSS[1] = 2600 Mhz while turbo state is 3400 Mhz.

Have you tested dynamic _PPC modification with acpi cpufreq with this change (after boot)? Suppose _PPC is changed from 3 to 0, then cpufreq_verify_within_limits will not be called to change to new max turbo performance state.

I haven't checked that but as I see acpi_processor_ppc_notifier() can only reduce maximum frequency. So, there should be no problem in this case.

Signature · 1 line
Konstantin
maintainer
#5 lore raw
Quoted 45 lines · Konstantin Khlebnikov
Quoted 35 lines · Srinivas Pandruvada
Quoted 34 lines · Konstantin Khlebnikov
Quoted 21 lines · Srinivas Pandruvada
Quoted 20 lines · Konstantin Khlebnikov

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
  drivers/acpi/processor_perflib.c |    3 +-
  drivers/cpufreq/intel_pstate.c   |   57 ++++++++++++++++++++++++++++++++++++++
  2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +0 -0
index cfc8aba72f86..781e328c9d5f 100644
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb,
Quoted 6 lines

drivers/acpi/processor_perflib.c static int acpi_processor_ppc_notifier(struct notifier_block *nb, · line 102

Quoted 5 lines
drivers/acpi/processor_perflib.c +2 -1
       ppc = (unsigned int)pr->performance_platform_limit;
 
-     if (ppc >= pr->performance->state_count)
+     /* Ignore limit of top state: it lower than turbo boost frequency */
+     if (!ppc || ppc >= pr->performance->state_count)

Why? Isn't the previous check enough?

Zero _PPC state must be top performance state but as I see frequency in _PSS is lower than maximum possible turbo frequency. So, in this case intel_pstate cannnot get "100%" for max bound even it there is no limit set.

Quoted 8 lines
Quoted 3 lines

For example: I saw _PSS[0] = 2601 Mhz, PSS[1] = 2600 Mhz while turbo state is 3400 Mhz.

Have you tested dynamic _PPC modification with acpi cpufreq with this change (after boot)? Suppose _PPC is changed from 3 to 0, then cpufreq_verify_within_limits will not be called to change to new max turbo performance state.

Quoted 3 lines

I haven't checked that but as I see acpi_processor_ppc_notifier() can only reduce maximum frequency. So, there should be no problem in this case.

No, it can also be used in both ways. Once reduced, it can increase as well. _PPC can be dynamically modified by BIOS to reduce and also to increase.

author
#6 lore raw
Quoted 46 lines · Srinivas Pandruvada
Quoted 45 lines · Konstantin Khlebnikov
Quoted 43 lines · Srinivas Pandruvada
Quoted 30 lines · Konstantin Khlebnikov
Quoted 21 lines · Srinivas Pandruvada
Quoted 20 lines · Konstantin Khlebnikov

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
   drivers/acpi/processor_perflib.c |    3 +-
   drivers/cpufreq/intel_pstate.c   |   57 ++++++++++++++++++++++++++++++++++++++
   2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +0 -0
index cfc8aba72f86..781e328c9d5f 100644
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb,
Quoted 6 lines

drivers/acpi/processor_perflib.c static int acpi_processor_ppc_notifier(struct notifier_block *nb, · line 102

Quoted 5 lines
drivers/acpi/processor_perflib.c +2 -1
        ppc = (unsigned int)pr->performance_platform_limit;
 
-     if (ppc >= pr->performance->state_count)
+     /* Ignore limit of top state: it lower than turbo boost frequency */
+     if (!ppc || ppc >= pr->performance->state_count)

Why? Isn't the previous check enough?

Quoted 7 lines

Zero _PPC state must be top performance state but as I see frequency in _PSS is lower than maximum possible turbo frequency. So, in this case intel_pstate cannnot get "100%" for max bound even it there is no limit set.

For example: I saw _PSS[0] = 2601 Mhz, PSS[1] = 2600 Mhz while turbo state is 3400 Mhz.

Have you tested dynamic _PPC modification with acpi cpufreq with this change (after boot)? Suppose _PPC is changed from 3 to 0, then cpufreq_verify_within_limits will not be called to change to new max turbo performance state.

Quoted 6 lines
Quoted 3 lines

I haven't checked that but as I see acpi_processor_ppc_notifier() can only reduce maximum frequency. So, there should be no problem in this case.

No, it can also be used in both ways. Once reduced, it can increase as well. _PPC can be dynamically modified by BIOS to reduce and also to increase.

Well, in this case BIOS will trigger ACPI_PROCESSOR_NOTIFY_PERFORMANCE:
kernel evaluate new _PPC and call cpufreq_update_policy() which set initial frequency min/max range according to user setup and apply all limits after that. Initial policy->user_policy.min/max stay unchanged. So, that dynamic modification works in both ways.

Signature · 1 line
Konstantin
maintainer
#7 lore raw
Quoted 54 lines · Konstantin Khlebnikov
Quoted 46 lines · Srinivas Pandruvada
Quoted 45 lines · Konstantin Khlebnikov
Quoted 43 lines · Srinivas Pandruvada
Quoted 30 lines · Konstantin Khlebnikov
Quoted 21 lines · Srinivas Pandruvada
Quoted 20 lines · Konstantin Khlebnikov

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
   drivers/acpi/processor_perflib.c |    3 +-
   drivers/cpufreq/intel_pstate.c   |   57 ++++++++++++++++++++++++++++++++++++++
   2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +0 -0
index cfc8aba72f86..781e328c9d5f 100644
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb,
Quoted 6 lines

drivers/acpi/processor_perflib.c static int acpi_processor_ppc_notifier(struct notifier_block *nb, · line 102

Quoted 5 lines
drivers/acpi/processor_perflib.c +2 -1
        ppc = (unsigned int)pr->performance_platform_limit;
 
-     if (ppc >= pr->performance->state_count)
+     /* Ignore limit of top state: it lower than turbo boost frequency */
+     if (!ppc || ppc >= pr->performance->state_count)

Why? Isn't the previous check enough?

Quoted 7 lines

Zero _PPC state must be top performance state but as I see frequency in _PSS is lower than maximum possible turbo frequency. So, in this case intel_pstate cannnot get "100%" for max bound even it there is no limit set.

For example: I saw _PSS[0] = 2601 Mhz, PSS[1] = 2600 Mhz while turbo state is 3400 Mhz.

Have you tested dynamic _PPC modification with acpi cpufreq with this change (after boot)? Suppose _PPC is changed from 3 to 0, then cpufreq_verify_within_limits will not be called to change to new max turbo performance state.

Quoted 6 lines
Quoted 3 lines

I haven't checked that but as I see acpi_processor_ppc_notifier() can only reduce maximum frequency. So, there should be no problem in this case.

No, it can also be used in both ways. Once reduced, it can increase as well. _PPC can be dynamically modified by BIOS to reduce and also to increase.

Quoted 6 lines

Well, in this case BIOS will trigger ACPI_PROCESSOR_NOTIFY_PERFORMANCE:
kernel evaluate new _PPC and call cpufreq_update_policy() which set initial frequency min/max range according to user setup and apply all limits after that. Initial policy->user_policy.min/max stay unchanged. So, that dynamic modification works in both ways.

Fair enough. We need to take account for _PSS. We have some changes for this, but not gone through test cycle. I will post them as RFC, please check. Thanks for your patience.

- Srinivas

#8 lore raw
In reply to Konstantin Khlebnikov
Quoted 19 lines · Konstantin Khlebnikov

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
  drivers/acpi/processor_perflib.c |    3 +-
  drivers/cpufreq/intel_pstate.c   |   57 ++++++++++++++++++++++++++++++++++++++
  2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +0 -0
index cfc8aba72f86..781e328c9d5f 100644

drivers/acpi/processor_perflib.c static int acpi_processor_ppc_notifier(struct notifier_block *nb, · line 102

Quoted 6 lines
drivers/acpi/processor_perflib.c +2 -1
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb,
 
  	ppc = (unsigned int)pr->performance_platform_limit;
 
-	if (ppc >= pr->performance->state_count)
+	/* Ignore limit of top state: it lower than turbo boost frequency */
+	if (!ppc || ppc >= pr->performance->state_count)

Perhaps the !ppc is wrong if we check it against ACPI spec. Zero value of ppc means:

"0 – States 0 through Nth state are available (all states available)"

Quoted 121 lines
drivers/acpi/processor_perflib.c +0 -0
  		goto out;
 
  	cpufreq_verify_within_limits(policy, 0,
drivers/cpufreq/intel_pstate.c +55 -0
index 15ada47bb720..4a34ddf4fa73 100644
@@ -26,6 +26,7 @@
  #include <linux/fs.h>
  #include <linux/debugfs.h>
  #include <linux/acpi.h>
+#include <acpi/processor.h>
  #include <linux/vmalloc.h>
  #include <trace/events/power.h>
 

@@ -113,6 +114,9 @@ struct cpudata {
  	u64	prev_mperf;
  	u64	prev_tsc;
  	struct sample sample;
+#ifdef CONFIG_ACPI_PROCESSOR
+	struct acpi_processor_performance acpi_data;
+#endif
  };
 
  static struct cpudata **all_cpu_data;

@@ -145,6 +149,7 @@ static int hwp_active;
 
  struct perf_limits {
  	int no_turbo;
+	int no_acpi;
  	int turbo_disabled;
  	int max_perf_pct;
  	int min_perf_pct;

@@ -158,6 +163,7 @@ struct perf_limits {
 
  static struct perf_limits limits = {
  	.no_turbo = 0,
+	.no_acpi = !IS_ENABLED(CONFIG_ACPI_PROCESSOR),
  	.turbo_disabled = 0,
  	.max_perf_pct = 100,
  	.max_perf = int_tofp(1),

@@ -449,6 +455,18 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
  	return count;
  }
 
+static ssize_t store_no_acpi(struct kobject *a, struct attribute *b,
+			     const char *buf, size_t count)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+	return kstrtouint(buf, 0, &limits.no_acpi) ?: count;
+#else
+	return -ENODEV;
+#endif
+}
+show_one(no_acpi, no_acpi);
+define_one_global_rw(no_acpi);
+
  show_one(max_perf_pct, max_perf_pct);
  show_one(min_perf_pct, min_perf_pct);
 

@@ -460,6 +478,7 @@ define_one_global_ro(num_pstates);
 
  static struct attribute *intel_pstate_attributes[] = {
  	&no_turbo.attr,
+	&no_acpi.attr,
  	&max_perf_pct.attr,
  	&min_perf_pct.attr,
  	&turbo_pct.attr,

@@ -1049,6 +1068,38 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
  	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
  	cpumask_set_cpu(policy->cpu, policy->cpus);
 
+#ifdef CONFIG_ACPI_PROCESSOR
+	if (!limits.no_acpi) {
+		/*
+		 * Minimum necessary to get acpi_processor_ppc_notifier() and
+		 * acpi_processor_get_bios_limit() working.
+		 */
+		if (!zalloc_cpumask_var(&cpu->acpi_data.shared_cpu_map,
+					GFP_KERNEL))
+			rc = -ENOMEM;
+		else
+			rc = acpi_processor_register_performance(
+					&cpu->acpi_data, policy->cpu);
+		if (rc) {
+			pr_err("intel_pstate: acpi init failed: %d\n", rc);
+			free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+			limits.no_acpi = 1;
+		}
+	}
+#endif
+	return 0;
+}
+
+static int intel_pstate_cpu_exit(struct cpufreq_policy *policy)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+	struct cpudata *cpu = all_cpu_data[policy->cpu];
+
+	if (cpu->acpi_data.state_count)
+		acpi_processor_unregister_performance(&cpu->acpi_data,
+						      policy->cpu);
+	free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+#endif
  	return 0;
  }
 

@@ -1057,7 +1108,11 @@ static struct cpufreq_driver intel_pstate_driver = {
  	.verify		= intel_pstate_verify_policy,
  	.setpolicy	= intel_pstate_set_policy,
  	.get		= intel_pstate_get,
+#ifdef CONFIG_ACPI_PROCESSOR
+	.bios_limit	= acpi_processor_get_bios_limit,
+#endif
  	.init		= intel_pstate_cpu_init,
+	.exit		= intel_pstate_cpu_exit,
  	.stop_cpu	= intel_pstate_stop_cpu,
  	.name		= "intel_pstate",
  };

drivers/cpufreq/intel_pstate.c static int __init intel_pstate_setup(char *str) · line 1348

Quoted 8 lines
drivers/cpufreq/intel_pstate.c +2 -0
@@ -1286,6 +1341,8 @@ static int __init intel_pstate_setup(char *str)
  		force_load = 1;
  	if (!strcmp(str, "hwp_only"))
  		hwp_only = 1;
+	if (!strcmp(str, "no_acpi"))
+		limits.no_acpi = 1;
  	return 0;
  }
  early_param("intel_pstate", intel_pstate_setup);

Thanks,
Ethan
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#9 lore raw
Quoted 34 lines · ethan zhao
Quoted 22 lines · Konstantin Khlebnikov

IPMI can control CPU P-states remotely: configuration is reported via common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal support in intel_pstate to receive and use these P-state limits.

* ignore limit of top state in _PPC: it lower than turbo boost frequency
* register intel_pstate in acpi-processor to get states from _PSS
* link acpi_processor_get_bios_limit: this adds attribute "bios_limit"

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
  drivers/acpi/processor_perflib.c |    3 +-
  drivers/cpufreq/intel_pstate.c   |   57
++++++++++++++++++++++++++++++++++++++
  2 files changed, 59 insertions(+), 1 deletion(-)
drivers/acpi/processor_perflib.c +0 -0
index cfc8aba72f86..781e328c9d5f 100644

drivers/acpi/processor_perflib.c static int acpi_processor_ppc_notifier(struct · line 103

Quoted 6 lines
drivers/acpi/processor_perflib.c +2 -0
@@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct
 notifier_block *nb,
        ppc = (unsigned int)pr->performance_platform_limit;
  -     if (ppc >= pr->performance->state_count)
+       /* Ignore limit of top state: it lower than turbo boost frequency
 */
+       if (!ppc || ppc >= pr->performance->state_count)

Perhaps the !ppc is wrong if we check it against ACPI spec. Zero value of ppc means:

Quoted 1 line

"0 – States 0 through Nth state are available (all states available)"

Yep, also states are ordered by power - state0 must must have highest performance.
This code below clamps range of available frequences to 0.._PSS[_PPC].frequency. So if _PPC = 0 then there should be no limit but then turbo is enabled frequency of state0 is actually could be lower than effective turbo frequency:
for hardware I have it's _PSS[0] = 2601 Mhz, PSS[1] = 2600 Mhz while turbo state is 3400 Mhz.

Quoted 136 lines
Quoted 119 lines
drivers/acpi/processor_perflib.c +0 -0
                goto out;
        cpufreq_verify_within_limits(policy, 0,
drivers/cpufreq/intel_pstate.c +1 -0
index 15ada47bb720..4a34ddf4fa73 100644
@@ -26,6 +26,7 @@
  #include <linux/fs.h>
  #include <linux/debugfs.h>
  #include <linux/acpi.h>
+#include <acpi/processor.h>
  #include <linux/vmalloc.h>
  #include <trace/events/power.h>
  @@ -113,6 +114,9 @@ struct cpudata {

        u64     prev_mperf;
        u64     prev_tsc;
        struct sample sample;
drivers/cpufreq/intel_pstate.c +20 -0
+#ifdef CONFIG_ACPI_PROCESSOR
+       struct acpi_processor_performance acpi_data;
+#endif
  };
    static struct cpudata **all_cpu_data;

@@ -145,6 +149,7 @@ static int hwp_active;
    struct perf_limits {
        int no_turbo;
+       int no_acpi;
        int turbo_disabled;
        int max_perf_pct;
        int min_perf_pct;

@@ -158,6 +163,7 @@ struct perf_limits {
    static struct perf_limits limits = {
        .no_turbo = 0,
+       .no_acpi = !IS_ENABLED(CONFIG_ACPI_PROCESSOR),
        .turbo_disabled = 0,
        .max_perf_pct = 100,
        .max_perf = int_tofp(1),

@@ -449,6 +455,18 @@ static ssize_t store_min_perf_pct(struct kobject *a,
 struct attribute *b,
        return count;
  }
  +static ssize_t store_no_acpi(struct kobject *a, struct attribute *b,
+                            const char *buf, size_t count)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+       return kstrtouint(buf, 0, &limits.no_acpi) ?: count;
+#else
+       return -ENODEV;
+#endif
+}
+show_one(no_acpi, no_acpi);
+define_one_global_rw(no_acpi);
+
  show_one(max_perf_pct, max_perf_pct);
  show_one(min_perf_pct, min_perf_pct);
  @@ -460,6 +478,7 @@ define_one_global_ro(num_pstates);
    static struct attribute *intel_pstate_attributes[] = {
        &no_turbo.attr,
+       &no_acpi.attr,
        &max_perf_pct.attr,
        &min_perf_pct.attr,
        &turbo_pct.attr,

@@ -1049,6 +1068,38 @@ static int intel_pstate_cpu_init(struct
 cpufreq_policy *policy)
        policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
        cpumask_set_cpu(policy->cpu, policy->cpus);
  +#ifdef CONFIG_ACPI_PROCESSOR
+       if (!limits.no_acpi) {
+               /*
+                * Minimum necessary to get acpi_processor_ppc_notifier()

and

drivers/cpufreq/intel_pstate.c +10 -0
+                * acpi_processor_get_bios_limit() working.
+                */
+               if (!zalloc_cpumask_var(&cpu->acpi_data.shared_cpu_map,
+                                       GFP_KERNEL))
+                       rc = -ENOMEM;
+               else
+                       rc = acpi_processor_register_performance(
+                                       &cpu->acpi_data, policy->cpu);
+               if (rc) {
+                       pr_err("intel_pstate: acpi init failed: %d\n",

rc);

drivers/cpufreq/intel_pstate.c +18 -0
+                       free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+                       limits.no_acpi = 1;
+               }
+       }
+#endif
+       return 0;
+}
+
+static int intel_pstate_cpu_exit(struct cpufreq_policy *policy)
+{
+#ifdef CONFIG_ACPI_PROCESSOR
+       struct cpudata *cpu = all_cpu_data[policy->cpu];
+
+       if (cpu->acpi_data.state_count)
+               acpi_processor_unregister_performance(&cpu->acpi_data,
+                                                     policy->cpu);
+       free_cpumask_var(cpu->acpi_data.shared_cpu_map);
+#endif
        return 0;
  }
  @@ -1057,7 +1108,11 @@ static struct cpufreq_driver intel_pstate_driver

= {

        .verify         = intel_pstate_verify_policy,
        .setpolicy      = intel_pstate_set_policy,
        .get            = intel_pstate_get,
drivers/cpufreq/intel_pstate.c +4 -0
+#ifdef CONFIG_ACPI_PROCESSOR
+       .bios_limit     = acpi_processor_get_bios_limit,
+#endif
        .init           = intel_pstate_cpu_init,
+       .exit           = intel_pstate_cpu_exit,
        .stop_cpu       = intel_pstate_stop_cpu,
        .name           = "intel_pstate",
  };

drivers/cpufreq/intel_pstate.c static int __init intel_pstate_setup(char *str) · line 1348

Quoted 8 lines
drivers/cpufreq/intel_pstate.c +2 -0
@@ -1286,6 +1341,8 @@ static int __init intel_pstate_setup(char *str)
                force_load = 1;
        if (!strcmp(str, "hwp_only"))
                hwp_only = 1;
+       if (!strcmp(str, "no_acpi"))
+               limits.no_acpi = 1;
        return 0;
  }
  early_param("intel_pstate", intel_pstate_setup);

Thanks,
Ethan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/