<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 16:32 schrieb Jason
Ekstrand:<br>
</div>
<blockquote type="cite"
cite="mid:CAOFGe96nXTjT6azcfUEi1D_rvsK-f8Ghwn6-J9PAw+gZ7iuzZg@mail.gmail.com">
<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"
moz-do-not-send="true">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" 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>> 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>
</div>
</blockquote>
<br>
<blockquote type="cite">However, this installs the proxy into
syncobj->fence with the result<br>
that any concurrent wait also become a WAIT_FOR_SUBMIT.</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe96nXTjT6azcfUEi1D_rvsK-f8Ghwn6-J9PAw+gZ7iuzZg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</blockquote>
<br>
Crap, that makes the whole thing much more harder to implement.<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe96nXTjT6azcfUEi1D_rvsK-f8Ghwn6-J9PAw+gZ7iuzZg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I haven't seen that yet, but when you now use
wait_even_interruptible() the problem should be gone.<br>
<br>
Regards,<br>
Christian.<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe96nXTjT6azcfUEi1D_rvsK-f8Ghwn6-J9PAw+gZ7iuzZg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><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" moz-do-not-send="true">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" moz-do-not-send="true">https://lists.freedesktop.org/<wbr>mailman/listinfo/dri-devel</a>
</pre>
</span></blockquote>
<p><br>
</p>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>