User friendly lsgpu default output

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

lib/igt_device_scan.c +98 -11

index c581a31ae55e..e66ccdc25aeb 100644
@@ -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 },
 };
 
 /**

lib/igt_device_scan.h +1 -0

index 99daee0c52d6..9822c22cb69c 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 298defa4e6ed..5230472d2af4 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;
 

tools/intel_gpu_top.c +0 -0

 		goto exit;
 	}
 

Signature · 1 line

2.25.1

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

lib/igt_device_scan.c +98 -11

index c581a31ae55e..e66ccdc25aeb 100644
@@ -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 },
  };
 
  /**

lib/igt_device_scan.h +1 -0

index 99daee0c52d6..9822c22cb69c 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 298defa4e6ed..5230472d2af4 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);
 

tools/intel_gpu_top.c int main(int argc, char **argv) · line 1390

Quoted 3 lines

tools/intel_gpu_top.c +1 -1

  	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

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

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

Quoted 207 lines · Zbigniew Kempczyński

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

lib/igt_device_scan.c +98 -11

index c581a31ae55e..e66ccdc25aeb 100644
@@ -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 },
  };
  /**

lib/igt_device_scan.h +1 -0

index 99daee0c52d6..9822c22cb69c 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 298defa4e6ed..5230472d2af4 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;

Quoted 8 lines

tools/intel_gpu_top.c int main(int argc, char **argv) · line 1389

Quoted 5 lines

tools/intel_gpu_top.c +1 -1

@@ -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?

Quoted 3 lines

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.

Quoted 1 line

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

Quoted 20 lines

Quoted 19 lines

Quoted 12 lines

tools/intel_gpu_top.c +0 -1

index 298defa4e6ed..5230472d2af4 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;

tools/intel_gpu_top.c int main(int argc, char **argv) · line 1389

Quoted 4 lines

tools/intel_gpu_top.c +1 -1

@@ -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

Quoted 2 lines

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

Yes. We know lsgpu and intel_gpu_top selection is same and you're also printing filter syntax on 'intel_gpu_top -h'. But without using 'lsgpu -s' you won't guess how to specify sys:... filter.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 1 line

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?

If it is not problem you could provide --pci and --sysfs too to be compliant with lsgpu switches. But there are minor nits and you can keep r-b with the code as it is.

On 10/11/2020 11:46, Zbigniew Kempczyński wrote:

[snip]

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

Quoted 12 lines

tools/intel_gpu_top.c +0 -1

index 298defa4e6ed..5230472d2af4 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;

tools/intel_gpu_top.c int main(int argc, char **argv) · line 1389

Quoted 4 lines

tools/intel_gpu_top.c +1 -1

@@ -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

Quoted 6 lines

Quoted 2 lines

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

Yes. We know lsgpu and intel_gpu_top selection is same and you're also printing filter syntax on 'intel_gpu_top -h'. But without using 'lsgpu -s' you won't guess how to specify sys:... filter.

My thinking is that typical intel_gpu_top user wouldn't want/need to concern themselves with different filters but just copy&paste what was listed from intel_gpu_top -L. In which case..

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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?

If it is not problem you could provide --pci and --sysfs too to be compliant with lsgpu switches. But there are minor nits and you can keep r-b with the code as it is.

... I did not see the value of supporting --drm/--pci/--sysfs from intel_gpu_top.

So "--pci" by default should "JustWork" and make end users happy. As long as the help text is clear enough what needs to be copy&pasted, which may need improvement. If I am not missing something else.

Regards,

Tvrtko

From: Tvrtko Ursulin <[email protected]>

New default user frindly device listing mode which replaces:

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

With:

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

Advantages are more compact, more readable, one entry per GPU.

Legacy format can be chose using the -s / --print-simple command line switches.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Petri Latvala <[email protected]>

Cc Zbigniew Kempczyński <[email protected]>

---
 tools/lsgpu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

tools/lsgpu.c +8 -2

index 2541d1c24e66..3b234b73361a 100644
@@ -70,6 +70,7 @@
  */
 
 enum {
+	OPT_PRINT_SIMPLE   = 's',
 	OPT_PRINT_DETAIL   = 'p',
 	OPT_LIST_VENDORS   = 'v',
 	OPT_LIST_FILTERS   = 'l',

@@ -85,6 +86,7 @@ static char *igt_device;
 static const char *usage_str =
 	"usage: lsgpu [options]\n\n"
 	"Options:\n"
+	"  -s, --print-simple          Print simple (legacy) device details\n"
 	"  -p, --print-details         Print devices with details\n"
 	"  -v, --list-vendors          List recognized vendors\n"
 	"  -l, --list-filter-types     List registered device filters types\n"

@@ -151,6 +153,7 @@ static char *get_device_from_rc(void)
 int main(int argc, char *argv[])
 {
 	static struct option long_options[] = {
+		{"print-simple",      no_argument,       NULL, OPT_PRINT_SIMPLE},
 		{"print-detail",      no_argument,       NULL, OPT_PRINT_DETAIL},
 		{"list-vendors",      no_argument,       NULL, OPT_LIST_VENDORS},
 		{"list-filter-types", no_argument,       NULL, OPT_LIST_FILTERS},

@@ -160,12 +163,15 @@ int main(int argc, char *argv[])
 	};
 	int c, index = 0;
 	char *env_device = NULL, *opt_device = NULL, *rc_device = NULL;
-	enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE;
+	enum igt_devices_print_type printtype = IGT_PRINT_USER;
 
-	while ((c = getopt_long(argc, argv, "pvld:h",
+	while ((c = getopt_long(argc, argv, "spvld:h",
 				long_options, &index)) != -1) {
 		switch(c) {
 
+		case OPT_PRINT_SIMPLE:
+			printtype = IGT_PRINT_SIMPLE;
+			break;
 		case OPT_PRINT_DETAIL:
 			printtype = IGT_PRINT_DETAIL;
 			break;

Signature · 1 line

2.25.1

Quoted 87 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

New default user frindly device listing mode which replaces:

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

With:

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

Advantages are more compact, more readable, one entry per GPU.

Legacy format can be chose using the -s / --print-simple command line switches.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Petri Latvala <[email protected]>

Cc Zbigniew Kempczyński <[email protected]>

---
  tools/lsgpu.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

tools/lsgpu.c +8 -2

index 2541d1c24e66..3b234b73361a 100644
@@ -70,6 +70,7 @@
   */
 
  enum {
+	OPT_PRINT_SIMPLE   = 's',
  	OPT_PRINT_DETAIL   = 'p',
  	OPT_LIST_VENDORS   = 'v',
  	OPT_LIST_FILTERS   = 'l',

@@ -85,6 +86,7 @@ static char *igt_device;
  static const char *usage_str =
  	"usage: lsgpu [options]\n\n"
  	"Options:\n"
+	"  -s, --print-simple          Print simple (legacy) device details\n"
  	"  -p, --print-details         Print devices with details\n"
  	"  -v, --list-vendors          List recognized vendors\n"
  	"  -l, --list-filter-types     List registered device filters types\n"

@@ -151,6 +153,7 @@ static char *get_device_from_rc(void)
  int main(int argc, char *argv[])
  {
  	static struct option long_options[] = {
+		{"print-simple",      no_argument,       NULL, OPT_PRINT_SIMPLE},
  		{"print-detail",      no_argument,       NULL, OPT_PRINT_DETAIL},
  		{"list-vendors",      no_argument,       NULL, OPT_LIST_VENDORS},
  		{"list-filter-types", no_argument,       NULL, OPT_LIST_FILTERS},

@@ -160,12 +163,15 @@ int main(int argc, char *argv[])
  	};
  	int c, index = 0;
  	char *env_device = NULL, *opt_device = NULL, *rc_device = NULL;
-	enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE;
+	enum igt_devices_print_type printtype = IGT_PRINT_USER;
 
-	while ((c = getopt_long(argc, argv, "pvld:h",
+	while ((c = getopt_long(argc, argv, "spvld:h",
  				long_options, &index)) != -1) {
  		switch(c) {
 
+		case OPT_PRINT_SIMPLE:
+			printtype = IGT_PRINT_SIMPLE;
+			break;
  		case OPT_PRINT_DETAIL:
  			printtype = IGT_PRINT_DETAIL;
  			break;

Signature · 1 line

2.25.1

Looks ok:

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

Quoted 88 lines · Zbigniew Kempczyński

Quoted 84 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

New default user frindly device listing mode which replaces:

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

With:

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

Advantages are more compact, more readable, one entry per GPU.

Legacy format can be chose using the -s / --print-simple command line switches.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Petri Latvala <[email protected]>

Cc Zbigniew Kempczyński <[email protected]>

---
  tools/lsgpu.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

tools/lsgpu.c +8 -2

index 2541d1c24e66..3b234b73361a 100644
@@ -70,6 +70,7 @@
   */
  enum {
+	OPT_PRINT_SIMPLE   = 's',
  	OPT_PRINT_DETAIL   = 'p',
  	OPT_LIST_VENDORS   = 'v',
  	OPT_LIST_FILTERS   = 'l',

@@ -85,6 +86,7 @@ static char *igt_device;
  static const char *usage_str =
  	"usage: lsgpu [options]\n\n"
  	"Options:\n"
+	"  -s, --print-simple          Print simple (legacy) device details\n"
  	"  -p, --print-details         Print devices with details\n"
  	"  -v, --list-vendors          List recognized vendors\n"
  	"  -l, --list-filter-types     List registered device filters types\n"

@@ -151,6 +153,7 @@ static char *get_device_from_rc(void)
  int main(int argc, char *argv[])
  {
  	static struct option long_options[] = {
+		{"print-simple",      no_argument,       NULL, OPT_PRINT_SIMPLE},
  		{"print-detail",      no_argument,       NULL, OPT_PRINT_DETAIL},
  		{"list-vendors",      no_argument,       NULL, OPT_LIST_VENDORS},
  		{"list-filter-types", no_argument,       NULL, OPT_LIST_FILTERS},

@@ -160,12 +163,15 @@ int main(int argc, char *argv[])
  	};
  	int c, index = 0;
  	char *env_device = NULL, *opt_device = NULL, *rc_device = NULL;
-	enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE;
+	enum igt_devices_print_type printtype = IGT_PRINT_USER;
-	while ((c = getopt_long(argc, argv, "pvld:h",
+	while ((c = getopt_long(argc, argv, "spvld:h",
  				long_options, &index)) != -1) {
  		switch(c) {
+		case OPT_PRINT_SIMPLE:
+			printtype = IGT_PRINT_SIMPLE;
+			break;
  		case OPT_PRINT_DETAIL:
  			printtype = IGT_PRINT_DETAIL;
  			break;

Signature · 1 line

2.25.1

Looks ok:

Quoted 1 line

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

Thanks.

Any concerns about potential existence of tools which parse lsgpu output and may depend on the default format?

Regards,

Tvrtko

From: Tvrtko Ursulin <[email protected]>

In the previous patch we switched the lsgpu output to a short and user friendly format but some users will need a shorthand for getting other types of device selection filters than the defaut drm.

Add some command line switches to enable this:

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

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

$ lsgpu --pci
card0                   8086:193B    pci:/sys/devices/pci0000:00/0000:00:02.0
└─renderD128

Signed-off-by Tvrtko Ursulin <[email protected]>

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

Cc Petri Latvala <[email protected]>

---
 lib/igt_device_scan.c | 69 ++++++++++++++++++++++++++++++++-----------
 lib/igt_device_scan.h | 15 ++++++++--
 tools/intel_gpu_top.c |  6 +++-
 tools/lsgpu.c         | 32 +++++++++++++++-----
 4 files changed, 95 insertions(+), 27 deletions(-)

lib/igt_device_scan.c +33 -2

index e66ccdc25aeb..54b565b68524 100644
@@ -748,7 +748,9 @@ static bool __check_empty(struct igt_list_head *view)
 	return false;
 }
 
-static void igt_devs_print_simple(struct igt_list_head *view)
+static void
+igt_devs_print_simple(struct igt_list_head *view,
+		      const struct igt_devices_print_format *fmt)
 {
 	struct igt_device *dev;
 

@@ -792,7 +794,36 @@ __find_pci(struct igt_list_head *view, const char *drm)
 	return NULL;
 }
 
-static void igt_devs_print_user(struct igt_list_head *view)
+static void __print_filter(char *buf, int len,
+			   const struct igt_devices_print_format *fmt,
+			   struct igt_device *dev,
+			   bool render)
+{
+	int ret;
+
+	switch (fmt->option) {
+	case IGT_PRINT_DRM:
+		ret = snprintf(buf, len, "drm:%s",
+			       render ? dev->drm_render : dev->drm_card);
+		igt_assert(ret < len);
+		break;
+	case IGT_PRINT_SYSFS:
+		ret = snprintf(buf, len, "sys:%s", dev->syspath);
+		igt_assert(ret < len);
+		break;
+	case IGT_PRINT_PCI:
+		if (!render) {
+			ret = snprintf(buf, len, "pci:%s",
+				       dev->parent->syspath);
+			igt_assert(ret < len);
+		}
+		break;
+	};
+}
+
+static void
+igt_devs_print_user(struct igt_list_head *view,
+		    const struct igt_devices_print_format *fmt)
 {
 	struct igt_device *dev;
 

lib/igt_device_scan.c +19 -15

 		char *drm_name;
-		int ret;
 
 		if (!is_drm_subsystem(dev))
 			continue;

@@ -816,8 +846,7 @@ static void igt_devs_print_user(struct igt_list_head *view)
 		if (!drm_name || !*++drm_name)
 			continue;
 
-		ret = snprintf(filter, sizeof(filter), "drm:%s", dev->drm_card);
-		igt_assert(ret < sizeof(filter));
+		__print_filter(filter, sizeof(filter), fmt, dev, false);
 
 		pci_dev = __find_pci(view, dev->drm_card);
 		if (pci_dev)

@@ -848,13 +877,15 @@ static void igt_devs_print_user(struct igt_list_head *view)
 			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);
+			printf("%s%-22s",
+				(++i == num_children) ? "└─" : "├─", drm_name);
+			if (fmt->option != IGT_PRINT_PCI) {
+				__print_filter(filter, sizeof(filter), fmt,
+					       dev2, true);
+				printf("             %s\n", filter);
+			} else {
+				printf("\n");
+			}
 		}
 	}
 }

@@ -879,7 +910,10 @@ static void print_ht(GHashTable *ht)
 	g_list_free(keys);
 }
 
-static void igt_devs_print_detail(struct igt_list_head *view)
+static void
+igt_devs_print_detail(struct igt_list_head *view,
+		      const struct igt_devices_print_format *fmt)
+
 {
 	struct igt_device *dev;
 

@@ -903,7 +937,8 @@ static void igt_devs_print_detail(struct igt_list_head *view)
 }
 
 static struct print_func {
-	void (*prn)(struct igt_list_head *view);
+	void (*prn)(struct igt_list_head *view,
+		    const struct igt_devices_print_format *);
 } print_functions[] = {
 	[IGT_PRINT_SIMPLE] = { .prn = igt_devs_print_simple },
 	[IGT_PRINT_DETAIL] = { .prn = igt_devs_print_detail },

@@ -912,15 +947,15 @@ static struct print_func {
 
 /**
  * igt_devices_print
- * @printtype: IGT_PRINT_SIMPLE or IGT_PRINT_DETAIL
+ * @fmt: Print format as specified by struct igt_devices_print_format
  *
  * Function can be used by external tool to print device array in simple
  * or detailed form. This function is added here to avoid exposing
  * internal implementation data structures.
  */
-void igt_devices_print(enum igt_devices_print_type printtype)
+void igt_devices_print(const struct igt_devices_print_format *fmt)
 {
-	print_functions[printtype].prn(&igt_devs.filtered);
+	print_functions[fmt->type].prn(&igt_devs.filtered, fmt);
 }
 
 /**

lib/igt_device_scan.h +13 -2

index 9822c22cb69c..bb4be72345fb 100644
@@ -35,11 +35,22 @@
 #include <unistd.h>
 
 enum igt_devices_print_type {
-	IGT_PRINT_SIMPLE,
+	IGT_PRINT_SIMPLE = 0,
 	IGT_PRINT_DETAIL,
 	IGT_PRINT_USER, /* End user friendly. */
 };
 
+enum igt_devices_print_option {
+	IGT_PRINT_DRM = 0,
+	IGT_PRINT_SYSFS,
+	IGT_PRINT_PCI,
+};
+
+struct igt_devices_print_format {
+	enum igt_devices_print_type   type;
+	enum igt_devices_print_option option;
+};
+
 #define INTEGRATED_I915_GPU_PCI_ID "0000:00:02.0"
 #define PCI_SLOT_NAME_SIZE 12
 struct igt_device_card {

@@ -51,7 +62,7 @@ struct igt_device_card {
 
 void igt_devices_scan(bool force);
 
-void igt_devices_print(enum igt_devices_print_type printtype);
+void igt_devices_print(const struct igt_devices_print_format *fmt);
 void igt_devices_print_vendors(void);
 void igt_device_print_filter_types(void);
 

tools/intel_gpu_top.c +5 -1

index 5230472d2af4..07f88d555dc8 100644
@@ -1387,7 +1387,11 @@ int main(int argc, char **argv)
 	igt_devices_scan(false);
 
 	if (list_device) {
-		igt_devices_print(IGT_PRINT_USER);
+		struct igt_devices_print_format fmt = {
+			.type = IGT_PRINT_USER
+		};
+
+		igt_devices_print(&fmt);
 		goto exit;
 	}
 

tools/lsgpu.c +25 -7

index 3b234b73361a..169ab0c29e50 100644
@@ -91,7 +91,11 @@ static const char *usage_str =
 	"  -v, --list-vendors          List recognized vendors\n"
 	"  -l, --list-filter-types     List registered device filters types\n"
 	"  -d, --device filter         Device filter, can be given multiple times\n"
-	"  -h, --help                  Show this help message and exit\n";
+	"  -h, --help                  Show this help message and exit\n"
+	"\nOptions valid for default print out mode only:\n"
+	"      --drm                   Show DRM filters (default) for each device\n"
+	"      --sysfs                 Show sysfs filters for each device\n"
+	"      --pci                   Show PCI filters for each device\n";
 
 static void test_device_open(struct igt_device_card *card)
 {

@@ -153,6 +157,9 @@ static char *get_device_from_rc(void)
 int main(int argc, char *argv[])
 {
 	static struct option long_options[] = {
+		{"drm",               no_argument,       NULL, 0},
+		{"sysfs",             no_argument,       NULL, 1},
+		{"pci",               no_argument,       NULL, 2},
 		{"print-simple",      no_argument,       NULL, OPT_PRINT_SIMPLE},
 		{"print-detail",      no_argument,       NULL, OPT_PRINT_DETAIL},
 		{"list-vendors",      no_argument,       NULL, OPT_LIST_VENDORS},

@@ -163,17 +170,19 @@ int main(int argc, char *argv[])
 	};
 	int c, index = 0;
 	char *env_device = NULL, *opt_device = NULL, *rc_device = NULL;
-	enum igt_devices_print_type printtype = IGT_PRINT_USER;
+	struct igt_devices_print_format fmt = {
+			.type = IGT_PRINT_USER,
+	};
 
 	while ((c = getopt_long(argc, argv, "spvld:h",
 				long_options, &index)) != -1) {
 		switch(c) {
 
 		case OPT_PRINT_SIMPLE:
-			printtype = IGT_PRINT_SIMPLE;
+			fmt.type = IGT_PRINT_SIMPLE;
 			break;
 		case OPT_PRINT_DETAIL:
-			printtype = IGT_PRINT_DETAIL;
+			fmt.type = IGT_PRINT_DETAIL;
 			break;
 		case OPT_LIST_VENDORS:
 			g_show_vendors = true;

@@ -187,6 +196,15 @@ int main(int argc, char *argv[])
 		case OPT_HELP:
 			g_help = true;
 			break;
+		case 0:
+			fmt.option = IGT_PRINT_DRM;
+			break;
+		case 1:
+			fmt.option = IGT_PRINT_SYSFS;
+			break;
+		case 2:
+			fmt.option = IGT_PRINT_PCI;
+			break;
 		}
 	}
 

@@ -239,14 +257,14 @@ int main(int argc, char *argv[])
 		printf("Device detail:\n");
 		print_card(&card);
 		test_device_open(&card);
-		if (printtype == IGT_PRINT_DETAIL) {
+		if (fmt.type == IGT_PRINT_DETAIL) {
 			printf("\n");
-			igt_devices_print(printtype);
+			igt_devices_print(&fmt);
 		}
 		printf("-------------------------------------------\n");
 
 	} else {
-		igt_devices_print(printtype);
+		igt_devices_print(&fmt);
 	}
 
 	free(rc_device);

Signature · 1 line

2.25.1