[PATCH 0/3] Auto configuration mode and error

[PATCH 0/3] Auto configuration mode and error

[Srinivas Pandruvada]

These are some changes, which help users to use the base-freq and
turbo-freq features without going through multiple steps for
basic configuration. Also add some error when user is trying
to disable core-power feature while it is getting used.

None of these patches are urgent and can wait for kernel version v5.5.

Srinivas Pandruvada (3):
  tools/power/x86/intel-speed-select: Base-freq feature auto mode
  tools/power/x86/intel-speed-select: Turbo-freq feature auto mode
  tools/power/x86/intel-speed-select: Refuse to disable core-power when
    getting used

 .../x86/intel-speed-select/isst-config.c      | 275 +++++++++++++++++-
 .../power/x86/intel-speed-select/isst-core.c  |  21 ++
 2 files changed, 281 insertions(+), 15 deletions(-)

-- 
2.17.2

[PATCH 1/3] tools/power/x86/intel-speed-select: Base-freq feature auto mode

[Srinivas Pandruvada]

Introduce --auto|-a option to base-freq enable feature, so that it
does in one step for users who are OK by setting all cores with higher
base frequency to be set in CLOS 0 and remaining in CLOS 3. This option
also sets corresponding clos.min to CLOS 0 and CLOS3. In this way, users
don't have to take multiple steps to enable base-freq feature. For users
who want more fine grain control, they can always use core-power feature
to set custom CLOS configuration and assignment.

Also adjust cpufreq/scaling_min_freq for higher and lower priority cores.

For example user can use:
intel-speed-select base-freq enable --auto

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 196 +++++++++++++++++-
 1 file changed, 191 insertions(+), 5 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 2a9890c8395a..5da59ff47306 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -39,6 +39,7 @@ static unsigned long long fact_trl;
 static int out_format_json;
 static int cmd_help;
 static int force_online_offline;
+static int auto_mode;
 
 /* clos related */
 static int current_clos = -1;
@@ -852,6 +853,174 @@ static void dump_pbf_config(void)
 	isst_ctdp_display_information_end(outf);
 }
 
