Am 10.08.2017 um 01:53 schrieb Jason Ekstrand:
On Wed, Aug 9, 2017 at 3:41 PM, Chris Wilson <ch...@chris-wilson.co.uk <mailto:ch...@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.

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.

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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to