[igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection

[Petri Latvala]

On Fri, Jan 27, 2023 at 11:53:38AM +0000, Tvrtko Ursulin wrote:
> 
> On 27/01/2023 11:39, Petri Latvala wrote:
> > On Fri, Jan 27, 2023 at 11:12:41AM +0000, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > 
> > > Now that DRM subsystem can contain PCI cards with the vendor set to Intel
> > > but they are not Intel GPUs, we need a better selection logic than looking
> > > at the vendor. Use the driver name instead.
> > > 
> > > Caveat that the driver key was on a blacklist so far, and although I can't
> > > imagine it can be slow to probe, this is something to double check.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > ---
> > >   lib/igt_device_scan.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> > > index ed128d24dd10..8b767eed202d 100644
> > > --- a/lib/igt_device_scan.c
> > > +++ b/lib/igt_device_scan.c
> > > @@ -237,6 +237,7 @@ struct igt_device {
> > >   	char *vendor;
> > >   	char *device;
> > >   	char *pci_slot_name;
> > > +	char *driver;
> > >   	int gpu_index; /* For more than one GPU with same vendor and device. */
> > >   	char *codename; /* For grouping by codename */
> > > @@ -440,7 +441,6 @@ static bool is_on_blacklist(const char *what)
> > >   				      "resource3", "resource4", "resource5",
> > >   				      "resource0_wc", "resource1_wc", "resource2_wc",
> > >   				      "resource3_wc", "resource4_wc", "resource5_wc",
> > > -				      "driver",
> > >   				      "uevent", NULL};
> > >   	const char *key;
> > >   	int i = 0;
> > > @@ -662,6 +662,8 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
> > >   		get_pci_vendor_device(idev, &vendor, &device);
> > >   		idev->codename = __pci_codename(vendor, device);
> > >   		idev->dev_type = __pci_devtype(vendor, device, idev->pci_slot_name);
> > > +		idev->driver = strdup_nullsafe(get_attr(idev, "driver"));
> > > +		igt_assert(idev->driver);
> > >   	}
> > >   	return idev;
> > > @@ -776,7 +778,7 @@ static bool __find_first_i915_card(struct igt_device_card *card, bool discrete)
> > >   	igt_list_for_each_entry(dev, &igt_devs.all, link) {
> > > -		if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
> > > +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))
> > 
> > Is this the time to prepare for that other driver as well?
> 
> Ha, didn't think of that TBH, but AFAICT this patch will work for that case
> too, no?

Ah, now that I read the surrounding code...

Indeed the function is for finding i915 devices in particular, used by
gem_wsim and intel_gpu_top. Having that function find devices driven
by xe might lead to incompatibilities that need to be resolved or at
least compatibility fully understood, which is not the case for either
at this time I assume. In other words, out of scope for this patch.


-- 
Petri Latvala