| Message ID | 20230227193535.2822389-8-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-fence: Deadline awareness | expand |
On Mon, 27 Feb 2023 11:35:13 -0800 Rob Clark <robdclark@gmail.com> wrote: > From: Rob Clark <robdclark@chromium.org> > > 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@chromium.org> > Reviewed-by: Christian König <christian.koenig@amd.com> > --- > 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? What if none has been set? > + */ > +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
On Tue, Feb 28, 2023 at 1:23 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > On Mon, 27 Feb 2023 11:35:13 -0800 > Rob Clark <robdclark@gmail.com> wrote: > > > From: Rob Clark <robdclark@chromium.org> > > > > 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@chromium.org> > > Reviewed-by: Christian König <christian.koenig@amd.com> > > --- > > 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
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) + */ +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 */ struct sync_set_deadline { __u64 deadline_ns;