[PATCH i-g-t v3 1/4] Enable finding all IGT devices for xe driver

[Lucas De Marchi]

On Fri, Feb 28, 2025 at 07:48:07PM +0530, Soham Purkait wrote:
>v2 : fix for refactoring GPUTOP into a
>     vendor-agnostic tool (Lucas)

Sorry, but I think the abstraction here is not there yet. I'm commenting
more below.

>
>v3 : Separate commit for lib (Kamil)
>
>---
> lib/igt_device_scan.c | 97 +++++++++++++++++++++++++++++++++++++++++++
> lib/igt_device_scan.h |  3 ++
> 2 files changed, 100 insertions(+)
>
>diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>index 711bedc5c..bdbe09761 100644
>--- a/lib/igt_device_scan.c
>+++ b/lib/igt_device_scan.c
>@@ -774,6 +774,10 @@ __copy_dev_to_card(struct igt_device *dev, struct igt_device_card *card)
> 		safe_strncpy(card->render, dev->drm_render,
> 			     sizeof(card->render));
>
>+	if (dev->driver != NULL)
>+		safe_strncpy(card->driver, dev->driver,
>+			     sizeof(card->driver));
>+
> 	if (dev->pci_slot_name != NULL)
> 		safe_strncpy(card->pci_slot_name, dev->pci_slot_name,
> 			     sizeof(card->pci_slot_name));
>@@ -820,6 +824,99 @@ static bool __find_first_intel_card_by_driver_name(struct igt_device_card *card,
> 	return false;
> }
>
>+/*
>+ * Iterate over all igt_devices array and find all discrete/integrated card.
>+ * @card: double pointer to igt_device_card structure, containing
>+ * an array of igt_device_card structure upon successful return.
>+ */
>+static int __find_all_intel_card_by_driver_name(struct igt_device_card **card,
>+						bool want_discrete, const char *drv_name)
>+{
>+	int count = 0;
>+	struct igt_device *dev;
>+	int is_integrated;
>+	struct igt_device_card *tmp;
>+	struct igt_device_card *crd =
>+		(struct igt_device_card *)calloc(1, sizeof(struct igt_device_card));
>+
>+	igt_assert(drv_name);
>+	memset(card, 0, sizeof(*card));
>+
>+	igt_list_for_each_entry(dev, &igt_devs.all, link) {
>+		if (!is_pci_subsystem(dev) || strcmp(dev->driver, drv_name))
>+			continue;
>+
>+		is_integrated = !strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
>+				PCI_SLOT_NAME_SIZE);
>+
>+		if (want_discrete && !is_integrated) {
>+			__copy_dev_to_card(dev, (crd + count));
>+			count++;
>+			tmp = realloc(crd, sizeof(struct igt_device_card) * (1 + count));
>+			if (!tmp) {
>+				free(crd);
>+				return -1;
>+			}
>+			crd = tmp;
>+
>+		} else if (!want_discrete && is_integrated) {
>+			__copy_dev_to_card(dev, (crd + count));
>+			count++;
>+			tmp = realloc(crd, sizeof(struct igt_device_card) * (1 + count));
>+			if (!tmp) {
>+				free(crd);
>+				return -1;
>+			}
>+			crd = tmp;
>+		}
>+	}
>+	if (count == 0) {
>+		free(crd);
>+		return 0;
>+	}
>+
>+	*card = crd;
>+	return count;
>+}
>+
>+/**
>+ * igt_device_find_all_xe_integrated_card
>+ * @card: double pointer to igt_device_card structure
>+ *
>+ * Iterate over all igt_devices array and find only xe integrated
>+ * cards and populate an array of igt_device_card structure upon
>+ * successful return.
>+ * card will be updated with the pointer to the array of
>+ * igt_device_card structure only if at least a single such device
>+ * is found.
>+ *
>+ * Returns: number of integrated xe device is found.
>+ */
>+int igt_device_find_all_xe_integrated_card(struct igt_device_card **card)
>+{
>+	igt_assert(card);
>+	return __find_all_intel_card_by_driver_name(card, false, "xe");
>+}
>+
>+/**
>+ * igt_device_find_all_xe_discrete_card
>+ * @card: double pointer to igt_device_card structure
>+ *
>+ * Iterate over all igt_devices array and find only xe discrete
>+ * cards and populate an array of igt_device_card structure upon
>+ * successful return.
>+ * card will be updated with the pointer to the array of
>+ * igt_device_card structure only if at least a single such device
>+ * is found.
>+ *
>+ * Returns: number of discrete xe device is found.
>+ */
>+int igt_device_find_all_xe_discrete_card(struct igt_device_card **card)
>+{
>+	igt_assert(card);
>+	return __find_all_intel_card_by_driver_name(card, true, "xe");
>+}
>+
> bool igt_device_find_first_i915_discrete_card(struct igt_device_card *card)
> {
> 	igt_assert(card);
>diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
>index 92741fe3c..25d3c462f 100644
>--- a/lib/igt_device_scan.h
>+++ b/lib/igt_device_scan.h
>@@ -59,6 +59,7 @@ struct igt_device_card {
> 	char subsystem[NAME_MAX];
> 	char card[NAME_MAX];
> 	char render[NAME_MAX];
>+	char driver[NAME_MAX];

For me this is the only part in this patch that should exist (together
with the patch above that is populating it).

IMO we shouldn't really add igt_device_find_all_{driver}_{type}_card()
functions. Imagine supporting 10 drivers. Are we going to create
(internal) APIs for everything?

I think it's better to use a composition strategy. For that you only
need to add the driver name in igt_device_card, i.e. what this part is
doing. When you enumerate the gpu devices, it saves the driver name. The
callers then decide what to do with the enumerated devices.  If a
"filter while enumerating" strategy is what you want, then you could add
a match callback or match criteria. Example: igt_device_card_match()
already implements the latter - filtering by driver could be added if
it's not already there.

If you are doing a long-running application like gputop, you'd likely
call igt_devices_scan() then walk the list to check what drivers you
have and do special things for each of them. You shouldn't really call
"find all i915 discrete cards" + "find all xe discrete cards" + "find
all foobar cards connected via usbc with display active", etc.  Handling
device disconnects would be nice to have too, but at least we shouldn't
crash.

Looking at patches 3 and 4, I'm not sure why you are separating the
lists for integrated/discrete if then later you do

        count_int = igt_device_find_all_xe_integrated_card(&card_int);
        count_dis = igt_device_find_all_xe_discrete_card(&card_dis);

and do the same thing for both.


Lucas De Marchi




> 	char pci_slot_name[PCI_SLOT_NAME_SIZE+1];
> 	uint16_t pci_vendor, pci_device;
> };
>@@ -92,6 +93,8 @@ bool igt_device_find_first_i915_discrete_card(struct igt_device_card *card);
> bool igt_device_find_integrated_card(struct igt_device_card *card);
> bool igt_device_find_first_xe_discrete_card(struct igt_device_card *card);
> bool igt_device_find_xe_integrated_card(struct igt_device_card *card);
>+int igt_device_find_all_xe_integrated_card(struct igt_device_card **card);
>+int igt_device_find_all_xe_discrete_card(struct igt_device_card **card);
> char *igt_device_get_pretty_name(struct igt_device_card *card, bool numeric);
> int igt_open_card(struct igt_device_card *card);
> int igt_open_render(struct igt_device_card *card);
>-- 
>2.34.1
>