Re: [Intel-xe] [PATCH v2 05/14] drm/xe/uapi: Align on a common way to return arrays (memory regions)

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 22 Nov 2023 06:38:24 -0800, Francois Dugast wrote:
>
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index da10d946930b..a9bbdf141fe2 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.
>	 *
> @@ -322,12 +322,12 @@ struct drm_xe_query_engine_cycles {
>   * struct drm_xe_query_mem_regions in .data.
>   */
>  struct drm_xe_query_mem_regions {
> -	/** @num_regions: number of memory regions returned in @regions */
> -	__u32 num_regions;
> +	/** @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[];

General question/comment about these query struct's. We have stuck
xe_user_extension's into struct's going into the kernel but haven't done
that for data coming out of the kernel (i.e. these kinds of query structs).

There are basically two options here:

* If the query data struct needs to change, invent a new query,
  e.g. instead of DRM_XE_DEVICE_QUERY_MEM_REGIONS have a new
  DRM_XE_DEVICE_QUERY_MEM_REGIONS_v2.

* The other option is to have xe_user_extension's in these query struct's
  too and provide the new query struct via the xe_user_extension (rather
  than add DRM_XE_DEVICE_QUERY_MEM_REGIONS_v2). So slight difference I
  think.

Comments?

Incidentally for some reason 'struct drm_xe_exec_queue_get_property' does
have an 'extensions' field.