Hi Zbigniew,

On 28-03-2025 13:36, Zbigniew Kempczyński wrote:
> 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@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.

As the function (igt_device_find_all_intel_card_by_driver_name) need to 
call with the device type as argument from gputop.c so it has been made 
extern to access enum dev_type from outside.

>
>>   #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?
Once the array of cards of type struct igt_device_card are prepared and 
populated,
it is matched with the respective supported drivers . Seems like this 
field is not present in case of non PCI  devices.
>> +
>> +		/* 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.
Via parameter drivers[] array, this function checks if devices of a few 
specific driver is found whose implantation is available in GPUTOP.
Even if driver 'intel' as a group is implemented for filtering, manually 
the cards/ devices need to check against all supported intel drivers (in 
GPUTOP).
>
>> + *
>> + * 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.
As already mentioned, the drivers of the respective devices need to 
check against the supported drivers[] array in the GPUTOP
to build igt_device_card array. Regards, Soham
>
> 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
>>