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

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

On Tue, 2024-11-26 at 10:13 +0100, Dominik Grzegorzek wrote:
> 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.
Sorry, I made two typos here:

s/idicating/indicating/
s/lenght/length/

Regards, 
Dominik

> 
> > + * 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);
>