Intel-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing
@ 2020-11-13 16:27 Tvrtko Ursulin
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 1/5] intel_gpu_top: User friendly " Tvrtko Ursulin
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2020-11-13 16:27 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Please see individual path commit messages for details, but essentially I
implemented this:

$ sudo tools/lsgpu
card1                   8086:4905    drm:/dev/dri/card1
└─renderD129                         drm:/dev/dri/renderD129
card0                   8086:3E98    drm:/dev/dri/card0
└─renderD128                         drm:/dev/dri/renderD128

$ sudo tools/lsgpu --pci
card1                   8086:4905    pci:vendor=8086,device=4905,card=0
└─renderD129
card0                   8086:3E98    pci:vendor=8086,device=3E98,card=0
└─renderD128

$ sudo tools/lsgpu --sysfs
card1                   8086:4905    sys:/sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:01.0/0000:03:00.0
└─renderD129                         sys:/sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:01.0/0000:03:00.0/drm/renderD129
card0                   8086:3E98    sys:/sys/devices/pci0000:00/0000:00:02.0
└─renderD128                         sys:/sys/devices/pci0000:00/0000:00:02.0/drm/renderD128

I need to find a machine with two GPUs of the same device id to finish testing
this hence still RFC.

Tvrtko Ursulin (5):
  intel_gpu_top: User friendly device listing
  lsgpu: User friendly device listing
  lib/igt_device_scan: Remember PCI card index after scanning
  lsgpu: Add filter type print-out selection
  intel_gpu_top: Default GPU list to PCI mode

 lib/igt_device_scan.c | 205 +++++++++++++++++++++++++++++++++++++-----
 lib/igt_device_scan.h |  16 +++-
 tools/intel_gpu_top.c |   8 +-
 tools/lsgpu.c         |  38 ++++++--
 4 files changed, 233 insertions(+), 34 deletions(-)

-- 
2.25.1

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Intel-gfx] [RFC i-g-t 1/5] intel_gpu_top: User friendly device listing
  2020-11-13 16:27 [Intel-gfx] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin
@ 2020-11-13 16:27 ` Tvrtko Ursulin
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 2/5] lsgpu: " Tvrtko Ursulin
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2020-11-13 16:27 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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.

v2:
 * Increase stack filter size. (Zbigniew)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Petri Latvala <petri.latvala@intel.com>
Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
---
 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..c0cd6757fc27 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[256];
+		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);
 		goto exit;
 	}
 
-- 
2.25.1

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Intel-gfx] [RFC i-g-t 2/5] lsgpu: User friendly device listing
  2020-11-13 16:27 [Intel-gfx] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 1/5] intel_gpu_top: User friendly " Tvrtko Ursulin
@ 2020-11-13 16:27 ` Tvrtko Ursulin
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning Tvrtko Ursulin
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2020-11-13 16:27 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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 <tvrtko.ursulin@intel.com>
Cc: Petri Latvala <petri.latvala@intel.com>
Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
---
 tools/lsgpu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/lsgpu.c b/tools/lsgpu.c
index 2541d1c24e66..3b234b73361a 100644
--- a/tools/lsgpu.c
+++ b/tools/lsgpu.c
@@ -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;
-- 
2.25.1

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Intel-gfx] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning
  2020-11-13 16:27 [Intel-gfx] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 1/5] intel_gpu_top: User friendly " Tvrtko Ursulin
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 2/5] lsgpu: " Tvrtko Ursulin
@ 2020-11-13 16:27 ` Tvrtko Ursulin
  2020-11-16 14:03   ` Zbigniew Kempczyński
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 4/5] lsgpu: Add filter type print-out selection Tvrtko Ursulin
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2020-11-13 16:27 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

After devices are sorted post scanning, assing a card index to each so it
can be easily accessed if PCI filter for a card needs to be printed out.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Petri Latvala <petri.latvala@intel.com>
Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
---
 lib/igt_device_scan.c | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
index c0cd6757fc27..72653403f705 100644
--- a/lib/igt_device_scan.c
+++ b/lib/igt_device_scan.c
@@ -166,6 +166,7 @@ struct igt_device {
 	char *vendor;
 	char *device;
 	char *pci_slot_name;
+	int pci_index;
 
 	struct igt_list_head link;
 };
@@ -601,6 +602,33 @@ static void sort_all_devices(void)
 	free(devs);
 }
 
