[igt-dev] [RFC i-g-t 3/5] lib/igt_device_scan: Remember PCI card index after scanning

[Tvrtko Ursulin]

On 16/11/2020 16:47, Zbigniew Kempczyński wrote:
> On Mon, Nov 16, 2020 at 03:09:43PM +0000, Tvrtko Ursulin wrote:
> 
> <cut>
>   
>>>> +
>>>>    /* Core scanning function.
>>>>     *
>>>>     * All scanned devices are kept inside igt_devs.all pointer array.
>>>> @@ -657,6 +685,7 @@ static void scan_drm_devices(void)
>>>>    	udev_unref(udev);
>>>>    	sort_all_devices();
>>>> +	index_pci_devices();
>>>>    	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>>>>    		struct igt_device *dev_dup = duplicate_device(dev);
>>>> @@ -1105,13 +1134,13 @@ static struct igt_list_head *filter_pci(const struct filter_class *fcls,
>>>>    		if (filter->data.device && strcasecmp(filter->data.device, dev->device))
>>>>    			continue;
>>>> -		/* We get n-th card */
>>>> -		if (!card) {
>>>> -			struct igt_device *dup = duplicate_device(dev);
>>>> -			igt_list_add_tail(&dup->link, &igt_devs.filtered);
>>>> -			break;
>>>> -		}
>>>> -		card--;
>>>> +		/* Skip if 'card' doesn't match */
>>>> +		if (card != dev->pci_index)
>>>> +			continue;
>>>> +
>>>> +		dev = duplicate_device(dev);
>>>> +		igt_list_add_tail(&dev->link, &igt_devs.filtered);
>>>> +		break;
>>>
>>> I may wrong (I got no such testing env) but devs_compare() function along with
>>> index_pci_devices() can lead us to such (example) situation:
>>>
>>> igt_devs.all contains devices with syspaths and subsystem pci:
>>> /sys/devices/pci0000:00/0000:00:04.0	-> vendor 8086, device 1234	pci_index == 0
>>> /sys/devices/pci0000:00/0000:00:05.0	-> vendor 8086, device 4321	pci_index == 1
>>> /sys/devices/pci0000:00/0000:00:06.0	-> vendor 8086, device 1234	pci_index == 2
>>>
>>> If would try filter: pci=vendor=8086,device=1234,card=1 I would expect
>>> to select (second card [index == 1]) from filtered devices which match
>>> device 1234.
>>
>> It is supposed to be like you say, each pci_index being in the namespace of
>> equal vendor & device only.
>>
>> Like from a dual GPU system:
>>
>> $ tools/lsgpu --pci
>> card1                   8086:4905    pci:vendor=8086,device=4905,card=0
>> └─renderD129
>> card0                   8086:3E98    pci:vendor=8086,device=3E98,card=0
>> └─renderD128
>>
>> Or two identical GPUs (mocked by manual addition of an almost duplicate
>> entry to list of scanned devices:
>>
>> $ tools/lsgpu --pci
>> card0                   8086:193B    pci:vendor=8086,device=193B,card=0
>> ├─renderD128
>> └─renderD128
>> card0x                  8086:193B    pci:vendor=8086,device=193B,card=1
>> ├─renderD128
>> └─renderD128
>>
>> Regards,
>>
>> Tvrtko
> 
> I made a mistake in above listing, it should be:
> /sys/devices/pci0000:00/0000:00:04.0	-> vendor 8086, device 1234	pci_index == 0
> /sys/devices/pci0000:00/0000:00:05.0	-> vendor 8086, device 4321	pci_index == 0
> /sys/devices/pci0000:00/0000:00:06.0	-> vendor 8086, device 1234	pci_index == 1
> 
> What if I would go over all Intel cards using pci filter:
> 
> pci:vendor=8086,card=N   where n is 0... until there's no Intel pci device.
> 
> This way we loose possibility to iterate over all pci devices imo.

Oh I get it know, I misunderstood how this all works. So the card 
argument to the pci filter is nothing about the actual PCI card, but 
just "match number" - match with this filter and return card=N Nth match.

So I should leave the code in filter_pci as is.

Will my new filters work like that. I think so. Ok, will update.

Regards,

Tvrtko