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