intel_gpu_top: User friendly device listing

From: Tvrtko Ursulin <[email protected]>

Adding a new device selection print type suitable for user-facing use cases like intel_gpu_top -L and potentially 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.

Quoting Tvrtko Ursulin (2020-10-14 11:48:53)

Quoted 29 lines

From: Tvrtko Ursulin <[email protected]>

Adding a new device selection print type suitable for user-facing use cases like intel_gpu_top -L and potentially 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

Quoted 2 lines

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

Could you present this as the typical commands and output? I think you mean the first as the output of intel_gpu_top -L. What exactly do we now feed to -d?

From: Tvrtko Ursulin <[email protected]>

Adding a new device selection print type suitable for user-facing use cases like intel_gpu_top -L and potentially 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(-)

lib/igt_device_scan.c +98 -11

index f4d43c733314..ce0ea61dda50 100644
@@ -695,18 +695,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)

@@ -728,6 +736,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);

@@ -752,14 +843,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);

@@ -781,6 +867,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 },
 };
 
 /**

lib/igt_device_scan.h +1 -0

index bd937d22752c..9e13ed9db406 100644
@@ -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"

tools/intel_gpu_top.c +1 -2

index 79a936ffbe1a..b984edc656c7 100644
@@ -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);
 		goto exit;
 	}
 

Signature · 1 line

2.25.1

Quoted 26 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Adding a new device selection print type suitable for user-facing use cases like intel_gpu_top -L and potentially 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:

Quoted 5 lines

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.

Looks nice and more intuitive.

1. Add -s switch to handle simple print (current default in lsgpu)

2. Change to user format as default.

--
Zbigniew

Quoted 193 lines

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(-)

lib/igt_device_scan.c +98 -11

index f4d43c733314..ce0ea61dda50 100644
@@ -695,18 +695,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)

@@ -728,6 +736,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);

@@ -752,14 +843,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);

@@ -781,6 +867,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 },
  };
 
  /**

lib/igt_device_scan.h +1 -0

index bd937d22752c..9e13ed9db406 100644
@@ -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"

tools/intel_gpu_top.c +1 -2

index 79a936ffbe1a..b984edc656c7 100644
@@ -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);
  		goto exit;
  	}
 

Quoted 3 lines

Signature · 1 line

2.25.1

Quoted 27 lines · Zbigniew Kempczyński

Quoted 26 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Adding a new device selection print type suitable for user-facing use cases like intel_gpu_top -L and potentially 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:

Quoted 7 lines

Quoted 5 lines

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.

Looks nice and more intuitive.

I wasn't sure about duplication of card/render name and again in filter string, but I have no better ideas at the moment.

Maybe one day some could replace the left column with some card names from some database but for now above is good enough.

Quoted 1 line

1. Add -s switch to handle simple print (current default in lsgpu)

I wanted to suggest -v (verbose) or -d (details) but both are taken in lsgpu. -s sounds like silent or in any case not obvious what "simple" means. Could use just long form --verbose or --details?

Quoted 2 lines

2. Change to user format as default.

Yep.

Regards,

Tvrtko

Quoted 181 lines

--
Zbigniew

Quoted 177 lines

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(-)

lib/igt_device_scan.c +98 -11

index f4d43c733314..ce0ea61dda50 100644
@@ -695,18 +695,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)

@@ -728,6 +736,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);

@@ -752,14 +843,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);

@@ -781,6 +867,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 },
  };
  /**

lib/igt_device_scan.h +1 -0

index bd937d22752c..9e13ed9db406 100644
@@ -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"

tools/intel_gpu_top.c +0 -1

index 79a936ffbe1a..b984edc656c7 100644
@@ -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);

Quoted 8 lines

Quoted 8 lines

tools/intel_gpu_top.c +1 -1

  	if (list_device) {
-		igt_devices_print(printtype);
+		igt_devices_print(IGT_PRINT_USER);
  		goto exit;
  	}

Signature · 1 line

2.25.1

Quoted 45 lines · Tvrtko Ursulin

Quoted 27 lines · Zbigniew Kempczyński

Quoted 26 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Adding a new device selection print type suitable for user-facing use cases like intel_gpu_top -L and potentially 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:

Quoted 7 lines

Quoted 5 lines

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.

Looks nice and more intuitive.

I wasn't sure about duplication of card/render name and again in filter string, but I have no better ideas at the moment.

Maybe one day some could replace the left column with some card names from some database but for now above is good enough.

Quoted 1 line

1. Add -s switch to handle simple print (current default in lsgpu)

Quoted 3 lines

I wanted to suggest -v (verbose) or -d (details) but both are taken in lsgpu. -s sounds like silent or in any case not obvious what "simple" means. Could use just long form --verbose or --details?

-v is vendor list we support in pci vendor ids, not verbose. If you have resistence to use -s just use -i (simple). Sometimes I need to go to device directory in /sys so I use lsgpu simple output with directory name.

--
Zbigniew

Quoted 190 lines

Quoted 2 lines

2. Change to user format as default.

Yep.

Regards,

Tvrtko

Quoted 181 lines

--
Zbigniew

Quoted 177 lines

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(-)

lib/igt_device_scan.c +98 -11

index f4d43c733314..ce0ea61dda50 100644
@@ -695,18 +695,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)

@@ -728,6 +736,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);

@@ -752,14 +843,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);

@@ -781,6 +867,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 },
   };
   /**

lib/igt_device_scan.h +1 -0

index bd937d22752c..9e13ed9db406 100644
@@ -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"

tools/intel_gpu_top.c +0 -1

index 79a936ffbe1a..b984edc656c7 100644
@@ -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);

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

tools/intel_gpu_top.c +1 -1

   	if (list_device) {
-		igt_devices_print(printtype);
+		igt_devices_print(IGT_PRINT_USER);
   		goto exit;
   	}

Signature · 1 line

2.25.1

Quoted 50 lines · Zbigniew Kempczyński

Quoted 48 lines · Tvrtko Ursulin

Quoted 27 lines · Zbigniew Kempczyński

Quoted 26 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Adding a new device selection print type suitable for user-facing use cases like intel_gpu_top -L and potentially 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:

Quoted 7 lines

Quoted 5 lines

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.

Looks nice and more intuitive.

I wasn't sure about duplication of card/render name and again in filter string, but I have no better ideas at the moment.

Maybe one day some could replace the left column with some card names from some database but for now above is good enough.

Quoted 1 line

1. Add -s switch to handle simple print (current default in lsgpu)

I wanted to suggest -v (verbose) or -d (details) but both are taken in lsgpu. -s sounds like silent or in any case not obvious what "simple" means. Could use just long form --verbose or --details?

Quoted 3 lines

-v is vendor list we support in pci vendor ids, not verbose. If you have resistence to use -s just use -i (simple). Sometimes I need to go to device directory in /sys so I use lsgpu simple output with directory name.

I don't think "simple" as a term should be user facing because it is more complex than the mode I am proposing. So cannot really be called simple in my mind.

To be clear I am working under the assumption intel_gpu_top and lsgpu will be getting more and more users and as such emphasis on making them less of a developer tools should be increased. (Former had an user bug submitted in Fedora, which was one of the triggers.)

I do agree sysfs path is an interesting addition, or alternate mode. Maybe user printout should have selectable modes of drm or sysfs?

$ lsgpu --drm --filter (filter adds the drm: prefix, can be default)

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

$ lsgpu --sysfs

card0 8086:193B /sys/devices/pci0000:00/0000:00:02.0/drm/card0
└─renderD128 /sys/devices/pci0000:00/0000:00:02.0/drm/renderD128

$ lsgpu --device

card0 8086:193B /sys/devices/pci0000:00/0000:00:02.0
└─renderD128

$ lsgpu --verbose (or --details)

... the current "simple" output ...

None of my complications from above do not fit directly into the current print type enum. I'll play with it time permitting and send another RFC.

Regards,

Tvrtko

Quoted 195 lines

--
Zbigniew

Quoted 190 lines

Quoted 2 lines

2. Change to user format as default.

Yep.

Regards,

Tvrtko

Quoted 181 lines

--
Zbigniew

Quoted 177 lines

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(-)

lib/igt_device_scan.c +98 -11

index f4d43c733314..ce0ea61dda50 100644
@@ -695,18 +695,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)

@@ -728,6 +736,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);

@@ -752,14 +843,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);

@@ -781,6 +867,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 },
   };
   /**

lib/igt_device_scan.h +1 -0

index bd937d22752c..9e13ed9db406 100644
@@ -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"

tools/intel_gpu_top.c +0 -1

index 79a936ffbe1a..b984edc656c7 100644
@@ -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);

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

tools/intel_gpu_top.c +1 -1

   	if (list_device) {
-		igt_devices_print(printtype);
+		igt_devices_print(IGT_PRINT_USER);
   		goto exit;
   	}

Signature · 1 line

2.25.1