[PATCH] i915: Add support for drm syncobjs

[Jason Ekstrand]

On Thu, Aug 3, 2017 at 10:00 AM, Chris Wilson <chris at chris-wilson.co.uk>
wrote:

> Quoting Jason Ekstrand (2017-07-05 22:15:09)
> > On Wed, Jul 5, 2017 at 2:13 PM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> >
> >     This commit adds support for waiting on or signaling DRM syncobjs as
> >     part of execbuf.  It does so by hijacking the currently unused
> cliprects
> >     pointer to instead point to an array of i915_gem_exec_fence structs
> >     which containe a DRM syncobj and a flags parameter which specifies
> >     whether to wait on it or to signal it.  This implementation
> >     theoretically allows for both flags to be set in which case it waits
> on
> >     the dma_fence that was in the syncobj and then immediately replaces
> it
> >     with the dma_fence from the current execbuf.
> >
> >     v2:
> >      - Rebase on new syncobj API
> >     v3:
> >      - Pull everything out into helpers
> >      - Do all allocation in gem_execbuffer2
> >      - Pack the flags in the bottom 2 bits of the drm_syncobj*
>
> Just noticed, no sign off. Could you please check
> https://developercertificate.org/ and apply your Signed-off-by, just a
> reply will do.
>

Sorry, not familiar with the kernel...

Signed-off-by: Jason Ekstrand <jason.ekstrand at intel.com>


> >     +static int await_fence_array(struct i915_execbuffer *eb,
> >     +                            struct drm_syncobj **fences)
> >     +{
> >     +       const unsigned int nfences = eb->args->num_cliprects;
> >     +       unsigned int n;
> >     +       int err;
> >     +
> >     +       for (n = 0; n < nfences; n++) {
> >     +               struct drm_syncobj *syncobj;
> >     +               unsigned int flags;
> >     +
> >     +               syncobj = ptr_unpack_bits(fences[n], &flags, 2);
> >     +               if (!(flags & I915_EXEC_FENCE_WAIT))
> >     +                       continue;
> >     +
> >     +               err = i915_gem_request_await_dma_fence(eb->request,
> >     +
> syncobj->fence);
> >
> >
> > Is there a race here?  What happens if some other process replaces the
> fence
> > between the syncobj->fence lookup and gem_request_await_dma_fence taking
> its
> > reference?
>
> Yes. It's inherently racy be via from objects, dmabuf or explicit
> fences. We obtain a snapshot of fences, and the only condition we must
> impose is that they are not cyclic - i.e. we must complete the snapshot
> of all fences before exposing our new &request->fence to the world.
>
> As to the race where the fence pointed to may change whilst we inspect
> it, there is nothing we can do to prevent that nor define what the right
> behaviour is. It is up to userspace to apply its own execution barriers
> if it requires strict control over the ordering of fences, i.e. what
> does if mean if client B changed the fence whilst client A was
> executing? Should client A use the original fence or the new fence? If
> it matters, the two clients need to coordinate usage of their shared fence.
>

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170803/a5bab7fb/attachment.html>