Re: [PATCH] [v2] intel_frequency: A tool to manipulate Intel GPU frequency

[Dave Gordon <david.s.gordon@intel.com>]

On 12/01/15 03:35, Ben Widawsky wrote:
> WARNING: very minimally tested
> 
> In general you should not need this tool. It's primary purpose is for
> benchmarking, and for debugging performance issues.
> 
> For many kernel releases now sysfs has supported reading and writing the GPU
> frequency. Therefore, this tool provides no new functionality. What it does
> provide is an easy to package (for distros) tool that handles the most common
> scenarios.
> 
> v2:
> Get rid of -f from the usage message (Jordan)
> Add space before [-s (Jordan)
> Add a -c/--custom example (Jordan)
> Add a setting for resetting to hardware default (Ken)
> Replicate examples in commit message in the source code. (me)
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Kenneth Graunke <kenneth@whitecape.org>

I think the syntax of some of these commands is rather strange; also, it
would be nicer to have consistent naming of inputs & outputs -
specifically, the input name "eff" is related to the output tag "RP1".
Please pick one of the other.

And are all the examples below actually correct?

> Here are some sample usages:
> $ sudo intel_frequency --get=cur,min,max,eff
> cur: 200 MHz
> min: 200 MHz
> RP1: 200 MHz
> max: 1200 MHz
> 
> $ sudo intel_frequency -g
> cur: 200 MHz
> min: 200 MHz
> RP1: 200 MHz
> max: 1200 MHz
> 
> $ sudo intel_frequency -geff
> RP1: 200 MHz

I wouldn't expect this syntax to work, because generally single-letter
options can be combined, so that "-geff" would be equivalent to "-g -e
-f -f". I'd expect to have to write "-g eff" or "-g=eff".

> $ sudo intel_frequency --set min=300
> $ sudo intel_frequency --get min
> cur: 300 MHz
> min: 300 MHz
> RP1: 200 MHz
> max: 1200 MHz

Surely if you specify "--get min" it should only print the "min" value,
not all the others; and the same with "--get max" below. (However, I
think you actually do need to show all values in these two examples to
clarify the effects of the parameter-setting commands, so I'd change
them to get rid of the parameter name).

Here, it isn't really clear what the difference between --custom and
--set is supposed to be. Partly this is just down to the numbers in
these specific examples, but also I think the examples don't agree with
the code below.

> $ sudo intel_frequency --custom max=900
> $ sudo intel_frequency --get max
> cur: 300 MHz
> min: 300 MHz
> RP1: 200 MHz
> max: 900 MHz

So from the code, I deduce that "--custom" lets you change either 'min'
or 'max', but doesn't /necessarily/ change the current frequency, only
if it becomes out-of-bounds.

OTOH "--set" is supposed to set both min *and* max, thus locking the
frequency to a specific value. But the --set example above doesn't show
that. Also, the "min=300" syntax would be illogical - you just need a
number, and doesn't appear to be what the code actually expects.

Finally, I think the choice of command names (especially --set vs
--custom) is confusing. I'd like to propose the following:

# Reading:
	--get		# get all
	--get namelist	# get specified
	-g [namelist]	# synonym for -g

# Set one parameter:
	--set name=freq	# set the specified parameter (min or max)
			# may or may not affect 'cur'
	-s name=freq	# synonym

# Reset (all) parameters:
	--defaults	# Reset all r/w parameters to h/w defaults
	-d		# synonym

# Lock to specific frequency:
	--max		# Lock to (h/w defined) max
	-m		# synonym
	--min		# Lock to (h/w defined) min
	-i		# synonym
	--eff		# Lock to (h/w defined) RPe
	-e		# synonym
	--lock freq	# Lock to custom frequency
	-l freq		# synonym

[Aside]
If the locking commands are implemented by setting both 'min' and 'max'
to the same value, as appears to be the case in the code below, then
defining "--max" and "--min to mean "lock to current value of max/min
parameter (as I was originally going to) would result in the sequence of
commands:

$ sudo intel_frequency --lock 900
$ sudo intel_frequency --max

not having the expected effect - the second command would have no
effect, because max would have been changed to 900 by the first. So it's
better to for --min and --max to lock the frequency to the h/w default
values.
[/Aside]

Only one command may be given per invocation. (It could be meaningful to
combine some permutations, but it would be complicated and/or confusing
and involve extra (arbitrary) decisions, such as whether options are
processed in command-line order or in fixed order, and hence whether or
not "-d -g -m" is equivalent to "-m -g -d", etc. So KISS).

.Dave.

> $ sudo intel_frequency --max
> $ sudo intel_frequency -g
> cur: 1200 MHz
> min: 1200 MHz
> RP1: 200 MHz
> max: 1200 MHz
> 
> $ sudo intel_frequency -e
> $ sudo intel_frequency -g
> cur: 200 MHz
> min: 200 MHz
> RP1: 200 MHz
> max: 200 MHz
> 
> $ sudo intel_frequency --max
> $ sudo intel_frequency -g
> cur: 1200 MHz
> min: 1200 MHz
> RP1: 200 MHz
> max: 1200 MHz
> 
> $ sudo intel_frequency --min
> $ sudo intel_frequency -g
> cur: 200 MHz
> min: 200 MHz
> RP1: 200 MHz
> max: 200 MHz
> ---
>  tools/Makefile.sources  |   1 +
>  tools/intel_frequency.c | 363 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 364 insertions(+)
>  create mode 100644 tools/intel_frequency.c
> 
> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> index b85a6b8..2bea389 100644
> --- a/tools/Makefile.sources
> +++ b/tools/Makefile.sources
> @@ -14,6 +14,7 @@ bin_PROGRAMS = 				\
>  	intel_dump_decode 		\
>  	intel_error_decode 		\
>  	intel_forcewaked		\
> +	intel_frequency			\
>  	intel_framebuffer_dump 		\
>  	intel_gpu_time 			\
>  	intel_gpu_top 			\
> diff --git a/tools/intel_frequency.c b/tools/intel_frequency.c
> new file mode 100644
> index 0000000..59f3814
> --- /dev/null
> +++ b/tools/intel_frequency.c
> @@ -0,0 +1,363 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Example:
> + * Get all frequencies:
> + * intel_frequency --get=cur,min,max,eff
> + *
> + * Same as above:
> + * intel_frequency -g
> + *
> + * Get the efficient frequency:
> + * intel_frequency -geff
> + *
> + * Lock the GPU frequency to 300MHz:
> + * intel_frequency --set min=300
> + *
> + * Set the maximum frequency to 900MHz:
> + * intel_frequency --custom max=900
> + *
> + * Lock the GPU frequency to its maximum frequency:
> + * intel_frequency --max
> + *
> + * Lock the GPU frequency to its most efficient frequency:
> + * intel_frequency -e
> + *
> + * Lock The GPU frequency to its minimum frequency:
> + * intel_frequency --min
> + *
> + * Reset the GPU to hardware defaults
> + * intel_frequency -d
> + */
> +
> +#define _GNU_SOURCE
> +#include <assert.h>
> +#include <getopt.h>
> +#include <stdio.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +#include "drmtest.h"
> +#include "intel_chipset.h"
> +
> +static int device, devid;
> +
> +enum {
> +	CUR=0,
> +	MIN,
> +	EFF,
> +	MAX,
> +	RP0,
> +	RPn
> +};
> +
> +struct freq_info {
> +	const char *name;
> +	const char *mode;
> +	FILE *filp;
> +	char *path;
> +};
> +
> +static struct freq_info info[] = {
> +	{ "cur", "r"  },
> +	{ "min", "rb+" },
> +	{ "RP1", "r" },
> +	{ "max", "rb+" },
> +	{ "RP0", "r" },
> +	{ "RPn", "r" }
> +};
> +
> +static char *
> +get_sysfs_path(const char *which)
> +{
> +	static const char fmt[] = "/sys/class/drm/card%1d/gt_%3s_freq_mhz";
> +	char *path;
> +	int ret;
> +
> +#define STATIC_STRLEN(string) (sizeof(string) / sizeof(string [0]))
> +	ret = asprintf(&path, fmt, device, which);
> +	assert(ret == (STATIC_STRLEN(fmt) - 3));
> +#undef STATIC_STRLEN
> +
> +	return path;
> +}
> +
> +static void
> +initialize_freq_info(struct freq_info *freq_info)
> +{
> +	if (freq_info->filp)
> +		return;
> +
> +	freq_info->path = get_sysfs_path(freq_info->name);
> +	assert(freq_info->path);
> +	freq_info->filp = fopen(freq_info->path, freq_info->mode);
> +	assert(freq_info->filp);
> +}
> +
> +static void wait_freq_settle(void)
> +{
> +	struct timespec ts;
> +
> +	/* FIXME: Lazy sleep without check. */
> +	ts.tv_sec = 0;
> +	ts.tv_nsec = 20000;
> +	clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, NULL);
> +}
> +
> +static void set_frequency(struct freq_info *freq_info, int val)
> +{
> +	initialize_freq_info(freq_info);
> +	rewind(freq_info->filp);
> +	assert(fprintf(freq_info->filp, "%d", val) > 0);
> +
> +	wait_freq_settle();
> +}
> +
> +static int get_frequency(struct freq_info *freq_info)
> +{
> +	int val;
> +
> +	initialize_freq_info(freq_info);
> +	rewind(freq_info->filp);
> +	assert(fscanf(freq_info->filp, "%d", &val)==1);
> +
> +	return val;
> +}
> +
> +static void
> +usage(const char *prog)
> +{
> +	printf("Usage: %s [-e] [--min | --max] [-g (min|max|efficient)] [-s frequency_mhz]\n\n", prog);
> +	printf("%s A program to manipulate Intel GPU frequencies.\n\n", prog);
> +	printf("Options: \n");
> +	printf("  -e		Lock frequency to the most efficient frequency\n");
> +	printf("  -g, --get=    Get the frequency (optional arg: \"cur\"|\"min\"|\"max\"|\"eff\")\n");
> +	printf("  -s, --set     Lock frequency to an absolute value (MHz)\n");
> +	printf("  -c, --custom  Set a min, or max frequency \"min=X | max=Y\"\n");
> +	printf("  -m  --max     Lock frequency to max frequency\n");
> +	printf("  -i  --min     Lock frequency to min (never a good idea, DEBUG ONLY)\n");
> +	printf("  -d  --defaults  Return the system to hardware defaults\n");
> +	printf("Examples:\n");
> +	printf("\tintel_frequency -gmin,cur Get the current and minimum frequency\n");
> +	printf("\tintel_frequency -s 400    Lock frequency to 400Mhz\n");
> +	printf("\tintel_frequency -c max=750 Set the max frequency to 750MHz\n");
> +	exit(EXIT_FAILURE);
> +}
> +
> +/* Returns read or write operation */
> +static bool
> +parse(int argc, char *argv[], bool *act_upon, int *new_freq)
> +{
> +	int c, tmp;
> +	bool write = false;
> +
> +	char *token[] = {
> +		(char *)info[CUR].name,
> +		(char *)info[MIN].name,
> +		(char *)"eff",
> +		(char *)info[MAX].name
> +	};
> +
> +	/* No args means -g" */
> +	if (argc == 1) {
> +		for (c = 0; c < ARRAY_SIZE(act_upon); c++)
> +			act_upon[c] = true;
> +		goto done;
> +	}
> +	while (1) {
> +		int option_index = 0;
> +		static struct option long_options[] = {
> +			{ "get", optional_argument, NULL, 'g' },
> +			{ "set", required_argument, NULL, 's' },
> +			{ "custom", required_argument, NULL, 'c'},
> +			{ "min", no_argument, NULL, 'i' },
> +			{ "max", no_argument, NULL, 'm' },
> +			{ "defaults", no_argument, NULL, 'd' },
> +			{ "help", no_argument, NULL, 'h' },
> +			{ NULL, 0, NULL, 0}
> +		};
> +
> +		c = getopt_long(argc, argv, "eg::s:c:midh", long_options, &option_index);
> +		if (c == -1)
> +			break;
> +
> +		switch (c) {
> +		case 'g':
> +			if (write == true)
> +				fprintf(stderr, "Read and write operations not support simultaneously.\n");
> +
> +			if (optarg) {
> +				char *value, *subopts = optarg;
> +				int x;
> +				while (*subopts != '\0') {
> +					x = getsubopt(&subopts, token, &value);
> +					if (x == -1) {
> +						fprintf(stderr, "Unrecognized option (%s)\n", value);
> +						break;
> +					} else
> +						act_upon[x] = true;
> +				}
> +			} else {
> +				int i;
> +				for (i = 0; i < ARRAY_SIZE(act_upon); i++)
> +					act_upon[i] = true;
> +			}
> +			break;
> +		case 's':
> +			if (!optarg)
> +				usage(argv[0]);
> +
> +			if (write == true) {
> +				fprintf(stderr, "Only one write may be specified at a time\n");
> +				exit(EXIT_FAILURE);
> +			}
> +
> +			write = true;
> +			act_upon[MIN] = true;
> +			act_upon[MAX] = true;
> +			sscanf(optarg, "%d", &new_freq[MAX]);
> +			new_freq[MIN] = new_freq[MAX];
> +			break;
> +		case 'c':
> +			if (!optarg)
> +				usage(argv[0]);
> +
> +			if (write == true) {
> +				fprintf(stderr, "Only one write may be specified at a time\n");
> +				exit(EXIT_FAILURE);
> +			}
> +
> +			write = true;
> +
> +			if (!strncmp("min=", optarg, 4)) {
> +				act_upon[MIN] = true;
> +				sscanf(optarg+4, "%d", &new_freq[MIN]);
> +			} else if (!strncmp("max=", optarg, 4)) {
> +				act_upon[MAX] = true;
> +				sscanf(optarg+4, "%d", &new_freq[MAX]);
> +			} else {
> +				fprintf(stderr, "Selected unmodifiable frequency\n");
> +				exit(EXIT_FAILURE);
> +			}
> +			break;
> +		case 'e': /* efficient */
> +			if (IS_VALLEYVIEW(devid) || IS_CHERRYVIEW(devid)) {
> +				/* the LP parts have special efficient frequencies */
> +				fprintf(stderr,
> +					"FIXME: Warning efficient frequency information is incorrect.\n");
> +				exit(EXIT_FAILURE);
> +			}
> +			tmp = get_frequency(&info[EFF]);
> +			new_freq[MIN] = tmp;
> +			new_freq[MAX] = tmp;
> +			act_upon[MIN] = true;
> +			act_upon[MAX] = true;
> +			write = true;
> +			break;
> +		case 'i': /* mIn */
> +			tmp = get_frequency(&info[RPn]);
> +			new_freq[MIN] = tmp;
> +			new_freq[MAX] = tmp;
> +			act_upon[MIN] = true;
> +			act_upon[MAX] = true;
> +			write = true;
> +			break;
> +		case 'm': /* max */
> +			tmp = get_frequency(&info[RP0]);
> +			new_freq[MIN] = tmp;
> +			new_freq[MAX] = tmp;
> +			act_upon[MIN] = true;
> +			act_upon[MAX] = true;
> +			write = true;
> +			break;
> +		case 'd': /* defaults */
> +			new_freq[MIN] = get_frequency(&info[RPn]);
> +			new_freq[MAX] = get_frequency(&info[RP0]);
> +			act_upon[MIN] = true;
> +			act_upon[MAX] = true;
> +			write = true;
> +			break;
> +		case 'h':
> +		default:
> +			usage(argv[0]);
> +		}
> +	}
> +
> +done:
> +	return write;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +
> +	bool write, fail, targets[MAX+1] = {false};
> +	int i, try = 1, set_freq[MAX+1] = {0};
> +
> +	devid = intel_get_drm_devid(drm_open_any());
> +	device = drm_get_card();
> +
> +	write = parse(argc, argv, targets, set_freq);
> +	fail = write;
> +
> +	/* If we've previously locked the frequency, we need to make sure to set things
> +	 * in the correct order, or else the operation will fail (ie. min = max = 200,
> +	 * and we set min to 300, we fail because it would try to set min >
> +	 * max). This can be accomplished be going either forward or reverse
> +	 * through the loop. MIN is always before MAX.
> +	 *
> +	 * XXX: Since only min and max are at play, the super lazy way is to do this
> +	 * 3 times and if we still fail after 3, it's for real.
> +	 */
> +again:
> +	if (try > 2) {
> +		fprintf(stderr, "Did not achieve desired freq.\n");
> +		exit(EXIT_FAILURE);
> +	}
> +	for (i = 0; i < ARRAY_SIZE(targets); i++) {
> +		if (targets[i] == false)
> +			continue;
> +
> +		if (write) {
> +			set_frequency(&info[i], set_freq[i]);
> +			if (get_frequency(&info[i]) != set_freq[i])
> +				fail = true;
> +			else
> +				fail = false;
> +		} else {
> +			printf("%s: %d MHz\n", info[i].name, get_frequency(&info[i]));
> +		}
> +	}
> +
> +	if (fail) {
> +		try++;
> +		goto again;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(targets); i++) {
> +		if (info[i].filp) {
> +			fclose(info[i].filp);
> +			free(info[i].path);
> +		}
> +	}
> +
> +	return EXIT_SUCCESS;
> +}
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx