On 10/11/2020 11:13, Zbigniew Kempczyński wrote:
On Mon, Nov 09, 2020 at 10:48:09AM +0000, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <[email protected]>

Adding a new device selection print type suitable for user-facing
use cases like intel_gpu_top -L and later lsgpu.

Instead of:

sys:/sys/devices/pci0000:00/0000:00:02.0/drm/card0
     subsystem       : drm
     drm card        : /dev/dri/card0
     parent          : sys:/sys/devices/pci0000:00/0000:00:02.0

sys:/sys/devices/pci0000:00/0000:00:02.0/drm/renderD128
     subsystem       : drm
     drm render      : /dev/dri/renderD128
     parent          : sys:/sys/devices/pci0000:00/0000:00:02.0

sys:/sys/devices/pci0000:00/0000:00:02.0
     subsystem       : pci
     drm card        : /dev/dri/card0
     drm render      : /dev/dri/renderD128
     vendor          : 8086
     device          : 193B

New format looks like:

card0                   8086:193B    drm:/dev/dri/card0
└─renderD128                         drm:/dev/dri/renderD128

Advantages are more compact, more readable, one entry per GPU, shorter
string to copy and paste to intel_gpu_top -d, or respective usage.

Signed-off-by: Tvrtko Ursulin <[email protected]>
Cc: Petri Latvala <[email protected]>
Cc: Zbigniew Kempczyński <[email protected]>
---
  lib/igt_device_scan.c | 109 +++++++++++++++++++++++++++++++++++++-----
  lib/igt_device_scan.h |   1 +
  tools/intel_gpu_top.c |   3 +-
  3 files changed, 100 insertions(+), 13 deletions(-)

diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
index c581a31ae55e..e66ccdc25aeb 100644
--- a/lib/igt_device_scan.c
+++ b/lib/igt_device_scan.c
@@ -735,18 +735,26 @@ static inline void _pr_simple2(const char *k, const char 
*v1, const char *v2)
        printf("    %-16s: %s:%s\n", k, v1, v2);
  }
-static void igt_devs_print_simple(struct igt_list_head *view)
+static bool __check_empty(struct igt_list_head *view)
  {
-       struct igt_device *dev;
-
        if (!view)
-               return;
+               return true;
if (igt_list_empty(view)) {
                printf("No GPU devices found\n");
-               return;
+               return true;
        }
+ return false;
+}
+
+static void igt_devs_print_simple(struct igt_list_head *view)
+{
+       struct igt_device *dev;
+
+       if (__check_empty(view))
+               return;
+
        igt_list_for_each_entry(dev, view, link) {
                printf("sys:%s\n", dev->syspath);
                if (dev->subsystem)
@@ -768,6 +776,89 @@ static void igt_devs_print_simple(struct igt_list_head 
*view)
        }
  }
