[PATCH <i-g-t> v5 1/4] lib/igt_device_scan: Enable finding all IGT devices for a specific driver

[Zbigniew Kempczyński]

On Wed, Mar 26, 2025 at 12:42:27AM +0530, Soham Purkait wrote:
> Add filter functions to find all the
> available GPUs or few among them
> by driver name and card type or card number.
> Add driver field to igt_device_card structure 
> for storing driver names.
> 
> v2 : fix for refactoring GPUTOP into a
>      vendor-agnostic tool (Lucas)
> 
> v3 : Separate commit for lib (Kamil)
> 
> v4 : Refactor to use composition strategy
>      for driver and device type filtering
>      Refactor code to improve memory
>      allocation and error handling (Lucas)
> 
> v5 : Rename function name as per convention.
>      Use "dev_type" enum for card_type. (Krzysztof)
>      Add new filter to return collection
>      of matching devices.               (Zbigniew)
> 
> Signed-off-by: Soham Purkait <soham.purkait at intel.com>
> ---
>  lib/igt_device_scan.c | 177 ++++++++++++++++++++++++++++++++++++++++--
>  lib/igt_device_scan.h |  12 +++
>  2 files changed, 183 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index 711bedc5c..452da2ee4 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -198,12 +198,6 @@
>  #define DBG(...) {}
>  #endif
>  
> -enum dev_type {
> -	DEVTYPE_ALL,
> -	DEVTYPE_INTEGRATED,
> -	DEVTYPE_DISCRETE,
> -};
> -

I think you don't need to extern this dev_type.