+static int set_clos_param(int cpu, int clos, int epp, int wt, int min,
+				  int max)
+{
+	struct isst_clos_config clos_config;
+	int ret;
+
+	ret = isst_pm_get_clos(cpu, clos, &clos_config);
+	if (ret) {
+		perror("isst_pm_get_clos");
+		return ret;
+	}
+	clos_config.clos_min = min;
+	clos_config.clos_max = max;
+	clos_config.epp = epp;
+	clos_config.clos_prop_prio = wt;
+	ret = isst_set_clos(cpu, clos, &clos_config);
+	if (ret) {
+		perror("isst_pm_set_clos");
+		return ret;
+	}
+
+	return 0;
+}
+
+/* No error processing as cpufreq can be absent */
+static void set_cpufreq_scaling_min(int cpu, int ratio)
+{
+	char buffer[128];
+	int fd, len;
+
+	snprintf(buffer, sizeof(buffer),
+		 "/sys/devices/system/cpu/cpu%d/cpufreq/scaling_min_freq", cpu);
+
+	fd = open(buffer, O_WRONLY);
+	if (fd < 0)
+		return;
+
+	len = snprintf(buffer, sizeof(buffer), "%d\n", ratio * 100000);
+	write(fd, buffer, len);
+	close(fd);
+}
+
+static void reset_cpufreq_scaling_min(void)
+{
+	int i;
+
+	for (i = 0; i < get_topo_max_cpus(); ++i) {
+		char buffer[128], min_freq[16];
+		int fd, len;
+
+		if (!CPU_ISSET_S(i, present_cpumask_size, present_cpumask))
+			continue;
+
+		snprintf(buffer, sizeof(buffer),
+			 "/sys/devices/system/cpu/cpu%d/cpufreq/cpuinfo_min_freq", i);
+
+		fd = open(buffer, O_RDONLY);
+		if (fd < 0)
+			break;
+
+		len = read(fd, min_freq, sizeof(min_freq));
+		close(fd);
+
+		if (len < 0)
+			break;
+
+		snprintf(buffer, sizeof(buffer),
+			 "/sys/devices/system/cpu/cpu%d/cpufreq/scaling_min_freq", i);
+
+		fd = open(buffer, O_WRONLY);
+		if (fd < 0)
+			break;
+
+		len = strlen(min_freq);
+		write(fd, min_freq, len);
+		close(fd);
+	}
+}
+
+static int set_core_priority_and_min(int cpu, int mask_size,
+				     cpu_set_t *cpu_mask, int min_high,
+				     int min_low)
+{
+	int pkg_id, die_id, ret, i;
+
+	if (!CPU_COUNT_S(mask_size, cpu_mask))
+		return -1;
+
+	ret = set_clos_param(cpu, 0, 0, 0, min_high, 0xff);
+	if (ret)
+		return ret;
+
+	ret = set_clos_param(cpu, 1, 15, 0, min_low, 0xff);
+	if (ret)
+		return ret;
+
+	ret = set_clos_param(cpu, 2, 15, 0, min_low, 0xff);
+	if (ret)
+		return ret;
+
+	ret = set_clos_param(cpu, 3, 15, 0, min_low, 0xff);
+	if (ret)
+		return ret;
+
+	pkg_id = get_physical_package_id(cpu);
+	die_id = get_physical_die_id(cpu);
+	for (i = 0; i < get_topo_max_cpus(); ++i) {
+		int clos;
+
+		if (pkg_id != get_physical_package_id(i) ||
+		    die_id != get_physical_die_id(i))
+			continue;
+
+		if (CPU_ISSET_S(i, mask_size, cpu_mask))
+			clos = 0;
+		else
+			clos = 3;
+
+		debug_printf("Associate cpu: %d clos: %d\n", i, clos);
+		ret = isst_clos_associate(i, clos);
+		if (ret) {
+			perror("isst_clos_associate");
+			return ret;
+		}
+		set_cpufreq_scaling_min(i, min_low);
+	}
+
+	return 0;
+}
+
+static int set_pbf_core_power(int cpu)
+{
+	struct isst_pbf_info pbf_info;
+	struct isst_pkg_ctdp pkg_dev;
+	int ret;
+
+	ret = isst_get_ctdp_levels(cpu, &pkg_dev);
+	if (ret) {
+		perror("isst_get_ctdp_levels");
+		return ret;
+	}
+	debug_printf("Current_level: %d\n", pkg_dev.current_level);
+
+	ret = isst_get_pbf_info(cpu, pkg_dev.current_level, &pbf_info);
+	if (ret) {
+		perror("isst_get_pbf_info");
+		return ret;
+	}
+	debug_printf("p1_high: %d p1_low: %d\n", pbf_info.p1_high,
+		     pbf_info.p1_low);
+
+	ret = set_core_priority_and_min(cpu, pbf_info.core_cpumask_size,
+					pbf_info.core_cpumask,
+					pbf_info.p1_high, pbf_info.p1_low);
+	if (ret) {
+		perror("set_core_priority_and_min");
+		return ret;
+	}
+
+	ret = isst_pm_qos_config(cpu, 1, 1);
+	if (ret) {
+		perror("isst_pm_qos_config");
+		return ret;
+	}
+
+	return 0;
+}
+
 static void set_pbf_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 			    void *arg4)
 {
@@ -862,12 +1031,17 @@ static void set_pbf_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 	if (ret) {
 		perror("isst_set_pbf");
 	} else {
-		if (status)
+		if (status) {
+			if (auto_mode)
+				ret = set_pbf_core_power(cpu);
 			isst_display_result(cpu, outf, "base-freq", "enable",
 					    ret);
-		else
+		} else {
+			if (auto_mode)
+				isst_pm_qos_config(cpu, 0, 0);
 			isst_display_result(cpu, outf, "base-freq", "disable",
 					    ret);
+		}
 	}
 }
 
@@ -877,7 +1051,10 @@ static void set_pbf_enable(void)
 
 	if (cmd_help) {
 		fprintf(stderr,
-			"Enable Intel Speed Select Technology base frequency feature [No command arguments are required]\n");
+			"Enable Intel Speed Select Technology base frequency feature\n");
+		fprintf(stderr,
+			"\tOptional Arguments: -a|--auto : Use priority of cores to set core-power associations\n");
+
 		exit(0);
 	}
 
@@ -897,7 +1074,9 @@ static void set_pbf_disable(void)
 
 	if (cmd_help) {
 		fprintf(stderr,
-			"Disable Intel Speed Select Technology base frequency feature [No command arguments are required]\n");
+			"Disable Intel Speed Select Technology base frequency feature\n");
+		fprintf(stderr,
+			"\tOptional Arguments: -a|--auto : Also disable core-power associations\n");
 		exit(0);
 	}
 
@@ -909,6 +1088,9 @@ static void set_pbf_disable(void)
 		for_each_online_package_in_set(set_pbf_for_cpu, NULL, NULL,
 					       NULL, &status);
 	isst_ctdp_display_information_end(outf);
