All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices
@ 2020-11-18 13:50 Zbigniew Kempczyński
  2020-11-18 13:50 ` [igt-dev] [PATCH i-g-t v4 2/3] tools/intel_gpu_top: Add generation info in header Zbigniew Kempczyński
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Zbigniew Kempczyński @ 2020-11-18 13:50 UTC (permalink / raw)
  To: igt-dev; +Cc: Petri Latvala, Tvrtko Ursulin

If we want to use pci device id for not opened device we need to
keep it in card structure.

Export igt_device_get_pretty_name() function to the caller to
allow return pretty name (for lsgpu, intel_gpu_top and others).

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Petri Latvala <petri.latvala@intel.com>
---
 lib/igt_device_scan.c | 150 ++++++++++++++++++++++++++++++++++++++----
 lib/igt_device_scan.h |   3 +
 2 files changed, 140 insertions(+), 13 deletions(-)

diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
index e97f7163..df41ee51 100644
--- a/lib/igt_device_scan.c
+++ b/lib/igt_device_scan.c
@@ -25,7 +25,9 @@
 #include "igt_core.h"
 #include "igt_device_scan.h"
 #include "igt_list.h"
+#include "intel_chipset.h"
 
+#include <ctype.h>
 #include <dirent.h>
 #include <fcntl.h>
 #include <glib.h>
@@ -178,12 +180,43 @@ static struct {
 	bool devs_scanned;
 } igt_devs;
 
+typedef char *(*devname_fn)(uint16_t, uint16_t);
+
+static char *devname_hex(uint16_t vendor, uint16_t device)
+{
+	char *s;
+
+	igt_assert(asprintf(&s, "%04x:%04x", vendor, device) == 9);
+
+	return s;
+}
+
+static char *devname_intel(uint16_t vendor, uint16_t device)
+{
+	const struct intel_device_info *info = intel_get_device_info(device);
+	char *devname, *s;
+
+	if (info->codename) {
+		devname = strdup(info->codename);
+		devname[0] = toupper(devname[0]);
+	} else {
+		asprintf(&devname, "%04x:%04x", vendor, device);
+	}
+
+	asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));
+
+	free(devname);
+
+	return s;
+}
+
 static struct {
 	const char *name;
 	const char *vendor_id;
+	devname_fn devname;
 } pci_vendor_mapping[] = {
-	{ "intel", "8086" },
-	{ "amd", "1002" },
+	{ "intel", "8086", devname_intel },
+	{ "amd", "1002", devname_hex },
 	{ NULL, },
 };
 
@@ -198,6 +231,43 @@ static const char *get_pci_vendor_id_by_name(const char *name)
 	return NULL;
 }
 
+static devname_fn get_pci_vendor_device_fn(const char *vendor_id)
+{
+	for (typeof(*pci_vendor_mapping) *vm = pci_vendor_mapping; vm->name; vm++) {
+		if (!strcasecmp(vendor_id, vm->vendor_id))
+			return vm->devname;
+	}
+
+	return devname_hex;
+}
+
+static void get_pci_vendor_device(const struct igt_device *dev,
+				  uint16_t *vendorp, uint16_t *devicep)
+{
+	igt_assert(dev && dev->vendor && dev->device);
+	igt_assert(vendorp && devicep);
+
+	igt_assert(sscanf(dev->vendor, "%hx", vendorp) == 1);
+	igt_assert(sscanf(dev->device, "%hx", devicep) == 1);
+}
+
+static char *__pci_pretty_name(uint16_t vendor, uint16_t device, bool numeric)
+{
+	char *devname, vendorstr[5];
+	devname_fn fn;
+
+	if (!numeric) {
+		snprintf(vendorstr, sizeof(vendorstr), "%04x", vendor);
+		fn = get_pci_vendor_device_fn(vendorstr);
+	} else {
+		fn = devname_hex;
+	}
+
+	devname = fn(vendor, device);
+
+	return devname;
+}
+
 /* Reading sysattr values can take time (even seconds),
  * we want to avoid reading such keys.
  */
@@ -446,23 +516,44 @@ static bool is_vendor_matched(struct igt_device *dev, const char *vendor)
 	return !strcasecmp(dev->vendor, vendor_id);
 }
 
+static char *safe_strncpy(char *dst, const char *src, int n)
+{
+	char *s;
+
+	igt_assert(n > 0);
+	igt_assert(dst && src);
+
+	s = strncpy(dst, src, n - 1);
+	s[n - 1] = '\0';
+
+	return s;
+}
+
 static void
 __copy_dev_to_card(struct igt_device *dev, struct igt_device_card *card)
 {
 	if (dev->subsystem != NULL)
-		strncpy(card->subsystem, dev->subsystem,
-			sizeof(card->subsystem) - 1);
+		safe_strncpy(card->subsystem, dev->subsystem,
+			     sizeof(card->subsystem));
 
 	if (dev->drm_card != NULL)
-		strncpy(card->card, dev->drm_card, sizeof(card->card) - 1);
+		safe_strncpy(card->card, dev->drm_card, sizeof(card->card));
 
 	if (dev->drm_render != NULL)
-		strncpy(card->render, dev->drm_render,
-			sizeof(card->render) - 1);
+		safe_strncpy(card->render, dev->drm_render,
+			     sizeof(card->render));
 
 	if (dev->pci_slot_name != NULL)
-		strncpy(card->pci_slot_name, dev->pci_slot_name,
-			PCI_SLOT_NAME_SIZE + 1);
+		safe_strncpy(card->pci_slot_name, dev->pci_slot_name,
+			     sizeof(card->pci_slot_name));
+
+	if (dev->vendor != NULL)
+		if (sscanf(dev->vendor, "%hx", &card->pci_vendor) != 1)
+			card->pci_vendor = 0;
+
+	if (dev->device != NULL)
+		if (sscanf(dev->device, "%hx", &card->pci_device) != 1)
+			card->pci_device = 0;
 }
 
 /*
@@ -852,6 +943,7 @@ static void __print_filter(char *buf, int len,
 	};
 }
 
+#define VENDOR_SIZE 30
 static void
 igt_devs_print_user(struct igt_list_head *view,
 		    const struct igt_devices_print_format *fmt)
@@ -883,11 +975,19 @@ igt_devs_print_user(struct igt_list_head *view,
 			continue;
 
 		if (pci_dev) {
+			uint16_t vendor, device;
+			char *devname;
+
+			get_pci_vendor_device(pci_dev, &vendor, &device);
+			devname = __pci_pretty_name(vendor, device, fmt->numeric);
+
 			__print_filter(filter, sizeof(filter), fmt, pci_dev,
 				       false);
-			printf("%-24s%4s:%4s    %s\n",
-			       drm_name, pci_dev->vendor, pci_dev->device,
-			       filter);
+
+			printf("%-24s %-*s    %s\n",
+			       drm_name, VENDOR_SIZE, devname, filter);
+
+			free(devname);
 		} else {
 			__print_filter(filter, sizeof(filter), fmt, dev, false);
 			printf("%-24s             %s\n", drm_name, filter);
@@ -919,7 +1019,7 @@ igt_devs_print_user(struct igt_list_head *view,
 			if (fmt->option != IGT_PRINT_PCI) {
 				__print_filter(filter, sizeof(filter), fmt,
 					       dev2, true);
-				printf("             %s\n", filter);
+				printf("%-*s     %s\n", VENDOR_SIZE, "", filter);
 			} else {
 				printf("\n");
 			}
@@ -1479,6 +1579,30 @@ bool igt_device_card_match_pci(const char *filter,
        return __igt_device_card_match(filter, card, true);
 }
 
+/**
+ * igt_device_get_pretty_name
+ * @card: pointer to igt_device_card struct
+ *
+ * For card function returns allocated string having pretty name
+ * or vendor:device as hex if no backend pretty-resolver is implemented.
+ *
+ * Returns: newly allocated string.
+ */
+char *igt_device_get_pretty_name(struct igt_device_card *card, bool numeric)
+{
+	char *devname;
+
+	igt_assert(card);
+
+	if (strlen(card->pci_slot_name))
+		devname = __pci_pretty_name(card->pci_vendor, card->pci_device,
+					    numeric);
+	else
+		devname = strdup(card->subsystem);
+
+	return devname;
+}
+
 /**
  * igt_open_card:
  * @card: pointer to igt_device_card structure
diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
index bb4be723..5f583a06 100644
--- a/lib/igt_device_scan.h
+++ b/lib/igt_device_scan.h
@@ -49,6 +49,7 @@ enum igt_devices_print_option {
 struct igt_devices_print_format {
 	enum igt_devices_print_type   type;
 	enum igt_devices_print_option option;
+	bool numeric;
 };
 
 #define INTEGRATED_I915_GPU_PCI_ID "0000:00:02.0"
@@ -58,6 +59,7 @@ struct igt_device_card {
 	char card[NAME_MAX];
 	char render[NAME_MAX];
 	char pci_slot_name[PCI_SLOT_NAME_SIZE+1];
+	uint16_t pci_vendor, pci_device;
 };
 
 void igt_devices_scan(bool force);
@@ -84,6 +86,7 @@ bool igt_device_card_match_pci(const char *filter,
 	struct igt_device_card *card);
 bool igt_device_find_first_i915_discrete_card(struct igt_device_card *card);
 bool igt_device_find_integrated_card(struct igt_device_card *card);
+char *igt_device_get_pretty_name(struct igt_device_card *card, bool numeric);
 int igt_open_card(struct igt_device_card *card);
 int igt_open_render(struct igt_device_card *card);
 
-- 
2.26.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* [igt-dev] [PATCH i-g-t v4 2/3] tools/intel_gpu_top: Add generation info in header
  2020-11-18 13:50 [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices Zbigniew Kempczyński
@ 2020-11-18 13:50 ` Zbigniew Kempczyński
  2020-11-18 15:20   ` Tvrtko Ursulin
  2020-11-18 13:50 ` [igt-dev] [PATCH i-g-t v4 3/3] tools/lsgpu: Add -n switch to list devices using vendor:device hex id Zbigniew Kempczyński
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Zbigniew Kempczyński @ 2020-11-18 13:50 UTC (permalink / raw)
  To: igt-dev; +Cc: Petri Latvala, Tvrtko Ursulin

