[PATCH] drm/syncobj: Allow wait for submit and signal behavior (v2)
Christian König
deathsimple at vodafone.de
Thu Aug 10 14:41:55 UTC 2017
Am 10.08.2017 um 16:32 schrieb Jason Ekstrand:
> On Thu, Aug 10, 2017 at 5:26 AM, Christian König
> <deathsimple at vodafone.de <mailto: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 <mailto: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?
> However, this installs the proxy into syncobj->fence with the result
> that any concurrent wait also become a WAIT_FOR_SUBMIT.
Installing that proxy. I'm always happy if someone reuses the code
(after all I wrote a good part of it), but I think we should rather try
to make this as clean as possible.
> 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.
Crap, that makes the whole thing much more harder to implement.
> 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?
I haven't seen that yet, but when you now use wait_even_interruptible()
the problem should be gone.
Regards,
Christian.
>
> --Jason
>
> Christian.
>
>>
>> --Jason
>>
>>
>> _______________________________________________
>> 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
>> <https://lists.freedesktop.org/mailman/listinfo/dri-devel>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170810/0c567784/attachment-0001.html>
More information about the dri-devel
mailing list