[PATCH <i-g-t> v5 1/4] lib/igt_device_scan: Enable finding all IGT devices for a specific driver
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Thu Apr 3 19:02:27 UTC 2025
On Tue, Apr 01, 2025 at 07:01:34PM +0530, Purkait, Soham wrote:
> Hi Zbigniew,
>
> On 28-03-2025 13:36, Zbigniew Kempczyński wrote:
>
> On Wed, Mar 26, 2025 at 12:42:27AM +0530, Soham Purkait wrote:
>
> Add filter functions to find all the
> available GPUs or few among them
> by driver name and card type or card number.
> Add driver field to igt_device_card structure
> for storing driver names.
>
> v2 : fix for refactoring GPUTOP into a
> vendor-agnostic tool (Lucas)
>
> v3 : Separate commit for lib (Kamil)
>
> v4 : Refactor to use composition strategy
> for driver and device type filtering
> Refactor code to improve memory
> allocation and error handling (Lucas)
>
> v5 : Rename function name as per convention.
> Use "dev_type" enum for card_type. (Krzysztof)
> Add new filter to return collection
> of matching devices. (Zbigniew)
>
> Signed-off-by: Soham Purkait <soham.purkait at intel.com>
> ---
> lib/igt_device_scan.c | 177 ++++++++++++++++++++++++++++++++++++++++--
> lib/igt_device_scan.h | 12 +++
> 2 files changed, 183 insertions(+), 6 deletions(-)
>
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index 711bedc5c..452da2ee4 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -198,12 +198,6 @@
> #define DBG(...) {}
> #endif
>
> -enum dev_type {
> - DEVTYPE_ALL,
> - DEVTYPE_INTEGRATED,
> - DEVTYPE_DISCRETE,
> -};
> -
>
> I think you don't need to extern this dev_type.
Excuse me, I'm not able to properly read your answers. May I ask for use
email client which allows to reply in emails prepending with > ?
--
Zbigniew
>
> As the function (igt_device_find_all_intel_card_by_driver_name) need to
> call with the device type as argument from gputop.c so it has been made
> extern to access enum dev_type from outside.
>
>
>
> #define STR_INTEGRATED "integrated"
> #define STR_DISCRETE "discrete"
>
> @@ -774,6 +768,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 +818,58 @@ 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 cards.
>
> This sentence is misleading, otherwise what for is card_type argument?
>
>
> + * @card: double pointer to igt_device_card structure, containing
> + * an array of igt_device_card structures upon successful return.
> + * @card_type: flag of type "enum igt_devices_card_type" to indicate
> + * whether to find discrete, integrated, or both types of cards.
> + * @drv_name: name of the driver to match.
> + *
> + * Returns the number of cards found, or -1 on error.
>
> You should add some note about freeing the memory allocated for the
> returned array.
>
>
> + */
> +int igt_device_find_all_intel_card_by_driver_name(struct igt_device_card **card,
> + enum dev_type card_type,
> + const char *drv_name)
> +{
> + int count = 0;
> + struct igt_device *dev;
> + int is_integrated;
> + struct igt_device_card *tmp;
> + struct igt_device_card *crd = NULL;
> +
> + igt_assert(drv_name);
> + *card = NULL;
> +
> + 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 ((card_type == DEVTYPE_INTEGRATED && !is_integrated) ||
> + (card_type == DEVTYPE_DISCRETE && is_integrated) ||
> + card_type == DEVTYPE_ALL) {
>
> Shouldn't this be:
>
> if ((card_type == DEVTYPE_INTEGRATED && is_integrated) ||
> (card_type == DEVTYPE_DISCRETE && !is_integrated) ||
> card_type == DEVTYPE_ALL) {
>
> ?
>
> Do you really need this function? I think according to device filter you
> could use:
>
> igt_device_card_match_all("device:driver=intel", &cards);
> igt_device_card_match_all("device:driver=intel,device=integrated", &cards);
> igt_device_card_match_all("device:driver=intel,device=discrete", &cards);
> igt_device_card_match_all("device:driver=xe", &cards);
> igt_device_card_match_all("device:driver=i915", &cards);
>
> (see explanation below)
>
>
> + tmp = realloc(crd, sizeof(struct igt_device_card) * (count + 1));
> + if (!tmp && crd) {
> + free(crd);
> + return -1;
> + }
> +
> + crd = tmp;
> + __copy_dev_to_card(dev, &crd[count]);
> + count++;
> + }
> + }
> +
> + if (!count)
> + return 0;
> +
> + *card = crd;
> + return count;
> +}
> +
> bool igt_device_find_first_i915_discrete_card(struct igt_device_card *card)
> {
> igt_assert(card);
> @@ -1705,6 +1755,55 @@ static struct igt_list_head *filter_sriov(const struct filter_class *fcls,
> return &igt_devs.filtered;
> }
>
> +/*
> + * Find appropriate gpu device matching driver/card filter arguments.
> + */
> +static struct igt_list_head *filter_device(const struct filter_class *fcls,
> + const struct filter *filter)
> +{
> + struct igt_device *dev;
> + int card = -1;
> + (void)fcls;
> +
> + DBG("filter device\n");
> +
> + if (filter->data.card) {
> + sscanf(filter->data.card, "%d", &card);
> + if (card < 0)
> + return &igt_devs.filtered;
> + } else {
> + card = -1;
> + }
> +
> + igt_list_for_each_entry(dev, &igt_devs.all, link) {
> + if (!is_pci_subsystem(dev))
> + continue;
>
> Why there's limitation to pci devices only?
>
> Once the array of cards of type struct igt_device_card are prepared and
> populated,
> it is matched with the respective supported drivers . Seems like this
> field is not present in case of non PCI devices.
>
> +
> + /* Skip if 'driver' doesn't match */
> + if (filter->data.driver && !strequal(filter->data.driver, dev->driver))
> + continue;
>
> Use may introduce 'driver=intel' which would match i915 and xe. See
> is_device_matched() function.
>
>
> +
> + /* We get n-th card */
> + if (!card) {
> + struct igt_device *dup = duplicate_device(dev);
> +
> + igt_list_add_tail(&dup->link, &igt_devs.filtered);
> + break;
> + }
> + /* Include all the card */
> + else if (card == -1) {
> + struct igt_device *dup = duplicate_device(dev);
> +
> + igt_list_add_tail(&dup->link, &igt_devs.filtered);
> + } else
> + card--;
> + }
> +
> + DBG("Filter device filtered size: %d\n", igt_list_length(&igt_devs.filtered));
> +
> + return &igt_devs.filtered;
> +}
>
> I wondered to alter a little bit pci filter, which currently limits
> filtered view to have single element, but that's not a problem to
> change it and keep matching card first in this view (I mean using
> igt_list_add() instead igt_list_add_tail().
>
>
> +
> static bool sys_path_valid(const struct filter_class *fcls,
> const struct filter *filter)
> {
> @@ -1746,6 +1845,12 @@ static struct filter_class filter_definition_list[] = {
> .help = "sriov:[vendor=%04x/name][,device=%04x][,card=%d][,pf=%d][,vf=%d]",
> .detail = "find pf or vf\n",
> },
> + {
> + .name = "device",
> + .filter_function = filter_device,
> + .help = "device:[driver=name][,card=%d]",
> + .detail = "find device by driver name and card number\n",
> + },
> {
> .name = NULL,
> },
> @@ -2059,6 +2164,66 @@ bool igt_device_card_match_pci(const char *filter,
> return __igt_device_card_match(filter, card, true);
> }
>
> +/**
> + * igt_device_card_match_all
> + * @filter: filter string
> + * @card: double pointer to igt_device_card structure, containing
> + * an array of igt_device_card structures upon successful return.
> + * @request_pci_ss: a boolean parameter determines whether to
> + * consider PCI subsystem information during this process.
> + * Function applies filter to match device from device array.
>
> Describe what drivers[] array is for. But according to my comment
> above consider to have meta driver 'intel' for both drivers.
>
> Via parameter drivers[] array, this function checks if devices of a few
> specific driver is found whose implantation is available in GPUTOP.
> Even if driver 'intel' as a group is implemented for filtering, manually
> the cards/ devices need to check against all supported intel drivers (in
> GPUTOP).
>
>
>
> + *
> + * Returns: the number of cards found.
> + */
> +int igt_device_card_match_all(const char *filter, struct igt_device_card **card,
> + bool request_pci_ss, const char * const drivers[])
> +{
> + struct igt_device *dev = NULL;
> + struct igt_device_card *crd = NULL;
> + struct igt_device_card *tmp;
> + int count = 0;
> +
> + /*
> + * Scan devices in case the user hasn't yet,
> + * but leave a decision on forced rescan on the user side.
> + */
>
> It seems this comment is not relevant anymore.
>
>
> + igt_devices_scan();
> +
> + if (igt_device_filter_apply(filter) == false)
> + return 0;
> +
> + if (igt_list_empty(&igt_devs.filtered))
> + return 0;
> +
> + igt_list_for_each_entry(dev, &igt_devs.filtered, link) {
>
> Having device= and driver= you could just simply iterate over
> this list and build igt_device_card array without any additional
> logic here.
>
> As already mentioned, the drivers of the respective devices need to check
> against the supported drivers[] array in the GPUTOP
> to build igt_device_card array. Regards, Soham
>
>
> Resume and some other nit:
>
> Divide this patch to at least two:
> a) in first introduce device_filter
> b) in second add igt_device_card_match_all()
>
> --
> Zbigniew
>
>
> + for (int i = 0;
> + drivers && drivers[i] && !strcmp(drivers[i],
> + dev->driver); i++) {
> + tmp = realloc(crd, sizeof(struct igt_device_card) * (count + 1));
> + if (!tmp && crd) {
> + free(crd);
> + return 0;
> + }
> +
> + crd = tmp;
> +
> + if (request_pci_ss && !is_pci_subsystem(dev) &&
> + dev->parent && is_pci_subsystem(dev->parent))
> + __copy_dev_to_card(dev->parent, crd);
> + else
> + __copy_dev_to_card(dev, crd);
> + count++;
> + break;
> + }
> + }
> +
> + if (!count)
> + return 0;
> +
> + *card = crd;
> + return count;
> +}
> +
> /**
> * igt_device_get_pretty_name
> * @card: pointer to igt_device_card struct
> diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
> index 92741fe3c..62e7a1737 100644
> --- a/lib/igt_device_scan.h
> +++ b/lib/igt_device_scan.h
> @@ -59,10 +59,17 @@ struct igt_device_card {
> char subsystem[NAME_MAX];
> char card[NAME_MAX];
> char render[NAME_MAX];
> + char driver[NAME_MAX];
> char pci_slot_name[PCI_SLOT_NAME_SIZE+1];
> uint16_t pci_vendor, pci_device;
> };
>
> +enum dev_type {
> + DEVTYPE_ALL,
> + DEVTYPE_INTEGRATED,
> + DEVTYPE_DISCRETE,
> +};
> +
> void igt_devices_scan(void);
> void igt_devices_scan_all_attrs(void);
>
> @@ -88,10 +95,15 @@ int igt_device_filter_pci(void);
> bool igt_device_card_match(const char *filter, struct igt_device_card *card);
> bool igt_device_card_match_pci(const char *filter,
> struct igt_device_card *card);
> +int igt_device_card_match_all(const char *filter, struct igt_device_card **card,
> + bool request_pci_ss, const char * const drivers[]);
> 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_intel_card_by_driver_name(struct igt_device_card **card,
> + enum dev_type card_type,
> + const char *drv_name);
> 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
More information about the igt-dev
mailing list