Date:   Tue, 28 Feb 2023 11:23:10 +0200
From:   Pekka Paalanen <ppaalanen@gmail.com>
To:     Rob Clark <robdclark@gmail.com>
Cc:     dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org,
        Daniel Vetter <daniel@ffwll.ch>,
        Christian =?UTF-8?B?S8O2bmln?= <ckoenig.leichtzumerken@gmail.com>,
        Michel =?UTF-8?B?RMOkbnplcg==?= <michel@daenzer.net>,
        Tvrtko Ursulin <tvrtko.ursulin@intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        Simon Ser <contact@emersion.fr>,
        Luben Tuikov <luben.tuikov@amd.com>,
        Rob Clark <robdclark@chromium.org>,
        Christian =?UTF-8?B?S8O2bmln?= <christian.koenig@amd.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Gustavo Padovan <gustavo@padovan.org>,
        linux-media@vger.kernel.org (open list:SYNC FILE FRAMEWORK),
        linaro-mm-sig@lists.linaro.org (moderated list:DMA BUFFER SHARING
        FRAMEWORK), linux-kernel@vger.kernel.org (open list)
Subject: Re: [PATCH v7 07/15] dma-buf/sw_sync: Add fence deadline support
Message-ID: <20230228112310.39274fcf@eldfell>
In-Reply-To: <20230227193535.2822389-8-robdclark@gmail.com>
References: <20230227193535.2822389-1-robdclark@gmail.com>
        <20230227193535.2822389-8-robdclark@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="Sig_/8aE4tGFLiLpdo+6qg3s=3bO";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Precedence: bulk

--Sig_/8aE4tGFLiLpdo+6qg3s=3bO
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Mon, 27 Feb 2023 11:35:13 -0800
Rob Clark <robdclark@gmail.com> wrote:

> From: Rob Clark <robdclark@chromium.org>
>=20
> 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.
>=20
> v2: Switch UABI to express deadline as u64
> v3: More verbose UAPI docs, show how to convert from timespec
>=20
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> Reviewed-by: Christian K=C3=B6nig <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(-)
>=20
> 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 */
>  };
> =20
> +/**
> + * 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'
> =20
>  #define SW_SYNC_IOC_CREATE_FENCE	_IOWR(SW_SYNC_IOC_MAGIC, 0,\
>  		struct sw_sync_create_fence_data)
> =20
>  #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)
> =20
>  static const struct dma_fence_ops timeline_fence_ops;
> =20
> @@ -171,6 +187,13 @@ static void timeline_fence_timeline_value_str(struct=
 dma_fence *fence,
>  	snprintf(str, size, "%d", parent->value);
>  }
> =20
> +static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t=
 deadline)
> +{
> +	struct sync_pt *pt =3D dma_fence_to_sync_pt(fence);
> +
> +	pt->deadline =3D deadline;
> +}
> +
>  static const struct dma_fence_ops timeline_fence_ops =3D {
>  	.get_driver_name =3D timeline_fence_get_driver_name,
>  	.get_timeline_name =3D timeline_fence_get_timeline_name,
> @@ -179,6 +202,7 @@ static const struct dma_fence_ops timeline_fence_ops =
=3D {
>  	.release =3D timeline_fence_release,
>  	.fence_value_str =3D timeline_fence_value_str,
>  	.timeline_value_str =3D timeline_fence_timeline_value_str,
> +	.set_deadline =3D timeline_fence_set_deadline,
>  };
> =20
>  /**
> @@ -387,6 +411,37 @@ static long sw_sync_ioctl_inc(struct sync_timeline *=
obj, unsigned long arg)
>  	return 0;
>  }
> =20
> +static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigne=
d 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 =3D sync_file_get_fence(data.fence_fd);
> +	if (!fence)
> +		return -EINVAL;
> +
> +	pt =3D dma_fence_to_sync_pt(fence);
> +	if (!pt)
> +		return -EINVAL;
> +
> +
> +	data.deadline_ns =3D 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);
> =20
> +	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;
>  };
> =20
>  extern const struct file_operations sw_sync_debugfs_fops;
> diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_fil=
e.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).  F=
or
> + * example:
> + *
> + *     clock_gettime(CLOCK_MONOTONIC, &t);
> + *     deadline_ns =3D (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

--Sig_/8aE4tGFLiLpdo+6qg3s=3bO
Content-Type: application/pgp-signature
Content-Description: OpenPGP digital signature

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEJQjwWQChkWOYOIONI1/ltBGqqqcFAmP9x/4ACgkQI1/ltBGq
qqcv6hAAkgjLuM8MZAr36UgGjARjMLvWOcq9adsey/FJJhu4S2+Ly3b/hDU9ZAMZ
Ri5QlZusfpYcpMdBK5xKUceapgPL9hmtbLkp2nQ8I70Bs9GL8qxnRDfXZNBSD+dD
8u5bHRVZy0TtmAfx7mTz/9EgQURTaHTmiQIHIwyMquQXFUep7H9EpA3YqlJFvPte
xPMVgQTJ+s/VT0aIkRwf4yGmPoBGTiRYrhJgMfZedP2huV1E3NXuPRRHDDxhIcbH
ODKSi7HOzuF3nmbsENPKL8TEpi7jblkUj3hVnrXdbzKpSfGlcK1PQ36n3X1SwKEJ
zQb5s6Qm3vWjkTYNljTfNDGv/w2DkJDQBYq6VkvkNoYB0IxJAEXVMYj1G1pTLPZy
bwVD3GC+gLdOrs9kAkbodzzRVAjEfer7RqrIQIfiB59MkWAl22zWWue65zU6jBIl
yIHahMl6JiCXSEdgBGeyLqDkba/WBD+hBic4ooI3JODFRH6xmcs2/zeLr2voY28T
lnAiQMh5JkPuuWdtd0w5HpeqzfMEPdWeEBWeScPXE3jX7utXf/aoHqDXvauH9PFl
xgH6JBqt1RX2e3QQ9IDZvZY6SyA7DeCutZJV0uJ24EoKhdFXjZO7pC0HUmblWo8J
vjgIIfjisKX3+B2tVlOMNYHvTzdkdmbWFV9Q8xf2nhwUpvMtakg=
=5sgc
-----END PGP SIGNATURE-----

--Sig_/8aE4tGFLiLpdo+6qg3s=3bO--