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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6084 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]