Re: [Intel-xe] [PATCH v1 5/8] drm/xe/uapi: Align on a common way to return arrays (memory regions)

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2023-11-16 at 14:43 +0000, Francois Dugast wrote:
> The uAPI provides queries which return arrays of elements. As of now
> the format used in the struct is different depending on which element
> is queried. Fix this for memory regions by applying the pattern below:
> 
>     struct drm_xe_query_X {
>        __u32 num_X;
>        struct drm_xe_X Xs[];
>        ...
>     }
> 
> This removes "query" in the name of struct drm_xe_query_mem_region
> as it is not returned from the query IOCTL. There is no functional
> change.
> 
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_query.c | 44 ++++++++++++++++++-----------------
>  include/uapi/drm/xe_drm.h     | 22 +++++++++---------
>  2 files changed, 34 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> index 0cbfeaeb1330..b31e00bd29bc 100644
> --- a/drivers/gpu/drm/xe/xe_query.c
> +++ b/drivers/gpu/drm/xe/xe_query.c
> @@ -240,15 +240,15 @@ static size_t calc_mem_regions_size(struct xe_device *xe)
>  		if (ttm_manager_type(&xe->ttm, i))
>  			num_managers++;
>  
> -	return offsetof(struct drm_xe_query_mem_regions, regions[num_managers]);
> +	return offsetof(struct drm_xe_query_mem_region, mem_regions[num_managers]);
>  }
>  
> -static int query_mem_regions(struct xe_device *xe,
> -			     struct drm_xe_device_query *query)
> +static int query_mem_region(struct xe_device *xe,
> +			    struct drm_xe_device_query *query)
>  {
>  	size_t size = calc_mem_regions_size(xe);
> -	struct drm_xe_query_mem_regions *usage;
> -	struct drm_xe_query_mem_regions __user *query_ptr =
> +	struct drm_xe_query_mem_region *usage;
> +	struct drm_xe_query_mem_region __user *query_ptr =
>  		u64_to_user_ptr(query->data);
>  	struct ttm_resource_manager *man;
>  	int ret, i;
> @@ -265,36 +265,38 @@ static int query_mem_regions(struct xe_device *xe,
>  		return -ENOMEM;
>  
>  	man = ttm_manager_type(&xe->ttm, XE_PL_TT);
> -	usage->regions[0].mem_class = DRM_XE_MEM_REGION_CLASS_SYSMEM;
> -	usage->regions[0].instance = 0;
> -	usage->regions[0].min_page_size = PAGE_SIZE;
> -	usage->regions[0].total_size = man->size << PAGE_SHIFT;
> +	usage->mem_regions[0].mem_class = DRM_XE_MEM_REGION_CLASS_SYSMEM;
> +	usage->mem_regions[0].instance = 0;
> +	usage->mem_regions[0].min_page_size = PAGE_SIZE;
> +	usage->mem_regions[0].total_size = man->size << PAGE_SHIFT;
>  	if (perfmon_capable())
> -		usage->regions[0].used = ttm_resource_manager_usage(man);
> -	usage->num_regions = 1;
> +		usage->mem_regions[0].used = ttm_resource_manager_usage(man);
> +	usage->num_mem_regions = 1;
>  
>  	for (i = XE_PL_VRAM0; i <= XE_PL_VRAM1; ++i) {
>  		man = ttm_manager_type(&xe->ttm, i);
>  		if (man) {
> -			usage->regions[usage->num_regions].mem_class =
> +			usage->mem_regions[usage->num_mem_regions].mem_class =
>  				DRM_XE_MEM_REGION_CLASS_VRAM;
> -			usage->regions[usage->num_regions].instance =
> -				usage->num_regions;
> -			usage->regions[usage->num_regions].min_page_size =
> +			usage->mem_regions[usage->num_mem_regions].instance =
> +				usage->num_mem_regions;
> +			usage->mem_regions[usage->num_mem_regions].min_page_size =
>  				xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ?
>  				SZ_64K : PAGE_SIZE;
> -			usage->regions[usage->num_regions].total_size =
> +			usage->mem_regions[usage->num_mem_regions].total_size =
>  				man->size;
>  
>  			if (perfmon_capable()) {
>  				xe_ttm_vram_get_used(man,
> -						     &usage->regions[usage->num_regions].used,
> -						     &usage->regions[usage->num_regions].cpu_visible_used);
> +						     &usage->mem_regions
> +						     [usage->num_mem_regions].used,
> +						     &usage->mem_regions
> +						     [usage->num_mem_regions].cpu_visible_used);
>  			}
>  
> -			usage->regions[usage->num_regions].cpu_visible_size =
> +			usage->mem_regions[usage->num_mem_regions].cpu_visible_size =
>  				xe_ttm_vram_get_cpu_visible_size(man);
> -			usage->num_regions++;
> +			usage->num_mem_regions++;
>  		}
>  	}
>  
> @@ -500,7 +502,7 @@ static int query_gt_topology(struct xe_device *xe,
>  static int (* const xe_query_funcs[])(struct xe_device *xe,
>  				      struct drm_xe_device_query *query) = {
>  	query_engines,
> -	query_mem_regions,
> +	query_mem_region,
>  	query_config,
>  	query_gt_list,
>  	query_hwconfig,
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index e02bef8dc229..f54e545cc4fb 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -182,10 +182,10 @@ enum drm_xe_memory_class {
>  };
>  
>  /**
> - * struct drm_xe_query_mem_region - Describes some region as known to
> + * struct drm_xe_mem_region - Describes some region as known to
>   * the driver.
>   */
> -struct drm_xe_query_mem_region {
> +struct drm_xe_mem_region {
>  	/**
>  	 * @mem_class: The memory class describing this region.
>  	 *
> @@ -315,19 +315,19 @@ struct drm_xe_query_engine_cycles {
>  };
>  
>  /**
> - * struct drm_xe_query_mem_regions - describe memory regions
> + * struct drm_xe_query_mem_region - describe memory regions
>   *
>   * If a query is made with a struct drm_xe_device_query where .query
> - * is equal to DRM_XE_DEVICE_QUERY_MEM_REGIONS, then the reply uses
> - * struct drm_xe_query_mem_regions in .data.
> + * is equal to DRM_XE_DEVICE_QUERY_MEM_REGION, then the reply uses
> + * struct drm_xe_query_mem_region in .data.
>   */
> -struct drm_xe_query_mem_regions {
> -	/** @num_regions: number of memory regions returned in @regions */
> -	__u32 num_regions;
> +struct drm_xe_query_mem_region {

ack on s/drm_xe_query_mem_region/drm_xe_mem_region but not on s/drm_xe_query_mem_regions/drm_xe_query_mem_region, the second one can return more than
one region.

> +	/** @num_mem_regions: number of memory regions returned in @mem_regions */
> +	__u32 num_mem_regions;
>  	/** @pad: MBZ */
>  	__u32 pad;
> -	/** @regions: The returned regions for this device */
> -	struct drm_xe_query_mem_region regions[];
> +	/** @mem_regions: The returned memory regions for this device */
> +	struct drm_xe_mem_region mem_regions[];
>  };
>  
>  /**
> @@ -493,7 +493,7 @@ struct drm_xe_device_query {
>  	__u64 extensions;
>  
>  #define DRM_XE_DEVICE_QUERY_ENGINES		0
> -#define DRM_XE_DEVICE_QUERY_MEM_REGIONS		1
> +#define DRM_XE_DEVICE_QUERY_MEM_REGION		1
>  #define DRM_XE_DEVICE_QUERY_CONFIG		2
>  #define DRM_XE_DEVICE_QUERY_GT_LIST		3
>  #define DRM_XE_DEVICE_QUERY_HWCONFIG		4