On Thu, Jan 15, 2015 at 02:00:06PM +0000, Dave Gordon wrote:
> 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 <b...@bwidawsk.net>
> > Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
> > Cc: Kenneth Graunke <kenn...@whitecape.org>
>
> I think the syntax of some of these commands is rather strange;
It all uses standard POSIX commands to parse them. If you think the names are
bad, we can discuss that.
> 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.
Yes, this was laziness feel free to fix it.
>
> 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".
This is a limitation of getsubopt() which is in the process of being removed
anyway, so I wouldn't worry about it.
>
> > $ 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.
>
Looks like I messed up the comment, but it's going away shortly anyway. There is
a man page and help that describe the usage in more detail. I changed a bunch of
this midflight, and probably forgot to update the comments. Hasn't anyone ever
told you not to read comments before? If not, let me be the first :P
> 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
--get is going to always get all. Initially I thought scripts could want to run
this app, but it's probably better if they just read the sysfs directly, if
the're scripting. Tim Gore already sent a patch which I've reviewed.
>
> # 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.
>
I honestly don't care too much as long as we don't roll our own parsing. It made
sense to me to do it this way when I wrote it - note I didn't initially have
--custom at all, it was added by request from Jordan. Feel free to submit a
patch to change it to what you want. Opinions are like a$$holes, and everyone
has one.
Do realize again that the comments may not match the reality, and see Tim Gore's
patch before you change anything.
> > $ 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
