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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
