[Intel-xe] [PATCH v3 1/2] drm/xe/uapi: add exec_queue_id member to drm_xe_wait_user_fence structure
Bommu, Krishnaiah
krishnaiah.bommu at intel.com
Wed Dec 6 16:58:09 UTC 2023
> -----Original Message-----
> From: Dugast, Francois <francois.dugast at intel.com>
> Sent: Wednesday, December 6, 2023 8:26 PM
> To: Bommu, Krishnaiah <krishnaiah.bommu at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [Intel-xe] [PATCH v3 1/2] drm/xe/uapi: add exec_queue_id member
> to drm_xe_wait_user_fence structure
>
> On Wed, Dec 06, 2023 at 10:37:24AM +0100, Francois Dugast wrote:
> > Hi Bommu,
> >
> > On Wed, Dec 06, 2023 at 09:11:29AM +0530, Bommu Krishnaiah wrote:
> > > remove the num_engines/instances members from drm_xe_wait_user_fence
> > > structure and add a exec_queue_id member
> > >
> > > Right now this is only checking if the engine list is sane and
> > > nothing else. In the end every operation with this IOCTL is a soft check.
> > > So, let's formalize that and only use this IOCTL to wait on the fence.
> > >
> > > exec_queue_id member will help to user space to get proper error
> > > code from kernel while in exec_queue reset
> > >
> > > Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu at intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Signed-off-by: Francois Dugast <francois.dugast at intel.com>
> > > Acked-by: Matthew Brost <matthew.brost at intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_wait_user_fence.c | 64 +------------------------
> > > include/uapi/drm/xe_drm.h | 16 ++-----
> > > 2 files changed, 4 insertions(+), 76 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > > b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > > index 4d5c2555ce41..9f272bbaad95 100644
> > > --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > > +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > > @@ -50,37 +50,7 @@ static int do_compare(u64 addr, u64 value, u64 mask,
> u16 op)
> > > return passed ? 0 : 1;
> > > }
> > >
> > > -static const enum xe_engine_class user_to_xe_engine_class[] = {
> > > - [DRM_XE_ENGINE_CLASS_RENDER] = XE_ENGINE_CLASS_RENDER,
> > > - [DRM_XE_ENGINE_CLASS_COPY] = XE_ENGINE_CLASS_COPY,
> > > - [DRM_XE_ENGINE_CLASS_VIDEO_DECODE] =
> XE_ENGINE_CLASS_VIDEO_DECODE,
> > > - [DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE] =
> XE_ENGINE_CLASS_VIDEO_ENHANCE,
> > > - [DRM_XE_ENGINE_CLASS_COMPUTE] = XE_ENGINE_CLASS_COMPUTE,
> > > -};
> > > -
> > > -static int check_hw_engines(struct xe_device *xe,
> > > - struct drm_xe_engine_class_instance *eci,
> > > - int num_engines)
> > > -{
> > > - int i;
> > > -
> > > - for (i = 0; i < num_engines; ++i) {
> > > - enum xe_engine_class user_class =
> > > - user_to_xe_engine_class[eci[i].engine_class];
> > > -
> > > - if (eci[i].gt_id >= xe->info.tile_count)
> > > - return -EINVAL;
> > > -
> > > - if (!xe_gt_hw_engine(xe_device_get_gt(xe, eci[i].gt_id),
> > > - user_class, eci[i].engine_instance, true))
> > > - return -EINVAL;
> > > - }
> > > -
> > > - return 0;
> > > -}
> > > -
> > > -#define VALID_FLAGS (DRM_XE_UFENCE_WAIT_FLAG_SOFT_OP | \
> > > - DRM_XE_UFENCE_WAIT_FLAG_ABSTIME)
> > > +#define VALID_FLAGS DRM_XE_UFENCE_WAIT_FLAG_ABSTIME
> > > #define MAX_OP DRM_XE_UFENCE_WAIT_OP_LTE
> > >
> > > static long to_jiffies_timeout(struct xe_device *xe, @@ -132,12
> > > +102,8 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void
> *data,
> > > struct xe_device *xe = to_xe_device(dev);
> > > DEFINE_WAIT_FUNC(w_wait, woken_wake_function);
> > > struct drm_xe_wait_user_fence *args = data;
> > > - struct drm_xe_engine_class_instance
> eci[XE_HW_ENGINE_MAX_INSTANCE];
> > > - struct drm_xe_engine_class_instance __user *user_eci =
> > > - u64_to_user_ptr(args->instances);
> > > u64 addr = args->addr;
> > > int err;
> > > - bool no_engines = args->flags &
> DRM_XE_UFENCE_WAIT_FLAG_SOFT_OP;
> > > long timeout;
> > > ktime_t start;
> > >
> > > @@ -151,41 +117,13 @@ int xe_wait_user_fence_ioctl(struct drm_device
> *dev, void *data,
> > > if (XE_IOCTL_DBG(xe, args->op > MAX_OP))
> > > return -EINVAL;
> > >
> > > - if (XE_IOCTL_DBG(xe, no_engines &&
> > > - (args->num_engines || args->instances)))
> > > - return -EINVAL;
> > > -
> > > - if (XE_IOCTL_DBG(xe, !no_engines && !args->num_engines))
> > > - return -EINVAL;
> > > -
> > > if (XE_IOCTL_DBG(xe, addr & 0x7))
> > > return -EINVAL;
> > >
> > > - if (XE_IOCTL_DBG(xe, args->num_engines >
> XE_HW_ENGINE_MAX_INSTANCE))
> > > - return -EINVAL;
> > > -
> > > - if (!no_engines) {
> > > - err = copy_from_user(eci, user_eci,
> > > - sizeof(struct drm_xe_engine_class_instance)
> *
> > > - args->num_engines);
> > > - if (XE_IOCTL_DBG(xe, err))
> > > - return -EFAULT;
> > > -
> > > - if (XE_IOCTL_DBG(xe, check_hw_engines(xe, eci,
> > > - args->num_engines)))
> > > - return -EINVAL;
> > > - }
> > > -
> > > timeout = to_jiffies_timeout(xe, args);
> > >
> > > start = ktime_get();
> > >
> > > - /*
> > > - * FIXME: Very simple implementation at the moment, single wait queue
> > > - * for everything. Could be optimized to have a wait queue for every
> > > - * hardware engine. Open coding as 'do_compare' can sleep which
> doesn't
> > > - * work with the wait_event_* macros.
> > > - */
> > > add_wait_queue(&xe->ufence_wq, &w_wait);
> > > for (;;) {
> > > err = do_compare(addr, args->value, args->mask, args->op);
> diff
> > > --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h index
> > > eb03a49c17a1..d922641e11e8 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -1030,8 +1030,7 @@ struct drm_xe_wait_user_fence {
> > > /** @op: wait operation (type of comparison) */
> > > __u16 op;
> > >
> > > -#define DRM_XE_UFENCE_WAIT_FLAG_SOFT_OP (1 << 0) /* e.g. Wait
> on VM bind */
> > > -#define DRM_XE_UFENCE_WAIT_FLAG_ABSTIME (1 << 1)
> > > +#define DRM_XE_UFENCE_WAIT_FLAG_ABSTIME (1 << 0)
> > > /** @flags: wait flags */
> > > __u16 flags;
> > >
> > > @@ -1064,17 +1063,8 @@ struct drm_xe_wait_user_fence {
> > > */
> > > __s64 timeout;
> > >
> > > - /**
> > > - * @num_engines: number of engine instances to wait on, must be zero
> > > - * when DRM_XE_UFENCE_WAIT_FLAG_SOFT_OP set
> > > - */
> > > - __u64 num_engines;
> > > -
> > > - /**
> > > - * @instances: user pointer to array of drm_xe_engine_class_instance
> to
> > > - * wait on, must be NULL when
> DRM_XE_UFENCE_WAIT_FLAG_SOFT_OP set
> > > - */
> > > - __u64 instances;
> > > + /** @exec_queue_id: exec_queue_id returned from
> xe_exec_queue_create_ioctl */
> > > + __u32 exec_queue_id;
> >
> > This creates a misalignment. Please add a __u32 pad2. With this:
> >
> > Reviewed-by: Francois Dugast <francois.dugast at intel.com>
> >
>
> The kernel doc in the header update in IGT seems incorrect:
> https://patchwork.freedesktop.org/patch/570752/?series=127364&rev=3
>
> Please make sure it matches the name pad2, so:
>
> /** @reserved: Reserved */
> __u32 pad2;
>
> should be:
>
> /** @pad2: MBZ */
> __u32 pad2;
>
> and just like for pad, pad2 must be zero and this must be checked:
>
> XE_IOCTL_DBG(xe, args->pad2)
>
> Francois
>
Thank you @Dugast, Francois for review
I will update as below
root at kbommu-desk:/home/kbommu/xe_public/kernel#
root at kbommu-desk:/home/kbommu/xe_public/kernel# git diff
diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
index 74f72dad2d0b..085b46ebe2f8 100644
--- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
+++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
@@ -123,6 +123,9 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
if (XE_IOCTL_DBG(xe, addr & 0x7))
return -EINVAL;
+ XE_IOCTL_DBG(xe, args->pad2)
+ return -EINVAL;
+
q = xe_exec_queue_lookup(xef, args->exec_queue_id);
if (XE_IOCTL_DBG(xe, !q))
return -ENOENT;
diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
index d922641e11e8..385c36e13cf2 100644
--- a/include/uapi/drm/xe_drm.h
+++ b/include/uapi/drm/xe_drm.h
@@ -1066,6 +1066,9 @@ struct drm_xe_wait_user_fence {
/** @exec_queue_id: exec_queue_id returned from xe_exec_queue_create_ioctl */
__u32 exec_queue_id;
+ /** @pad2: MBZ */
+ __u32 pad2;
+
/** @reserved: Reserved */
__u64 reserved[2];
};
root at kbommu-desk:/home/kbommu/xe_public/kernel#
> > >
> > > /** @reserved: Reserved */
> > > __u64 reserved[2];
> > > --
> > > 2.25.1
> > >
More information about the Intel-xe
mailing list