[Intel-xe] [PATCH v3 1/2] drm/xe/uapi: add exec_queue_id member to drm_xe_wait_user_fence structure
Francois Dugast
francois.dugast at intel.com
Wed Dec 6 14:55:36 UTC 2023
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
> >
> > /** @reserved: Reserved */
> > __u64 reserved[2];
> > --
> > 2.25.1
> >
More information about the Intel-xe
mailing list