[PATCH 2/5] drm/syncobj: add sync obj wait interface. (v3)
Chris Wilson
chris at chris-wilson.co.uk
Thu May 25 08:47:54 UTC 2017
On Wed, May 24, 2017 at 05:06:12PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> This interface will allow sync object to be used to back
> Vulkan fences. This API is pretty much the vulkan fence waiting
> API, and I've ported the code from amdgpu.
>
> v2: accept relative timeout, pass remaining time back
> to userspace.
> v3: return to absolute timeouts.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
> drivers/gpu/drm/drm_internal.h | 2 +
> drivers/gpu/drm/drm_ioctl.c | 2 +
> drivers/gpu/drm/drm_syncobj.c | 164 +++++++++++++++++++++++++++++++++++++++++
> include/uapi/drm/drm.h | 14 ++++
> 4 files changed, 182 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 3fdef2c..53e3f6b 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -156,3 +156,5 @@ int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_private);
> int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_private);
> +int drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index f1e5681..385ce74 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl,
> + DRM_UNLOCKED|DRM_RENDER_ALLOW),
> };
>
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index b611480..8b87594 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1,5 +1,7 @@
> /*
> * Copyright 2017 Red Hat
> + * Parts ported from amdgpu (fence wait code).
> + * Copyright 2016 Advanced Micro Devices, Inc.
> *
> * Permission is hereby granted, free of charge, to any person obtaining a
> * copy of this software and associated documentation files (the "Software"),
> @@ -31,6 +33,9 @@
> * that contain an optional fence. The fence can be updated with a new
> * fence, or be NULL.
> *
> + * syncobj's can be waited upon, where it will wait for the underlying
> + * fence.
> + *
> * syncobj's can be export to fd's and back, these fd's are opaque and
> * have no other use case, except passing the syncobj between processes.
> *
> @@ -375,3 +380,162 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> return drm_syncobj_fd_to_handle(file_private, args->fd,
> &args->handle);
> }
> +
> +
> +/**
> + * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute value
> + *
> + * @timeout_ns: timeout in ns
> + *
> + * Calculate the timeout in jiffies from an absolute timeout in ns.
> + */
> +unsigned long drm_timeout_abs_to_jiffies(uint64_t timeout_ns)
Not in any header or otherwise exported, so static?
> +{
> + unsigned long timeout_jiffies;
> + ktime_t timeout;
> +
> + /* clamp timeout if it's to large */
> + if (((int64_t)timeout_ns) < 0)
> + return MAX_SCHEDULE_TIMEOUT;
> +
> + timeout = ktime_sub(ns_to_ktime(timeout_ns), ktime_get());
> + if (ktime_to_ns(timeout) < 0)
> + return 0;
> +
> + timeout_jiffies = nsecs_to_jiffies(ktime_to_ns(timeout));
> + /* clamp timeout to avoid unsigned-> signed overflow */
> + if (timeout_jiffies > MAX_SCHEDULE_TIMEOUT )
> + return MAX_SCHEDULE_TIMEOUT - 1;
This about avoiding the conversion into an infinite timeout, right?
I think the comment is misleading (certainly doesn't explain
MAX_SCHEDULE_TIMEOUT-1) and the test should be >= MAX_SCHEDULE_TIMEOUT.
> +
> + return timeout_jiffies;
Timeouts tend to use timeout_jiffies + 1 to avoid the timer interrupt
screwing up the calculation, or just generally returning a fraction too
early.
> +}
> +
> +static int drm_syncobj_wait_all_fences(struct drm_device *dev,
> + struct drm_file *file_private,
> + struct drm_syncobj_wait *wait,
> + uint32_t *handles)
> +{
> + uint32_t i;
> + int ret;
> + unsigned long timeout = drm_timeout_abs_to_jiffies(wait->timeout_ns);
Also note that this doesn't handle timeout = 0 very gracefully with
multiple fences. (dma_fence_wait_timeout will convert that on return to
1). Hmm, by using an absolute timeout we can't pass into timeout=0 to do a
poll.
> +
> + for (i = 0; i < wait->count_handles; i++) {
> + struct dma_fence *fence;
> +
> + ret = drm_syncobj_fence_get(file_private, handles[i],
> + &fence);
> + if (ret)
> + return ret;
> +
> + if (!fence)
> + continue;
I thought no fence for the syncobj was the *unsignaled* case, and to
wait upon it was a user error. I second Jason's idea to make the lookup
and error checking on the fences uniform between WAIT_ALL and WAIT_ANY.
> + ret = dma_fence_wait_timeout(fence, true, timeout);
> +
> + dma_fence_put(fence);
> + if (ret < 0)
> + return ret;
> + if (ret == 0)
> + break;
> + timeout = ret;
> + }
> +
> + wait->out_timeout_ns = jiffies_to_nsecs(ret);
> + wait->out_status = (ret > 0);
> + wait->first_signaled = 0;
> + return 0;
> +}
> +
> +static int drm_syncobj_wait_any_fence(struct drm_device *dev,
> + struct drm_file *file_private,
> + struct drm_syncobj_wait *wait,
> + uint32_t *handles)
> +{
> + unsigned long timeout = drm_timeout_abs_to_jiffies(wait->timeout_ns);
> + struct dma_fence **array;
> + uint32_t i;
> + int ret;
> + uint32_t first = ~0;
> +
> + /* Prepare the fence array */
> + array = kcalloc(wait->count_handles,
> + sizeof(struct dma_fence *), GFP_KERNEL);
> +
> + if (array == NULL)
> + return -ENOMEM;
> +
> + for (i = 0; i < wait->count_handles; i++) {
> + struct dma_fence *fence;
> +
> + ret = drm_syncobj_fence_get(file_private, handles[i],
> + &fence);
> + if (ret)
> + goto err_free_fence_array;
> + else if (fence)
> + array[i] = fence;
> + else { /* NULL, the fence has been already signaled */
> + ret = 1;
> + goto out;
> + }
> + }
> +
> + ret = dma_fence_wait_any_timeout(array, wait->count_handles, true, timeout,
> + &first);
> + if (ret < 0)
> + goto err_free_fence_array;
> +out:
> + wait->out_timeout_ns = jiffies_to_nsecs(ret);
> + wait->out_status = (ret > 0);
Didn't the amdgpu interface report which fence completed first? (I may
be imagining that.)
> + wait->first_signaled = first;
> + /* set return value 0 to indicate success */
> + ret = 0;
> +
> +err_free_fence_array:
> + for (i = 0; i < wait->count_handles; i++)
> + dma_fence_put(array[i]);
> + kfree(array);
> +
> + return ret;
> +}
> +
> +int
> +drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private)
> +{
> + struct drm_syncobj_wait *args = data;
> + uint32_t *handles;
> + int ret = 0;
> +
> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + return -ENODEV;
> +
> + if (args->flags != 0 && args->flags != DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)
> + return -EINVAL;
> +
> + if (args->count_handles == 0)
> + return 0;
Hmm, returning success without updating any of the status fields.
-EINVAL? -ENOTHINGTODO.
> +
> + /* Get the handles from userspace */
> + handles = kmalloc_array(args->count_handles, sizeof(uint32_t),
> + GFP_KERNEL);
> + if (handles == NULL)
> + return -ENOMEM;
> +
> + if (copy_from_user(handles,
> + (void __user *)(unsigned long)(args->handles),
> + sizeof(uint32_t) * args->count_handles)) {
> + ret = -EFAULT;
> + goto err_free_handles;
> + }
> +
> + if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)
> + ret = drm_syncobj_wait_all_fences(dev, file_private,
> + args, handles);
> + else
> + ret = drm_syncobj_wait_any_fence(dev, file_private,
> + args, handles);
> +err_free_handles:
> + kfree(handles);
> +
> + return ret;
> +}
--
Chris Wilson, Intel Open Source Technology Centre
More information about the dri-devel
mailing list