[PATCH] drm/syncobj: Allow wait for submit and signal behavior (v2)
Jason Ekstrand
jason at jlekstrand.net
Thu Aug 10 14:32:32 UTC 2017
On Thu, Aug 10, 2017 at 5:26 AM, Christian König <deathsimple at vodafone.de>
wrote:
> Am 10.08.2017 um 01:53 schrieb Jason Ekstrand:
>
> On Wed, Aug 9, 2017 at 3:41 PM, Chris Wilson <chris at chris-wilson.co.uk>
> wrote:
>
>> Quoting Chris Wilson (2017-08-09 18:57:44)
>> > So we are taking a snapshot here. It looks like this could have been
>> > done using a dma_fence_array + dma_fence_proxy for capturing the future
>> > fence.
>>
>> A quick sketch of this idea looks like:
>>
>> void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>> struct dma_fence *fence)
>> {
>> - struct dma_fence *old_fence;
>> + unsigned long flags;
>>
>> - if (fence)
>> - dma_fence_get(fence);
>> - old_fence = xchg(&syncobj->fence, fence);
>> -
>> - dma_fence_put(old_fence);
>> + spin_lock_irqsave(&syncobj->lock, flags);
>> + dma_fence_replace_proxy(&syncobj->fence, fence);
>> + spin_unlock_irqrestore(&syncobj->lock, flags);
>> }
>>
>> +int
>> +drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
>> + struct drm_file *file_private)
>> +{
>> + struct drm_syncobj_wait *args = data;
>> + struct dma_fence **fences;
>> + struct dma_fence_array *array;
>> + unsigned long timeout;
>> + unsigned int count;
>> + u32 *handles;
>> + int ret = 0;
>> + u32 i;
>> +
>> + BUILD_BUG_ON(sizeof(*fences) < sizeof(*handles));
>> +
>> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
>> + return -ENODEV;
>> +
>> + if (args->flags != 0 && args->flags !=
>> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)
>> + return -EINVAL;
>> +
>> + count = args->count_handles;
>> + if (!count)
>> + return -EINVAL;
>> +
>> + /* Get the handles from userspace */
>> + fences = kmalloc_array(count,
>> + sizeof(struct dma_fence *),
>> + __GFP_NOWARN | GFP_KERNEL);
>> + if (!fences)
>> + return -ENOMEM;
>> +
>> + handles = (void *)fences + count * (sizeof(*fences) -
>> sizeof(*handles));
>> + if (copy_from_user(handles,
>> + u64_to_user_ptr(args->handles),
>> + sizeof(u32) * count)) {
>> + ret = -EFAULT;
>> + goto err;
>> + }
>> +
>> + for (i = 0; i < count; i++) {
>> + struct drm_syncobj *s;
>> +
>> + ret = -ENOENT;
>> + s = drm_syncobj_find(file_private, handles[i]);
>> + if (s) {
>> + ret = 0;
>> + spin_lock_irq(&s->lock);
>> + if (!s->fence) {
>> + if (args->flags &
>> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
>> + s->fence =
>> dma_fence_create_proxy();
>> + else
>> + ret = -EINVAL;
>> + }
>> + if (s->fence)
>> + fences[i] = dma_fence_get(s->fence);
>> + spin_unlock_irq(&s->lock);
>> + }
>> + if (ret) {
>> + count = i;
>> + goto err_fences;
>> + }
>> + }
>> +
>> + array = dma_fence_array_create(count, fences, 0, 0,
>> + !(args->flags &
>> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL));
>> + if (!array) {
>> + ret = -ENOMEM;
>> + goto err_fences;
>> + }
>> +
>> + timeout = drm_timeout_abs_to_jiffies(args->timeout_nsec);
>> + timeout = dma_fence_wait_timeout(&array->base, true, timeout);
>> + args->first_signaled = array->first_signaled;
>> + dma_fence_put(&array->base);
>> +
>> + return timeout < 0 ? timeout : 0;
>> +
>> +err_fences:
>> + for (i = 0; i < count; i++)
>> + dma_fence_put(fences[i]);
>> +err:
>> + kfree(fences);
>> + return ret;
>> +}
>>
>> The key advantage is that this translate the ioctl into a dma-fence-array
>> which already has to deal with the mess, sharing the burden. (But it
>> does require a trivial patch to dma-fence-array to record the first
>> signaled fence.)
>>
>> However, this installs the proxy into syncobj->fence with the result
>> that any concurrent wait also become a WAIT_FOR_SUBMIT. The behaviour
>> of drm_syncobj is then quite inconsistent, sometimes it will wait for a
>> future fence, sometimes it will report an error.
>>
>
> Yeah, that's not good. I thought about a variety of solutions to try and
> re-use more core dma_fence code. Ultimately I chose the current one
> because it takes a snapshot of the syncobjs and then, from that point
> forward, it's consistent with its snapshot. Nothing I was able to come up
> with based on core dma_fence wait routines does that.
>
>
> As Chris pointed out, that's really not a good idea.
>
What isn't a good idea?
> Most of the time we need the behavior of reporting an error and only when
> the flag is given wait until some fence shows up.
>
> In general I suggest that we only support this use case in the form of a
> wait_event_interruptible() on setting the first fence on an object.
>
> Waiting on the first one of multiple objects wouldn't be possible (you
> would always wait till all objects have fences), but I think that this is
> acceptable.
>
Not really. If you're doing a wait-any, then you want it to return as soon
as you have a signaled fence even if half your sync objects never get
fences. At least that's what's required for implementing vkWaitForFences.
The idea is that, if the WAIT_FOR_SUBMIT flag is set, then a syncobj
without a fence and a syncobj with an untriggered fence are identical as
far as waiting is concerned: You wait until it has a signaled fence.
> The big advantage of the wait_event_interruptible() interface is that your
> condition checking and process handling is bullet prove, as far as I have
> seen so far your code is a bit buggy in that direction.
>
In v3, I used wait_event_interruptible. Does it have those same bugs?
--Jason
> Christian.
>
>
> --Jason
>
>
> _______________________________________________
> dri-devel mailing listdri-devel at lists.freedesktop.orghttps://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170810/ea416169/attachment-0001.html>
More information about the dri-devel
mailing list