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

[Andi Shyti]

Hi Zbigniew,

On Wed, May 31, 2023 at 09:21:47PM +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.
> 
> Change alters timeout to be expected in nanoseconds returning remaining
> timeout for relative and elapsed for absolute.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> Cc: Andi Shyti <andi.shyti at linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_wait_user_fence.c | 39 +++++++++++++++++++------
>  include/uapi/drm/xe_drm.h               |  5 +++-
>  2 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> index 6c8a60c60087..e0db0ba904ec 100644
> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> @@ -7,6 +7,7 @@
>  
>  #include <drm/drm_device.h>
>  #include <drm/drm_file.h>
> +#include <drm/drm_utils.h>
>  #include <drm/xe_drm.h>
>  
>  #include "xe_device.h"
> @@ -84,6 +85,24 @@ static int check_hw_engines(struct xe_device *xe,
>  			 DRM_XE_UFENCE_WAIT_VM_ERROR)
>  #define MAX_OP		DRM_XE_UFENCE_WAIT_LTE
>  
> +static unsigned long to_jiffies_timeout(struct drm_xe_wait_user_fence *args)
> +{
> +	unsigned long timeout;
> +
> +	if (args->flags & DRM_XE_UFENCE_WAIT_ABSTIME)
> +		return drm_timeout_abs_to_jiffies(args->timeout);
> +
> +	if (args->timeout < 0)
> +		return MAX_SCHEDULE_TIMEOUT;

are we talking hundreds of years?

> +	if (args->timeout == 0)
> +		return 0;

why do you need this? nsec_to_jiffies will return 0 and this
function would return '0'. Right?

> +
> +	timeout = nsecs_to_jiffies(args->timeout);
> +
> +	return timeout ?: 1;
> +}
> +
>  int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>  			     struct drm_file *file)
>  {
> @@ -98,7 +117,8 @@ 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;
> +	ktime_t start;
>  
>  	if (XE_IOCTL_ERR(xe, args->extensions) || XE_IOCTL_ERR(xe, args->pad) ||
>  	    XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
> @@ -149,8 +169,8 @@ 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))
> -		return -EINVAL;
> +	timeout = to_jiffies_timeout(args);
> +	start = ktime_get();
>  
>  	/*
>  	 * FIXME: Very simple implementation at the moment, single wait queue
> @@ -194,12 +214,13 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>  	else if (XE_IOCTL_ERR(xe, !timeout))
>  		return -ETIME;
>  
> -	/*
> -	 * Again very simple, return the time in jiffies that has past, may need
> -	 * a more precision
> -	 */
> -	if (args->flags & DRM_XE_UFENCE_WAIT_ABSTIME)
> -		args->timeout = args->timeout - timeout;
> +	if (args->flags & DRM_XE_UFENCE_WAIT_ABSTIME) {
> +		args->timeout = ktime_sub(ktime_get(), start);
> +	} else {
> +		args->timeout -= ktime_to_ns(ktime_sub(ktime_get(), start));
> +		if (args->timeout < 0)
> +			args->timeout = 0;
> +	}
>  
>  	return 0;
>  }
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index d5fc54b5be74..5c81e595c8bb 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -752,7 +752,10 @@ 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 nanoseconds.
> +	 * Returns timeout left for relative or elapsed for absolute.

It doesn't return anything... maybe "Contains the timeout..." is
better?

Rest looks good.

Andi