[PATCH] drm/syncobj: Add documentation for timeline syncobj

Sun Aug 25 21:01:52 UTC 2019

On Fri, Aug 23, 2019 at 8:53 PM Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> On Thu, Aug 22, 2019 at 5:28 PM Lionel Landwerlin <lionel.g.landwerlin at intel.com> wrote:
>>
>> On 22/08/2019 21:24, Jason Ekstrand wrote:
>>
>> On Thu, Aug 22, 2019 at 9:55 AM Lionel Landwerlin <lionel.g.landwerlin at intel.com> wrote:
>>>
>>> We've added a set of new APIs to manipulate syncobjs holding timelines
>>> of dma_fence. This adds a bit of documentation about how this works.
>>>
>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>> Cc: Christian Koenig <Christian.Koenig at amd.com>
>>> Cc: Jason Ekstrand <jason at jlekstrand.net>
>>> Cc: David(ChunMing) Zhou <David1.Zhou at amd.com>
>>> ---
>>>  drivers/gpu/drm/drm_syncobj.c | 87 +++++++++++++++++++++++++++++------
>>>  1 file changed, 74 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>>> index b5ad73330a48..32ffded6d2c0 100644
>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>> @@ -43,27 +43,66 @@
>>>   *  - Signal a syncobj (set a trivially signaled fence)
>>>   *  - Wait for a syncobj's fence to appear and be signaled
>>>   *
>>> + * The syncobj userspace API also provides operations to manipulate a syncobj
>>> + * in terms of a timeline of struct &dma_fence rather than a single struct
>>> + * &dma_fence, through the following operations:
>>> + *
>>> + *   - Signal a given point on the timeline
>>> + *   - Wait for a given point to appear and/or be signaled
>>> + *   - Import and export from/to a given point of a timeline
>>> + *
>>>   * At it's core, a syncobj is simply a wrapper around a pointer to a struct
>>>   * &dma_fence which may be NULL.
>>>   * When a syncobj is first created, its pointer is either NULL or a pointer
>>>   * to an already signaled fence depending on whether the
>>>   * &DRM_SYNCOBJ_CREATE_SIGNALED flag is passed to
>>>   * &DRM_IOCTL_SYNCOBJ_CREATE.
>>> - * When GPU work which signals a syncobj is enqueued in a DRM driver,
>>> - * the syncobj fence is replaced with a fence which will be signaled by the
>>> - * completion of that work.
>>> - * When GPU work which waits on a syncobj is enqueued in a DRM driver, the
>>> - * driver retrieves syncobj's current fence at the time the work is enqueued
>>> - * waits on that fence before submitting the work to hardware.
>>> - * If the syncobj's fence is NULL, the enqueue operation is expected to fail.
>>> - * All manipulation of the syncobjs's fence happens in terms of the current
>>> - * fence at the time the ioctl is called by userspace regardless of whether
>>> - * that operation is an immediate host-side operation (signal or reset) or
>>> - * or an operation which is enqueued in some driver queue.
>>> - * &DRM_IOCTL_SYNCOBJ_RESET and &DRM_IOCTL_SYNCOBJ_SIGNAL can be used to
>>> - * manipulate a syncobj from the host by resetting its pointer to NULL or
>>> + *
>>> + * If the syncobj is considered as a binary (signal/unsignaled) primitive,
>>
>>
>> What does "considered as a binary" mean?  Is it an inherent property of the syncobj given at create time?  Is it a state the syncobj can be in?  Or is it a property of how the submit ioctl in the DRM driver references it?  I'm really hoping it's either 1 or 3....
>>
>>
>> 3: you either use it binary/legacy apis, or timeline apis. timeline apis also provide some binary compatibility with the point 0 (in particular for wait).
>
>
> Right.  Maybe we should say something like  "When GPU work is enqueued which signals a non-zero time point" or something like that?  I guess that implies a certain unification across drivers that maybe we don't want....

[Just jumping in on this comment here]

I thought the point of syncobj is that you can share them across
drivers (not just within drivers)? Otherwise not much sense in the
common infrastructure. Hence I'd say we should spec all these things.
Concern from someone who's seen way too many cross-driver apis that
turned out the be decidedly cross-driver than planned ...

Cheers, Daniel

