<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 10, 2017 at 5:26 AM, Christian König <span dir="ltr"><<a href="mailto:deathsimple@vodafone.de" target="_blank">deathsimple@vodafone.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"><div><div class="h5">
<div class="m_5953209542720980333moz-cite-prefix">Am 10.08.2017 um 01:53 schrieb Jason
Ekstrand:<br>
</div>
<blockquote type="cite">
<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">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>> 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(stru<wbr>ct 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->l<wbr>ock,
flags);<br>
+ dma_fence_replace_proxy(&sync<wbr>obj->fence,
fence);<br>
+ spin_unlock_irqrestore(&synco<wbr>bj->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_AL<wbr>L)<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_FO<wbr>R_SUBMIT)<br>
+ s->fence =
dma_fence_create_proxy();<br>
+ else<br>
<span>+ 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_AL<wbr>L));<br>
+ if (!array) {<br>
+ ret = -ENOMEM;<br>
+ goto err_fences;<br>
+ }<br>
+<br>
+ timeout = drm_timeout_abs_to_jiffies(arg<wbr>s->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="m_5953209542720980333HOEnZb"><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></div></div>
As Chris pointed out, that's really not a good idea.<br></div></blockquote><div><br></div><div>What isn't a good idea?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
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></div></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
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></div></blockquote><div><br></div><div>In v3, I used wait_event_interruptible. Does it have those same bugs?<br><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
Christian.<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">--Jason<br>
</div>
</div><span class="">
<br>
<fieldset class="m_5953209542720980333mimeAttachmentHeader"></fieldset>
<br>
<pre>______________________________<wbr>_________________
dri-devel mailing list
<a class="m_5953209542720980333moz-txt-link-abbreviated" href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.<wbr>org</a>
<a class="m_5953209542720980333moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/dri-devel</a>
</pre>
</span></blockquote>
<p><br>
</p>
</div>
</blockquote></div><br></div></div>