+static void index_pci_devices(void)
+{
+	struct igt_device *dev;
+
+	igt_list_for_each_entry(dev, &igt_devs.all, link) {
+		struct igt_device *dev2;
+		int index = 0;
+
+		if (!is_pci_subsystem(dev))
+			continue;
+
+		igt_list_for_each_entry(dev2, &igt_devs.all, link) {
+			if (!is_pci_subsystem(dev2))
+				continue;
+
+			if (dev2 == dev)
+				break;
+
+			if (!strcasecmp(dev->vendor, dev2->vendor) &&
+			    !strcasecmp(dev->device, dev2->device))
+				index++;
+		}
+
+		dev->pci_index = index;
+	}
+}
+
 /* Core scanning function.
  *
  * All scanned devices are kept inside igt_devs.all pointer array.
@@ -657,6 +685,7 @@ static void scan_drm_devices(void)
 	udev_unref(udev);
 
 	sort_all_devices();
+	index_pci_devices();
 
 	igt_list_for_each_entry(dev, &igt_devs.all, link) {
 		struct igt_device *dev_dup = duplicate_device(dev);
@@ -1105,13 +1134,13 @@ static struct igt_list_head *filter_pci(const struct filter_class *fcls,
 		if (filter->data.device && strcasecmp(filter->data.device, dev->device))
 			continue;
 
-		/* We get n-th card */
-		if (!card) {
-			struct igt_device *dup = duplicate_device(dev);
-			igt_list_add_tail(&dup->link, &igt_devs.filtered);
-			break;
-		}
-		card--;
+		/* Skip if 'card' doesn't match */
+		if (card != dev->pci_index)
+			continue;
+
+		dev = duplicate_device(dev);
+		igt_list_add_tail(&dev->link, &igt_devs.filtered);
+		break;
 	}
 
 	DBG("Filter pci filtered size: %d\n", igt_list_length(&igt_devs.filtered));
-- 
2.25.1

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Intel-gfx] [RFC i-g-t 4/5] lsgpu: Add filter type print-out selection
  2020-11-13 16:27 [Intel-gfx] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin
                   ` (2 preceding siblings ...)
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning Tvrtko Ursulin
@ 2020-11-13 16:27 ` Tvrtko Ursulin
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 5/5] intel_gpu_top: Default GPU list to PCI mode Tvrtko Ursulin
  2020-11-16 13:08 ` [Intel-gfx] [igt-dev] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin
  5 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2020-11-13 16:27 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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:vendor=8086,device=193B,card=0
└─renderD128

v2
 * Fix pci filter format.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Suggested-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Petri Latvala <petri.latvala@intel.com>
---
 lib/igt_device_scan.c | 83 ++++++++++++++++++++++++++++++++-----------
 lib/igt_device_scan.h | 15 ++++++--
 tools/intel_gpu_top.c |  6 +++-
 tools/lsgpu.c         | 32 +++++++++++++----
 4 files changed, 106 insertions(+), 30 deletions(-)

diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
index 72653403f705..028c8bfbbb44 100644
--- a/lib/igt_device_scan.c
+++ b/lib/igt_device_scan.c
@@ -777,7 +777,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;
 
@@ -821,7 +823,38 @@ __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:vendor=%s,device=%s,card=%d",
+				       dev->vendor, dev->device,
+				       dev->pci_index);
+			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;
 
@@ -834,7 +867,6 @@ static void igt_devs_print_user(struct igt_list_head *view)
 		struct igt_device *dev2;
 		char filter[256];
 		char *drm_name;
-		int ret;
 
 		if (!is_drm_subsystem(dev))
 			continue;
@@ -845,16 +877,21 @@ 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));
-
 		pci_dev = __find_pci(view, dev->drm_card);
-		if (pci_dev)
+
+		if (fmt->option == IGT_PRINT_PCI && !pci_dev)
+			continue;
+
+		if (pci_dev) {
+			__print_filter(filter, sizeof(filter), fmt, pci_dev,
+				       false);
 			printf("%-24s%4s:%4s    %s\n",
 			       drm_name, pci_dev->vendor, pci_dev->device,
 			       filter);
-		else
+		} else {
+			__print_filter(filter, sizeof(filter), fmt, dev, false);
 			printf("%-24s             %s\n", drm_name, filter);
+		}
 
 		num_children = 0;
 		igt_list_for_each_entry(dev2, view, link) {
@@ -877,13 +914,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");
+			}
 		}
 	}
 }
@@ -908,7 +947,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;
 
@@ -932,7 +974,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 },
@@ -941,15 +984,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);
 }
 
 /**
diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
index 9822c22cb69c..bb4be72345fb 100644
--- a/lib/igt_device_scan.h
+++ b/lib/igt_device_scan.h
@@ -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);
 
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 5230472d2af4..07f88d555dc8 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -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;
 	}
 
diff --git a/tools/lsgpu.c b/tools/lsgpu.c
index 3b234b73361a..169ab0c29e50 100644
--- a/tools/lsgpu.c
+++ b/tools/lsgpu.c
@@ -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);
-- 
2.25.1

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Intel-gfx] [RFC i-g-t 5/5] intel_gpu_top: Default GPU list to PCI mode
  2020-11-13 16:27 [Intel-gfx] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin
                   ` (3 preceding siblings ...)
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 4/5] lsgpu: Add filter type print-out selection Tvrtko Ursulin
@ 2020-11-13 16:27 ` Tvrtko Ursulin
  2020-11-16 13:08 ` [Intel-gfx] [igt-dev] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin
  5 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2020-11-13 16:27 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

