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

Fri Aug 23 18:53:20 UTC 2019

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

>
>
>> + * 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
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190823/82154702/attachment-0001.html>