On Thu, Aug 10, 2017 at 5:26 AM, Christian König <deathsimple@xxxxxxxxxxx> wrote:
As Chris pointed out, that's really not a good idea.Am 10.08.2017 um 01:53 schrieb Jason Ekstrand:
On Wed, Aug 9, 2017 at 3:41 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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.
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 list dri-devel@lists.freedesktop. org https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
