Re: [PATCH i-g-t v9 1/5] lib/igt_device_scan: Add support for the device filter

["Purkait, Soham" <soham.purkait@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4699 bytes --]


On 23-04-2025 12:01, Zbigniew Kempczyński wrote:
> On Tue, Apr 22, 2025 at 10:41:04PM +0530, Soham Purkait wrote:
>> Add support for the device filter based on
>> driver string, device type (integrated or discrete)
>> and card number.
>>
>> v5 : Add device filter to filter out
>>       matching devices.           (Zbigniew)
>>
>> v6 : Move device filter with Separate
>>       commit.                     (Zbigniew)
>>
>> v7 : Fix interpretation of card numbering
>>       and add 'all' option for all the cards.
>>                                   (Zbigniew)
>>
>> v8 : Fix for card filter output. (Zbigniew)
>>
>> v9 : Render node enabled with 'device:' option
>>       through iterating only pci subsystem
>>       devices in filter.          (Zbigniew)
>>
>> Signed-off-by: Soham Purkait<soham.purkait@intel.com>
>> ---
>>   lib/igt_device_scan.c | 74 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 74 insertions(+)
>>
>> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>> index 3f26a1737..8859b9489 100644
>> --- a/lib/igt_device_scan.c
>> +++ b/lib/igt_device_scan.c
>> @@ -1709,6 +1709,74 @@ static struct igt_list_head *filter_sriov(const struct filter_class *fcls,
>>   	return &igt_devs.filtered;
>>   }
>>   
>> +/*
>> + * Find appropriate gpu device through matching driver, device type and
>> + * card filter arguments.
>> + */
>> +static struct igt_list_head *filter_device(const struct filter_class *fcls,
>> +					   const struct filter *filter)
>> +{
>> +	struct igt_device *dev;
>> +	bool allcards = false;
>> +	int card = 0;
>> +	(void)fcls;
>> +
>> +	DBG("filter device\n");
>> +	if (filter->data.card) {
>> +		char crdop[5] = {0};
>> +
>> +		if (sscanf(filter->data.card, "%d", &card) == 1) {
>> +			if (card < 0)
>> +				return &igt_devs.filtered;
>> +		} else {
>> +			card = 0;
>> +			if (sscanf(filter->data.card, "%4s", crdop) == 1) {
>> +				if (!strcmp(crdop, "all"))
>> +					allcards = true;
>> +				else
>> +					return &igt_devs.filtered;
>> +			} else {
>> +				return &igt_devs.filtered;
>> +			}
>> +		}
>> +	} else {
>> +		card = 0;
>> +	}
>> +
>> +	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>> +		if (!is_pci_subsystem(dev))
>> +			continue;
> For 'device' filter this condition should be removed. You contain
> 'subsystem' field in igt_device_card so you may skip cards which
> are not pci on populate_devices().
As far as GPUTOP is concern this could be managed through various means 
as previously it was being handled with in igt_device_card_match_all()
through 'request_pci_ss' flag parameter or through comparing 'subsystem' 
field in populate_devices(), as the case may be.

But if this is removed and then this filter is used with lsgpu, it may 
not open render node if any of the filtered device is not the part of 
pci subsystem.

that is :
lsgpu -d device:
(doesn't open render node).

Regards,
Soham

>
>> +
>> +		/* Skip if 'driver' doesn't match */
>> +		if (filter->data.driver && !strequal(filter->data.driver, dev->driver))
>> +			continue;
>> +
>> +		/* Skip if 'device' doesn't match */
>> +		if (filter->data.device && !is_device_matched(dev, filter->data.device))
>> +			continue;
>> +
>> +		/* We get n-th card */
>> +		if (!allcards && !card) {
>> +			struct igt_device *dup = duplicate_device(dev);
>> +
>> +			igt_list_add_tail(&dup->link, &igt_devs.filtered);
>> +			break;
>> +		} else if (!allcards) {
>> +			card--;
>> +		}
>> +		/* Include all the cards */
>> +		else if (allcards) {
>> +			struct igt_device *dup = duplicate_device(dev);
>> +
>> +			igt_list_add(&dup->link, &igt_devs.filtered);
>> +		}
>> +	}
>> +
>> +	DBG("Filter device filtered size: %d\n", igt_list_length(&igt_devs.filtered));
>> +
>> +	return &igt_devs.filtered;
>> +}
>> +
>>   static bool sys_path_valid(const struct filter_class *fcls,
>>   			   const struct filter *filter)
>>   {
>> @@ -1750,6 +1818,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][,device=type][,card=%d|all]",
> Or you may add 'bus=pci' in 'device:[driver=name][,bus=all(default)|pci|nonpci][,device=type][,card=all]'
>
> and then use filter in gputop.c:
>
> device:bus=pci,card=all
>
> Another alternative is to fix 'pci' filter to collect all devices
> like 'device' filter is doing now.
>
> --
> Zbigniew
>
>> +		.detail = "find device by driver name, device type and card number\n",
>> +	},
>>   	{
>>   		.name = NULL,
>>   	},
>> -- 
>> 2.34.1
>>

[-- Attachment #2: Type: text/html, Size: 5855 bytes --]