It is more obvious for the user to only shows filters for DRM master
nodes since those are the ones that intel_gpu_top monitors.

v2:
 * Filter prefix needs to be sys: when listing PCI devices.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tools/intel_gpu_top.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 07f88d555dc8..37b2141e784d 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -1388,7 +1388,8 @@ int main(int argc, char **argv)
 
 	if (list_device) {
 		struct igt_devices_print_format fmt = {
-			.type = IGT_PRINT_USER
+			.type = IGT_PRINT_USER,
+			.option = IGT_PRINT_PCI,
 		};
 
 		igt_devices_print(&fmt);
-- 
2.25.1

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [igt-dev] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing
  2020-11-13 16:27 [Intel-gfx] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin
                   ` (4 preceding siblings ...)
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 5/5] intel_gpu_top: Default GPU list to PCI mode Tvrtko Ursulin
@ 2020-11-16 13:08 ` Tvrtko Ursulin
  5 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2020-11-16 13:08 UTC (permalink / raw)
  To: igt-dev; +Cc: Intel-gfx


On 13/11/2020 16:27, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Please see individual path commit messages for details, but essentially I
> implemented this:
> 
> $ sudo tools/lsgpu
> card1                   8086:4905    drm:/dev/dri/card1
> └─renderD129                         drm:/dev/dri/renderD129
> card0                   8086:3E98    drm:/dev/dri/card0
> └─renderD128                         drm:/dev/dri/renderD128
> 
> $ sudo tools/lsgpu --pci
> card1                   8086:4905    pci:vendor=8086,device=4905,card=0
> └─renderD129
> card0                   8086:3E98    pci:vendor=8086,device=3E98,card=0
> └─renderD128
> 
> $ sudo tools/lsgpu --sysfs
> card1                   8086:4905    sys:/sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:01.0/0000:03:00.0
> └─renderD129                         sys:/sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:01.0/0000:03:00.0/drm/renderD129
> card0                   8086:3E98    sys:/sys/devices/pci0000:00/0000:00:02.0
> └─renderD128                         sys:/sys/devices/pci0000:00/0000:00:02.0/drm/renderD128
> 
> I need to find a machine with two GPUs of the same device id to finish testing
> this hence still RFC.

I mock tested the PCI card index generating code and it seems to work fine:

$ tools/lsgpu --pci
card0                   8086:193B    pci:vendor=8086,device=193B,card=0
├─renderD128
└─renderD128
card0x                  8086:193B    pci:vendor=8086,device=193B,card=1
├─renderD128
└─renderD128

Regards,

Tvrtko

> Tvrtko Ursulin (5):
>    intel_gpu_top: User friendly device listing
>    lsgpu: User friendly device listing
>    lib/igt_device_scan: Remember PCI card index after scanning
>    lsgpu: Add filter type print-out selection
>    intel_gpu_top: Default GPU list to PCI mode
> 
>   lib/igt_device_scan.c | 205 +++++++++++++++++++++++++++++++++++++-----
>   lib/igt_device_scan.h |  16 +++-
>   tools/intel_gpu_top.c |   8 +-
>   tools/lsgpu.c         |  38 ++++++--
>   4 files changed, 233 insertions(+), 34 deletions(-)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning
  2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning Tvrtko Ursulin
@ 2020-11-16 14:03   ` Zbigniew Kempczyński
  2020-11-16 15:09     ` Tvrtko Ursulin
  0 siblings, 1 reply; 11+ messages in thread
From: Zbigniew Kempczyński @ 2020-11-16 14:03 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx

On Fri, Nov 13, 2020 at 04:27:34PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> After devices are sorted post scanning, assing a card index to each so it
> can be easily accessed if PCI filter for a card needs to be printed out.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> ---
>  lib/igt_device_scan.c | 43 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index c0cd6757fc27..72653403f705 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -166,6 +166,7 @@ struct igt_device {
>  	char *vendor;
>  	char *device;
>  	char *pci_slot_name;
> +	int pci_index;
>  
>  	struct igt_list_head link;
>  };
> @@ -601,6 +602,33 @@ static void sort_all_devices(void)
>  	free(devs);
>  }
>  
> +static void index_pci_devices(void)
> +{
> +	struct igt_device *dev;
> +
> +	igt_list_for_each_entry(dev, &igt_devs.all, link) {
> +		struct igt_device *dev2;
> +		int index = 0;
> +
> +		if (!is_pci_subsystem(dev))
> +			continue;
> +
> +		igt_list_for_each_entry(dev2, &igt_devs.all, link) {
> +			if (!is_pci_subsystem(dev2))
> +				continue;
> +
> +			if (dev2 == dev)
> +				break;
> +
> +			if (!strcasecmp(dev->vendor, dev2->vendor) &&
> +			    !strcasecmp(dev->device, dev2->device))
> +				index++;
> +		}
> +
> +		dev->pci_index = index;
> +	}
> +}
> +
>  /* Core scanning function.
>   *
>   * All scanned devices are kept inside igt_devs.all pointer array.
> @@ -657,6 +685,7 @@ static void scan_drm_devices(void)
>  	udev_unref(udev);
>  
>  	sort_all_devices();
> +	index_pci_devices();
>  
>  	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>  		struct igt_device *dev_dup = duplicate_device(dev);
> @@ -1105,13 +1134,13 @@ static struct igt_list_head *filter_pci(const struct filter_class *fcls,
>  		if (filter->data.device && strcasecmp(filter->data.device, dev->device))
>  			continue;
>  
> -		/* We get n-th card */
> -		if (!card) {
> -			struct igt_device *dup = duplicate_device(dev);
> -			igt_list_add_tail(&dup->link, &igt_devs.filtered);
> -			break;
> -		}
> -		card--;
> +		/* Skip if 'card' doesn't match */
> +		if (card != dev->pci_index)
> +			continue;
> +
> +		dev = duplicate_device(dev);
> +		igt_list_add_tail(&dev->link, &igt_devs.filtered);
> +		break;

