[Intel-xe] [PATCH v1 6/8] drm/xe/uapi: Align on a common way to return arrays (gt)
Matt Roper
matthew.d.roper at intel.com
Thu Nov 16 21:28:24 UTC 2023
On Thu, Nov 16, 2023 at 03:56:29PM -0500, Rodrigo Vivi wrote:
> 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 at 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 at 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"?
When we were first upstreaming multi-GT stuff for i915, there was some
pretty firm push-back on the "gts" usage that we had in the original
patches. I think the discussion was kicked off on the mailing list by
Jani here: https://lore.kernel.org/intel-gfx/87pmrqj3va.fsf@intel.com/
but I believe there was some followup discussion and agreement with more
people on IRC that we should try to avoid the potentially ambiguous
plural "gts."
Personally I find having "query [singular] foo" return a list of foos
somewhat surprising, and would have expected something more like
"drm_xe_query_foo_list" or "drm_xe_query_each_foo." But I don't really
have strong feelings about it; since this is uapi, whatever the
userspace guys think is most understandable is fine with me.
Matt
>
> >
> > > /** @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
> >
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list