Re: [PATCH 2/3] lib/xe_query: add xe_hwconfig_lookup_value() helper

["Grzegorzek, Dominik" <dominik.grzegorzek@intel.com>]

On Wed, 2024-11-20 at 22:28 +0000, Jan Maslak wrote:
Hi Jan,

> Add a new helper function, xe_hw_config_lookup_value(), that
> returns a pointer to the value of a given hwconfig attribute.
> 
> Signed-off-by: Jan Maslak <jan.maslak@intel.com>
> ---
>  lib/xe/xe_query.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/xe/xe_query.h |  2 ++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> index 49bed033b..73c8ed948 100644
> --- a/lib/xe/xe_query.c
> +++ b/lib/xe/xe_query.c
> @@ -839,6 +839,50 @@ uint16_t xe_gt_get_tile_id(int fd, int gt)
>  	return xe_dev->gt_list->gt_list[gt].tile_id;
>  }
>  
> +/**
> + * xe_hwconfig_lookup_value:
> + * @fd: xe device fd
> + * @attribute: hwconfig attribute id
> + * @len: pointer to store length of the value
> + *
> + * Returns a pointer to the value of the hwconfig attribute @attribute and
> + * writes the length of the value to @len.

Nit: Imo it would be worth to underline that len is in units of uint32_t,
not as someone could think in bytes. Maybe sth like:
    * writes the number of uint32_t elements idicating the lenght of the value to @len.

> + * The caller is responsible for freeing the returned pointer.
> + */
> +uint32_t *xe_hwconfig_lookup_value(int fd, enum intel_hwconfig attribute, uint32_t *len)
> +{
> +	struct xe_device *xe_dev;
> +	uint32_t *hwconfig;
> +	uint32_t pos, hwconfig_len;
> +	uint32_t *value = NULL;
> +
> +	xe_dev = find_in_cache(fd);
> +	igt_assert(xe_dev);
> +
> +	hwconfig = xe_dev->hwconfig;
> +	igt_assert(hwconfig);
> +
> +	/* Extract the value from the hwconfig */
> +	pos = 0;
> +	hwconfig_len = xe_dev->hwconfig_size / sizeof(uint32_t);
> +	while (pos + 2 < hwconfig_len) {
> +		uint32_t attribute_id = hwconfig[pos];
> +		uint32_t attribute_len = hwconfig[pos + 1];
> +		uint32_t *attribute_data = &hwconfig[pos + 2];
> +
> +		if (attribute_id == attribute) {
> +			value = malloc(attribute_len * sizeof(uint32_t));
> +			igt_assert(value);
> +			memcpy(value, attribute_data, attribute_len * sizeof(uint32_t));
> +			*len = attribute_len;
> +			break;

I think we could just return an address to cached hwconfig as it remains valid until
the last user calls drm_close_driver(). Caller would be no longer responsible for
freeing returned pointer.

Imo, your apprach is a little bit cleaner, however looking at other simillar
cases (e.g. xe_mem_region, xe_engine) it looks like the preference is 
different. I would propose to make it coherent with behaviour already present in 
that library. 


Regards, 
Dominik
> +		}
> +		pos += 2 + attribute_len;
> +	}
> +
> +	return value;
> +}
> +
>  igt_constructor
>  {
>  	xe_device_cache_init();
> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
> index f8836578e..30ea5ad41 100644
> --- a/lib/xe/xe_query.h
> +++ b/lib/xe/xe_query.h
> @@ -15,6 +15,7 @@
>  #include "igt_aux.h"
>  #include "igt_list.h"
>  #include "igt_sizes.h"
> +#include "intel_hwconfig_types.h"
>  
>  #define XE_DEFAULT_ALIGNMENT           SZ_4K
>  #define XE_DEFAULT_ALIGNMENT_64K       SZ_64K
> @@ -118,6 +119,7 @@ struct drm_xe_engine *xe_find_engine_by_class(int fd, uint16_t engine_class);
>  bool xe_has_media_gt(int fd);
>  bool xe_is_media_gt(int fd, int gt);
>  uint16_t xe_gt_get_tile_id(int fd, int gt);
> +uint32_t *xe_hwconfig_lookup_value(int fd, enum intel_hwconfig attribute, uint32_t *len);
>  
>  struct xe_device *xe_device_get(int fd);
>  void xe_device_put(int fd);