I may wrong (I got no such testing env) but devs_compare() function along with
index_pci_devices() can lead us to such (example) situation:

igt_devs.all contains devices with syspaths and subsystem pci: 
/sys/devices/pci0000:00/0000:00:04.0	-> vendor 8086, device 1234	pci_index == 0
/sys/devices/pci0000:00/0000:00:05.0	-> vendor 8086, device 4321	pci_index == 1
/sys/devices/pci0000:00/0000:00:06.0	-> vendor 8086, device 1234	pci_index == 2

If would try filter: pci=vendor=8086,device=1234,card=1 I would expect
to select (second card [index == 1]) from filtered devices which match
device 1234.

--
Zbigniew

>  	}
>  
>  	DBG("Filter pci filtered size: %d\n", igt_list_length(&igt_devs.filtered));
> -- 
> 2.25.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning
  2020-11-16 14:03   ` Zbigniew Kempczyński
@ 2020-11-16 15:09     ` Tvrtko Ursulin
  2020-11-16 16:47       ` Zbigniew Kempczyński
  0 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2020-11-16 15:09 UTC (permalink / raw)
  To: Zbigniew Kempczyński; +Cc: igt-dev, Intel-gfx


On 16/11/2020 14:03, Zbigniew Kempczyński wrote:
> On Fri, Nov 13, 2020 at 04:27:34PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> After devices are sorted post scanning, assing a card index to each so it
>> can be easily accessed if PCI filter for a card needs to be printed out.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Petri Latvala <petri.latvala@intel.com>
>> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>> ---
>>   lib/igt_device_scan.c | 43 ++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>> index c0cd6757fc27..72653403f705 100644
>> --- a/lib/igt_device_scan.c
>> +++ b/lib/igt_device_scan.c
>> @@ -166,6 +166,7 @@ struct igt_device {
>>   	char *vendor;
>>   	char *device;
>>   	char *pci_slot_name;
>> +	int pci_index;
>>   
>>   	struct igt_list_head link;
>>   };
>> @@ -601,6 +602,33 @@ static void sort_all_devices(void)
>>   	free(devs);
>>   }
>>   
>> +static void index_pci_devices(void)
>> +{
>> +	struct igt_device *dev;
>> +
>> +	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>> +		struct igt_device *dev2;
>> +		int index = 0;
>> +
>> +		if (!is_pci_subsystem(dev))
>> +			continue;
>> +
>> +		igt_list_for_each_entry(dev2, &igt_devs.all, link) {
>> +			if (!is_pci_subsystem(dev2))
>> +				continue;
>> +
>> +			if (dev2 == dev)
>> +				break;
>> +
>> +			if (!strcasecmp(dev->vendor, dev2->vendor) &&
>> +			    !strcasecmp(dev->device, dev2->device))
>> +				index++;
>> +		}
>> +
>> +		dev->pci_index = index;
>> +	}
>> +}
>> +
>>   /* Core scanning function.
>>    *
>>    * All scanned devices are kept inside igt_devs.all pointer array.
>> @@ -657,6 +685,7 @@ static void scan_drm_devices(void)
>>   	udev_unref(udev);
>>   
>>   	sort_all_devices();
>> +	index_pci_devices();
>>   
>>   	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>>   		struct igt_device *dev_dup = duplicate_device(dev);
>> @@ -1105,13 +1134,13 @@ static struct igt_list_head *filter_pci(const struct filter_class *fcls,
>>   		if (filter->data.device && strcasecmp(filter->data.device, dev->device))
>>   			continue;
>>   
>> -		/* We get n-th card */
>> -		if (!card) {
>> -			struct igt_device *dup = duplicate_device(dev);
>> -			igt_list_add_tail(&dup->link, &igt_devs.filtered);
>> -			break;
>> -		}
>> -		card--;
>> +		/* Skip if 'card' doesn't match */
>> +		if (card != dev->pci_index)
>> +			continue;
>> +
>> +		dev = duplicate_device(dev);
>> +		igt_list_add_tail(&dev->link, &igt_devs.filtered);
>> +		break;
> 
> I may wrong (I got no such testing env) but devs_compare() function along with
> index_pci_devices() can lead us to such (example) situation:
> 
> igt_devs.all contains devices with syspaths and subsystem pci:
> /sys/devices/pci0000:00/0000:00:04.0	-> vendor 8086, device 1234	pci_index == 0
> /sys/devices/pci0000:00/0000:00:05.0	-> vendor 8086, device 4321	pci_index == 1
> /sys/devices/pci0000:00/0000:00:06.0	-> vendor 8086, device 1234	pci_index == 2
> 
> If would try filter: pci=vendor=8086,device=1234,card=1 I would expect
> to select (second card [index == 1]) from filtered devices which match
> device 1234.

It is supposed to be like you say, each pci_index being in the namespace 
of equal vendor & device only.

Like from a dual GPU system:

$ tools/lsgpu --pci
card1                   8086:4905    pci:vendor=8086,device=4905,card=0
└─renderD129
card0                   8086:3E98    pci:vendor=8086,device=3E98,card=0
└─renderD128

Or two identical GPUs (mocked by manual addition of an almost duplicate 
entry to list of scanned devices:

$ tools/lsgpu --pci
card0                   8086:193B    pci:vendor=8086,device=193B,card=0
├─renderD128
└─renderD128
card0x                  8086:193B    pci:vendor=8086,device=193B,card=1
├─renderD128
└─renderD128

Regards,

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning
  2020-11-16 15:09     ` Tvrtko Ursulin
