[Intel-xe] [PATCH v2 03/14] drm/xe: Make DRM_XE_DEVICE_QUERY_ENGINES future proof

[Matthew Brost]

On Wed, Nov 22, 2023 at 02:38:22PM +0000, Francois Dugast wrote:
> From: José Roberto de Souza <jose.souza at intel.com>
> 
> We have at least 2 future features(OA and future media engines
> capabilities) that will require Xe to provide more information about
> engines to UMDs.
> 
> But this information should not just be added to
> drm_xe_engine_class_instance for a couple of reasons:
> - drm_xe_engine_class_instance is used as input to other structs/uAPIs
> and those uAPIs don't care about any of these future new engine fields
> - those new fields are useless information after initialization for
> some UMDs, so it should not need to carry that around
> 
> So here my proposal is to make DRM_XE_DEVICE_QUERY_ENGINES return an
> array of drm_xe_query_engine_info that contain
> drm_xe_engine_class_instance and 3 u64s to be used for future features.

There is 5 u64 in this patch.

Anyways agree this better but what if we improve this a bit more making
each __drm_xe_query_engine_info a dynamic size.

e.g.
struct __drm_xe_query_engine_info {	/* Bad name, just for example */
     /** @instance: The @drm_xe_engine_class_instance */
     struct drm_xe_engine_class_instance instance;

     /** @reserved: Reserved, dynamic size */
     __u64 reserved[0];
};

struct drm_xe_query_engine_info {
     /** @num_engines: Number engines */
     __u64 num_engines;
     /** @engine_info: Array of engine info */
     struct __drm_xe_query_engine_info engine_info[0];
};

The size of __drm_xe_query_engine_info then change and can be calculated
to be:

(returned size of query - sizoeof(__u64)) / drm_xe_query_engine_info.num_engines

We then should be able to add additional fields to
__drm_xe_query_engine_info indefinitely.

What does everyone think? We likely can apply this idea to other
queries too as it makes sense.

Matt

> 
> Reference OA:
> https://patchwork.freedesktop.org/patch/558362/?series=121084&rev=6
> 
> Cc: Francois Dugast <francois.dugast at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> [Rodrigo Rebased]
> Signed-off-by: Francois Dugast <francois.dugast at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_query.c | 15 ++++++++-------
>  include/uapi/drm/xe_drm.h     | 24 +++++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> index 61a7d92b7e88..0cbfeaeb1330 100644
> --- a/drivers/gpu/drm/xe/xe_query.c
> +++ b/drivers/gpu/drm/xe/xe_query.c
> @@ -53,7 +53,7 @@ static size_t calc_hw_engine_info_size(struct xe_device *xe)
>  			i++;
>  		}
>  
> -	return i * sizeof(struct drm_xe_engine_class_instance);
> +	return i * sizeof(struct drm_xe_query_engine_info);
>  }
>  
>  typedef u64 (*__ktime_func_t)(void);
> @@ -186,9 +186,9 @@ static int query_engines(struct xe_device *xe,
>  			 struct drm_xe_device_query *query)
>  {
>  	size_t size = calc_hw_engine_info_size(xe);
> -	struct drm_xe_engine_class_instance __user *query_ptr =
> +	struct drm_xe_query_engine_info __user *query_ptr =
>  		u64_to_user_ptr(query->data);
> -	struct drm_xe_engine_class_instance *hw_engine_info;
> +	struct drm_xe_query_engine_info *hw_engine_info;
>  	struct xe_hw_engine *hwe;
>  	enum xe_hw_engine_id id;
>  	struct xe_gt *gt;
> @@ -211,12 +211,13 @@ static int query_engines(struct xe_device *xe,
>  			if (xe_hw_engine_is_reserved(hwe))
>  				continue;
>  
> -			hw_engine_info[i].engine_class =
> +			hw_engine_info[i].instance.engine_class =
>  				xe_to_user_engine_class[hwe->class];
> -			hw_engine_info[i].engine_instance =
> +			hw_engine_info[i].instance.engine_instance =
>  				hwe->logical_instance;
> -			hw_engine_info[i].gt_id = gt->info.id;
> -			hw_engine_info[i].pad = 0;
> +			hw_engine_info[i].instance.gt_id = gt->info.id;
> +			hw_engine_info[i].instance.pad = 0;
> +			memset(hw_engine_info->reserved, 0, sizeof(hw_engine_info->reserved));
>  
>  			i++;
>  		}
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 1bdd20d3c4a8..c80e03b61489 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -124,7 +124,14 @@ struct xe_user_extension {
>  #define DRM_IOCTL_XE_EXEC_QUEUE_GET_PROPERTY	DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_EXEC_QUEUE_GET_PROPERTY, struct drm_xe_exec_queue_get_property)
>  #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
>  
> -/** struct drm_xe_engine_class_instance - instance of an engine class */
> +/**
> + * struct drm_xe_engine_class_instance - instance of an engine class
> + *
> + * It is returned as part of the @drm_xe_query_engine_info, but it also is
> + * used as the input of engine selection for both @drm_xe_exec_queue_create
> + * and @drm_xe_query_engine_cycles
> + *
> + */
>  struct drm_xe_engine_class_instance {
>  #define DRM_XE_ENGINE_CLASS_RENDER		0
>  #define DRM_XE_ENGINE_CLASS_COPY		1
> @@ -145,6 +152,21 @@ struct drm_xe_engine_class_instance {
>  	__u16 pad;
>  };
>  
> +/**
> + * struct drm_xe_query_engine_info - describe hardware engine
> + *
> + * If a query is made with a struct @drm_xe_device_query where .query
> + * is equal to %DRM_XE_DEVICE_QUERY_ENGINES, then the reply uses an array of
> + * struct @drm_xe_query_engine_info in .data.
> + */
> +struct drm_xe_query_engine_info {
> +	/** @instance: The @drm_xe_engine_class_instance */
> +	struct drm_xe_engine_class_instance instance;
> +
> +	/** @reserved: Reserved */
> +	__u64 reserved[5];
> +};
> +
>  /**
>   * enum drm_xe_memory_class - Supported memory classes.
>   */
> -- 
> 2.34.1
>