<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 3, 2017 at 10:00 AM, 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Jason Ekstrand (2017-07-05 22:15:09)<br>
<span class="gmail-">> On Wed, Jul 5, 2017 at 2:13 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
><br>
> This commit adds support for waiting on or signaling DRM syncobjs as<br>
> part of execbuf. It does so by hijacking the currently unused cliprects<br>
> pointer to instead point to an array of i915_gem_exec_fence structs<br>
> which containe a DRM syncobj and a flags parameter which specifies<br>
> whether to wait on it or to signal it. This implementation<br>
> theoretically allows for both flags to be set in which case it waits on<br>
> the dma_fence that was in the syncobj and then immediately replaces it<br>
> with the dma_fence from the current execbuf.<br>
><br>
> v2:<br>
> - Rebase on new syncobj API<br>
> v3:<br>
> - Pull everything out into helpers<br>
> - Do all allocation in gem_execbuffer2<br>
> - Pack the flags in the bottom 2 bits of the drm_syncobj*<br>
<br>
</span>Just noticed, no sign off. Could you please check<br>
<a href="https://developercertificate.org/" rel="noreferrer" target="_blank">https://developercertificate.<wbr>org/</a> and apply your Signed-off-by, just a<br>
reply will do.<span class="gmail-"><br></span></blockquote><div><br></div><div>Sorry, not familiar with the kernel...<br><br></div><div>Signed-off-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> +static int await_fence_array(struct i915_execbuffer *eb,<br>
> + struct drm_syncobj **fences)<br>
> +{<br>
> + const unsigned int nfences = eb->args->num_cliprects;<br>
> + unsigned int n;<br>
> + int err;<br>
> +<br>
> + for (n = 0; n < nfences; n++) {<br>
> + struct drm_syncobj *syncobj;<br>
> + unsigned int flags;<br>
> +<br>
> + syncobj = ptr_unpack_bits(fences[n], &flags, 2);<br>
> + if (!(flags & I915_EXEC_FENCE_WAIT))<br>
> + continue;<br>
> +<br>
> + err = i915_gem_request_await_dma_<wbr>fence(eb->request,<br>
> + syncobj->fence);<br>
><br>
><br>
> Is there a race here? What happens if some other process replaces the fence<br>
> between the syncobj->fence lookup and gem_request_await_dma_fence taking its<br>
> reference?<br>
<br>
</span>Yes. It's inherently racy be via from objects, dmabuf or explicit<br>
fences. We obtain a snapshot of fences, and the only condition we must<br>
impose is that they are not cyclic - i.e. we must complete the snapshot<br>
of all fences before exposing our new &request->fence to the world.<br>
<br>
As to the race where the fence pointed to may change whilst we inspect<br>
it, there is nothing we can do to prevent that nor define what the right<br>
behaviour is. It is up to userspace to apply its own execution barriers<br>
if it requires strict control over the ordering of fences, i.e. what<br>
does if mean if client B changed the fence whilst client A was<br>
executing? Should client A use the original fence or the new fence? If<br>
it matters, the two clients need to coordinate usage of their shared fence.<span class="gmail-HOEnZb"><font color="#888888"><br>
</font></span></blockquote></div><br></div><div class="gmail_extra">I'm not concerned about what happens to racy clients. They get what they get. What concerns me is what happens if somehow the fence is replaced and deleted before i915_gem_request_await_dma_fence takes it's reference. Can this cause the kernel to segfault?<br></div></div>