<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Am 10.08.2017 um 01:53 schrieb Jason
Ekstrand:<br>
</div>
<blockquote type="cite"
cite="mid:CAOFGe974Zp6V9gKMmD0Buw9oR1guQ1Kms9JBy5b2Aj9XU9idWg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">On Wed, Aug 9, 2017 at 3:41 PM, Chris
Wilson <span dir="ltr"><<a
href="mailto:chris@chris-wilson.co.uk" target="_blank"
moz-do-not-send="true">chris@chris-wilson.co.uk</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Quoting
Chris Wilson (2017-08-09 18:57:44)<br>
<span class="">> So we are taking a snapshot here. It
looks like this could have been<br>
> done using a dma_fence_array + dma_fence_proxy for
capturing the future<br>
> fence.<br>
<br>
</span>A quick sketch of this idea looks like:<br>
<br>
 void drm_syncobj_replace_fence(<wbr>struct drm_syncobj
*syncobj,<br>
                struct dma_fence *fence)<br>
 {<br>
-Â Â Â Â struct dma_fence *old_fence;<br>
+Â Â Â Â unsigned long flags;<br>
<br>
-Â Â Â Â if (fence)<br>
-Â Â Â Â Â Â Â Â dma_fence_get(fence);<br>
-Â Â Â Â old_fence = xchg(&syncobj->fence, fence);<br>
-<br>
-Â Â Â Â dma_fence_put(old_fence);<br>
+Â Â Â Â spin_lock_irqsave(&syncobj-><wbr>lock,
flags);<br>
+Â Â Â Â dma_fence_replace_proxy(&<wbr>syncobj->fence,
fence);<br>
+Â Â Â Â spin_unlock_irqrestore(&<wbr>syncobj->lock,
flags);<br>
 }<br>