@ 2020-11-16 16:47       ` Zbigniew Kempczyński
  2020-11-16 17:07         ` Tvrtko Ursulin
  0 siblings, 1 reply; 11+ messages in thread
From: Zbigniew Kempczyński @ 2020-11-16 16:47 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx

On Mon, Nov 16, 2020 at 03:09:43PM +0000, Tvrtko Ursulin wrote:

<cut>
 
> > > +
> > >   /* Core scanning function.
> > >    *
> > >    * All scanned devices are kept inside igt_devs.all pointer array.
> > > @@ -657,6 +685,7 @@ static void scan_drm_devices(void)
> > >   	udev_unref(udev);
> > >   	sort_all_devices();
> > > +	index_pci_devices();
> > >   	igt_list_for_each_entry(dev, &igt_devs.all, link) {
> > >   		struct igt_device *dev_dup = duplicate_device(dev);
> > > @@ -1105,13 +1134,13 @@ static struct igt_list_head *filter_pci(const struct filter_class *fcls,
> > >   		if (filter->data.device && strcasecmp(filter->data.device, dev->device))
> > >   			continue;
> > > -		/* We get n-th card */
> > > -		if (!card) {
> > > -			struct igt_device *dup = duplicate_device(dev);
> > > -			igt_list_add_tail(&dup->link, &igt_devs.filtered);
> > > -			break;
> > > -		}
> > > -		card--;
> > > +		/* Skip if 'card' doesn't match */
> > > +		if (card != dev->pci_index)
> > > +			continue;
> > > +
> > > +		dev = duplicate_device(dev);
> > > +		igt_list_add_tail(&dev->link, &igt_devs.filtered);
> > > +		break;
> > 
> > I may wrong (I got no such testing env) but devs_compare() function along with
> > index_pci_devices() can lead us to such (example) situation:
> > 
> > igt_devs.all contains devices with syspaths and subsystem pci:
> > /sys/devices/pci0000:00/0000:00:04.0	-> vendor 8086, device 1234	pci_index == 0
> > /sys/devices/pci0000:00/0000:00:05.0	-> vendor 8086, device 4321	pci_index == 1
> > /sys/devices/pci0000:00/0000:00:06.0	-> vendor 8086, device 1234	pci_index == 2
> > 
> > If would try filter: pci=vendor=8086,device=1234,card=1 I would expect
> > to select (second card [index == 1]) from filtered devices which match
> > device 1234.
> 
> It is supposed to be like you say, each pci_index being in the namespace of
> equal vendor & device only.
> 
> Like from a dual GPU system:
> 
> $ tools/lsgpu --pci
> card1                   8086:4905    pci:vendor=8086,device=4905,card=0
> └─renderD129
> card0                   8086:3E98    pci:vendor=8086,device=3E98,card=0
> └─renderD128
> 
> Or two identical GPUs (mocked by manual addition of an almost duplicate
> entry to list of scanned devices:
> 
> $ tools/lsgpu --pci
> card0                   8086:193B    pci:vendor=8086,device=193B,card=0
> ├─renderD128
> └─renderD128
> card0x                  8086:193B    pci:vendor=8086,device=193B,card=1
> ├─renderD128
> └─renderD128
> 
> Regards,
> 
> Tvrtko

I made a mistake in above listing, it should be:
/sys/devices/pci0000:00/0000:00:04.0	-> vendor 8086, device 1234	pci_index == 0
/sys/devices/pci0000:00/0000:00:05.0	-> vendor 8086, device 4321	pci_index == 0
/sys/devices/pci0000:00/0000:00:06.0	-> vendor 8086, device 1234	pci_index == 1

What if I would go over all Intel cards using pci filter:

pci:vendor=8086,card=N   where n is 0... until there's no Intel pci device.

This way we loose possibility to iterate over all pci devices imo.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning
  2020-11-16 16:47       ` Zbigniew Kempczyński
@ 2020-11-16 17:07         ` Tvrtko Ursulin
  0 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2020-11-16 17:07 UTC (permalink / raw)
  To: Zbigniew Kempczyński; +Cc: igt-dev, Intel-gfx


On 16/11/2020 16:47, Zbigniew Kempczyński wrote:
> On Mon, Nov 16, 2020 at 03:09:43PM +0000, Tvrtko Ursulin wrote:
> 
> <cut>
>   
>>>> +
>>>>    /* Core scanning function.
>>>>     *
>>>>     * All scanned devices are kept inside igt_devs.all pointer array.
>>>> @@ -657,6 +685,7 @@ static void scan_drm_devices(void)
>>>>    	udev_unref(udev);
>>>>    	sort_all_devices();
>>>> +	index_pci_devices();
>>>>    	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>>>>    		struct igt_device *dev_dup = duplicate_device(dev);
>>>> @@ -1105,13 +1134,13 @@ static struct igt_list_head *filter_pci(const struct filter_class *fcls,
>>>>    		if (filter->data.device && strcasecmp(filter->data.device, dev->device))
>>>>    			continue;
>>>> -		/* We get n-th card */
>>>> -		if (!card) {
>>>> -			struct igt_device *dup = duplicate_device(dev);
>>>> -			igt_list_add_tail(&dup->link, &igt_devs.filtered);
>>>> -			break;
>>>> -		}
>>>> -		card--;
>>>> +		/* Skip if 'card' doesn't match */
>>>> +		if (card != dev->pci_index)
>>>> +			continue;
>>>> +
>>>> +		dev = duplicate_device(dev);
>>>> +		igt_list_add_tail(&dev->link, &igt_devs.filtered);
>>>> +		break;
>>>
>>> I may wrong (I got no such testing env) but devs_compare() function along with
>>> index_pci_devices() can lead us to such (example) situation:
>>>
>>> igt_devs.all contains devices with syspaths and subsystem pci:
>>> /sys/devices/pci0000:00/0000:00:04.0	-> vendor 8086, device 1234	pci_index == 0
>>> /sys/devices/pci0000:00/0000:00:05.0	-> vendor 8086, device 4321	pci_index == 1
>>> /sys/devices/pci0000:00/0000:00:06.0	-> vendor 8086, device 1234	pci_index == 2
>>>
>>> If would try filter: pci=vendor=8086,device=1234,card=1 I would expect
>>> to select (second card [index == 1]) from filtered devices which match
>>> device 1234.
>>
>> It is supposed to be like you say, each pci_index being in the namespace of
>> equal vendor & device only.
>>
>> Like from a dual GPU system:
>>
>> $ tools/lsgpu --pci
>> card1                   8086:4905    pci:vendor=8086,device=4905,card=0
>> └─renderD129
>> card0                   8086:3E98    pci:vendor=8086,device=3E98,card=0
>> └─renderD128
>>
>> Or two identical GPUs (mocked by manual addition of an almost duplicate
>> entry to list of scanned devices:
>>
>> $ tools/lsgpu --pci
>> card0                   8086:193B    pci:vendor=8086,device=193B,card=0
>> ├─renderD128
>> └─renderD128
>> card0x                  8086:193B    pci:vendor=8086,device=193B,card=1
>> ├─renderD128
>> └─renderD128
>>
>> Regards,
>>
>> Tvrtko
> 
> I made a mistake in above listing, it should be:
> /sys/devices/pci0000:00/0000:00:04.0	-> vendor 8086, device 1234	pci_index == 0
> /sys/devices/pci0000:00/0000:00:05.0	-> vendor 8086, device 4321	pci_index == 0
> /sys/devices/pci0000:00/0000:00:06.0	-> vendor 8086, device 1234	pci_index == 1
> 
> What if I would go over all Intel cards using pci filter:
> 
> pci:vendor=8086,card=N   where n is 0... until there's no Intel pci device.
> 
> This way we loose possibility to iterate over all pci devices imo.

Oh I get it know, I misunderstood how this all works. So the card 
argument to the pci filter is nothing about the actual PCI card, but 
just "match number" - match with this filter and return card=N Nth match.

So I should leave the code in filter_pci as is.

Will my new filters work like that. I think so. Ok, will update.

Regards,

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-11-16 17:08 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-13 16:27 [Intel-gfx] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin
2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 1/5] intel_gpu_top: User friendly " Tvrtko Ursulin
2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 2/5] lsgpu: " Tvrtko Ursulin
2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning Tvrtko Ursulin
2020-11-16 14:03   ` Zbigniew Kempczyński
2020-11-16 15:09     ` Tvrtko Ursulin
2020-11-16 16:47       ` Zbigniew Kempczyński
2020-11-16 17:07         ` Tvrtko Ursulin
2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 4/5] lsgpu: Add filter type print-out selection Tvrtko Ursulin
2020-11-13 16:27 ` [Intel-gfx] [RFC i-g-t 5/5] intel_gpu_top: Default GPU list to PCI mode Tvrtko Ursulin
2020-11-16 13:08 ` [Intel-gfx] [igt-dev] [RFC i-g-t 0/5] User friendly lsgpu/intel_gpu_top device listing Tvrtko Ursulin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox