<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> Hi Zbigniew, <div class="moz-cite-prefix">On 28-03-2025 13:36, Zbigniew Kempczyński wrote: </div> <blockquote type="cite" cite="mid:20250328080624.73bhyl3ygvzc2bkk@zkempczy-mobl2"> <pre wrap="" class="moz-quote-pre">On Wed, Mar 26, 2025 at 12:42:27AM +0530, Soham Purkait wrote: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">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 <a class="moz-txt-link-rfc2396E" href="mailto:soham.purkait@intel.com"><soham.purkait@intel.com></a> --- 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, -}; - </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> I think you don't need to extern this dev_type.</pre> </blockquote> 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. <blockquote type="cite" cite="mid:20250328080624.73bhyl3ygvzc2bkk@zkempczy-mobl2"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> #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. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> This sentence is misleading, otherwise what for is card_type argument? </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ * @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. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> You should add some note about freeing the memory allocated for the returned array. </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ */ +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) { </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> 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) </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ 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; </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> Why there's limitation to pci devices only?</pre> </blockquote> 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. <blockquote type="cite" cite="mid:20250328080624.73bhyl3ygvzc2bkk@zkempczy-mobl2"> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ + /* Skip if 'driver' doesn't match */ + if (filter->data.driver && !strequal(filter->data.driver, dev->driver)) + continue; </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> Use may introduce 'driver=intel' which would match i915 and xe. See is_device_matched() function. </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ + /* 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; +} </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> 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(). </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ 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. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> Describe what drivers[] array is for. But according to my comment above consider to have meta driver 'intel' for both drivers. </pre> </blockquote> 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). <blockquote type="cite" cite="mid:20250328080624.73bhyl3ygvzc2bkk@zkempczy-mobl2"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ * + * 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. + */ </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> It seems this comment is not relevant anymore. </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ 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) { </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> Having device= and driver= you could just simply iterate over this list and build igt_device_card array without any additional logic here. </pre> </blockquote> 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 <blockquote type="cite" cite="mid:20250328080624.73bhyl3ygvzc2bkk@zkempczy-mobl2"> <pre wrap="" class="moz-quote-pre"> 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 </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ 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 </pre> </blockquote> </blockquote> </body> </html>