Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection
Hi,
On 2023-01-30 at 11:04:07 +, Tvrtko Ursulin wrote:
>
> On 27/01/2023 16:17, Kamil Konieczny wrote:
> > Hi Tvrtko,
> >
> > On 2023-01-27 at 11:12:41 +, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin
> > >
> > > 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
> > > Cc: Kamil Konieczny
> > > Cc: Zbigniew Kempczyński
> >
> > Please send this as separate patch, not in this series.
>
> Yeah I was lazy and wanting to save time so okay.
>
Well maybe next time, I already merged your series without 5/6,
that one were merged some time ago.
Regards,
Kamil
> > > ---
> > > 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, , );
> > > 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, _devs.all, link) {
> > > - if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
> > > + if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))
> >
> > Put the comment here why it can be problematic to relay on driver name.
>
> Function name being __find_first_*i915*_card is IMO enough so it feels any
> comment to the same effect would be redundant.
>
> Hm if anything igt_device_find_integrated_card should be renamed..
>
> Regards,
>
> Tvrtko
>
> >
> > Regards,
> > Kamil
> >
> > > continue;
> > > cmp = strncmp(dev->pci_slot_name,
> > > INTEGRATED_I915_GPU_PCI_ID,
> > > @@ -1023,6 +1025,7 @@ static void igt_device_free(struct igt_device *dev)
> > > free(dev->drm_render);
> > > free(dev->vendor);
> > > free(dev->device);
> > > + free(dev->driver);
> > > free(dev->pci_slot_name);
> > > g_hash_table_destroy(dev->attrs_ht);
> > > g_hash_table_destroy(dev->props_ht);
> > > --
> > > 2.34.1
> > >
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection
On 27/01/2023 16:17, Kamil Konieczny wrote:
Hi Tvrtko,
On 2023-01-27 at 11:12:41 +, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
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
Cc: Kamil Konieczny
Cc: Zbigniew Kempczyński
Please send this as separate patch, not in this series.
Yeah I was lazy and wanting to save time so okay.
---
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, , );
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, _devs.all, link) {
- if (!is_pci_subsystem(dev) || !is_vendor_matched(dev, "intel"))
+ if (!is_pci_subsystem(dev) || strcmp(dev->driver, "i915"))
Put the comment here why it can be problematic to relay on driver name.
Function name being __find_first_*i915*_card is IMO enough so it feels
any comment to the same effect would be redundant.
Hm if anything igt_device_find_integrated_card should be renamed..
Regards,
Tvrtko
Regards,
Kamil
continue;
cmp = strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
@@ -1023,6 +1025,7 @@ static void igt_device_free(struct igt_device *dev)
free(dev->drm_render);
free(dev->vendor);
free(dev->device);
+ free(dev->driver);
free(dev->pci_slot_name);
g_hash_table_destroy(dev->attrs_ht);
g_hash_table_destroy(dev->props_ht);
--
2.34.1
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection
On Fri, Jan 27, 2023 at 11:53:38AM +, Tvrtko Ursulin wrote:
>
> On 27/01/2023 11:39, Petri Latvala wrote:
> > On Fri, Jan 27, 2023 at 11:12:41AM +, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin
> > >
> > > 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
> > > Cc: Kamil Konieczny
> > > Cc: Zbigniew Kempczyński
> > > ---
> > > 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, , );
> > > 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, _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
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection
On 27/01/2023 11:39, Petri Latvala wrote:
On Fri, Jan 27, 2023 at 11:12:41AM +, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
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
Cc: Kamil Konieczny
Cc: Zbigniew Kempczyński
---
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, , );
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, _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?
Regards,
Tvrtko
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/6] lib/igt_device_scan: Improve Intel discrete GPU selection
On Fri, Jan 27, 2023 at 11:12:41AM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> 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
> Cc: Kamil Konieczny
> Cc: Zbigniew Kempczyński
> ---
> 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, , );
> 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, _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?
--
Petri Latvala
> continue;
>
> cmp = strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
> @@ -1023,6 +1025,7 @@ static void igt_device_free(struct igt_device *dev)
> free(dev->drm_render);
> free(dev->vendor);
> free(dev->device);
> + free(dev->driver);
> free(dev->pci_slot_name);
> g_hash_table_destroy(dev->attrs_ht);
> g_hash_table_destroy(dev->props_ht);
> --
> 2.34.1
>