>  #define STR_INTEGRATED "integrated"
>  #define STR_DISCRETE "discrete"
>  
> @@ -774,6 +768,10 @@ __copy_dev_to_card(struct igt_device *dev, struct igt_device_card *card)
>  		safe_strncpy(card->render, dev->drm_render,
>  			     sizeof(card->render));
>  
> +	if (dev->driver != NULL)
> +		safe_strncpy(card->driver, dev->driver,
> +			     sizeof(card->driver));
> +
>  	if (dev->pci_slot_name != NULL)
>  		safe_strncpy(card->pci_slot_name, dev->pci_slot_name,
>  			     sizeof(card->pci_slot_name));
> @@ -820,6 +818,58 @@ static bool __find_first_intel_card_by_driver_name(struct igt_device_card *card,
>  	return false;
>  }
>  
> +/**
> + * Iterate over all igt_devices array and find all discrete/integrated cards.

This sentence is misleading, otherwise what for is card_type argument?

> + * @card: double pointer to igt_device_card structure, containing
> + * an array of igt_device_card structures upon successful return.
> + * @card_type: flag of type "enum igt_devices_card_type" to indicate
> + * whether to find discrete, integrated, or both types of cards.
> + * @drv_name: name of the driver to match.
> + *
> + * Returns the number of cards found, or -1 on error.

You should add some note about freeing the memory allocated for the
returned array.

> + */
> +int igt_device_find_all_intel_card_by_driver_name(struct igt_device_card **card,
> +						  enum dev_type card_type,
> +						  const char *drv_name)
> +{
> +	int count = 0;
> +	struct igt_device *dev;
> +	int is_integrated;
> +	struct igt_device_card *tmp;
> +	struct igt_device_card *crd = NULL;
> +
> +	igt_assert(drv_name);
> +	*card = NULL;
> +
> +	igt_list_for_each_entry(dev, &igt_devs.all, link) {
> +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, drv_name))
> +			continue;
> +
> +		is_integrated = !strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
> +						PCI_SLOT_NAME_SIZE);
> +
> +		if ((card_type == DEVTYPE_INTEGRATED && !is_integrated) ||
> +		    (card_type == DEVTYPE_DISCRETE && is_integrated) ||
> +		    card_type == DEVTYPE_ALL) {

Shouldn't this be:

if ((card_type == DEVTYPE_INTEGRATED && is_integrated) ||
    (card_type == DEVTYPE_DISCRETE && !is_integrated) ||
    card_type == DEVTYPE_ALL) {

?

Do you really need this function? I think according to device filter you
could use:

igt_device_card_match_all("device:driver=intel", &cards);
igt_device_card_match_all("device:driver=intel,device=integrated", &cards);
igt_device_card_match_all("device:driver=intel,device=discrete", &cards);
igt_device_card_match_all("device:driver=xe", &cards);
igt_device_card_match_all("device:driver=i915", &cards);

(see explanation below)

> +			tmp = realloc(crd, sizeof(struct igt_device_card) * (count + 1));
> +			if (!tmp && crd) {
> +				free(crd);
> +				return -1;
> +			}
> +
> +			crd = tmp;
> +			__copy_dev_to_card(dev, &crd[count]);
> +			count++;
> +		}
> +	}
> +
> +	if (!count)
> +		return 0;
> +
> +	*card = crd;
> +	return count;
> +}
> +
>  bool igt_device_find_first_i915_discrete_card(struct igt_device_card *card)
>  {
>  	igt_assert(card);
> @@ -1705,6 +1755,55 @@ static struct igt_list_head *filter_sriov(const struct filter_class *fcls,
>  	return &igt_devs.filtered;
>  }
>  
> +/*
> + * Find appropriate gpu device matching driver/card filter arguments.
> + */
> +static struct igt_list_head *filter_device(const struct filter_class *fcls,
> +					   const struct filter *filter)
> +{
> +	struct igt_device *dev;
> +	int card = -1;
> +	(void)fcls;
> +
> +	DBG("filter device\n");
> +
> +	if (filter->data.card) {
> +		sscanf(filter->data.card, "%d", &card);
> +		if (card < 0)
> +			return &igt_devs.filtered;
> +	} else {
> +		card = -1;
> +	}
> +
> +	igt_list_for_each_entry(dev, &igt_devs.all, link) {
> +		if (!is_pci_subsystem(dev))
> +			continue;

Why there's limitation to pci devices only?

> +
> +		/* Skip if 'driver' doesn't match */
> +		if (filter->data.driver && !strequal(filter->data.driver, dev->driver))
> +			continue;

Use may introduce 'driver=intel' which would match i915 and xe. See
is_device_matched() function.

> +
> +		/* We get n-th card */
> +		if (!card) {
> +			struct igt_device *dup = duplicate_device(dev);
> +
> +			igt_list_add_tail(&dup->link, &igt_devs.filtered);
> +			break;
> +		}
> +		/* Include all the card */
> +		else if (card == -1) {
> +			struct igt_device *dup = duplicate_device(dev);
> +
> +			igt_list_add_tail(&dup->link, &igt_devs.filtered);
> +		} else
> +			card--;
> +	}
> +
> +	DBG("Filter device filtered size: %d\n", igt_list_length(&igt_devs.filtered));
> +
> +	return &igt_devs.filtered;
> +}

I wondered to alter a little bit pci filter, which currently limits
filtered view to have single element, but that's not a problem to
change it and keep matching card first in this view (I mean using
igt_list_add() instead igt_list_add_tail().

> +
>  static bool sys_path_valid(const struct filter_class *fcls,
>  			   const struct filter *filter)
>  {
> @@ -1746,6 +1845,12 @@ static struct filter_class filter_definition_list[] = {
>  		.help = "sriov:[vendor=%04x/name][,device=%04x][,card=%d][,pf=%d][,vf=%d]",
>  		.detail = "find pf or vf\n",
>  	},
> +	{
> +		.name = "device",
> +		.filter_function = filter_device,
> +		.help = "device:[driver=name][,card=%d]",
> +		.detail = "find device by driver name and card number\n",
> +	},
>  	{
>  		.name = NULL,
>  	},
> @@ -2059,6 +2164,66 @@ bool igt_device_card_match_pci(const char *filter,
>         return __igt_device_card_match(filter, card, true);
>  }
>  
> +/**
> + * igt_device_card_match_all
> + * @filter: filter string
> + * @card: double pointer to igt_device_card structure, containing
> + * an array of igt_device_card structures upon successful return.
> + * @request_pci_ss: a boolean parameter determines whether to
> + * consider PCI subsystem information during this process.
> + * Function applies filter to match device from device array.

Describe what drivers[] array is for. But according to my comment
above consider to have meta driver 'intel' for both drivers.

> + *
> + * Returns: the number of cards found.
> + */
> +int igt_device_card_match_all(const char *filter, struct igt_device_card **card,
> +			      bool request_pci_ss, const char * const drivers[])
> +{
> +	struct igt_device *dev = NULL;
> +	struct igt_device_card *crd = NULL;
> +	struct igt_device_card *tmp;
> +	int count = 0;
> +
> +	/*
> +	 * Scan devices in case the user hasn't yet,
> +	 * but leave a decision on forced rescan on the user side.
> +	 */

It seems this comment is not relevant anymore.

> +	igt_devices_scan();
> +
> +	if (igt_device_filter_apply(filter) == false)
> +		return 0;
> +
> +	if (igt_list_empty(&igt_devs.filtered))
> +		return 0;
> +
> +	igt_list_for_each_entry(dev, &igt_devs.filtered, link) {

Having device= and driver= you could just simply iterate over
this list and build igt_device_card array without any additional
logic here.

Resume and some other nit:

Divide this patch to at least two:
  a) in first introduce device_filter
  b) in second add igt_device_card_match_all()

--
Zbigniew

> +		for (int i = 0;
> +		     drivers && drivers[i] && !strcmp(drivers[i],
> +		     dev->driver); i++) {
> +			tmp = realloc(crd, sizeof(struct igt_device_card) * (count + 1));
> +			if (!tmp && crd) {
> +				free(crd);
> +				return 0;
> +			}
> +
> +			crd = tmp;
> +
> +			if (request_pci_ss && !is_pci_subsystem(dev) &&
> +			    dev->parent && is_pci_subsystem(dev->parent))
> +				__copy_dev_to_card(dev->parent, crd);
> +			else
> +				__copy_dev_to_card(dev, crd);
> +			count++;
> +			break;
> +		}
> +	}
> +
> +	if (!count)
> +		return 0;
> +
> +	*card = crd;
> +	return count;
> +}
> +
>  /**
>   * igt_device_get_pretty_name
>   * @card: pointer to igt_device_card struct
> diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
> index 92741fe3c..62e7a1737 100644
> --- a/lib/igt_device_scan.h
> +++ b/lib/igt_device_scan.h
> @@ -59,10 +59,17 @@ struct igt_device_card {
>  	char subsystem[NAME_MAX];
>  	char card[NAME_MAX];
>  	char render[NAME_MAX];
> +	char driver[NAME_MAX];
>  	char pci_slot_name[PCI_SLOT_NAME_SIZE+1];
>  	uint16_t pci_vendor, pci_device;
>  };
>  
> +enum dev_type {
> +	DEVTYPE_ALL,
> +	DEVTYPE_INTEGRATED,
> +	DEVTYPE_DISCRETE,
> +};
> +
>  void igt_devices_scan(void);
>  void igt_devices_scan_all_attrs(void);
>  
> @@ -88,10 +95,15 @@ int igt_device_filter_pci(void);
>  bool igt_device_card_match(const char *filter, struct igt_device_card *card);
>  bool igt_device_card_match_pci(const char *filter,
>  	struct igt_device_card *card);
> +int igt_device_card_match_all(const char *filter, struct igt_device_card **card,
> +			      bool request_pci_ss, const char * const drivers[]);
>  bool igt_device_find_first_i915_discrete_card(struct igt_device_card *card);
>  bool igt_device_find_integrated_card(struct igt_device_card *card);
>  bool igt_device_find_first_xe_discrete_card(struct igt_device_card *card);
>  bool igt_device_find_xe_integrated_card(struct igt_device_card *card);
> +int igt_device_find_all_intel_card_by_driver_name(struct igt_device_card **card,
> +						  enum dev_type card_type,
> +						  const char *drv_name);
>  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.34.1
>