+
+	if (auto_mode)
+		reset_cpufreq_scaling_min();
 }
 
 static void dump_fact_config_for_cpu(int cpu, void *arg1, void *arg2,
@@ -1417,15 +1599,19 @@ static void parse_cmd_args(int argc, int start, char **argv)
 		{ "max", required_argument, 0, 'm' },
 		{ "priority", required_argument, 0, 'p' },
 		{ "weight", required_argument, 0, 'w' },
+		{ "auto", no_argument, 0, 'a' },
 		{ 0, 0, 0, 0 }
 	};
 
 	option_index = start;
 
 	optind = start + 1;
-	while ((opt = getopt_long(argc, argv, "b:l:t:c:d:e:n:m:p:w:ho",
+	while ((opt = getopt_long(argc, argv, "b:l:t:c:d:e:n:m:p:w:hoa",
 				  long_options, &option_index)) != -1) {
 		switch (opt) {
+		case 'a':
+			auto_mode = 1;
+			break;
 		case 'b':
 			fact_bucket = atoi(optarg);
 			break;
-- 
2.17.2

[PATCH 2/3] tools/power/x86/intel-speed-select: Turbo-freq feature auto mode

[Srinivas Pandruvada]

Introduce --auto|-a option to turbo-freq enable feature, so that it
does in one step for users who are OK by setting all passed target cores
as high priority and set in CLOS 0 and remaining in CLOS 3. In this way,
users don't have to take multiple steps to enable turbo-freq feature. For
users who want more fine grain control, they can always use core-power
feature to set custom CLOS configuration and assignment.

While here also print the error to output when clos configuration fails.

For example
intel-speed-select -c 0-4 turbo-freq enable --auto

The above command will enable turbo-freq and core-power feature. Also
mark CPU 0 to CPU 4 as high priority.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 79 ++++++++++++++++---
 1 file changed, 69 insertions(+), 10 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 5da59ff47306..edc4c50853bd 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1163,19 +1163,24 @@ static void set_fact_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 			isst_set_trl_from_current_tdp(cpu, fact_trl);
 			isst_display_result(cpu, outf, "turbo-freq", "disable",
 					    ret);
+			if (auto_mode)
+				isst_pm_qos_config(cpu, 0, 0);
 		}
 	}
 }
 
 static void set_fact_enable(void)
 {
-	int status = 1;
+	int status = 1, i, j, ret;
 
 	if (cmd_help) {
 		fprintf(stderr,
 			"Enable Intel Speed Select Technology Turbo frequency feature\n");
 		fprintf(stderr,
 			"Optional: -t|--trl : Specify turbo ratio limit\n");
+		fprintf(stderr,
+			"\tOptional Arguments: -a|--auto : Designate specified target CPUs with");
+		fprintf(stderr, "-C|--cpu option as as high priority using core-power feature\n");
 		exit(0);
 	}
 
@@ -1187,6 +1192,59 @@ static void set_fact_enable(void)
 		for_each_online_package_in_set(set_fact_for_cpu, NULL, NULL,
 					       NULL, &status);
 	isst_ctdp_display_information_end(outf);
+
+	if (auto_mode) {
+		for (i = 0; i < get_topo_max_cpus(); ++i) {
+			int clos;
+
+			if (!CPU_ISSET_S(i, present_cpumask_size, present_cpumask))
+				continue;
+
+			ret = set_clos_param(i, 0, 0, 0, 0, 0xff);
+			if (ret)
+				goto error_disp;
+
+			ret = set_clos_param(i, 1, 15, 0, 0, 0xff);
+			if (ret)
+				goto error_disp;
+
+			ret = set_clos_param(i, 2, 15, 0, 0, 0xff);
+			if (ret)
+				goto error_disp;
+
+			ret = set_clos_param(i, 3, 15, 0, 0, 0xff);
+			if (ret)
+				goto error_disp;
+
+			if (CPU_ISSET_S(i, target_cpumask_size, target_cpumask))
+				clos = 0;
+			else
+				clos = 3;
+
+			ret = isst_pm_qos_config(i, 1, 1);
+			if (ret)
+				goto error_disable;
+
+			debug_printf("Associate cpu: %d clos: %d\n", i, clos);
+			ret = isst_clos_associate(i, clos);
+			if (ret)
+				goto error_disp;
+		}
+		isst_display_result(i, outf, "turbo-freq --auto", "enable", 0);
+	}
+
+	return;
+
+error_disable:
+	for (j = 0; j < i; ++j) {
+		if (!CPU_ISSET_S(j, present_cpumask_size, present_cpumask))
+				continue;
+
+		isst_pm_qos_config(j, 0, 0);
+	}
+error_disp:
+	isst_display_result(i, outf, "turbo-freq --auto", "enable", ret);
+
 }
 
 static void set_fact_disable(void)
@@ -1198,6 +1256,8 @@ static void set_fact_disable(void)
 			"Disable Intel Speed Select Technology turbo frequency feature\n");
 		fprintf(stderr,
 			"Optional: -t|--trl : Specify turbo ratio limit\n");
+		fprintf(stderr,
+			"\tOptional Arguments: -a|--auto : Also disable core-power associations\n");
 		exit(0);
 	}
 