In multi device world we may want to see generation of device we're
tracking counters. Add pretty name of the device to be more verbose.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Petri Latvala <petri.latvala@intel.com>
---
 lib/Makefile.am       | 4 +++-
 lib/meson.build       | 1 +
 tools/intel_gpu_top.c | 8 ++++++--
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/lib/Makefile.am b/lib/Makefile.am
index fecf34cd..c476eeab 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -118,7 +118,9 @@ libigt_device_scan_la_SOURCES = \
 	igt_list.c		\
 	igt_tools_stub.c	\
 	igt_device_scan.c	\
-	igt_device_scan.h
+	igt_device_scan.h	\
+	intel_device_info.c	\
+	intel_chipset.h
 
 libigt_perf_la_SOURCES = \
 	igt_perf.c	 \
diff --git a/lib/meson.build b/lib/meson.build
index 090a10cd..540facb2 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -182,6 +182,7 @@ lib_igt_device_scan_build = static_library('igt_device_scan',
 	['igt_device_scan.c',
 	'igt_list.c',
 	'igt_tools_stub.c',
+	'intel_device_info.c',
 	],
 	dependencies : scan_dep,
 	include_directories : inc)
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 86de09aa..3e97be2c 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -1028,6 +1028,7 @@ static bool print_groups(struct cnt_group **groups)
 
 static int
 print_header(const struct igt_device_card *card,
+	     const char *codename,
 	     struct engines *engines, double t,
 	     int lines, int con_w, int con_h, bool *consumed)
 {
@@ -1107,7 +1108,7 @@ print_header(const struct igt_device_card *card,
 		printf("\033[H\033[J");
 
 		if (lines++ < con_h) {
-			printf("intel-gpu-top: %s - ", card->card);
+			printf("intel-gpu-top: %s @ %s - ", codename, card->card);
 			if (!engines->discrete)
 				printf("%s/%s MHz;  %s%% RC6; %s %s; %s irqs/s\n",
 					freq_items[1].buf, freq_items[0].buf,
@@ -1317,6 +1318,7 @@ int main(int argc, char **argv)
 	bool list_device = false;
 	char *pmu_device, *opt_device = NULL;
 	struct igt_device_card card;
+	char *codename = NULL;
 
 	/* Parse options */
 	while ((ch = getopt(argc, argv, "o:s:d:JLlh")) != -1) {
@@ -1441,6 +1443,7 @@ int main(int argc, char **argv)
 	ret = EXIT_SUCCESS;
 
 	pmu_sample(engines);
+	codename = igt_device_get_pretty_name(&card, false);
 
 	while (!stop_top) {
 		bool consumed = false;
@@ -1463,7 +1466,7 @@ int main(int argc, char **argv)
 			break;
 
 		while (!consumed) {
-			lines = print_header(&card, engines,
+			lines = print_header(&card, codename, engines,
 					     t, lines, con_w, con_h,
 					     &consumed);
 
@@ -1490,6 +1493,7 @@ int main(int argc, char **argv)
 		usleep(period_us);
 	}
 
+	free(codename);
 err:
 	free(engines);
 	free(pmu_device);
-- 
2.26.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* [igt-dev] [PATCH i-g-t v4 3/3] tools/lsgpu: Add -n switch to list devices using vendor:device hex id
  2020-11-18 13:50 [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices Zbigniew Kempczyński
  2020-11-18 13:50 ` [igt-dev] [PATCH i-g-t v4 2/3] tools/intel_gpu_top: Add generation info in header Zbigniew Kempczyński
@ 2020-11-18 13:50 ` Zbigniew Kempczyński
  2020-11-18 15:21   ` Tvrtko Ursulin
  2020-11-18 15:17 ` [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices Tvrtko Ursulin
  2020-11-18 15:30 ` [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,v4,1/3] " Patchwork
  3 siblings, 1 reply; 9+ messages in thread
From: Zbigniew Kempczyński @ 2020-11-18 13:50 UTC (permalink / raw)
  To: igt-dev; +Cc: Petri Latvala, Tvrtko Ursulin

Default device list prefers vendor and device names. Add -n switch
to display vendor/device as hex strings.

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

diff --git a/tools/lsgpu.c b/tools/lsgpu.c
index 169ab0c2..25358bbe 100644
--- a/tools/lsgpu.c
+++ b/tools/lsgpu.c
@@ -72,6 +72,7 @@
 enum {
 	OPT_PRINT_SIMPLE   = 's',
 	OPT_PRINT_DETAIL   = 'p',
+	OPT_NUMERIC        = 'n',
 	OPT_LIST_VENDORS   = 'v',
 	OPT_LIST_FILTERS   = 'l',
 	OPT_DEVICE         = 'd',
@@ -86,6 +87,7 @@ static char *igt_device;
 static const char *usage_str =
 	"usage: lsgpu [options]\n\n"
 	"Options:\n"
+	"  -n, --numeric               Print vendor/device as hex\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"
@@ -160,6 +162,7 @@ int main(int argc, char *argv[])
 		{"drm",               no_argument,       NULL, 0},
 		{"sysfs",             no_argument,       NULL, 1},
 		{"pci",               no_argument,       NULL, 2},
+		{"numeric",           no_argument,       NULL, OPT_NUMERIC},
 		{"print-simple",      no_argument,       NULL, OPT_PRINT_SIMPLE},
 		{"print-detail",      no_argument,       NULL, OPT_PRINT_DETAIL},
 		{"list-vendors",      no_argument,       NULL, OPT_LIST_VENDORS},
@@ -174,10 +177,13 @@ int main(int argc, char *argv[])
 			.type = IGT_PRINT_USER,
 	};
 
-	while ((c = getopt_long(argc, argv, "spvld:h",
+	while ((c = getopt_long(argc, argv, "nspvld:h",
 				long_options, &index)) != -1) {
 		switch(c) {
 
+		case OPT_NUMERIC:
+			fmt.numeric = true;
+			break;
 		case OPT_PRINT_SIMPLE:
 			fmt.type = IGT_PRINT_SIMPLE;
 			break;
-- 
2.26.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices
  2020-11-18 13:50 [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices Zbigniew Kempczyński
  2020-11-18 13:50 ` [igt-dev] [PATCH i-g-t v4 2/3] tools/intel_gpu_top: Add generation info in header Zbigniew Kempczyński
  2020-11-18 13:50 ` [igt-dev] [PATCH i-g-t v4 3/3] tools/lsgpu: Add -n switch to list devices using vendor:device hex id Zbigniew Kempczyński
@ 2020-11-18 15:17 ` Tvrtko Ursulin
  2020-11-19  7:25   ` Zbigniew Kempczyński
  2020-11-18 15:30 ` [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,v4,1/3] " Patchwork
  3 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2020-11-18 15:17 UTC (permalink / raw)
  To: Zbigniew Kempczyński, igt-dev; +Cc: Petri Latvala, Tvrtko Ursulin


On 18/11/2020 13:50, Zbigniew Kempczyński wrote:
> If we want to use pci device id for not opened device we need to
> keep it in card structure.
> 
> Export igt_device_get_pretty_name() function to the caller to
> allow return pretty name (for lsgpu, intel_gpu_top and others).
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> ---
>   lib/igt_device_scan.c | 150 ++++++++++++++++++++++++++++++++++++++----
>   lib/igt_device_scan.h |   3 +
>   2 files changed, 140 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index e97f7163..df41ee51 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -25,7 +25,9 @@
>   #include "igt_core.h"
>   #include "igt_device_scan.h"
>   #include "igt_list.h"
> +#include "intel_chipset.h"
>   
> +#include <ctype.h>
>   #include <dirent.h>
>   #include <fcntl.h>
>   #include <glib.h>
> @@ -178,12 +180,43 @@ static struct {
>   	bool devs_scanned;
>   } igt_devs;
>   
> +typedef char *(*devname_fn)(uint16_t, uint16_t);
> +
> +static char *devname_hex(uint16_t vendor, uint16_t device)
> +{
> +	char *s;
> +
> +	igt_assert(asprintf(&s, "%04x:%04x", vendor, device) == 9);
> +
> +	return s;
> +}
> +
> +static char *devname_intel(uint16_t vendor, uint16_t device)
> +{
> +	const struct intel_device_info *info = intel_get_device_info(device);
> +	char *devname, *s;
> +
> +	if (info->codename) {
> +		devname = strdup(info->codename);
> +		devname[0] = toupper(devname[0]);

Strictly could be a null ptr deref here.

> +	} else {
> +		asprintf(&devname, "%04x:%04x", vendor, device);

Could call devname_hex, for some questionable value of improvement, up to you.

> +	}
> +
> +	asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));

Is info->gen always valid? Looks like not, we'd gen Gen0 on unknown devices. Weird API, anyway.. Maybe try like this then:

if (info->codename) {
	char *devname = strdup(info->codename);

	if (devname) {
		devname[0] = toupper(devname[0]);
		asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));
		free(devname);
	}
}

if (!devname)
	s = devname_hex(vendor, device);

return s;

I think that should cover all the cases.

> +
> +	free(devname);
> +
> +	return s;
> +}
> +
>   static struct {
>   	const char *name;
>   	const char *vendor_id;
> +	devname_fn devname;
>   } pci_vendor_mapping[] = {
> -	{ "intel", "8086" },
> -	{ "amd", "1002" },
> +	{ "intel", "8086", devname_intel },
> +	{ "amd", "1002", devname_hex },
>   	{ NULL, },
>   };
>   
> @@ -198,6 +231,43 @@ static const char *get_pci_vendor_id_by_name(const char *name)
>   	return NULL;
>   }
>   
> +static devname_fn get_pci_vendor_device_fn(const char *vendor_id)
> +{
> +	for (typeof(*pci_vendor_mapping) *vm = pci_vendor_mapping; vm->name; vm++) {
> +		if (!strcasecmp(vendor_id, vm->vendor_id))
> +			return vm->devname;
> +	}
> +
> +	return devname_hex;
> +}
> +
> +static void get_pci_vendor_device(const struct igt_device *dev,
> +				  uint16_t *vendorp, uint16_t *devicep)
> +{
> +	igt_assert(dev && dev->vendor && dev->device);
> +	igt_assert(vendorp && devicep);
> +
> +	igt_assert(sscanf(dev->vendor, "%hx", vendorp) == 1);
> +	igt_assert(sscanf(dev->device, "%hx", devicep) == 1);
> +}
> +
> +static char *__pci_pretty_name(uint16_t vendor, uint16_t device, bool numeric)
> +{
> +	char *devname, vendorstr[5];
> +	devname_fn fn;
> +
> +	if (!numeric) {
> +		snprintf(vendorstr, sizeof(vendorstr), "%04x", vendor);

I did not get why this snprintf? Ah.. you need the hex in string format.. a bit awkward. Maybe add u16 representation to struct pci_vendor_mapping for convenience? Up to you. And same might apply to struct igt_device, just to reduce the number of string <-> u16 conversions (igt_devs_print_user ends up doing it both ways). But again more of "up to you".

> +		fn = get_pci_vendor_device_fn(vendorstr);
> +	} else {
> +		fn = devname_hex;
> +	}
> +
> +	devname = fn(vendor, device);
> +
> +	return devname;
> +}
> +
>   /* Reading sysattr values can take time (even seconds),
>    * we want to avoid reading such keys.
>    */
> @@ -446,23 +516,44 @@ static bool is_vendor_matched(struct igt_device *dev, const char *vendor)
>   	return !strcasecmp(dev->vendor, vendor_id);
>   }
>   
> +static char *safe_strncpy(char *dst, const char *src, int n)
> +{
> +	char *s;
> +
> +	igt_assert(n > 0);
> +	igt_assert(dst && src);
> +
> +	s = strncpy(dst, src, n - 1);
> +	s[n - 1] = '\0';
> +
> +	return s;
> +}
> +
>   static void
>   __copy_dev_to_card(struct igt_device *dev, struct igt_device_card *card)
>   {
>   	if (dev->subsystem != NULL)
> -		strncpy(card->subsystem, dev->subsystem,
> -			sizeof(card->subsystem) - 1);
> +		safe_strncpy(card->subsystem, dev->subsystem,
> +			     sizeof(card->subsystem));
>   
>   	if (dev->drm_card != NULL)
> -		strncpy(card->card, dev->drm_card, sizeof(card->card) - 1);
> +		safe_strncpy(card->card, dev->drm_card, sizeof(card->card));
>   
>   	if (dev->drm_render != NULL)
> -		strncpy(card->render, dev->drm_render,
> -			sizeof(card->render) - 1);
> +		safe_strncpy(card->render, dev->drm_render,
> +			     sizeof(card->render));
>   
>   	if (dev->pci_slot_name != NULL)
> -		strncpy(card->pci_slot_name, dev->pci_slot_name,
> -			PCI_SLOT_NAME_SIZE + 1);
> +		safe_strncpy(card->pci_slot_name, dev->pci_slot_name,
> +			     sizeof(card->pci_slot_name));
> +
> +	if (dev->vendor != NULL)
> +		if (sscanf(dev->vendor, "%hx", &card->pci_vendor) != 1)
> +			card->pci_vendor = 0;
> +
> +	if (dev->device != NULL)
> +		if (sscanf(dev->device, "%hx", &card->pci_device) != 1)
> +			card->pci_device = 0;
>   }
>   
>   /*
> @@ -852,6 +943,7 @@ static void __print_filter(char *buf, int len,
>   	};
>   }
>   
> +#define VENDOR_SIZE 30
>   static void
>   igt_devs_print_user(struct igt_list_head *view,
>   		    const struct igt_devices_print_format *fmt)
> @@ -883,11 +975,19 @@ igt_devs_print_user(struct igt_list_head *view,
>   			continue;
>   
>   		if (pci_dev) {
> +			uint16_t vendor, device;
> +			char *devname;
> +
> +			get_pci_vendor_device(pci_dev, &vendor, &device);
> +			devname = __pci_pretty_name(vendor, device, fmt->numeric);
> +
>   			__print_filter(filter, sizeof(filter), fmt, pci_dev,
>   				       false);
> -			printf("%-24s%4s:%4s    %s\n",
> -			       drm_name, pci_dev->vendor, pci_dev->device,
> -			       filter);
> +
> +			printf("%-24s %-*s    %s\n",
> +			       drm_name, VENDOR_SIZE, devname, filter);
> +
> +			free(devname);
>   		} else {
>   			__print_filter(filter, sizeof(filter), fmt, dev, false);
>   			printf("%-24s             %s\n", drm_name, filter);
> @@ -919,7 +1019,7 @@ igt_devs_print_user(struct igt_list_head *view,
>   			if (fmt->option != IGT_PRINT_PCI) {
>   				__print_filter(filter, sizeof(filter), fmt,
>   					       dev2, true);
> -				printf("             %s\n", filter);
> +				printf("%-*s     %s\n", VENDOR_SIZE, "", filter);
>   			} else {
>   				printf("\n");
>   			}
> @@ -1479,6 +1579,30 @@ bool igt_device_card_match_pci(const char *filter,
>          return __igt_device_card_match(filter, card, true);
>   }
>   
> +/**
> + * igt_device_get_pretty_name
> + * @card: pointer to igt_device_card struct
> + *
> + * For card function returns allocated string having pretty name
> + * or vendor:device as hex if no backend pretty-resolver is implemented.
> + *
> + * Returns: newly allocated string.
> + */
> +char *igt_device_get_pretty_name(struct igt_device_card *card, bool numeric)
> +{
> +	char *devname;
> +
> +	igt_assert(card);
> +
> +	if (strlen(card->pci_slot_name))
> +		devname = __pci_pretty_name(card->pci_vendor, card->pci_device,
> +					    numeric);
> +	else
> +		devname = strdup(card->subsystem);
> +
> +	return devname;
> +}
> +
>   /**
>    * igt_open_card:
>    * @card: pointer to igt_device_card structure
> diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
> index bb4be723..5f583a06 100644
> --- a/lib/igt_device_scan.h
> +++ b/lib/igt_device_scan.h
> @@ -49,6 +49,7 @@ enum igt_devices_print_option {
>   struct igt_devices_print_format {
>   	enum igt_devices_print_type   type;
>   	enum igt_devices_print_option option;
> +	bool numeric;
>   };
>   
>   #define INTEGRATED_I915_GPU_PCI_ID "0000:00:02.0"
> @@ -58,6 +59,7 @@ struct igt_device_card {
>   	char card[NAME_MAX];
>   	char render[NAME_MAX];
>   	char pci_slot_name[PCI_SLOT_NAME_SIZE+1];
> +	uint16_t pci_vendor, pci_device;
>   };
>   
>   void igt_devices_scan(bool force);
> @@ -84,6 +86,7 @@ bool igt_device_card_match_pci(const char *filter,
>   	struct igt_device_card *card);
>   bool igt_device_find_first_i915_discrete_card(struct igt_device_card *card);
>   bool igt_device_find_integrated_card(struct igt_device_card *card);
> +char *igt_device_get_pretty_name(struct igt_device_card *card, bool numeric);
>   int igt_open_card(struct igt_device_card *card);
>   int igt_open_render(struct igt_device_card *card);
>   
> 
Rest looks passable.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t v4 2/3] tools/intel_gpu_top: Add generation info in header
  2020-11-18 13:50 ` [igt-dev] [PATCH i-g-t v4 2/3] tools/intel_gpu_top: Add generation info in header Zbigniew Kempczyński
@ 2020-11-18 15:20   ` Tvrtko Ursulin
  0 siblings, 0 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2020-11-18 15:20 UTC (permalink / raw)
  To: Zbigniew Kempczyński, igt-dev; +Cc: Petri Latvala, Tvrtko Ursulin


On 18/11/2020 13:50, Zbigniew Kempczyński wrote:
> In multi device world we may want to see generation of device we're
> tracking counters. Add pretty name of the device to be more verbose.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> ---
>   lib/Makefile.am       | 4 +++-
>   lib/meson.build       | 1 +
>   tools/intel_gpu_top.c | 8 ++++++--
>   3 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/Makefile.am b/lib/Makefile.am
> index fecf34cd..c476eeab 100644
> --- a/lib/Makefile.am
> +++ b/lib/Makefile.am
> @@ -118,7 +118,9 @@ libigt_device_scan_la_SOURCES = \
>   	igt_list.c		\
>   	igt_tools_stub.c	\
>   	igt_device_scan.c	\
> -	igt_device_scan.h
> +	igt_device_scan.h	\
> +	intel_device_info.c	\
> +	intel_chipset.h
>   
>   libigt_perf_la_SOURCES = \
>   	igt_perf.c	 \
> diff --git a/lib/meson.build b/lib/meson.build
> index 090a10cd..540facb2 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -182,6 +182,7 @@ lib_igt_device_scan_build = static_library('igt_device_scan',
>   	['igt_device_scan.c',
>   	'igt_list.c',
>   	'igt_tools_stub.c',
> +	'intel_device_info.c',
>   	],
>   	dependencies : scan_dep,
>   	include_directories : inc)
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index 86de09aa..3e97be2c 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -1028,6 +1028,7 @@ static bool print_groups(struct cnt_group **groups)
>   
>   static int
>   print_header(const struct igt_device_card *card,
> +	     const char *codename,
>   	     struct engines *engines, double t,
>   	     int lines, int con_w, int con_h, bool *consumed)
>   {
> @@ -1107,7 +1108,7 @@ print_header(const struct igt_device_card *card,
>   		printf("\033[H\033[J");
>   
>   		if (lines++ < con_h) {
> -			printf("intel-gpu-top: %s - ", card->card);
> +			printf("intel-gpu-top: %s @ %s - ", codename, card->card);
>   			if (!engines->discrete)
>   				printf("%s/%s MHz;  %s%% RC6; %s %s; %s irqs/s\n",
>   					freq_items[1].buf, freq_items[0].buf,
> @@ -1317,6 +1318,7 @@ int main(int argc, char **argv)
>   	bool list_device = false;
>   	char *pmu_device, *opt_device = NULL;
>   	struct igt_device_card card;
> +	char *codename = NULL;
>   
>   	/* Parse options */
>   	while ((ch = getopt(argc, argv, "o:s:d:JLlh")) != -1) {
> @@ -1441,6 +1443,7 @@ int main(int argc, char **argv)
>   	ret = EXIT_SUCCESS;
>   
>   	pmu_sample(engines);
> +	codename = igt_device_get_pretty_name(&card, false);
>   
>   	while (!stop_top) {
>   		bool consumed = false;
> @@ -1463,7 +1466,7 @@ int main(int argc, char **argv)
>   			break;
>   
>   		while (!consumed) {
> -			lines = print_header(&card, engines,
> +			lines = print_header(&card, codename, engines,
>   					     t, lines, con_w, con_h,
>   					     &consumed);
>   
> @@ -1490,6 +1493,7 @@ int main(int argc, char **argv)
>   		usleep(period_us);
>   	}
>   
> +	free(codename);
>   err:
>   	free(engines);
>   	free(pmu_device);
> 

Works for me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t v4 3/3] tools/lsgpu: Add -n switch to list devices using vendor:device hex id
  2020-11-18 13:50 ` [igt-dev] [PATCH i-g-t v4 3/3] tools/lsgpu: Add -n switch to list devices using vendor:device hex id Zbigniew Kempczyński
@ 2020-11-18 15:21   ` Tvrtko Ursulin
  0 siblings, 0 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2020-11-18 15:21 UTC (permalink / raw)
  To: Zbigniew Kempczyński, igt-dev; +Cc: Petri Latvala, Tvrtko Ursulin


On 18/11/2020 13:50, Zbigniew Kempczyński wrote:
> Default device list prefers vendor and device names. Add -n switch
> to display vendor/device as hex strings.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> ---
>   tools/lsgpu.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lsgpu.c b/tools/lsgpu.c
> index 169ab0c2..25358bbe 100644
> --- a/tools/lsgpu.c
> +++ b/tools/lsgpu.c
> @@ -72,6 +72,7 @@
>   enum {
>   	OPT_PRINT_SIMPLE   = 's',
>   	OPT_PRINT_DETAIL   = 'p',
> +	OPT_NUMERIC        = 'n',
>   	OPT_LIST_VENDORS   = 'v',
>   	OPT_LIST_FILTERS   = 'l',
>   	OPT_DEVICE         = 'd',
> @@ -86,6 +87,7 @@ static char *igt_device;
>   static const char *usage_str =
>   	"usage: lsgpu [options]\n\n"
>   	"Options:\n"
> +	"  -n, --numeric               Print vendor/device as hex\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"
> @@ -160,6 +162,7 @@ int main(int argc, char *argv[])
>   		{"drm",               no_argument,       NULL, 0},
>   		{"sysfs",             no_argument,       NULL, 1},
>   		{"pci",               no_argument,       NULL, 2},
> +		{"numeric",           no_argument,       NULL, OPT_NUMERIC},
>   		{"print-simple",      no_argument,       NULL, OPT_PRINT_SIMPLE},
>   		{"print-detail",      no_argument,       NULL, OPT_PRINT_DETAIL},
>   		{"list-vendors",      no_argument,       NULL, OPT_LIST_VENDORS},
> @@ -174,10 +177,13 @@ int main(int argc, char *argv[])
>   			.type = IGT_PRINT_USER,
>   	};
>   
> -	while ((c = getopt_long(argc, argv, "spvld:h",
> +	while ((c = getopt_long(argc, argv, "nspvld:h",
>   				long_options, &index)) != -1) {
>   		switch(c) {
>   
> +		case OPT_NUMERIC:
> +			fmt.numeric = true;
> +			break;
>   		case OPT_PRINT_SIMPLE:
>   			fmt.type = IGT_PRINT_SIMPLE;
>   			break;
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,v4,1/3] lib/igt_device_scan: Remember vendor/device for pci devices
  2020-11-18 13:50 [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices Zbigniew Kempczyński
                   ` (2 preceding siblings ...)
  2020-11-18 15:17 ` [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices Tvrtko Ursulin
@ 2020-11-18 15:30 ` Patchwork
  3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2020-11-18 15:30 UTC (permalink / raw)
  To: Zbigniew Kempczyński; +Cc: igt-dev


[-- Attachment #1.1: Type: text/plain, Size: 4660 bytes --]

== Series Details ==

Series: series starting with [i-g-t,v4,1/3] lib/igt_device_scan: Remember vendor/device for pci devices
URL   : https://patchwork.freedesktop.org/series/84023/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9352 -> IGTPW_5186
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_5186 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_5186, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_5186:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@blt:
    - fi-ivb-3770:        [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9352/fi-ivb-3770/igt@i915_selftest@live@blt.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/fi-ivb-3770/igt@i915_selftest@live@blt.html

  
Known issues
------------

  Here are the changes found in IGTPW_5186 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-byt-j1900:       [PASS][3] -> [DMESG-WARN][4] ([i915#1982])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9352/fi-byt-j1900/igt@i915_module_load@reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/fi-byt-j1900/igt@i915_module_load@reload.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       [PASS][5] -> [FAIL][6] ([i915#1161] / [i915#262])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9352/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload:
    - fi-tgl-u2:          [DMESG-WARN][7] ([i915#1982] / [k.org#205379]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9352/fi-tgl-u2/igt@i915_module_load@reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/fi-tgl-u2/igt@i915_module_load@reload.html

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-lmem:        [DMESG-WARN][9] ([i915#2605]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9352/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-byt-j1900:       [DMESG-WARN][11] ([i915#1982]) -> [PASS][12] +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9352/fi-byt-j1900/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/fi-byt-j1900/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - fi-icl-u2:          [DMESG-WARN][13] ([i915#1982]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9352/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1161]: https://gitlab.freedesktop.org/drm/intel/issues/1161
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2605]: https://gitlab.freedesktop.org/drm/intel/issues/2605
  [i915#262]: https://gitlab.freedesktop.org/drm/intel/issues/262
  [k.org#205379]: https://bugzilla.kernel.org/show_bug.cgi?id=205379


Participating hosts (42 -> 39)
------------------------------

  Additional (1): fi-tgl-y 
  Missing    (4): fi-ilk-m540 fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_5857 -> IGTPW_5186

  CI-20190529: 20190529
  CI_DRM_9352: 61c456be035d0c104159134e4efb555ccde6077a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_5186: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/index.html
  IGT_5857: 0e3ec8945fd30fe8e4a38ec3d7acacc0268b225c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5186/index.html

[-- Attachment #1.2: Type: text/html, Size: 5647 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices
  2020-11-18 15:17 ` [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices Tvrtko Ursulin
@ 2020-11-19  7:25   ` Zbigniew Kempczyński
  2020-11-19  9:25     ` Tvrtko Ursulin
  0 siblings, 1 reply; 9+ messages in thread
From: Zbigniew Kempczyński @ 2020-11-19  7:25 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev, Petri Latvala, Tvrtko Ursulin

On Wed, Nov 18, 2020 at 03:17:08PM +0000, Tvrtko Ursulin wrote:
> 
> On 18/11/2020 13:50, Zbigniew Kempczyński wrote:
> > If we want to use pci device id for not opened device we need to
> > keep it in card structure.
> > 
> > Export igt_device_get_pretty_name() function to the caller to
> > allow return pretty name (for lsgpu, intel_gpu_top and others).
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Petri Latvala <petri.latvala@intel.com>
> > ---
> >   lib/igt_device_scan.c | 150 ++++++++++++++++++++++++++++++++++++++----
> >   lib/igt_device_scan.h |   3 +
> >   2 files changed, 140 insertions(+), 13 deletions(-)
> > 
> > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> > index e97f7163..df41ee51 100644
> > --- a/lib/igt_device_scan.c
> > +++ b/lib/igt_device_scan.c
> > @@ -25,7 +25,9 @@
> >   #include "igt_core.h"
> >   #include "igt_device_scan.h"
> >   #include "igt_list.h"
> > +#include "intel_chipset.h"
> >   
> > +#include <ctype.h>
> >   #include <dirent.h>
> >   #include <fcntl.h>
> >   #include <glib.h>
> > @@ -178,12 +180,43 @@ static struct {
> >   	bool devs_scanned;
> >   } igt_devs;
> >   
> > +typedef char *(*devname_fn)(uint16_t, uint16_t);
> > +
> > +static char *devname_hex(uint16_t vendor, uint16_t device)
> > +{
> > +	char *s;
> > +
> > +	igt_assert(asprintf(&s, "%04x:%04x", vendor, device) == 9);
> > +
> > +	return s;
> > +}
> > +
> > +static char *devname_intel(uint16_t vendor, uint16_t device)
> > +{
> > +	const struct intel_device_info *info = intel_get_device_info(device);
> > +	char *devname, *s;
> > +
> > +	if (info->codename) {
> > +		devname = strdup(info->codename);
> > +		devname[0] = toupper(devname[0]);
> 
> Strictly could be a null ptr deref here.

IMO not. We can have info->codename NULL for missing gen, so we enter else path.
Other cases are info->codename lead to valid string and even if it could be ""
toupper does nothing there. 

> 
> > +	} else {
> > +		asprintf(&devname, "%04x:%04x", vendor, device);
> 
> Could call devname_hex, for some questionable value of improvement, up to you.

Ack.

> 
> > +	}
> > +
> > +	asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));
> 
> Is info->gen always valid? Looks like not, we'd gen Gen0 on unknown devices. Weird API, anyway.. Maybe try like this then:
> 
> if (info->codename) {
> 	char *devname = strdup(info->codename);
> 
> 	if (devname) {
> 		devname[0] = toupper(devname[0]);
> 		asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));
> 		free(devname);
> 	}
> }
> 
> if (!devname)
> 	s = devname_hex(vendor, device);
> 
> return s;
> 
> I think that should cover all the cases.

free(devname) then if (!devname) is likely not we want here. But I got
your point we should properly display at least vendor:device.

> 
> > +
> > +	free(devname);
> > +
> > +	return s;
> > +}
> > +
> >   static struct {
> >   	const char *name;
> >   	const char *vendor_id;
> > +	devname_fn devname;
> >   } pci_vendor_mapping[] = {
> > -	{ "intel", "8086" },
> > -	{ "amd", "1002" },
> > +	{ "intel", "8086", devname_intel },
> > +	{ "amd", "1002", devname_hex },
> >   	{ NULL, },
> >   };
> >   
> > @@ -198,6 +231,43 @@ static const char *get_pci_vendor_id_by_name(const char *name)
> >   	return NULL;
> >   }
> >   
> > +static devname_fn get_pci_vendor_device_fn(const char *vendor_id)
> > +{
> > +	for (typeof(*pci_vendor_mapping) *vm = pci_vendor_mapping; vm->name; vm++) {
> > +		if (!strcasecmp(vendor_id, vm->vendor_id))
> > +			return vm->devname;
> > +	}
> > +
> > +	return devname_hex;
> > +}
> > +
> > +static void get_pci_vendor_device(const struct igt_device *dev,
> > +				  uint16_t *vendorp, uint16_t *devicep)
> > +{
> > +	igt_assert(dev && dev->vendor && dev->device);
> > +	igt_assert(vendorp && devicep);
> > +
> > +	igt_assert(sscanf(dev->vendor, "%hx", vendorp) == 1);
> > +	igt_assert(sscanf(dev->device, "%hx", devicep) == 1);
> > +}
> > +
> > +static char *__pci_pretty_name(uint16_t vendor, uint16_t device, bool numeric)
> > +{
> > +	char *devname, vendorstr[5];
> > +	devname_fn fn;
> > +
> > +	if (!numeric) {
> > +		snprintf(vendorstr, sizeof(vendorstr), "%04x", vendor);
> 
> I did not get why this snprintf? Ah.. you need the hex in string format.. a bit awkward. Maybe add u16 representation to struct pci_vendor_mapping for convenience? Up to you. And same might apply to struct igt_device, just to reduce the number of string <-> u16 conversions (igt_devs_print_user ends up doing it both ways). But again more of "up to you".

You're right, better place is get_pci_vendor_device_fn() but I need to
do u16 -> string conversion anyway because I don't want duplicate values.

--
Zbigniew

> 
> > +		fn = get_pci_vendor_device_fn(vendorstr);
> > +	} else {
> > +		fn = devname_hex;
> > +	}
> > +
> > +	devname = fn(vendor, device);
> > +
> > +	return devname;
> > +}
> > +
> >   /* Reading sysattr values can take time (even seconds),
> >    * we want to avoid reading such keys.
> >    */
> > @@ -446,23 +516,44 @@ static bool is_vendor_matched(struct igt_device *dev, const char *vendor)
> >   	return !strcasecmp(dev->vendor, vendor_id);
> >   }
> >   
> > +static char *safe_strncpy(char *dst, const char *src, int n)
> > +{
> > +	char *s;
> > +
> > +	igt_assert(n > 0);
> > +	igt_assert(dst && src);
> > +
> > +	s = strncpy(dst, src, n - 1);
> > +	s[n - 1] = '\0';
> > +
> > +	return s;
> > +}
> > +
> >   static void
> >   __copy_dev_to_card(struct igt_device *dev, struct igt_device_card *card)
> >   {
> >   	if (dev->subsystem != NULL)
> > -		strncpy(card->subsystem, dev->subsystem,
> > -			sizeof(card->subsystem) - 1);
> > +		safe_strncpy(card->subsystem, dev->subsystem,
> > +			     sizeof(card->subsystem));
> >   
> >   	if (dev->drm_card != NULL)
> > -		strncpy(card->card, dev->drm_card, sizeof(card->card) - 1);
> > +		safe_strncpy(card->card, dev->drm_card, sizeof(card->card));
> >   
> >   	if (dev->drm_render != NULL)
> > -		strncpy(card->render, dev->drm_render,
> > -			sizeof(card->render) - 1);
> > +		safe_strncpy(card->render, dev->drm_render,
> > +			     sizeof(card->render));
> >   
> >   	if (dev->pci_slot_name != NULL)
> > -		strncpy(card->pci_slot_name, dev->pci_slot_name,
> > -			PCI_SLOT_NAME_SIZE + 1);
> > +		safe_strncpy(card->pci_slot_name, dev->pci_slot_name,
> > +			     sizeof(card->pci_slot_name));
> > +
> > +	if (dev->vendor != NULL)
> > +		if (sscanf(dev->vendor, "%hx", &card->pci_vendor) != 1)
> > +			card->pci_vendor = 0;
> > +
> > +	if (dev->device != NULL)
> > +		if (sscanf(dev->device, "%hx", &card->pci_device) != 1)
> > +			card->pci_device = 0;
> >   }
> >   
> >   /*
> > @@ -852,6 +943,7 @@ static void __print_filter(char *buf, int len,
> >   	};
> >   }
> >   
> > +#define VENDOR_SIZE 30
> >   static void
> >   igt_devs_print_user(struct igt_list_head *view,
> >   		    const struct igt_devices_print_format *fmt)
> > @@ -883,11 +975,19 @@ igt_devs_print_user(struct igt_list_head *view,
> >   			continue;
> >   
> >   		if (pci_dev) {
> > +			uint16_t vendor, device;
> > +			char *devname;
> > +
> > +			get_pci_vendor_device(pci_dev, &vendor, &device);
> > +			devname = __pci_pretty_name(vendor, device, fmt->numeric);
> > +
> >   			__print_filter(filter, sizeof(filter), fmt, pci_dev,
> >   				       false);
> > -			printf("%-24s%4s:%4s    %s\n",
> > -			       drm_name, pci_dev->vendor, pci_dev->device,
> > -			       filter);
> > +
> > +			printf("%-24s %-*s    %s\n",
> > +			       drm_name, VENDOR_SIZE, devname, filter);
> > +
> > +			free(devname);
> >   		} else {
> >   			__print_filter(filter, sizeof(filter), fmt, dev, false);
> >   			printf("%-24s             %s\n", drm_name, filter);
> > @@ -919,7 +1019,7 @@ igt_devs_print_user(struct igt_list_head *view,
> >   			if (fmt->option != IGT_PRINT_PCI) {
> >   				__print_filter(filter, sizeof(filter), fmt,
> >   					       dev2, true);
> > -				printf("             %s\n", filter);
> > +				printf("%-*s     %s\n", VENDOR_SIZE, "", filter);
> >   			} else {
> >   				printf("\n");
> >   			}
> > @@ -1479,6 +1579,30 @@ bool igt_device_card_match_pci(const char *filter,
> >          return __igt_device_card_match(filter, card, true);
> >   }
> >   
> > +/**
> > + * igt_device_get_pretty_name
> > + * @card: pointer to igt_device_card struct
> > + *
> > + * For card function returns allocated string having pretty name
> > + * or vendor:device as hex if no backend pretty-resolver is implemented.
> > + *
> > + * Returns: newly allocated string.
> > + */
> > +char *igt_device_get_pretty_name(struct igt_device_card *card, bool numeric)
> > +{
> > +	char *devname;
> > +
> > +	igt_assert(card);
> > +
> > +	if (strlen(card->pci_slot_name))
> > +		devname = __pci_pretty_name(card->pci_vendor, card->pci_device,
> > +					    numeric);
> > +	else
> > +		devname = strdup(card->subsystem);
> > +
> > +	return devname;
> > +}
> > +
> >   /**
> >    * igt_open_card:
> >    * @card: pointer to igt_device_card structure
> > diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
> > index bb4be723..5f583a06 100644
> > --- a/lib/igt_device_scan.h
> > +++ b/lib/igt_device_scan.h
> > @@ -49,6 +49,7 @@ enum igt_devices_print_option {
> >   struct igt_devices_print_format {
> >   	enum igt_devices_print_type   type;
> >   	enum igt_devices_print_option option;
> > +	bool numeric;
> >   };
> >   
> >   #define INTEGRATED_I915_GPU_PCI_ID "0000:00:02.0"
> > @@ -58,6 +59,7 @@ struct igt_device_card {
> >   	char card[NAME_MAX];
> >   	char render[NAME_MAX];
> >   	char pci_slot_name[PCI_SLOT_NAME_SIZE+1];
> > +	uint16_t pci_vendor, pci_device;
> >   };
> >   
> >   void igt_devices_scan(bool force);
> > @@ -84,6 +86,7 @@ bool igt_device_card_match_pci(const char *filter,
> >   	struct igt_device_card *card);
> >   bool igt_device_find_first_i915_discrete_card(struct igt_device_card *card);
> >   bool igt_device_find_integrated_card(struct igt_device_card *card);
> > +char *igt_device_get_pretty_name(struct igt_device_card *card, bool numeric);
> >   int igt_open_card(struct igt_device_card *card);
> >   int igt_open_render(struct igt_device_card *card);
> >   
> > 
> Rest looks passable.
> 
> Regards,
> 
> Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices
  2020-11-19  7:25   ` Zbigniew Kempczyński
@ 2020-11-19  9:25     ` Tvrtko Ursulin
  0 siblings, 0 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2020-11-19  9:25 UTC (permalink / raw)
  To: Zbigniew Kempczyński; +Cc: igt-dev, Petri Latvala, Tvrtko Ursulin


On 19/11/2020 07:25, Zbigniew Kempczyński wrote:
> On Wed, Nov 18, 2020 at 03:17:08PM +0000, Tvrtko Ursulin wrote:
>>
>> On 18/11/2020 13:50, Zbigniew Kempczyński wrote:
>>> If we want to use pci device id for not opened device we need to
>>> keep it in card structure.
>>>
>>> Export igt_device_get_pretty_name() function to the caller to
>>> allow return pretty name (for lsgpu, intel_gpu_top and others).
>>>
>>> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Petri Latvala <petri.latvala@intel.com>
>>> ---
>>>    lib/igt_device_scan.c | 150 ++++++++++++++++++++++++++++++++++++++----
>>>    lib/igt_device_scan.h |   3 +
>>>    2 files changed, 140 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>>> index e97f7163..df41ee51 100644
>>> --- a/lib/igt_device_scan.c
>>> +++ b/lib/igt_device_scan.c
>>> @@ -25,7 +25,9 @@
>>>    #include "igt_core.h"
>>>    #include "igt_device_scan.h"
>>>    #include "igt_list.h"
>>> +#include "intel_chipset.h"
>>>    
>>> +#include <ctype.h>
>>>    #include <dirent.h>
>>>    #include <fcntl.h>
>>>    #include <glib.h>
>>> @@ -178,12 +180,43 @@ static struct {
>>>    	bool devs_scanned;
>>>    } igt_devs;
>>>    
>>> +typedef char *(*devname_fn)(uint16_t, uint16_t);
>>> +
>>> +static char *devname_hex(uint16_t vendor, uint16_t device)
>>> +{
>>> +	char *s;
>>> +
>>> +	igt_assert(asprintf(&s, "%04x:%04x", vendor, device) == 9);
>>> +
>>> +	return s;
>>> +}
>>> +
>>> +static char *devname_intel(uint16_t vendor, uint16_t device)
>>> +{
>>> +	const struct intel_device_info *info = intel_get_device_info(device);
>>> +	char *devname, *s;
>>> +
>>> +	if (info->codename) {
>>> +		devname = strdup(info->codename);
>>> +		devname[0] = toupper(devname[0]);
>>
>> Strictly could be a null ptr deref here.
> 
> IMO not. We can have info->codename NULL for missing gen, so we enter else path.
> Other cases are info->codename lead to valid string and even if it could be ""
> toupper does nothing there.

I was referring to strdup returning NULL.

>>
>>> +	} else {
>>> +		asprintf(&devname, "%04x:%04x", vendor, device);
>>
>> Could call devname_hex, for some questionable value of improvement, up to you.
> 
> Ack.
> 
>>
>>> +	}
>>> +
>>> +	asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));
>>
>> Is info->gen always valid? Looks like not, we'd gen Gen0 on unknown devices. Weird API, anyway.. Maybe try like this then:
>>
>> if (info->codename) {
>> 	char *devname = strdup(info->codename);
>>
>> 	if (devname) {
>> 		devname[0] = toupper(devname[0]);
>> 		asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));
>> 		free(devname);
>> 	}
>> }
>>
>> if (!devname)
>> 	s = devname_hex(vendor, device);
>>
>> return s;
>>
>> I think that should cover all the cases.
> 
> free(devname) then if (!devname) is likely not we want here. But I got
> your point we should properly display at least vendor:device.

Did not get what you meant but yes, fallback (even if strdup fails!) and 
possibly simplified flow.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

end of thread, other threads:[~2020-11-19  9:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18 13:50 [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices Zbigniew Kempczyński
2020-11-18 13:50 ` [igt-dev] [PATCH i-g-t v4 2/3] tools/intel_gpu_top: Add generation info in header Zbigniew Kempczyński
2020-11-18 15:20   ` Tvrtko Ursulin
2020-11-18 13:50 ` [igt-dev] [PATCH i-g-t v4 3/3] tools/lsgpu: Add -n switch to list devices using vendor:device hex id Zbigniew Kempczyński
2020-11-18 15:21   ` Tvrtko Ursulin
2020-11-18 15:17 ` [igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices Tvrtko Ursulin
2020-11-19  7:25   ` Zbigniew Kempczyński
2020-11-19  9:25     ` Tvrtko Ursulin
2020-11-18 15:30 ` [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,v4,1/3] " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.