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

[Tvrtko Ursulin]

On 16/11/2020 14:03, Zbigniew Kempczyński wrote:
> On Fri, Nov 13, 2020 at 04:27:34PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> After devices are sorted post scanning, assing a card index to each so it
>> can be easily accessed if PCI filter for a card needs to be printed out.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Petri Latvala <petri.latvala at intel.com>
>> Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
>> ---
>>   lib/igt_device_scan.c | 43 ++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>> index c0cd6757fc27..72653403f705 100644
>> --- a/lib/igt_device_scan.c
>> +++ b/lib/igt_device_scan.c
>> @@ -166,6 +166,7 @@ struct igt_device {
>>   	char *vendor;
>>   	char *device;
>>   	char *pci_slot_name;
>> +	int pci_index;
>>   
>>   	struct igt_list_head link;
>>   };
>> @@ -601,6 +602,33 @@ static void sort_all_devices(void)
>>   	free(devs);
>>   }
>>   
>> +static void index_pci_devices(void)
>> +{
>> +	struct igt_device *dev;
>> +
>> +	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>> +		struct igt_device *dev2;
>> +		int index = 0;
>> +
>> +		if (!is_pci_subsystem(dev))
>> +			continue;
>> +
>> +		igt_list_for_each_entry(dev2, &igt_devs.all, link) {
>> +			if (!is_pci_subsystem(dev2))
>> +				continue;
>> +
>> +			if (dev2 == dev)
>> +				break;
>> +
>> +			if (!strcasecmp(dev->vendor, dev2->vendor) &&
>> +			    !strcasecmp(dev->device, dev2->device))
>> +				index++;
>> +		}
>> +
>> +		dev->pci_index = index;
>> +	}
>> +}
>> +
>>   /* 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