Re: [PATCH] i915: Add support for drm syncobjs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 3, 2017 at 10:00 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
Quoting Jason Ekstrand (2017-07-05 22:15:09)
> On Wed, Jul 5, 2017 at 2:13 PM, Jason Ekstrand <jason@xxxxxxxxxxxxxx> 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@xxxxxxxxx>
 
>     +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?
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux