Re: [Intel-xe] [PATCH v2 45/50] drm/xe/uapi: Remove bogus engine list from the wait_user_fence IOCTL

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

On Tue, Nov 07, 2023 at 04:05:54PM -0800, Welty, Brian wrote:
> 
> 
> On 11/3/2023 7:34 AM, Francois Dugast wrote:
> > From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > 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.
> > 
> > Upon timeout, userspace need then to inspect the engine properties
> > like BAN, in order to determine the reset status and any other
> > information that can be (or be added) there.
> 
> I think the point of a per-engine wait was for long-running context?
> In that case, a large timeout value would be specified...  and then if
> engine is reset (due to hang for example), it would abort the wait_ufence
> early and return some error other than -ETIME.
> But as you say, understand this is not even implemented right now so
> makes sense to delete it.
> 
> If this is indeed needed in future, this can be restored again using
> the drm_xe_wait_user_fence.extensions, correct?

yes, we could do that. or even returning earlier with a different error
wouldn't break the compatibility, and then the userspace inspecting the
engine status with the properties.

> 
> -Brian
> 
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_wait_user_fence.c | 56 +------------------------
> >   include/uapi/drm/xe_drm.h               | 17 +-------
> >   2 files changed, 3 insertions(+), 70 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > index dcbb1c578b22..a9d231548498 100644
> > --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > @@ -58,29 +58,7 @@ static const enum xe_engine_class user_to_xe_engine_class[] = {
> >   	[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].sched_group_id >= xe->info.tile_count)
> > -			return -EINVAL;
> > -
> > -		if (!xe_gt_hw_engine(xe_device_get_gt(xe, eci[i].sched_group_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 +110,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 +125,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 757e6da97f87..aada6f75b905 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -1278,8 +1278,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;
> > @@ -1312,20 +1311,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;
> > -
> >   	/** @reserved: Reserved */
> > -	__u64 reserved[2];
> > +	__u64 reserved[4];
> >   };
> >   /**