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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Nov 16, 2023 at 08:40:26PM +0000, Souza, Jose wrote:
> 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 gt by applying the pattern below:
> > 
> >     struct drm_xe_query_X {
> >        __u32 num_X;
> >        struct drm_xe_X Xs[];
> >        ...
> >     }
> > 
> > However, strictly following this rule would bring back the name "gts"
> > which is avoided as per commit ("drm/xe/uapi: Rename gts to gt_list")
> > so leave exceptions in the case of gt with num_gt (singular) and
> > "gt_list". Also, this change removes "query" in the name of struct
> > drm_xe_query_gt 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 |  8 ++++----
> >  include/uapi/drm/xe_drm.h     | 18 +++++++++---------
> >  2 files changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > index b31e00bd29bc..5756d8eaf972 100644
> > --- a/drivers/gpu/drm/xe/xe_query.c
> > +++ b/drivers/gpu/drm/xe/xe_query.c
> > @@ -353,11 +353,11 @@ static int query_config(struct xe_device *xe, struct drm_xe_device_query *query)
> >  static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query)
> >  {
> >  	struct xe_gt *gt;
> > -	size_t size = sizeof(struct drm_xe_query_gt_list) +
> > -		xe->info.gt_count * sizeof(struct drm_xe_query_gt);
> > -	struct drm_xe_query_gt_list __user *query_ptr =
> > +	size_t size = sizeof(struct drm_xe_query_gt) +
> > +		xe->info.gt_count * sizeof(struct drm_xe_gt);
> > +	struct drm_xe_query_gt __user *query_ptr =
> >  		u64_to_user_ptr(query->data);
> > -	struct drm_xe_query_gt_list *gt_list;
> > +	struct drm_xe_query_gt *gt_list;
> >  	u8 id;
> >  
> >  	if (query->size == 0) {
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index f54e545cc4fb..fa4f554b5264 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -356,14 +356,14 @@ struct drm_xe_query_config {
> >  };
> >  
> >  /**
> > - * struct drm_xe_query_gt - describe an individual GT.
> > + * struct drm_xe_gt - describe an individual GT.
> >   *
> > - * To be used with drm_xe_query_gt_list, which will return a list with all the
> > + * To be used with drm_xe_query_gt, which will return a list with all the
> >   * existing GT individual descriptions.
> >   * Graphics Technology (GT) is a subset of a GPU/tile that is responsible for
> >   * implementing graphics and/or media operations.
> >   */
> > -struct drm_xe_query_gt {
> > +struct drm_xe_gt {
> >  #define DRM_XE_QUERY_GT_TYPE_MAIN		0
> >  #define DRM_XE_QUERY_GT_TYPE_MEDIA		1
> >  	/** @type: GT type: Main or Media */
> > @@ -390,19 +390,19 @@ struct drm_xe_query_gt {
> >  };
> >  
> >  /**
> > - * struct drm_xe_query_gt_list - A list with GT description items.
> > + * struct drm_xe_query_gt - A list with GT description items.
> >   *
> >   * If a query is made with a struct drm_xe_device_query where .query
> > - * is equal to DRM_XE_DEVICE_QUERY_GT_LIST, then the reply uses struct
> > - * drm_xe_query_gt_list in .data.
> > + * is equal to DRM_XE_DEVICE_QUERY_GT, then the reply uses struct
> > + * drm_xe_query_gt in .data.
> >   */
> > -struct drm_xe_query_gt_list {
> > +struct drm_xe_query_gt {
> 
> In my opinion it should be drm_xe_query_gts or keep drm_xe_query_gt_list

Cc: Matt Roper <matthew.d.roper@intel.com>

I honestly prefer gts better to align better with the plural of
everything else. But Matt's point is that a plural of an acronym
looks another acronym and people might ask what is "gee-tee-ess"?

so, I like Francois attempt to align them all in a consistent way.
But maybe we need to keep plural everywhere and accept "gts"?

> 
> >  	/** @num_gt: number of GT items returned in gt_list */
> >  	__u32 num_gt;
> >  	/** @pad: MBZ */
> >  	__u32 pad;
> >  	/** @gt_list: The GT list returned for this device */
> > -	struct drm_xe_query_gt gt_list[];
> > +	struct drm_xe_gt gt_list[];
> >  };
> >  
> >  /**
> > @@ -495,7 +495,7 @@ struct drm_xe_device_query {
> >  #define DRM_XE_DEVICE_QUERY_ENGINES		0
> >  #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_GT			3
> >  #define DRM_XE_DEVICE_QUERY_HWCONFIG		4
> >  #define DRM_XE_DEVICE_QUERY_GT_TOPOLOGY		5
> >  #define DRM_XE_DEVICE_QUERY_ENGINE_CYCLES	6
>