[Intel-xe] [PATCH] drm/xe: Use milliseconds instead of jiffies in uapi for user fence

[Andi Shyti]

Hi Zbigniew,

On Fri, May 19, 2023 at 12:01:26PM +0200, Zbigniew Kempczyński wrote:
> Using jiffies as a timeout from userspace is weird even it theoretically
> exists possiblity of acquiring jiffies via getconf. Unfortunately this
> method is unreliable and returned value may vary from configured in the
> kernel config.
> 
> Due to kernel constraints regarding milliseconds type (unsigned int)
> conversion to jiffies and uapi timeout defined as u64 return an error
> if timeout exceeds unsigned int size.

Thus... this should go on a different patch.

> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_wait_user_fence.c | 13 +++++++++++--
>  include/uapi/drm/xe_drm.h               |  2 +-
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> index 15c2e5aa08d2..585681b5fa9a 100644
> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> @@ -79,6 +79,13 @@ static int check_hw_engines(struct xe_device *xe,
>  	return 0;
>  }
>  
> +static unsigned long msecs_to_jiffies_timeout(const unsigned int m)
> +{
> +	unsigned long j = msecs_to_jiffies(m);
> +
> +	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> +}
> +

mmhhh... this is a copy paste from i915 and given that there are
already two drivers using it, I think this should go in the
library in jiffies.h.

Besides, do we really need this? already msecs_to_jiffies().
performs this check.

Andi

>  #define VALID_FLAGS	(DRM_XE_UFENCE_WAIT_SOFT_OP | \
>  			 DRM_XE_UFENCE_WAIT_ABSTIME | \
>  			 DRM_XE_UFENCE_WAIT_VM_ERROR)
> @@ -98,7 +105,7 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>  	int err;
>  	bool no_engines = args->flags & DRM_XE_UFENCE_WAIT_SOFT_OP ||
>  		args->flags & DRM_XE_UFENCE_WAIT_VM_ERROR;
> -	unsigned long timeout = args->timeout;
> +	unsigned long timeout;
>  
>  	if (XE_IOCTL_ERR(xe, args->extensions))
>  		return -EINVAL;
> @@ -148,9 +155,11 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>  		addr = vm->async_ops.error_capture.addr;
>  	}
>  
> -	if (XE_IOCTL_ERR(xe, timeout > MAX_SCHEDULE_TIMEOUT))
> +	if (XE_IOCTL_ERR(xe, args->timeout > UINT_MAX)
>  		return -EINVAL;
>  
> +	timeout = msecs_to_jiffies_timeout(args->timeout);
> +
>  	/*
>  	 * FIXME: Very simple implementation at the moment, single wait queue
>  	 * for everything. Could be optimized to have a wait queue for every
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index b0b80aae3ee8..3e6f679ab534 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -726,7 +726,7 @@ struct drm_xe_wait_user_fence {
>  #define DRM_XE_UFENCE_WAIT_U32		0xffffffffu
>  #define DRM_XE_UFENCE_WAIT_U64		0xffffffffffffffffu
>  	__u64 mask;
> -	/** @timeout: how long to wait before bailing, value in jiffies */
> +	/** @timeout: how long to wait before bailing, value in milliseconds */
>  	__s64 timeout;
>  	/**
>  	 * @num_engines: number of engine instances to wait on, must be zero
> -- 
> 2.40.0