[PATCH] drm/syncobj: add sync obj wait interface. (v6)
Christian König
deathsimple at vodafone.de
Mon Jul 10 16:13:12 UTC 2017
Am 10.07.2017 um 17:58 schrieb Xie, AlexBin:
>
> I understand this discussion from closes source driver terminology.
>
> If a process is killed before it sends out the signaling command, will
> some part of the GPU be in a waiting situation forever?
>
Yes, exactly that's the problem here and the reason why that even
Microsoft forbids that under windows.
Christian.
> Alex Bin Xie
>
> *From:*amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] *On
> Behalf Of *Jason Ekstrand
> *Sent:* Monday, July 10, 2017 11:53 AM
> *To:* Christian König <deathsimple at vodafone.de>
> *Cc:* Dave Airlie <airlied at gmail.com>; Maling list - DRI developers
> <dri-devel at lists.freedesktop.org>; amd-gfx mailing list
> <amd-gfx at lists.freedesktop.org>
> *Subject:* Re: [PATCH] drm/syncobj: add sync obj wait interface. (v6)
>
> On Mon, Jul 10, 2017 at 8:45 AM, Christian König
> <deathsimple at vodafone.de <mailto:deathsimple at vodafone.de>> wrote:
>
> Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:
>
> On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie <airlied at gmail.com
> <mailto:airlied at gmail.com>> wrote:
>
> From: Dave Airlie <airlied at redhat.com
> <mailto: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.
> v4: absolute zero = poll,
> rewrite any/all code to have same operation for arrays
> return -EINVAL for 0 fences.
> v4.1: fixup fences allocation check, use u64_to_user_ptr
> v5: move to sec/nsec, and use timespec64 for calcs.
> v6: use -ETIME and drop the out status flag. (-ETIME
> is suggested by ickle, I can feel a shed painting)
>
> Signed-off-by: Dave Airlie <airlied at redhat.com
> <mailto:airlied at redhat.com>>
> ---
> drivers/gpu/drm/drm_internal.h | 2 +
> drivers/gpu/drm/drm_ioctl.c | 2 +
> drivers/gpu/drm/drm_syncobj.c | 142
> +++++++++++++++++++++++++++++++++++++++++
> include/uapi/drm/drm.h | 13 ++++
> 4 files changed, 159 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_internal.h
> b/drivers/gpu/drm/drm_internal.h
> index 5cecc97..d71b50d 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -157,3 +157,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 89441bc..2d5a7a1 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.
> *
> @@ -451,3 +456,140 @@
> 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_sec: timeout sec component, 0 for poll
> + * @timeout_nsec: timeout nsec component in ns, 0 for poll
> + * both must be 0 for poll.
> + *
> + * Calculate the timeout in jiffies from an absolute time
> in sec/nsec.
> + */
> +static unsigned long drm_timeout_abs_to_jiffies(int64_t
> timeout_sec, uint64_t timeout_nsec)
> +{
> + struct timespec64 abs_timeout, timeout,
> max_jiffy_timespec;
> + unsigned long timeout_jiffies;
> +
> + /* make 0 timeout means poll - absolute 0 doesn't
> seem valid */
> + if (timeout_sec == 0 && timeout_nsec == 0)
> + return 0;
> +
> + abs_timeout.tv_sec = timeout_sec;
> + abs_timeout.tv_nsec = timeout_nsec;
> +
> + /* clamp timeout if it's to large */
> + if (!timespec64_valid_strict(&abs_timeout))
> + return MAX_SCHEDULE_TIMEOUT - 1;
> +
> + timeout = timespec64_sub(abs_timeout,
> ktime_to_timespec64(ktime_get()));
> + if (!timespec64_valid(&timeout))
> + return 0;
> +
> + jiffies_to_timespec64(MAX_JIFFY_OFFSET,
> &max_jiffy_timespec);
> + if (timespec64_compare(&timeout,
> &max_jiffy_timespec) >= 0)
> + return MAX_SCHEDULE_TIMEOUT - 1;
> +
> + timeout_jiffies = timespec64_to_jiffies(&timeout);
> + /* clamp timeout to avoid infinite timeout */
> + if (timeout_jiffies >= MAX_SCHEDULE_TIMEOUT)
> + return MAX_SCHEDULE_TIMEOUT - 1;
> +
> + return timeout_jiffies + 1;
> +}
> +
> +static int drm_syncobj_wait_fences(struct drm_device *dev,
> + struct drm_file *file_private,
> + struct drm_syncobj_wait *wait,
> + struct dma_fence **fences)
> +{
> + unsigned long timeout =
> drm_timeout_abs_to_jiffies(wait->timeout_sec,
> wait->timeout_nsec);
> + int ret = 0;
> + uint32_t first = ~0;
> +
> + if (wait->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) {
> + int i;
> + for (i = 0; i < wait->count_handles; i++) {
> + ret =
> dma_fence_wait_timeout(fences[i], true, timeout);
> +
> + if (ret < 0)
> + return ret;
> + if (ret == 0)
> + break;
> + timeout = ret;
> + }
> + first = 0;
> + } else {
> + ret = dma_fence_wait_any_timeout(fences,
> + wait->count_handles,
> + true, timeout,
> + &first);
> + }
> +
> + if (ret < 0)
> + return ret;
> +
> + wait->first_signaled = first;
> + if (ret == 0)
> + return -ETIME;
> + return 0;
> +}
> +
> +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;
> + struct dma_fence **fences;
> + int ret = 0;
> + int i;
> +
> + 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 -EINVAL;
> +
> + /* 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,
> + u64_to_user_ptr(args->handles),
> + sizeof(uint32_t) * args->count_handles)) {
> + ret = -EFAULT;
> + goto err_free_handles;
> + }
> +
> + fences = kcalloc(args->count_handles,
> + sizeof(struct dma_fence *),
> GFP_KERNEL);
> + if (!fences) {
> + ret = -ENOMEM;
> + goto err_free_handles;
> + }
> +
> + for (i = 0; i < args->count_handles; i++) {
> + ret = drm_syncobj_fence_get(file_private,
> handles[i],
> + &fences[i]);
> + if (ret)
> + goto err_free_fence_array;
> + }
> +
> + ret = drm_syncobj_wait_fences(dev, file_private,
> + args, fences);
>
> So, reading some CTS tests again, and I think we have a
> problem here. The Vulkan spec allows you to wait on a fence
> that is in the unsignaled state.
>
> At least on the closed source driver that would be illegal as far
> as I know.
>
> Then they are doing workarounds in userspace. There are definitely
> CTS tests for this:
>
> https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74
>
> You can't wait on a semaphore before the signal operation is send
> down to the kerel.
>
> We (Intel) deal with this today by tracking whether or not the fence
> has been submitted and using a condition variable in userspace to sort
> it all out. If we ever want to share fences across processes (which
> we do), then this needs to be sorted in the kernel.
>
> --Jason
>
> Regards,
> Christian.
>
>
>
>
> In theory, you could have thread A start waiting on a fence
> before thread B submits the work which triggers that fence.
> This means that the dma_fence may not exist yet when
> vkWaitForFences gets called. If we really want to support the
> full Vulkan usage, we need to somehow support missing
> dma_fences by waiting for the dma_fence to show up.
> Unfortunately, I don't know enough about the internal kernel
> APIs to know what that would look like.
>
> +
> +err_free_fence_array:
> + for (i = 0; i < args->count_handles; i++)
> + dma_fence_put(fences[i]);
> + kfree(fences);
> +err_free_handles:
> + kfree(handles);
> +
> + return ret;
> +}
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 101593a..91746a7 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -718,6 +718,18 @@ struct drm_syncobj_handle {
> __u32 pad;
> };
>
> +#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
> +struct drm_syncobj_wait {
> + __u64 handles;
> + /* absolute timeout */
> + __s64 timeout_sec;
> + __s64 timeout_nsec;
> + __u32 count_handles;
> + __u32 flags;
> + __u32 first_signaled; /* only valid when not
> waiting all */
> + __u32 pad;
> +};
> +
> #if defined(__cplusplus)
> }
> #endif
> @@ -840,6 +852,7 @@ extern "C" {
> #define DRM_IOCTL_SYNCOBJ_DESTROY DRM_IOWR(0xC0, struct
> drm_syncobj_destroy)
> #define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1,
> struct drm_syncobj_handle)
> #define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2,
> struct drm_syncobj_handle)
> +#define DRM_IOCTL_SYNCOBJ_WAIT DRM_IOWR(0xC3, struct
> drm_syncobj_wait)
>
> /**
> * Device specific ioctls should only be in their
> respective headers
> --
> 2.9.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> <mailto:dri-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> _______________________________________________
>
> amd-gfx mailing list
>
> amd-gfx at lists.freedesktop.org
> <mailto:amd-gfx at lists.freedesktop.org>
>
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170710/f39d3397/attachment-0001.html>
More information about the amd-gfx
mailing list