<div dir="ltr"><div dir="ltr"><br></div><div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 22, 2019 at 5:28 PM Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com" target="_blank">lionel.g.landwerlin@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF">
    <div class="gmail-m_-5272467495377520358gmail-m_-1548110483521490893moz-cite-prefix">On 22/08/2019 21:24, Jason Ekstrand
      wrote:<br>
    </div>
    <blockquote type="cite">
      
      <div dir="ltr">
        <div class="gmail_quote">
          <div dir="ltr" class="gmail_attr">On Thu, Aug 22, 2019 at 9:55
            AM Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com" target="_blank">lionel.g.landwerlin@intel.com</a>>
            wrote:<br>
          </div>
          <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">We've added a set of new
            APIs to manipulate syncobjs holding timelines<br>
            of dma_fence. This adds a bit of documentation about how
            this works.<br>
            <br>
            Signed-off-by: Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com" target="_blank">lionel.g.landwerlin@intel.com</a>><br>
            Cc: Christian Koenig <<a href="mailto:Christian.Koenig@amd.com" target="_blank">Christian.Koenig@amd.com</a>><br>
            Cc: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>><br>
            Cc: David(ChunMing) Zhou <<a href="mailto:David1.Zhou@amd.com" target="_blank">David1.Zhou@amd.com</a>><br>
            ---<br>
             drivers/gpu/drm/drm_syncobj.c | 87
            +++++++++++++++++++++++++++++------<br>
             1 file changed, 74 insertions(+), 13 deletions(-)<br>
            <br>
            diff --git a/drivers/gpu/drm/drm_syncobj.c
            b/drivers/gpu/drm/drm_syncobj.c<br>
            index b5ad73330a48..32ffded6d2c0 100644<br>
            --- a/drivers/gpu/drm/drm_syncobj.c<br>
            +++ b/drivers/gpu/drm/drm_syncobj.c<br>
            @@ -43,27 +43,66 @@<br>
              *  - Signal a syncobj (set a trivially signaled fence)<br>
              *  - Wait for a syncobj's fence to appear and be signaled<br>
              *<br>
            + * The syncobj userspace API also provides operations to
            manipulate a syncobj<br>
            + * in terms of a timeline of struct &dma_fence rather
            than a single struct<br>
            + * &dma_fence, through the following operations:<br>
            + *<br>
            + *   - Signal a given point on the timeline<br>
            + *   - Wait for a given point to appear and/or be signaled<br>
            + *   - Import and export from/to a given point of a
            timeline<br>
            + *<br>
              * At it's core, a syncobj is simply a wrapper around a
            pointer to a struct<br>
              * &dma_fence which may be NULL.<br>
              * When a syncobj is first created, its pointer is either
            NULL or a pointer<br>
              * to an already signaled fence depending on whether the<br>
              * &DRM_SYNCOBJ_CREATE_SIGNALED flag is passed to<br>
              * &DRM_IOCTL_SYNCOBJ_CREATE.<br>
            - * When GPU work which signals a syncobj is enqueued in a
            DRM driver,<br>
            - * the syncobj fence is replaced with a fence which will be
            signaled by the<br>
            - * completion of that work.<br>
            - * When GPU work which waits on a syncobj is enqueued in a
            DRM driver, the<br>
            - * driver retrieves syncobj's current fence at the time the
            work is enqueued<br>
            - * waits on that fence before submitting the work to
            hardware.<br>
            - * If the syncobj's fence is NULL, the enqueue operation is
            expected to fail.<br>
            - * All manipulation of the syncobjs's fence happens in
            terms of the current<br>
            - * fence at the time the ioctl is called by userspace
            regardless of whether<br>
            - * that operation is an immediate host-side operation
            (signal or reset) or<br>
            - * or an operation which is enqueued in some driver queue.<br>
            - * &DRM_IOCTL_SYNCOBJ_RESET and
            &DRM_IOCTL_SYNCOBJ_SIGNAL can be used to<br>
            - * manipulate a syncobj from the host by resetting its
            pointer to NULL or<br>
            + *<br>
            + * If the syncobj is considered as a binary
            (signal/unsignaled) primitive,<br>
          </blockquote>
          <div><br>
          </div>
          <div>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....<br>
          </div>
        </div>
      </div>
    </blockquote>
    <p><br>
    </p>
    <p>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).<br>
    </p>
    </div></blockquote><div><br></div><div>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....<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"><div bgcolor="#FFFFFF">
    <blockquote type="cite">
      <div dir="ltr">
        <div class="gmail_quote">
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
            + * when GPU work is enqueued in a DRM driver to signal the
            syncobj, the fence<br>
            + * is replaced with a fence which will be signaled by the
            completion of that<br>
            + * work.<br>
            + * If the syncobj is considered as a timeline primitive,
            when GPU work is<br>
            + * enqueued in a DRM driver to signal the a given point of
            the syncobj, a new<br>
            + * struct &dma_fence_chain pointing to the DRM driver's
            fence and also<br>
            + * pointing to the previous fence that was in the syncobj.
            The new struct<br>
            + * &dma_fence_chain fence put into the syncobj will be
            signaled by completion<br>
            + * of the DRM driver's work and also any work associated
            with the fence<br>
            + * previously in the syncobj.<br>
            + *<br>
            + * When GPU work which waits on a syncobj is enqueued in a
            DRM driver, at the<br>
            + * time the work is enqueued, it waits on the fence coming
            from the syncobj<br>
            + * before submitting the work to hardware. That fence is
            either :<br>
            + *<br>
            + *    - The syncobj's current fence if the syncobj is
            considered as a binary<br>
            + *      primitive.<br>
            + *    - The struct &dma_fence associated with a given
            point if the syncobj is<br>
            + *      considered as a timeline primitive.<br>
            + *<br>
            + * If the syncobj's fence is NULL or not present in the
            syncobj's timeline,<br>
            + * the enqueue operation is expected to fail.<br>
            + *<br>
            + * With binary syncobj, all manipulation of the syncobjs's
            fence happens in<br>
            + * terms of the current fence at the time the ioctl is
            called by userspace<br>
            + * regardless of whether that operation is an immediate
            host-side operation<br>
            + * (signal or reset) or or an operation which is enqueued
            in some driver<br>
            + * queue. &DRM_IOCTL_SYNCOBJ_RESET and
            &DRM_IOCTL_SYNCOBJ_SIGNAL can be used<br>
            + * to manipulate a syncobj from the host by resetting its
            pointer to NULL or<br>
              * setting its pointer to a fence which is already
            signaled.<br>
              *<br>
            + * With timeline syncobj, all manipulation of the timeline
            fences happens in<br>
            + * terms of the fence referred to in the timeline. See<br>
            + * dma_fence_chain_find_seqno() to see how a given point is
            found in the<br>
            + * timeline.<br>
            + *<br>
            + * Note that applications should be careful to always use
            timeline set of<br>
            + * ioctl() when dealing with syncobj considered as
            timeline. Using a binary<br>
            + * set of ioctl() with a syncobj considered as timeline
            could result incorrect<br>
            + * synchronization. The use of binary syncobj is supported
            through the<br>
            + * timeline set of ioctl() by using a point value of 0,
            this will reproduce<br>
            + * the behavior of the binary set of ioctl() (for example
            replace the<br>
            + * syncobj's fence when signaling).<br>
          </blockquote>
          <div> </div>
          <div>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.<br>
          </div>
        </div>
      </div>
    </blockquote>
    <p><br>
    </p>
    <p>For one, you might have to mix both types of syncobjs in a given
      wait/signal operation. So 0 ensures we can do that.</p></div></blockquote><div><br></div><div>Right, that sounds like a useful feature.<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"><div bgcolor="#FFFFFF"><p>Second, drm-syncobj is a container and its payload is an
      interface (dma_fence) which has several implementations.</p>
    <p>The kernel primitive is just less restrictive than the Vulkan API
      here.<br>
    </p>
    <p>I guess we could add a flag at creation to ensure the replacement
      of the fence in a timeline syncobj cannot happen.</p></div></blockquote><div><br></div><div>I would be in favor of that but I'd be interested to hear what Christian or David think.<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"><div bgcolor="#FFFFFF">
    <p>I haven't thought of all the implications that might have
      though... Should we allow reset on a timeline syncobj?</p></div></blockquote><div><br></div><div>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.<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"><div bgcolor="#FFFFFF"><p>-Lionel<br>
    </p>
    <p><br>
    </p>
    <blockquote type="cite">
      <div dir="ltr">
        <div class="gmail_quote">
          <div> <br>
          </div>
          <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
            + *<br>
              *<br>
              * Host-side wait on syncobjs<br>
              * --------------------------<br>
            @@ -87,6 +126,16 @@<br>
              * synchronize between the two.<br>
              * This requirement is inherited from the Vulkan fence API.<br>
              *<br>
            + * Similarly, &DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT takes an
            array of syncobj<br>
            + * handles as well as an array of u64 points and does a
            host-side wait on all<br>
            + * of syncobj fences at the given points simultaneously.<br>
            + *<br>
            + * &DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT also adds the
            ability to wait for a given<br>
            + * fence to materialize on the timeline without waiting for
            the fence to be<br>
            + * signaled by using the
            &DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE flag. This<br>
            + * requirement is inherited from the wait-before-signal
            behavior required by<br>
            + * the Vulkan timeline semaphore API.<br>
            + *<br>
              *<br>
              * Import/export of syncobjs<br>
              * -------------------------<br>
            @@ -120,6 +169,18 @@<br>
              * Because sync files are immutable, resetting or signaling
            the syncobj<br>
              * will not affect any sync files whose fences have been
            imported into the<br>
              * syncobj.<br>
            + *<br>
            + *<br>
            + * Import/export of timeline points in timeline syncobjs<br>
            + * -----------------------------------------------------<br>
            + *<br>
            + * &DRM_IOCTL_SYNCOBJ_TRANSFER provides a mechanism to
            transfer a struct<br>
            + * &dma_fence of at a given point from a timeline
            syncobj to another point<br>
            + * into another timeline syncobj.<br>
            + *<br>
            + * Note that if you want to transfer a struct
            &dma_fence from a given point on<br>
            + * a timeline syncobj from/into a binary syncobj, you can
            use the point 0 to<br>
            + * mean take/replace the fence in the syncobj.<br>
              */<br>
            <br>
             #include <linux/anon_inodes.h><br>
            -- <br>
            2.23.0<br>
            <br>
          </blockquote>
        </div>
      </div>
    </blockquote>
    <p><br>
    </p>
  </div>

</blockquote></div></div>
</div>