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
