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

[Welty, Brian]

On 11/3/2023 7:34 AM, Francois Dugast wrote:
> From: Rodrigo Vivi <rodrigo.vivi at 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?

-Brian

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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];
>   };
>   
>   /**