Re: [PATCH v7 07/15] dma-buf/sw_sync: Add fence deadline support

[Rob Clark]

On Tue, Feb 28, 2023 at 1:23 AM Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote:
>
> On Mon, 27 Feb 2023 11:35:13 -0800
> Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> >
> > This consists of simply storing the most recent deadline, and adding an
> > ioctl to retrieve the deadline.  This can be used in conjunction with
> > the SET_DEADLINE ioctl on a fence fd for testing.  Ie. create various
> > sw_sync fences, merge them into a fence-array, set deadline on the
> > fence-array and confirm that it is propagated properly to each fence.
> >
> > v2: Switch UABI to express deadline as u64
> > v3: More verbose UAPI docs, show how to convert from timespec
> >
> > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > Reviewed-by: Christian König <christian.koenig@xxxxxxx>
> > ---
> >  drivers/dma-buf/sw_sync.c      | 58 ++++++++++++++++++++++++++++++++++
> >  drivers/dma-buf/sync_debug.h   |  2 ++
> >  include/uapi/linux/sync_file.h |  6 +++-
> >  3 files changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > index 348b3a9170fa..3e2315ee955b 100644
> > --- a/drivers/dma-buf/sw_sync.c
> > +++ b/drivers/dma-buf/sw_sync.c
> > @@ -52,12 +52,28 @@ struct sw_sync_create_fence_data {
> >       __s32   fence; /* fd of new fence */
> >  };
> >
> > +/**
> > + * struct sw_sync_get_deadline - get the deadline hint of a sw_sync fence
> > + * @deadline_ns: absolute time of the deadline
> > + * @pad:     must be zero
> > + * @fence_fd:        the sw_sync fence fd (in)
> > + *
> > + * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
>
> Hi,
>
> the commit message explains this returns the "most recent" deadline,
> but the doc here forgets to mention that. I suppose that means the
> most recently set deadline and not the deadline furthest forward in
> time (largest value).
>
> Is "most recent" the appropriate behaviour when multiple deadlines have
> been set? Would you not want the earliest deadline set so far instead?

It's not what a "normal" implementation of ->set_deadline() would do.
But it was useful for determining that the deadline propagates
correctly through composite (array/chain) fences.

I guess I could change the test to work with a more normal
->set_deadline() implementation (which would just track the nearest
(in time) deadline).

> What if none has been set?

you'd get zero.. I suppose I could make it return an error instead..

BR,
-R

> > + */
> > +struct sw_sync_get_deadline {
> > +     __u64   deadline_ns;
> > +     __u32   pad;
> > +     __s32   fence_fd;
> > +};
> > +
> >  #define SW_SYNC_IOC_MAGIC    'W'
> >
> >  #define SW_SYNC_IOC_CREATE_FENCE     _IOWR(SW_SYNC_IOC_MAGIC, 0,\
> >               struct sw_sync_create_fence_data)
> >
> >  #define SW_SYNC_IOC_INC                      _IOW(SW_SYNC_IOC_MAGIC, 1, __u32)
> > +#define SW_SYNC_GET_DEADLINE         _IOWR(SW_SYNC_IOC_MAGIC, 2, \
> > +             struct sw_sync_get_deadline)
> >
> >  static const struct dma_fence_ops timeline_fence_ops;
> >
> > @@ -171,6 +187,13 @@ static void timeline_fence_timeline_value_str(struct dma_fence *fence,
> >       snprintf(str, size, "%d", parent->value);
> >  }
> >
> > +static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
> > +{
> > +     struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> > +
> > +     pt->deadline = deadline;
> > +}
> > +
> >  static const struct dma_fence_ops timeline_fence_ops = {
> >       .get_driver_name = timeline_fence_get_driver_name,
> >       .get_timeline_name = timeline_fence_get_timeline_name,
> > @@ -179,6 +202,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
> >       .release = timeline_fence_release,
> >       .fence_value_str = timeline_fence_value_str,
> >       .timeline_value_str = timeline_fence_timeline_value_str,
> > +     .set_deadline = timeline_fence_set_deadline,
> >  };
> >
> >  /**
> > @@ -387,6 +411,37 @@ static long sw_sync_ioctl_inc(struct sync_timeline *obj, unsigned long arg)
> >       return 0;
> >  }
> >
> > +static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long arg)
> > +{
> > +     struct sw_sync_get_deadline data;
> > +     struct dma_fence *fence;
> > +     struct sync_pt *pt;
> > +
> > +     if (copy_from_user(&data, (void __user *)arg, sizeof(data)))
> > +             return -EFAULT;
> > +
> > +     if (data.deadline_ns || data.pad)
> > +             return -EINVAL;
> > +
> > +     fence = sync_file_get_fence(data.fence_fd);
> > +     if (!fence)
> > +             return -EINVAL;
> > +
> > +     pt = dma_fence_to_sync_pt(fence);
> > +     if (!pt)
> > +             return -EINVAL;
> > +
> > +
> > +     data.deadline_ns = ktime_to_ns(pt->deadline);
> > +
> > +     dma_fence_put(fence);
> > +
> > +     if (copy_to_user((void __user *)arg, &data, sizeof(data)))
> > +             return -EFAULT;
> > +
> > +     return 0;
> > +}
> > +
> >  static long sw_sync_ioctl(struct file *file, unsigned int cmd,
> >                         unsigned long arg)
> >  {
> > @@ -399,6 +454,9 @@ static long sw_sync_ioctl(struct file *file, unsigned int cmd,
> >       case SW_SYNC_IOC_INC:
> >               return sw_sync_ioctl_inc(obj, arg);
> >
> > +     case SW_SYNC_GET_DEADLINE:
> > +             return sw_sync_ioctl_get_deadline(obj, arg);
> > +
> >       default:
> >               return -ENOTTY;
> >       }
> > diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
> > index 6176e52ba2d7..2e0146d0bdbb 100644
> > --- a/drivers/dma-buf/sync_debug.h
> > +++ b/drivers/dma-buf/sync_debug.h
> > @@ -55,11 +55,13 @@ static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
> >   * @base: base fence object
> >   * @link: link on the sync timeline's list
> >   * @node: node in the sync timeline's tree
> > + * @deadline: the most recently set fence deadline
> >   */
> >  struct sync_pt {
> >       struct dma_fence base;
> >       struct list_head link;
> >       struct rb_node node;
> > +     ktime_t deadline;
> >  };
> >
> >  extern const struct file_operations sw_sync_debugfs_fops;
> > diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
> > index 49325cf6749b..dc6645b2598b 100644
> > --- a/include/uapi/linux/sync_file.h
> > +++ b/include/uapi/linux/sync_file.h
> > @@ -72,7 +72,11 @@ struct sync_file_info {
> >   * @deadline_ns: absolute time of the deadline
> >   * @pad:     must be zero
> >   *
> > - * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
> > + * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank).  For
> > + * example:
> > + *
> > + *     clock_gettime(CLOCK_MONOTONIC, &t);
> > + *     deadline_ns = (t.tv_sec * 1000000000L) + t.tv_nsec + duration_ns
>
> Shouldn't this hunk be in patch 5 instead?
>
> What's duration_ns? Maybe ns_until_my_deadline would be more clear that
> it is something userspace freely chooses?
>
> >   */
> >  struct sync_set_deadline {
> >       __u64   deadline_ns;
>
>
> Thanks,
> pq