[PATCH v3 i-g-t 1/2] lib/igt_sysfs: Introduce engine_class property helpers

[Kamil Konieczny]

Hi Dominik,
On 2024-10-07 at 12:27:33 +0200, Dominik Karol Piątkowski wrote:
> Introduce the following engine_class helpers:
>  - xe_sysfs_engine_class_get_property
>    Convenience wrapper to get value of given property for given engine
>    class on given gt.
>  - xe_sysfs_engine_class_set_property
>    Convenience wrapper to set given property for given engine class on
>    given gt to given value.

I have few nits, even if that was already merged.

> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski at intel.com>
> ---
>  lib/igt_sysfs.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_sysfs.h |  5 +++
>  2 files changed, 90 insertions(+)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index b7746a1ec..26a3aa31f 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -1414,3 +1414,88 @@ int xe_sysfs_driver_do(int xe_device, char pci_slot[], enum xe_sysfs_driver_acti
>  
>  	return xe_device;
>  }
> +
> +/**
> + * xe_sysfs_engine_class_get_property
> + * @xe_device: fd of the device
> + * @gt: gt number
> + * @class: engine class
> + * @property: property of engine class to retrieve
> + * @value: pointer for storing read value
> + *
> + * Convenience wrapper to get value of given property for given engine class on given gt.

s/given//g
Also write about u32 type.

> + *
> + * Returns: true on success, false on failure.
> + */
> +bool xe_sysfs_engine_class_get_property(int xe_device, int gt, uint16_t class, const char *property,
> +					uint32_t *value)

imho this should be named __xe_sysfs_engine_class_get_u32

> +{
> +	int engines_fd;
> +
> +	engines_fd = xe_sysfs_engine_open(xe_device, gt, class);
> +
> +	if (engines_fd == -1) {
> +		igt_debug("Failed to open %s on gt%d.\n", xe_engine_class_to_str(class), gt);
> +
> +		return false;
> +	}
> +
> +	if (!__igt_sysfs_get_u32(engines_fd, property, value)) {
> +		igt_debug("Failed to read %s property of %s on gt%d.\n", property,
> +			  xe_engine_class_to_str(class), gt);
> +		close(engines_fd);
> +
> +		return false;
> +	}
> +
> +	close(engines_fd);
> +
> +	return true;
> +}
> +
> +/**
> + * xe_sysfs_engine_class_set_property
> + * @xe_device: fd of the device
> + * @gt: gt number
> + * @class: engine class
> + * @property: property of engine class to be modified
> + * @new_value: value to be set
> + * @old_value: pointer for storing old value, can be NULL
> + *
> + * Convenience wrapper to set given property for given engine class on given gt to given value.

s/given//g

Function combines getting and setting, it should be more accuratly described,
also mention u32 type.

> + *
> + * Returns: true on success, false on failure.
> + */
> +bool xe_sysfs_engine_class_set_property(int xe_device, int gt, uint16_t class, const char *property,
> +					uint32_t new_value, uint32_t *old_value)

imho here also __xe_sysfs_engine_class_set_u32
but it looks more like get_and_set

Regards,
Kamil

> +{
> +	int engines_fd;
> +
> +	engines_fd = xe_sysfs_engine_open(xe_device, gt, class);
> +
> +	if (engines_fd == -1) {
> +		igt_debug("Failed to open %s on gt%d.\n", xe_engine_class_to_str(class), gt);
> +
> +		return false;
> +	}
> +
> +	if (old_value && !__igt_sysfs_get_u32(engines_fd, property, old_value)) {
> +		igt_debug("Failed to read %s property of %s on gt%d.\n", property,
> +			  xe_engine_class_to_str(class), gt);
> +		close(engines_fd);
> +
> +		return false;
> +	}
> +
> +	if (!__igt_sysfs_set_u32(engines_fd, property, new_value)) {
> +		igt_debug("Failed to write %s property of %s on gt%d.\n", property,
> +			  xe_engine_class_to_str(class), gt);
> +		close(engines_fd);
> +
> +		return false;
> +	}
> +
> +	close(engines_fd);
> +
> +	return true;
> +}
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index 0ee253826..852b95053 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -184,4 +184,9 @@ enum xe_sysfs_driver_action {
>  
>  int xe_sysfs_driver_do(int xe_device, char pci_slot[], enum xe_sysfs_driver_action action);
>  
> +bool xe_sysfs_engine_class_get_property(int xe_device, int gt, uint16_t class, const char *property,
> +					uint32_t *value);
> +bool xe_sysfs_engine_class_set_property(int xe_device, int gt, uint16_t class, const char *property,
> +					uint32_t new_value, uint32_t *old_value);
> +
>  #endif /* __IGT_SYSFS_H__ */
> -- 
> 2.34.1
>