@@ -1218,16 +1278,15 @@ static void enable_clos_qos_config(int cpu, void *arg1, void *arg2, void *arg3,
 	int status = *(int *)arg4;
 
 	ret = isst_pm_qos_config(cpu, status, clos_priority_type);
-	if (ret) {
+	if (ret)
 		perror("isst_pm_qos_config");
-	} else {
-		if (status)
-			isst_display_result(cpu, outf, "core-power", "enable",
-					    ret);
-		else
-			isst_display_result(cpu, outf, "core-power", "disable",
-					    ret);
-	}
+
+	if (status)
+		isst_display_result(cpu, outf, "core-power", "enable",
+				    ret);
+	else
+		isst_display_result(cpu, outf, "core-power", "disable",
+				    ret);
 }
 
 static void set_clos_enable(void)
-- 
2.17.2

[PATCH 3/3] tools/power/x86/intel-speed-select: Refuse to disable core-power when getting used

[Srinivas Pandruvada]

The turbo-freq feature is dependent on the core-power feature. If the
core-power feature is disabled while the turbo-freq feature is enabled,
this will break the turbo-freq feature. This is a firmware limitation,
where they can't return error under this scenario.

So when trying to disable core-power, make sure that the turbo-freq
feature is not enabled. If it enabled, return error if user is trying to
disable the core-power feature.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../power/x86/intel-speed-select/isst-core.c  | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/power/x86/intel-speed-select/isst-core.c b/tools/power/x86/intel-speed-select/isst-core.c
index 6dee5332c9d3..67d32f2b9bea 100644
--- a/tools/power/x86/intel-speed-select/isst-core.c
+++ b/tools/power/x86/intel-speed-select/isst-core.c
@@ -649,6 +649,27 @@ int isst_pm_qos_config(int cpu, int enable_clos, int priority_type)
 	unsigned int req, resp;
 	int ret;
 
+	if (!enable_clos) {
+		struct isst_pkg_ctdp pkg_dev;
+		struct isst_pkg_ctdp_level_info ctdp_level;
+
+		ret = isst_get_ctdp_levels(cpu, &pkg_dev);
+		if (ret) {
+			debug_printf("isst_get_ctdp_levels\n");
+			return ret;
+		}
+
+		ret = isst_get_ctdp_control(cpu, pkg_dev.current_level,
+					    &ctdp_level);
+		if (ret)
+			return ret;
+
+		if (ctdp_level.fact_enabled) {
+			debug_printf("Turbo-freq feature must be disabled first\n");
+			return -EINVAL;
+		}
+	}
+
 	ret = isst_send_mbox_command(cpu, CONFIG_CLOS, CLOS_PM_QOS_CONFIG, 0, 0,
 				     &resp);
 	if (ret)
-- 
2.17.2

Re: [PATCH 0/3] Auto configuration mode and error

[Andy Shevchenko]

On Thu, Sep 26, 2019 at 1:32 AM Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> These are some changes, which help users to use the base-freq and
> turbo-freq features without going through multiple steps for
> basic configuration. Also add some error when user is trying
> to disable core-power feature while it is getting used.
>
> None of these patches are urgent and can wait for kernel version v5.5.

I'm completely lost in ISST patches.
Please send an updated version of the entire bunch of changes for ISST
driver and do the same for tools.
Thanks.

I'm going to drop these ones from my review queue, so, I'm expecting
it as a part of new version against tools.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] Auto configuration mode and error

[Srinivas Pandruvada]

On Wed, 2019-10-09 at 14:46 +0300, Andy Shevchenko wrote:
> On Thu, Sep 26, 2019 at 1:32 AM Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > These are some changes, which help users to use the base-freq and
> > turbo-freq features without going through multiple steps for
> > basic configuration. Also add some error when user is trying
> > to disable core-power feature while it is getting used.
> > 
> > None of these patches are urgent and can wait for kernel version
> > v5.5.
> 
> I'm completely lost in ISST patches.
> Please send an updated version of the entire bunch of changes for
> ISST
> driver and do the same for tools.
There are no changes in driver.
Only tool.

Let me send in one series, both mine and Prarit's patches, which can be
applied on top of 5.4-rc2.

Thanks,
Srinivas


> Thanks.
> 
> I'm going to drop these ones from my review queue, so, I'm expecting
> it as a part of new version against tools.
>