<br>
+int<br>
+drm_syncobj_wait_ioctl(struct drm_device *dev, void
*data,<br>
+Â Â Â Â Â Â Â Â Â Â Â struct drm_file *file_private)<br>
+{<br>
+Â Â Â Â struct drm_syncobj_wait *args = data;<br>
+Â Â Â Â struct dma_fence **fences;<br>
+Â Â Â Â struct dma_fence_array *array;<br>
+Â Â Â Â unsigned long timeout;<br>
+Â Â Â Â unsigned int count;<br>
+Â Â Â Â u32 *handles;<br>
+Â Â Â Â int ret = 0;<br>
+Â Â Â Â u32 i;<br>
+<br>
+Â Â Â Â BUILD_BUG_ON(sizeof(*fences) <
sizeof(*handles));<br>
+<br>
+Â Â Â Â if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))<br>
+Â Â Â Â Â Â Â Â return -ENODEV;<br>
+<br>
+Â Â Â Â if (args->flags != 0 && args->flags
!= DRM_SYNCOBJ_WAIT_FLAGS_WAIT_<wbr>ALL)<br>
+Â Â Â Â Â Â Â Â return -EINVAL;<br>
+<br>
+Â Â Â Â count = args->count_handles;<br>
+Â Â Â Â if (!count)<br>
+Â Â Â Â Â Â Â Â return -EINVAL;<br>
+<br>
+Â Â Â Â /* Get the handles from userspace */<br>
+Â Â Â Â fences = kmalloc_array(count,<br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â sizeof(struct dma_fence *),<br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __GFP_NOWARN | GFP_KERNEL);<br>
+Â Â Â Â if (!fences)<br>
+Â Â Â Â Â Â Â Â return -ENOMEM;<br>
+<br>
+Â Â Â Â handles = (void *)fences + count *
(sizeof(*fences) - sizeof(*handles));<br>
+Â Â Â Â if (copy_from_user(handles,<br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â
u64_to_user_ptr(args->handles)<wbr>,<br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â sizeof(u32) * count)) {<br>
+Â Â Â Â Â Â Â Â ret = -EFAULT;<br>
+Â Â Â Â Â Â Â Â goto err;<br>
+Â Â Â Â }<br>
+<br>
+Â Â Â Â for (i = 0; i < count; i++) {<br>
+Â Â Â Â Â Â Â Â struct drm_syncobj *s;<br>
+<br>
+Â Â Â Â Â Â Â Â ret = -ENOENT;<br>
+Â Â Â Â Â Â Â Â s = drm_syncobj_find(file_private,
handles[i]);<br>
+Â Â Â Â Â Â Â Â if (s) {<br>
+Â Â Â Â Â Â Â Â Â Â Â Â ret = 0;<br>
+Â Â Â Â Â Â Â Â Â Â Â Â spin_lock_irq(&s->lock);<br>
+Â Â Â Â Â Â Â Â Â Â Â Â if (!s->fence) {<br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (args->flags &
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_<wbr>FOR_SUBMIT)<br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â s->fence =
dma_fence_create_proxy();<br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â else<br>
<span class="">+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ret
= -EINVAL;<br>
+Â Â Â Â Â Â Â Â Â Â Â Â }<br>
</span>+Â Â Â Â Â Â Â Â Â Â Â Â if (s->fence)<br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â fences[i] =
dma_fence_get(s->fence);<br>
+Â Â Â Â Â Â Â Â Â Â Â Â spin_unlock_irq(&s->lock);<br>
+Â Â Â Â Â Â Â Â }<br>
+Â Â Â Â Â Â Â Â if (ret) {<br>
+Â Â Â Â Â Â Â Â Â Â Â Â count = i;<br>
+Â Â Â Â Â Â Â Â Â Â Â Â goto err_fences;<br>
+Â Â Â Â Â Â Â Â }<br>
+Â Â Â Â }<br>
+<br>
+Â Â Â Â array = dma_fence_array_create(count, fences, 0,
0,<br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â !(args->flags
& DRM_SYNCOBJ_WAIT_FLAGS_WAIT_<wbr>ALL));<br>
+Â Â Â Â if (!array) {<br>
+Â Â Â Â Â Â Â Â ret = -ENOMEM;<br>
+Â Â Â Â Â Â Â Â goto err_fences;<br>
+Â Â Â Â }<br>
+<br>
+Â Â Â Â timeout = drm_timeout_abs_to_jiffies(<wbr>args->timeout_nsec);<br>
+Â Â Â Â timeout = dma_fence_wait_timeout(&array-<wbr>>base,
true, timeout);<br>
+Â Â Â Â args->first_signaled =
array->first_signaled;<br>
+Â Â Â Â dma_fence_put(&array->base);<br>
+<br>
+Â Â Â Â return timeout < 0 ? timeout : 0;<br>
+<br>
+err_fences:<br>
+Â Â Â Â for (i = 0; i < count; i++)<br>
+Â Â Â Â Â Â Â Â dma_fence_put(fences[i]);<br>
+err:<br>
+Â Â Â Â kfree(fences);<br>
+Â Â Â Â return ret;<br>
+}<br>
<br>
The key advantage is that this translate the ioctl into a
dma-fence-array<br>
which already has to deal with the mess, sharing the
burden. (But it<br>
does require a trivial patch to dma-fence-array to record
the first<br>
signaled fence.)<br>
<br>
However, this installs the proxy into syncobj->fence
with the result<br>
that any concurrent wait also become a WAIT_FOR_SUBMIT.
The behaviour<br>
of drm_syncobj is then quite inconsistent, sometimes it
will wait for a<br>
future fence, sometimes it will report an error.<span
class="HOEnZb"><font color="#888888"><br>
</font></span></blockquote>
</div>
<br>
</div>
<div class="gmail_extra">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.<br>
</div>
</div>
</blockquote>
<br>
As Chris pointed out, that's really not a good idea.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
Christian.<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe974Zp6V9gKMmD0Buw9oR1guQ1Kms9JBy5b2Aj9XU9idWg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">--Jason<br>
</div>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
dri-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a>
<a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/dri-devel">https://lists.freedesktop.org/mailman/listinfo/dri-devel</a>
</pre>
</blockquote>
<p><br>
</p>
</body>
</html>