+static struct igt_device *
+__find_pci(struct igt_list_head *view, const char *drm)
+{
+       struct igt_device *dev;
+
+       igt_list_for_each_entry(dev, view, link) {
+               if (!is_pci_subsystem(dev) || !dev->drm_card)
+                       continue;
+
+               if (!strcmp(dev->drm_card, drm))
+                       return dev;
+       }
+
+       return NULL;
+}
+
+static void igt_devs_print_user(struct igt_list_head *view)
+{
+       struct igt_device *dev;
+
+       if (__check_empty(view))
+               return;
+
+       igt_list_for_each_entry(dev, view, link) {
+               unsigned int i, num_children;
+               struct igt_device *pci_dev;
+               struct igt_device *dev2;
+               char filter[64];
+               char *drm_name;
+               int ret;
+
+               if (!is_drm_subsystem(dev))
+                       continue;
+               if (!dev->drm_card || dev->drm_render)
+                       continue;
+
+               drm_name = rindex(dev->drm_card, '/');
+               if (!drm_name || !*++drm_name)
+                       continue;
+
+               ret = snprintf(filter, sizeof(filter), "drm:%s", dev->drm_card);
+               igt_assert(ret < sizeof(filter));
+
+               pci_dev = __find_pci(view, dev->drm_card);
+               if (pci_dev)
+                       printf("%-24s%4s:%4s    %s\n",
+                              drm_name, pci_dev->vendor, pci_dev->device,
+                              filter);
+               else
+                       printf("%-24s             %s\n", drm_name, filter);
+
+               num_children = 0;
+               igt_list_for_each_entry(dev2, view, link) {
+                       if (!is_drm_subsystem(dev2) || !dev2->drm_render)
+                               continue;
+                       if (strcmp(dev2->parent->syspath, dev->parent->syspath))
+                               continue;
+
+                       num_children++;
+               }
+
+               i = 0;
+               igt_list_for_each_entry(dev2, view, link) {
+                       if (!is_drm_subsystem(dev2) || !dev2->drm_render)
+                               continue;
+                       if (strcmp(dev2->parent->syspath, dev->parent->syspath))
+                               continue;
+
+                       drm_name = rindex(dev2->drm_render, '/');
+                       if (!drm_name || !*++drm_name)
+                               continue;
+
+                       ret = snprintf(filter, sizeof(filter), "drm:%s",
+                                      dev2->drm_render);
+                       igt_assert(ret < sizeof(filter));
+
+                       printf("%s%-22s             %s\n",
+                              (++i == num_children) ? "└─" : "├─",
+                              drm_name, filter);
+               }
+       }
+}
+
  static inline void _print_key_value(const char* k, const char *v)
  {
        printf("%-32s: %s\n", k, v);
@@ -792,14 +883,9 @@ static void igt_devs_print_detail(struct igt_list_head 
*view)
  {
        struct igt_device *dev;
- if (!view)
+       if (__check_empty(view))
                return;
- if (igt_list_empty(view)) {
-               printf("No GPU devices found\n");
-               return;
-       }
-
        igt_list_for_each_entry(dev, view, link) {
                printf("========== %s:%s ==========\n",
                       dev->subsystem, dev->syspath);
@@ -821,6 +907,7 @@ static struct print_func {
  } print_functions[] = {
        [IGT_PRINT_SIMPLE] = { .prn = igt_devs_print_simple },
        [IGT_PRINT_DETAIL] = { .prn = igt_devs_print_detail },
+       [IGT_PRINT_USER] = { .prn = igt_devs_print_user },
  };
/**
diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
index 99daee0c52d6..9822c22cb69c 100644
--- a/lib/igt_device_scan.h
+++ b/lib/igt_device_scan.h
@@ -37,6 +37,7 @@
  enum igt_devices_print_type {
        IGT_PRINT_SIMPLE,
        IGT_PRINT_DETAIL,
+       IGT_PRINT_USER, /* End user friendly. */
  };
#define INTEGRATED_I915_GPU_PCI_ID "0000:00:02.0"
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 298defa4e6ed..5230472d2af4 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -1313,7 +1313,6 @@ int main(int argc, char **argv)
        unsigned int i;
        int ret = 0, ch;
        bool list_device = false;
-       enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE;
        char *pmu_device, *opt_device = NULL;
        struct igt_device_card card;
@@ -1388,7 +1387,7 @@ int main(int argc, char **argv)
        igt_devices_scan(false);
if (list_device) {
-               igt_devices_print(printtype);
+               igt_devices_print(IGT_PRINT_USER);

I would add at least possibility to use simple view to suggest
how to use pci/sys filter. With USER print format we see only

You mean the blurb printed out by igt_device_print_filter_types (currently printed as part of help text) or something else?

drm paths. But I won't insist for that, using drm selection
is ok for me.

Good point actually, intel_gpu_top should probably default to "--pci" listing type since it monitors GPUs with no notion of DRM master/render.

Reviewed-by: Zbigniew Kempczyński <[email protected]>

Thanks! Let me know if you agree with "--pci" by default for intel_gpu_top and if it is okay to keep the r-b?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to