[PATCH i-g-t] lib/igt_device_scan: limit fetched device attributes from udev
Kamil Konieczny
kamil.konieczny at linux.intel.com
Wed Jan 29 14:48:49 UTC 2025
Hi Zbigniew,
On 2025-01-28 at 09:30:14 +0100, Zbigniew Kempczyński wrote:
> Tests don't need all attributes so default device scanning is now
> limited to small list directly used in device filters. To be backward
> compatible tools like lsgpu still may scan whole attribute list what
> keeps this behavior intact.
>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> ---
> lib/igt_device_scan.c | 69 ++++++++++++++++++++++++++++++-------------
> lib/igt_device_scan.h | 1 +
> tools/lsgpu.c | 2 +-
> 3 files changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> index c36b0efa90..15f6bf5728 100644
> --- a/lib/igt_device_scan.c
> +++ b/lib/igt_device_scan.c
> @@ -22,6 +22,7 @@
> *
> */
>
> +#include "drmtest.h"
Could you add a note here why this is needed? Something like
#include "drmtest.h" /* for macro ARRAY_SIZE */
> #include "igt_core.h"
> #include "igt_device_scan.h"
> #include "igt_list.h"
> @@ -206,6 +207,8 @@ enum dev_type {
> #define STR_INTEGRATED "integrated"
> #define STR_DISCRETE "discrete"
>
> +static const char * const attrs[] = { "driver", "sriov_numvfs", "physfn" };
> +
> static inline bool strequal(const char *a, const char *b)
> {
> if (a == NULL || b == NULL)
> @@ -548,24 +551,35 @@ static void get_props(struct udev_device *dev, struct igt_device *idev)
> * Function skips sysattrs from blacklist ht (acquiring some values can take
> * seconds).
> */
> -static void get_attrs(struct udev_device *dev, struct igt_device *idev)
> +static void get_attrs(struct udev_device *dev, struct igt_device *idev,
> + bool limit_attrs)
Why not just two functions instead of a bool var? Below
you split control flow between two separate paths, imho
it is better to have two separate functions instead. So
there will be two functions:
get_attrs_limited(struct udev_device *dev, struct igt_device *idev)
get_attrs_all(struct udev_device *dev, struct igt_device *idev)
and used below.
> {
> struct udev_list_entry *entry;
> + const char *value;
>
> - entry = udev_device_get_sysattr_list_entry(dev);
> - while (entry) {
> - const char *key = udev_list_entry_get_name(entry);
> - const char *value;
> + if (limit_attrs) {
> + for (int i = 0; i < ARRAY_SIZE(attrs); i++) {
> + value = udev_device_get_sysattr_value(dev, attrs[i]);
> + if (!value)
> + continue;
> + igt_device_add_attr(idev, attrs[i], value);
> + DBG("attr: %s, val: %s\n", attrs[i], value);
> + }
> + } else {
> + entry = udev_device_get_sysattr_list_entry(dev);
> + while (entry) {
> + const char *key = udev_list_entry_get_name(entry);
>
> - if (is_on_blacklist(key)) {
> + if (is_on_blacklist(key)) {
> + entry = udev_list_entry_get_next(entry);
> + continue;
> + }
> +
> + value = udev_device_get_sysattr_value(dev, key);
> + igt_device_add_attr(idev, key, value);
> entry = udev_list_entry_get_next(entry);
> - continue;
> + DBG("attr: %s, val: %s\n", key, value);
> }
> -
> - value = udev_device_get_sysattr_value(dev, key);
> - igt_device_add_attr(idev, key, value);
> - entry = udev_list_entry_get_next(entry);
> - DBG("attr: %s, val: %s\n", key, value);
> }
> }
>
> @@ -639,7 +653,8 @@ static char* strdup_nullsafe(const char* str)
> * Fills structure with most usable udev device variables, properties
> * and sysattrs.
> */
> -static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
> +static struct igt_device *igt_device_new_from_udev(struct udev_device *dev,
> + bool limit_attrs)
> {
> struct igt_device *idev = igt_device_new();
>
> @@ -654,7 +669,7 @@ static struct igt_device *igt_device_new_from_udev(struct udev_device *dev)
> idev->drm_render = strdup(idev->devnode);
>
> get_props(dev, idev);
> - get_attrs(dev, idev);
> + get_attrs(dev, idev, limit_attrs);
So here:
limit_attrs ? get_attrs_limited(dev, idev) :
get_attrs_all(dev, idev);
It is not a blocker so with or without it
Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
Regards,
Kamil
>
> if (is_pci_subsystem(idev)) {
> uint16_t vendor, device;
> @@ -868,7 +883,8 @@ static struct igt_device *igt_device_from_syspath(const char *syspath)
> */
> static void update_or_add_parent(struct udev *udev,
> struct udev_device *dev,
> - struct igt_device *idev)
> + struct igt_device *idev,
> + bool limit_attrs)
> {
> struct udev_device *parent_dev;
> struct igt_device *parent_idev;
> @@ -899,7 +915,7 @@ static void update_or_add_parent(struct udev *udev,
> * return fresh udev device.
> */
> parent_dev = udev_device_new_from_syspath(udev, syspath);
> - parent_idev = igt_device_new_from_udev(parent_dev);
> + parent_idev = igt_device_new_from_udev(parent_dev, limit_attrs);
> udev_device_unref(parent_dev);
>
> if (parent_idev)
> @@ -1000,7 +1016,7 @@ static void index_pci_devices(void)
> * Function sorts all found devices to keep same order of bus devices
> * for providing predictable search.
> */
> -static void scan_drm_devices(void)
> +static void scan_drm_devices(bool limit_attrs)
> {
> struct udev *udev;
> struct udev_enumerate *enumerate;
> @@ -1035,9 +1051,9 @@ static void scan_drm_devices(void)
>
> path = udev_list_entry_get_name(dev_list_entry);
> udev_dev = udev_device_new_from_syspath(udev, path);
> - idev = igt_device_new_from_udev(udev_dev);
> + idev = igt_device_new_from_udev(udev_dev, limit_attrs);
> igt_list_add_tail(&idev->link, &igt_devs.all);
> - update_or_add_parent(udev, udev_dev, idev);
> + update_or_add_parent(udev, udev_dev, idev, limit_attrs);
>
> udev_device_unref(udev_dev);
> }
> @@ -1092,17 +1108,28 @@ void igt_devices_free(void)
> *
> * Function scans udev in search of gpu devices.
> */
> -void igt_devices_scan(void)
> +
> +static void __igt_devices_scan(bool limit_attrs)
> {
> if (igt_devs.devs_scanned)
> igt_devices_free();
>
> prepare_scan();
> - scan_drm_devices();
> + scan_drm_devices(limit_attrs);
>
> igt_devs.devs_scanned = true;
> }
>
> +void igt_devices_scan(void)
> +{
> + __igt_devices_scan(true);
> +}
> +
> +void igt_devices_scan_all_attrs(void)
> +{
> + __igt_devices_scan(false);
> +}
> +
> static inline void _pr_simple(const char *k, const char *v)
> {
> printf(" %-16s: %s\n", k, v);
> diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
> index fa90134aa3..92741fe3c7 100644
> --- a/lib/igt_device_scan.h
> +++ b/lib/igt_device_scan.h
> @@ -64,6 +64,7 @@ struct igt_device_card {
> };
>
> void igt_devices_scan(void);
> +void igt_devices_scan_all_attrs(void);
>
> void igt_devices_print(const struct igt_devices_print_format *fmt);
> void igt_devices_print_vendors(void);
> diff --git a/tools/lsgpu.c b/tools/lsgpu.c
> index e482ca6b75..e683900833 100644
> --- a/tools/lsgpu.c
> +++ b/tools/lsgpu.c
> @@ -362,7 +362,7 @@ int main(int argc, char *argv[])
> printf("Notice: Using filter from .igtrc\n");
> }
>
> - igt_devices_scan();
> + igt_devices_scan_all_attrs();
>
> if (igt_device != NULL) {
> struct igt_device_card card;
> --
> 2.34.1
>
More information about the igt-dev
mailing list