[PATCH i-g-t v7 2/5] lib/igt_device_scan: Enable finding all matched IGT devices

[Zbigniew Kempczyński]

On Wed, Apr 16, 2025 at 04:28:48PM +0530, Soham Purkait wrote:
> Use filter to find all the available
> GPUs or few among them by driver name
> and card type or card number.
> 
> 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 : Introduce device card match function
>      to return collection of matching
>      devices using device filter.
> 
> v6 : Separate commit for device card match
>      function.                     (Zbigniew)
>      Function description modification for device
>      card match function.          (Zbigniew)
> 
> v7 : Single return for card match function.
>                                   (Krzysztof)
> 
> Signed-off-by: Soham Purkait <soham.purkait at intel.com>
> ---
>  lib/igt_device_scan.c | 61 +++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_device_scan.h |  2 ++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index 1613fc3ed..67a00f0ed 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -2133,6 +2133,67 @@ 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.
> + * @drivers: this holds the list of supported driver string by
> + * the caller.
> + * Function applies filter to match device from device array.
> + *
> + * Returns: the number of cards found.
> + *
> + * Note: The caller is responsible for freeing the memory which is
> + * dynamically allocated for the array of igt_device_card structures
> + * upon successful return.
> + */
> +int igt_device_card_match_all(const char *filter, struct igt_device_card **card,
> +			      bool request_pci_ss, const char * const drivers[])

I've mixed feelings about 'drivers' array. Filter should be enough to
limit the search to a specific driver. Here filter search is additionally
limited to 'drivers' array. I mean apart of device filter 'driver' field
you're limiting the search to drivers from the array. Avoiding calling
igt_devices_scan() may be the reason, but I don't like building double
filtering logic where first is in the device filter and second is in the
igt_device_card_match_all().

Moreover - logic in gputop main() iterates over drivers as well, I mean:

gputop.c:main()
	...
	for (int i = 0; drivers[i]; i++)
		populate_device_instances(NULL, i);
	...


then in populate_device_instances() you call igt_device_card_match_all()
preparing filter string with driver name:

	asprintf(&fltr, "device:driver=%s,card=all", drivers[i]);
	count = igt_device_card_match_all(fltr, &cards, true, drivers);

and finally in igt_device_card_match_all() you're iterating again over
'drivers' to match the driver again, even if filter already did it.

I think you should:

1. if user didn't pass device filter use:
	'device:'
   collecting all available devices, then filter out according to your
   drivers list in gputop.c populate_device_instances().

2. if user passed the filter, do the same as above, filtering out in
   populate_device_instances() as well.

If I'm not wrong both cases may be handled within single code, without
any conditionals.

--
Zbigniew


> +{
> +	struct igt_device *dev = NULL;
> +	struct igt_device_card *crd = NULL;
> +	struct igt_device_card *tmp;
> +	int count = 0;
> +
> +	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) {
> +		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)
> +		*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 f1cd3b1e9..c7ddccc57 100644
> --- a/lib/igt_device_scan.h
> +++ b/lib/igt_device_scan.h
> @@ -89,6 +89,8 @@ 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);
> -- 
> 2.34.1
>