<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>