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 <ch...@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursu...@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);

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

Reply via email to