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
