Re: [PATCH i-g-t] lib/igt_device_scan: limit fetched device attributes from udev

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 1/30/2025 1:00 PM, Kamil Konieczny wrote:
> Hi Zbigniew,
> On 2025-01-30 at 11:27:41 +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@intel.com>
>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> 
> +Cc sriov developers, please wait with merge until you hear
> opinion from them
> 
> Cc: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> Cc: Lukasz Laguna <lukasz.laguna@intel.com>
> 
>> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>> ---
>> v2: refactor code a bit (Kamil)
> 
> Now it looks better, change is smaller and easy to read.
> 
> Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> 
> 
>> ---
>>   lib/igt_device_scan.c | 50 ++++++++++++++++++++++++++++++++++---------
>>   lib/igt_device_scan.h |  1 +
>>   tools/lsgpu.c         |  2 +-
>>   3 files changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>> index c36b0efa90..711bedc5c8 100644
>> --- a/lib/igt_device_scan.c
>> +++ b/lib/igt_device_scan.c
>> @@ -22,6 +22,7 @@
>>    *
>>    */
>>   
>> +#include "drmtest.h"
>>   #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,7 +551,7 @@ 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_all(struct udev_device *dev, struct igt_device *idev)
>>   {
>>   	struct udev_list_entry *entry;
>>   
>> @@ -569,6 +572,19 @@ static void get_attrs(struct udev_device *dev, struct igt_device *idev)
>>   	}
>>   }
>>   
>> +static void get_attrs_limited(struct udev_device *dev, struct igt_device *idev)
>> +{
>> +	const char *value;
>> +
>> +	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);
>> +	}
>> +}
>> +
>>   #define get_prop(dev, prop) ((char *) g_hash_table_lookup(dev->props_ht, prop))
>>   #define get_attr(dev, attr) ((char *) g_hash_table_lookup(dev->attrs_ht, attr))
>>   #define get_prop_subsystem(dev) get_prop(dev, "SUBSYSTEM")
>> @@ -639,7 +655,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 +671,8 @@ 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);
>> +	limit_attrs ? get_attrs_limited(dev, idev) :
>> +		      get_attrs_all(dev, idev);
>>   
>>   	if (is_pci_subsystem(idev)) {
>>   		uint16_t vendor, device;
>> @@ -868,7 +886,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 +918,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 +1019,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 +1054,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 +1111,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;
LGTM,
Reviewed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> -- 
>> 2.34.1
>>