[Intel-xe] [PATCH v2 03/14] drm/xe: Make DRM_XE_DEVICE_QUERY_ENGINES future proof
Rodrigo Vivi
rodrigo.vivi at intel.com
Wed Nov 29 22:52:17 UTC 2023
On Wed, Nov 29, 2023 at 03:04:37PM -0500, Souza, Jose wrote:
> On Wed, 2023-11-29 at 12:35 +0000, Matthew Brost wrote:
> > On Wed, Nov 29, 2023 at 11:54:51AM -0500, Rodrigo Vivi wrote:
> > > On Tue, Nov 28, 2023 at 09:17:51PM +0000, Matthew Brost wrote:
> > > > 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.
> > >
> > > my bad, sorry. just trying to prepare for the many capabilities and other
> > > stuff we will likely need. But likely 3 would be enough.
> > >
> > > >
> > > > Anyways agree this better but what if we improve this a bit more making
> > > > each __drm_xe_query_engine_info a dynamic size.
> > >
> > > hmmm... I think I like this idea. pahole seems to have a good output
> > > with our other existent dynamic array.
> > >
> >
> > Cool, looking forward to what we come up with.
>
> v2 of drm_xe_query_engine_info would be something like this?
>
> struct drm_xe_query_engine_info {
> struct drm_xe_engine_class_instance instance;
> __u64 foo;
> __u64 reserved[0];
> }
something like that. But when I was coding it here I was not comfortable with
having an open ended reserved field.
>
> maybe already add a pad enough for the OA counter and media engine capabilities usages and leave reserved[0] at the end for future.
Maybe just reduce that to 2 or 3 u64?
>
>
> >
> > > >
> > > > 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];
> > >
> > > I believe [] instead of [0] is the preferred way nowadays.
> > >
> >
> > Yep.
> >
> > Matt
> >
> > > > };
> > > >
> > > > 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
> > > > >
>
More information about the Intel-xe
mailing list