[igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices

Tvrtko Ursulin tvrtko.ursulin at linux.intel.com
Wed Nov 18 15:17:08 UTC 2020


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 at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Petri Latvala <petri.latvala at 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


More information about the igt-dev mailing list