* [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.