[Intel-gfx] [igt-dev] [PATCH i-g-t 2/2] tools/intel_gpu_top: Consolidate imc to use pmu_counter
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Wed Nov 25 08:58:09 UTC 2020
On 24/11/2020 23:27, Chris Wilson wrote:
> Follow the same pattern as rapl for imc, and consolidate everything to
> work on struct pmu_counter.
Looks nicer than before. Could you also do a common helper for imc_parse
and rapl_parse, since only the pmu name string would be different and
path could easily be constructed in the existing buf?
Regards,
Tvrtko
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> tools/intel_gpu_top.c | 249 ++++++++++++++----------------------------
> 1 file changed, 82 insertions(+), 167 deletions(-)
>
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 0a49cfecc..9af1c29b6 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -55,6 +55,7 @@ struct pmu_counter {
> unsigned int idx;
> struct pmu_pair val;
> double scale;
> + const char *units;
> bool present;
> };
>
> @@ -85,17 +86,14 @@ struct engines {
> unsigned int num_rapl;
>
> int imc_fd;
> - double imc_reads_scale;
> - const char *imc_reads_unit;
> - double imc_writes_scale;
> - const char *imc_writes_unit;
> + struct pmu_counter imc_reads;
> + struct pmu_counter imc_writes;
> + unsigned int num_imc;
>
> struct pmu_counter freq_req;
> struct pmu_counter freq_act;
> struct pmu_counter irq;
> struct pmu_counter rc6;
> - struct pmu_counter imc_reads;
> - struct pmu_counter imc_writes;
>
> bool discrete;
> char *device;
> @@ -400,146 +398,93 @@ static struct engines *discover_engines(char *device)
> return engines;
> }
>
> -static int
> -filename_to_buf(const char *filename, char *buf, unsigned int bufsize)
> -{
> - int fd, err;
> - ssize_t ret;
> -
> - fd = open(filename, O_RDONLY);
> - if (fd < 0)
> - return -1;
> -
> - ret = read(fd, buf, bufsize - 1);
> - err = errno;
> - close(fd);
> - if (ret < 1) {
> - errno = ret < 0 ? err : ENOMSG;
> -
> - return -1;
> - }
> +#define _open_pmu(type, cnt, pmu, fd) \
> +({ \
> + int fd__; \
> +\
> + fd__ = igt_perf_open_group((type), (pmu)->config, (fd)); \
> + if (fd__ >= 0) { \
> + if ((fd) == -1) \
> + (fd) = fd__; \
> + (pmu)->present = true; \
> + (pmu)->idx = (cnt)++; \
> + } \
> +\
> + fd__; \
> +})
>
> - if (ret > 1 && buf[ret - 1] == '\n')
> - buf[ret - 1] = '\0';
> - else
> - buf[ret] = '\0';
> +static int imc_parse(struct pmu_counter *pmu, const char *str)
> +{
> + locale_t locale, oldlocale;
> + bool result = true;
> + char buf[128] = {};
> + int dir;
>
> - return 0;
> -}
> + dir = open("/sys/devices/uncore_imc", O_RDONLY);
> + if (dir < 0)
> + return -errno;
>
> -static uint64_t filename_to_u64(const char *filename, int base)
> -{
> - char buf[64], *b;
> + /* Replace user environment with plain C to match kernel format */
> + locale = newlocale(LC_ALL, "C", 0);
> + oldlocale = uselocale(locale);
>
> - if (filename_to_buf(filename, buf, sizeof(buf)))
> - return 0;
> + result &= igt_sysfs_scanf(dir, "type", "%"PRIu64, &pmu->type) == 1;
>
> - /*
> - * Handle both single integer and key=value formats by skipping
> - * leading non-digits.
> - */
> - b = buf;
> - while (*b && !isdigit(*b))
> - b++;
> + snprintf(buf, sizeof(buf) - 1, "events/%s", str);
> + result &= igt_sysfs_scanf(dir, buf, "event=%"PRIx64, &pmu->config) == 1;
>
> - return strtoull(b, NULL, base);
> -}
> + snprintf(buf, sizeof(buf) - 1, "events/%s.scale", str);
> + result &= igt_sysfs_scanf(dir, buf, "%lf", &pmu->scale) == 1;
>
> -static double filename_to_double(const char *filename)
> -{
> - char *oldlocale;
> - char buf[80];
> - double v;
> + snprintf(buf, sizeof(buf) - 1, "events/%s.unit", str);
> + result &= igt_sysfs_scanf(dir, buf, "%127s", buf) == 1;
> + pmu->units = strdup(buf);
>
> - if (filename_to_buf(filename, buf, sizeof(buf)))
> - return 0;
> + uselocale(oldlocale);
> + freelocale(locale);
>
> - oldlocale = setlocale(LC_ALL, "C");
> - v = strtod(buf, NULL);
> - setlocale(LC_ALL, oldlocale);
> + close(dir);
>
> - return v;
> -}
> + if (!result)
> + return -EINVAL;
>
> -#define IMC_ROOT "/sys/devices/uncore_imc/"
> -#define IMC_EVENT "/sys/devices/uncore_imc/events/"
> + if (isnan(pmu->scale) || !pmu->scale)
> + return -ERANGE;
>
> -static uint64_t imc_type_id(void)
> -{
> - return filename_to_u64(IMC_ROOT "type", 10);
> + return 0;
> }
>
> -static uint64_t imc_data_reads(void)
> +static void
> +imc_open(struct pmu_counter *pmu,
> + const char *domain,
> + struct engines *engines)
> {
> - return filename_to_u64(IMC_EVENT "data_reads", 0);
> -}
> + int fd;
>
> -static double imc_data_reads_scale(void)
> -{
> - return filename_to_double(IMC_EVENT "data_reads.scale");
> -}
> + if (imc_parse(pmu, domain) < 0)
> + return;
>
> -static const char *imc_data_reads_unit(void)
> -{
> - char buf[32];
> + fd = igt_perf_open_group(pmu->type, pmu->config, engines->imc_fd);
> + if (fd < 0)
> + return;
>
> - if (filename_to_buf(IMC_EVENT "data_reads.unit", buf, sizeof(buf)) == 0)
> - return strdup(buf);
> - else
> - return NULL;
> -}
> + if (engines->imc_fd == -1)
> + engines->imc_fd = fd;
>
> -static uint64_t imc_data_writes(void)
> -{
> - return filename_to_u64(IMC_EVENT "data_writes", 0);
> + pmu->idx = engines->num_imc++;
> + pmu->present = true;
> }
>
> -static double imc_data_writes_scale(void)
> +static void imc_writes_open(struct pmu_counter *pmu, struct engines *engines)
> {
> - return filename_to_double(IMC_EVENT "data_writes.scale");
> + imc_open(pmu, "data_writes", engines);
> }
>
> -static const char *imc_data_writes_unit(void)
> +static void imc_reads_open(struct pmu_counter *pmu, struct engines *engines)
> {
> - char buf[32];
> -
> - if (filename_to_buf(IMC_EVENT "data_writes.unit",
> - buf, sizeof(buf)) == 0)
> - return strdup(buf);
> - else
> - return NULL;
> + imc_open(pmu, "data_reads", engines);
> }
>
> -#define _open_pmu(type, cnt, pmu, fd) \
> -({ \
> - int fd__; \
> -\
> - fd__ = igt_perf_open_group((type), (pmu)->config, (fd)); \
> - if (fd__ >= 0) { \
> - if ((fd) == -1) \
> - (fd) = fd__; \
> - (pmu)->present = true; \
> - (pmu)->idx = (cnt)++; \
> - } \
> -\
> - fd__; \
> -})
> -
> -#define _open_imc(cnt, pmu, fd) \
> -({ \
> - int fd__; \
> -\
> - fd__ = igt_perf_open_group(imc_type_id(), (pmu)->config, (fd)); \
> - if (fd__ >= 0) { \
> - if ((fd) == -1) \
> - (fd) = fd__; \
> - (pmu)->present = true; \
> - (pmu)->idx = (cnt)++; \
> - } \
> -\
> - fd__; \
> -})
> -
> static int pmu_init(struct engines *engines)
> {
> unsigned int i;
> @@ -595,38 +540,8 @@ static int pmu_init(struct engines *engines)
> }
>
> engines->imc_fd = -1;
> - if (imc_type_id()) {
> - unsigned int num = 0;
> -
> - engines->imc_reads_scale = imc_data_reads_scale();
> - engines->imc_writes_scale = imc_data_writes_scale();
> -
> - engines->imc_reads_unit = imc_data_reads_unit();
> - if (!engines->imc_reads_unit)
> - return -1;
> -
> - engines->imc_writes_unit = imc_data_writes_unit();
> - if (!engines->imc_writes_unit)
> - return -1;
> -
> - engines->imc_reads.config = imc_data_reads();
> - if (!engines->imc_reads.config)
> - return -1;
> -
> - engines->imc_writes.config = imc_data_writes();
> - if (!engines->imc_writes.config)
> - return -1;
> -
> - fd = _open_imc(num, &engines->imc_reads, engines->imc_fd);
> - if (fd < 0)
> - return -1;
> - fd = _open_imc(num, &engines->imc_writes, engines->imc_fd);
> - if (fd < 0)
> - return -1;
> -
> - engines->imc_reads.present = true;
> - engines->imc_writes.present = true;
> - }
> + imc_reads_open(&engines->imc_reads, engines);
> + imc_writes_open(&engines->imc_writes, engines);
>
> return 0;
> }
> @@ -692,13 +607,6 @@ static void pmu_sample(struct engines *engines)
> unsigned int i;
>
> engines->ts.prev = engines->ts.cur;
> -
> - if (engines->imc_fd >= 0) {
> - pmu_read_multi(engines->imc_fd, 2, val);
> - update_sample(&engines->imc_reads, val);
> - update_sample(&engines->imc_writes, val);
> - }
> -
> engines->ts.cur = pmu_read_multi(engines->fd, num_val, val);
>
> update_sample(&engines->freq_req, val);
> @@ -719,6 +627,12 @@ static void pmu_sample(struct engines *engines)
> update_sample(&engines->r_gpu, val);
> update_sample(&engines->r_pkg, val);
> }
> +
> + if (engines->num_imc) {
> + pmu_read_multi(engines->imc_fd, engines->num_imc, val);
> + update_sample(&engines->imc_reads, val);
> + update_sample(&engines->imc_writes, val);
> + }
> }
>
> static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
> @@ -1172,9 +1086,9 @@ static int
> print_imc(struct engines *engines, double t, int lines, int con_w, int con_h)
> {
> struct cnt_item imc_items[] = {
> - { &engines->imc_reads, 6, 0, 1.0, t, engines->imc_reads_scale,
> + { &engines->imc_reads, 6, 0, 1.0, t, engines->imc_reads.scale,
> "reads", "rd" },
> - { &engines->imc_writes, 6, 0, 1.0, t, engines->imc_writes_scale,
> + { &engines->imc_writes, 6, 0, 1.0, t, engines->imc_writes.scale,
> "writes", "wr" },
> { NULL, 0, 0, 0.0, 0.0, 0.0, "unit" },
> { },
> @@ -1189,12 +1103,15 @@ print_imc(struct engines *engines, double t, int lines, int con_w, int con_h)
> };
> int ret;
>
> + if (!engines->num_imc)
> + return lines;
> +
> ret = asprintf((char **)&imc_group.display_name, "IMC %s/s",
> - engines->imc_reads_unit);
> + engines->imc_reads.units);
> assert(ret >= 0);
>
> ret = asprintf((char **)&imc_items[2].unit, "%s/s",
> - engines->imc_reads_unit);
> + engines->imc_reads.units);
> assert(ret >= 0);
>
> print_groups(groups);
> @@ -1205,11 +1122,11 @@ print_imc(struct engines *engines, double t, int lines, int con_w, int con_h)
> if (output_mode == INTERACTIVE) {
> if (lines++ < con_h)
> printf(" IMC reads: %s %s/s\n",
> - imc_items[0].buf, engines->imc_reads_unit);
> + imc_items[0].buf, engines->imc_reads.units);
>
> if (lines++ < con_h)
> printf(" IMC writes: %s %s/s\n",
> - imc_items[1].buf, engines->imc_writes_unit);
> + imc_items[1].buf, engines->imc_writes.units);
>
> if (lines++ < con_h)
> printf("\n");
> @@ -1509,9 +1426,7 @@ int main(int argc, char **argv)
> t, lines, con_w, con_h,
> &consumed);
>
> - if (engines->imc_fd)
> - lines = print_imc(engines, t, lines, con_w,
> - con_h);
> + lines = print_imc(engines, t, lines, con_w, con_h);
>
> lines = print_engines_header(engines, t, lines, con_w,
> con_h);
>
More information about the Intel-gfx
mailing list