[PATCH 2/5] drm/syncobj: add sync obj wait interface. (v3)
Christian König
deathsimple at vodafone.de
Thu May 25 16:42:05 UTC 2017
Am 25.05.2017 um 10:47 schrieb Chris Wilson:
> 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?
That function should be exported and the version we have in amdgpu
replaced by using this instead.
But feel free to make it static for now, I can take care of the cleanup
later on.
Christian.
>
>> +{
>> + 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;
>> +}
More information about the amd-gfx
mailing list