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

[Zbigniew Kempczyński]

On Fri, May 19, 2023 at 02:51:54PM +0200, Andi Shyti wrote:
> 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.

Thank you for the review.

Previously I wanted to treat 0 as at least 1 jiffy to avoid
getting -ETIME for the call. But that's the user responsibility
to properly configure required timeout as positive integer so
msecs_to_jiffies_timeout() isn't really needed here. Negative means
neverending sleep so user has wide spectrum of timeout to choose.

Real problem with this patch is uapi change. Normally msecs passed
via ioctl() should be integer (s32) but I would break current umd
which uses s64 right now (too big). I wondered to split to s32 +
s32 (reserved) but I should handle little/big endianess and I'm
not sure such constructs should be introduced.

Anyway I tested this patch without _timeout() and values:
* 0: -ETIME
* positive <= UINT_MAX -> real timeout, works fine
* positive > UINT_MAX -> -EINVAL
* negative -> MAX_JIFFY_OFFSET, unlimited timeout

The best would be change to s32 to avoid third case (-EINVAL)
but this requires umd recompilation.

--
Zbigniew

> 
> 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