>>
>>
>>>
>>> + * when GPU work is enqueued in a DRM driver to signal the syncobj, the fence
>>> + * is replaced with a fence which will be signaled by the completion of that
>>> + * work.
>>> + * If the syncobj is considered as a timeline primitive, when GPU work is
>>> + * enqueued in a DRM driver to signal the a given point of the syncobj, a new
>>> + * struct &dma_fence_chain pointing to the DRM driver's fence and also
>>> + * pointing to the previous fence that was in the syncobj. The new struct
>>> + * &dma_fence_chain fence put into the syncobj will be signaled by completion
>>> + * of the DRM driver's work and also any work associated with the fence
>>> + * previously in the syncobj.
>>> + *
>>> + * When GPU work which waits on a syncobj is enqueued in a DRM driver, at the
>>> + * time the work is enqueued, it waits on the fence coming from the syncobj
>>> + * before submitting the work to hardware. That fence is either :
>>> + *
>>> + *    - The syncobj's current fence if the syncobj is considered as a binary
>>> + *      primitive.
>>> + *    - The struct &dma_fence associated with a given point if the syncobj is
>>> + *      considered as a timeline primitive.
>>> + *
>>> + * If the syncobj's fence is NULL or not present in the syncobj's timeline,
>>> + * the enqueue operation is expected to fail.
>>> + *
>>> + * With binary syncobj, all manipulation of the syncobjs's fence happens in
>>> + * terms of the current fence at the time the ioctl is called by userspace
>>> + * regardless of whether that operation is an immediate host-side operation
>>> + * (signal or reset) or or an operation which is enqueued in some driver
>>> + * queue. &DRM_IOCTL_SYNCOBJ_RESET and &DRM_IOCTL_SYNCOBJ_SIGNAL can be used
>>> + * to manipulate a syncobj from the host by resetting its pointer to NULL or
>>>   * setting its pointer to a fence which is already signaled.
>>>   *
>>> + * With timeline syncobj, all manipulation of the timeline fences happens in
>>> + * terms of the fence referred to in the timeline. See
>>> + * dma_fence_chain_find_seqno() to see how a given point is found in the
>>> + * timeline.
>>> + *
>>> + * Note that applications should be careful to always use timeline set of
>>> + * ioctl() when dealing with syncobj considered as timeline. Using a binary
>>> + * set of ioctl() with a syncobj considered as timeline could result incorrect
>>> + * synchronization. The use of binary syncobj is supported through the
>>> + * timeline set of ioctl() by using a point value of 0, this will reproduce
>>> + * the behavior of the binary set of ioctl() (for example replace the
>>> + * syncobj's fence when signaling).
>>
>>
>> I know I've asked this before but I feel compelled to ask it again.  Why do we allow them to mix and match?  Why not just have a create flag and enforce meaningful behavior?  I'm a bit concerned that userspace is going to start relying on the subtlties of the interaction between timeline and binary syncobjs which are neither documented nor properly tested in IGT.
>>
>>
>> For one, you might have to mix both types of syncobjs in a given wait/signal operation. So 0 ensures we can do that.
>
>
> Right, that sounds like a useful feature.
>
>>
>> Second, drm-syncobj is a container and its payload is an interface (dma_fence) which has several implementations.
>>
>> The kernel primitive is just less restrictive than the Vulkan API here.
>>
>> I guess we could add a flag at creation to ensure the replacement of the fence in a timeline syncobj cannot happen.
>
>
> I would be in favor of that but I'd be interested to hear what Christian or David think.
>
>>
>> I haven't thought of all the implications that might have though... Should we allow reset on a timeline syncobj?
>
>
> Good question.  I'm inclined to say "yes" because it's pretty well-defined what such a reset means.  However, it's not really needed.
>
>>
>> -Lionel
>>
>>
>>
>>> + *
>>>   *
>>>   * Host-side wait on syncobjs
>>>   * --------------------------
>>> @@ -87,6 +126,16 @@
>>>   * synchronize between the two.
>>>   * This requirement is inherited from the Vulkan fence API.
>>>   *
>>> + * Similarly, &DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT takes an array of syncobj
>>> + * handles as well as an array of u64 points and does a host-side wait on all
>>> + * of syncobj fences at the given points simultaneously.
>>> + *
>>> + * &DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT also adds the ability to wait for a given
>>> + * fence to materialize on the timeline without waiting for the fence to be
>>> + * signaled by using the &DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE flag. This
>>> + * requirement is inherited from the wait-before-signal behavior required by
>>> + * the Vulkan timeline semaphore API.
>>> + *
>>>   *
>>>   * Import/export of syncobjs
>>>   * -------------------------
>>> @@ -120,6 +169,18 @@
>>>   * Because sync files are immutable, resetting or signaling the syncobj
>>>   * will not affect any sync files whose fences have been imported into the
>>>   * syncobj.
>>> + *
>>> + *
>>> + * Import/export of timeline points in timeline syncobjs
>>> + * -----------------------------------------------------
>>> + *
>>> + * &DRM_IOCTL_SYNCOBJ_TRANSFER provides a mechanism to transfer a struct
>>> + * &dma_fence of at a given point from a timeline syncobj to another point
>>> + * into another timeline syncobj.
>>> + *
>>> + * Note that if you want to transfer a struct &dma_fence from a given point on
>>> + * a timeline syncobj from/into a binary syncobj, you can use the point 0 to
>>> + * mean take/replace the fence in the syncobj.
>>>   */
>>>
>>>  #include <linux/anon_inodes.h>
>>> --
>>> 2.23.0
>>>
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch