Re: [igt-dev] [PATCH i-g-t 02/24] lib: Add GPU power measurement

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t 02/24] lib: Add GPU power measurement
Date: Tue, 26 Mar 2019 08:49:07 +0000	[thread overview]
Message-ID: <155359014752.15930.11670960100247903065@skylake-alporthouse-com> (raw)
In-Reply-To: <204a2a2c-4372-0284-d715-9cf474de6dac@linux.intel.com>

Quoting Tvrtko Ursulin (2019-03-26 08:36:18)
> 
> On 22/03/2019 09:21, Chris Wilson wrote:
> > Read the RAPL power metrics courtesy of perf. Or your local HW
> > equivalent?
> > 
> > v2: uselocale()
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   lib/Makefile.am      |   1 +
> >   lib/Makefile.sources |   2 +
> >   lib/igt_gpu_power.c  | 114 +++++++++++++++++++++++++++++++++++++++++++
> >   lib/igt_gpu_power.h  |  59 ++++++++++++++++++++++
> >   lib/meson.build      |   2 +
> >   5 files changed, 178 insertions(+)
> >   create mode 100644 lib/igt_gpu_power.c
> >   create mode 100644 lib/igt_gpu_power.h
> > 
> > diff --git a/lib/Makefile.am b/lib/Makefile.am
> > index 3e6a7fdba..62e8bda73 100644
> > --- a/lib/Makefile.am
> > +++ b/lib/Makefile.am
> > @@ -84,4 +84,5 @@ libintel_tools_la_LIBADD = \
> >       $(LIBUDEV_LIBS) \
> >       $(PIXMAN_LIBS) \
> >       $(GLIB_LIBS) \
> > +     libigt_perf.la \
> >       -lm
> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> > index cf2720981..e00347f94 100644
> > --- a/lib/Makefile.sources
> > +++ b/lib/Makefile.sources
> > @@ -26,6 +26,8 @@ lib_source_list =           \
> >       igt_color_encoding.c    \
> >       igt_color_encoding.h    \
> >       igt_edid_template.h     \
> > +     igt_gpu_power.c         \
> > +     igt_gpu_power.h         \
> >       igt_gt.c                \
> >       igt_gt.h                \
> >       igt_gvt.c               \
> > diff --git a/lib/igt_gpu_power.c b/lib/igt_gpu_power.c
> > new file mode 100644
> > index 000000000..a4e3c1420
> > --- /dev/null
> > +++ b/lib/igt_gpu_power.c
> > @@ -0,0 +1,114 @@
> > +#include <ctype.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <locale.h>
> > +#include <math.h>
> > +#include <unistd.h>
> > +
> > +#include "igt_gpu_power.h"
> > +#include "igt_perf.h"
> > +
> > +static int filename_to_buf(const char *filename, char *buf, unsigned int sz)
> > +{
> > +     int fd;
> > +     ssize_t ret;
> > +
> > +     fd = open(filename, O_RDONLY);
> > +     if (fd < 0)
> > +             return -1;
> > +
> > +     ret = read(fd, buf, sz - 1);
> > +     close(fd);
> > +     if (ret < 1)
> > +             return -1;
> > +
> > +     buf[ret] = '\0';
> > +
> > +     return 0;
> > +}
> > +
> > +static uint64_t filename_to_u64(const char *filename, int base)
> > +{
> > +     char buf[64], *b;
> > +
> > +     if (filename_to_buf(filename, buf, sizeof(buf)))
> > +             return 0;
> > +
> > +     /*
> > +      * Handle both single integer and key=value formats by skipping
> > +      * leading non-digits.
> > +      */
> > +     b = buf;
> > +     while (*b && !isdigit(*b))
> > +             b++;
> > +
> > +     return strtoull(b, NULL, base);
> > +}
> > +
> > +static double filename_to_double(const char *filename)
> > +{
> > +     locale_t locale, oldlocale;
> > +     char buf[80];
> > +     double v;
> > +
> > +     if (filename_to_buf(filename, buf, sizeof(buf)))
> > +             return 0;
> > +
> > +     /* Replace user environment with plain C to match kernel format */
> > +     locale = newlocale(LC_ALL, "C", 0);
> > +     oldlocale = uselocale(locale);
> > +
> > +     v = strtod(buf, NULL);
> > +
> > +     uselocale(oldlocale);
> > +     freelocale(locale);
> > +
> > +     return v;
> > +}
> 
> Hey you know what our guidelines for introducing a 3rd copy of something 
> are. ;)

I'm not even sure this is the third copy... Except for the handling of
plain C strtod().

> Maybe it would be fair game to stuff these helpers to igt_perf.la - 
> since both overlay and intel_gpu_gpu link with it already and it is 
> about interfacing with perf after all.

As far as igt goes, imo we should move the strtod handling into igt_sysfs
with the other scanf.

> There are some small differences between the functions here and in 
> intel_gpu_top, someone is right or wrong, or at least more right?

Only changed to use threadsafe uselocale() iirc.

> > +static uint64_t rapl_type_id(void)
> > +{
> > +     return filename_to_u64("/sys/devices/power/type", 10);
> > +}
> > +
> > +static uint64_t rapl_gpu_power(void)
> > +{
> > +     return filename_to_u64("/sys/devices/power/events/energy-gpu", 0);
> > +}
> > +
> > +static double rapl_gpu_power_scale(void)
> > +{
> > +     return filename_to_double("/sys/devices/power/events/energy-gpu.scale");
> > +}
> > +
> > +int gpu_power_open(struct gpu_power *power)
> > +{
> > +     power->fd = igt_perf_open(rapl_type_id(), rapl_gpu_power());
> > +     if (power->fd < 0) {
> > +             power->fd = -errno;
> > +             goto err;
> > +     }
> > +
> > +     power->scale = rapl_gpu_power_scale();
> > +     if (isnan(power->scale) || !power->scale) {
> > +             close(power->fd);
> > +             goto err;
> > +     }
> > +     power->scale *= 1e9;
> 
> Scale has no unit so I think this would go better into gpu_power_W. 
> Would avoid * 1e9 * 1e-9 in gpu_power_J as well.

It's 6 of one, half-a-dozen of the other. I was tempted to move it
around, but you still end up scaling somewhere.

gpu_power_J()
gpu_power_s()
gpu_power_W()

and hope for the best.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx