Am 10.08.2017 um 16:32 schrieb Jason Ekstrand:
On Thu, Aug 10, 2017 at 5:26 AM, Christian König <deathsim...@vodafone.de <mailto:deathsim...@vodafone.de>> wrote:


    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.


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