Re: [PATCH] drm/syncobj: Allow wait for submit and signal behavior (v2)

[Christian König <deathsimple@xxxxxxxxxxx>]

Am 10.08.2017 um 16:32 schrieb Jason Ekstrand:

On Thu, Aug 10, 2017 at 5:26 AM, Christian König <deathsimple@xxxxxxxxxxx> wrote:

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.

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@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