[igt-dev] [PATCH i-g-t v3 1/3] lib/igt_device_scan: Remember vendor/device for pci devices
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Wed Nov 18 12:10:45 UTC 2020
On Wed, Nov 18, 2020 at 09:56:43AM +0000, Tvrtko Ursulin wrote:
>
> On 17/11/2020 16:34, Zbigniew Kempczyński wrote:
> > If we want to use pci device id for not opened device we need to
> > keep it in card structure.
> >
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Petri Latvala <petri.latvala at intel.com>
> > ---
> > lib/igt_device_scan.c | 88 +++++++++++++++++++++++++++++++++++++++----
> > lib/igt_device_scan.h | 2 +
> > 2 files changed, 82 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> > index e97f7163..6c8ab8b1 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,39 @@ 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);
>
> -1 or 9 are the only possible returns here, okay.
>
> > +
> > + return s;
> > +}
> > +
> > +static char *devname_intel(uint16_t vendor, uint16_t device)
>
> Plan export so later patch to intel_gpu_top does not need to re-implement
> it?
Hmm, didn't plan it before but is most reasonable option.
>
> > +{
> > + const struct intel_device_info *info = intel_get_device_info(device);
> > + char *devname, *s;
> > +
> > + (void) vendor;
> > +
> > + devname = info->codename ? strdup(info->codename) : strdup("unknown");
>
> 1) Leaks devname?
> 2) I think hex value would be better if unknown.
Yes, missed that.
>
> > + devname[0] = toupper(devname[0]);
> > +
> > + asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));
> > +
> > + 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 +227,26 @@ 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);
> > +}
> > +
> > /* Reading sysattr values can take time (even seconds),
> > * we want to avoid reading such keys.
> > */
> > @@ -460,9 +509,19 @@ __copy_dev_to_card(struct igt_device *dev, struct igt_device_card *card)
> > strncpy(card->render, dev->drm_render,
> > sizeof(card->render) - 1);
> > - if (dev->pci_slot_name != NULL)
> > + if (dev->pci_slot_name != NULL) {
> > strncpy(card->pci_slot_name, dev->pci_slot_name,
> > - PCI_SLOT_NAME_SIZE + 1);
> > + PCI_SLOT_NAME_SIZE);
> > + card->pci_slot_name[PCI_SLOT_NAME_SIZE] = '\0';
>
> I would still prefer the same "sizeof - 1" pattern as the other copies in
> this function.
>
> Then indeed ensuring strings are null terminated is something which could be
> added to all. Possibly a local wrapper on strncpy?
>
> PCI slot length is probably the most defined of the bunch so it feels wrong
> to have safest handling compared to other strings.
Ok, I'm going to add safe_strncpy() local wrapper for this situation.
>
> > + }
> > +
> > + if (dev->vendor != NULL && strlen(dev->vendor) == 4)
>
> Can strlen be not four here in some situations or this could be an assert
> just as well?
You're right, we parse PCI_ID so we're sure vendor/device strings
have length 4.
>
> > + if (sscanf(dev->vendor, "%hx", &card->pci_vendor) != 1)
> > + card->pci_vendor = 0;
> > +
> > + if (dev->device != NULL && strlen(dev->device) == 4)
> > + if (sscanf(dev->device, "%hx", &card->pci_device) != 1)
> > + card->pci_device = 0;
> > }
> > /*
> > @@ -852,6 +911,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 +943,23 @@ igt_devs_print_user(struct igt_list_head *view,
> > continue;
> > if (pci_dev) {
> > + uint16_t vendor, device;
> > + char *devname;
> > + devname_fn fn = devname_hex;
> > +
> > + if (!fmt->numeric)
> > + fn = get_pci_vendor_device_fn(pci_dev->vendor);
>
> Nit - maybe I would assing devname_hex so the flow of fn assignment is
> totally consolidated and obvious.
Ok.
>
> > +
> > + get_pci_vendor_device(pci_dev, &vendor, &device);
> > + devname = fn(vendor, device);
> > +
> > __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 +991,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");
> > }
> > diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
> > index bb4be723..42066eeb 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);
> >
>
> Regards,
>
> Tvrtko
More information about the igt-dev
mailing list