[RFC PATCH] drm/syncobj: add IOCTL to register an eventfd for a timeline

Christian König christian.koenig at amd.com
Sun Oct 9 18:00:10 UTC 2022


Am 09.10.22 um 16:40 schrieb Simon Ser:
> Introduce a new DRM_IOCTL_SYNCOBJ_TIMELINE_REGISTER_EVENTFD IOCTL
> which signals an eventfd when a timeline point completes.

I was entertaining the same though for quite a while, but I would even 
go a step further and actually always base the wait before signal 
functionality of the drm_syncobj and the eventfd functionality. That 
would save us quite a bit of complexity I think.

As a general note I think the name 
DRM_IOCTL_SYNCOBJ_TIMELINE_REGISTER_EVENTFD is just to long, just make 
that DRM_IOCTL_SYNCOBJ_EVENTFD. Same for the function names as well.

Additional to that I think we should also always have a graceful 
handling for binary syncobjs. So please try to avoid making this special 
for the timeline case (the timeline case should of course still be 
supported).

Regards,
Christian.


>
> This is useful for Wayland compositors to handle wait-before-submit.
> Wayland clients can send a timeline point to the compositor
> before the point has materialized yet, then compositors can wait
> for the point to materialize via this new IOCTL.
>
> The existing DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT IOCTL is not suitable
> because it blocks. Compositors want to integrate the wait with
> their poll(2)-based event loop.
>
> Signed-off-by: Simon Ser <contact at emersion.fr>
> Cc: Jason Ekstrand <jason at jlekstrand.net>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
> Cc: Daniel Stone <daniel at fooishbar.org>
> Cc: Pekka Paalanen <ppaalanen at gmail.com>
> Cc: James Jones <jajones at nvidia.com>
> ---
>   drivers/gpu/drm/drm_internal.h |   3 +
>   drivers/gpu/drm/drm_ioctl.c    |   3 +
>   drivers/gpu/drm/drm_syncobj.c  | 113 +++++++++++++++++++++++++++++++--
>   include/drm/drm_syncobj.h      |   6 +-
>   include/uapi/drm/drm.h         |  15 +++++
>   5 files changed, 133 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 1fbbc19f1ac0..4244e929b7f9 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -242,6 +242,9 @@ int drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
>   			   struct drm_file *file_private);
>   int drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
>   				    struct drm_file *file_private);
> +int drm_syncobj_timeline_register_eventfd_ioctl(struct drm_device *dev,
> +						void *data,
> +						struct drm_file *file_private);
>   int drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
>   			    struct drm_file *file_private);
>   int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index ca2a6e6101dc..dcd18dba28b7 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -702,6 +702,9 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>   		      DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT, drm_syncobj_timeline_wait_ioctl,
>   		      DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_TIMELINE_REGISTER_EVENTFD,
> +		      drm_syncobj_timeline_register_eventfd_ioctl,
> +		      DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_RESET, drm_syncobj_reset_ioctl,
>   		      DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 0c2be8360525..401d09b56cf1 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -185,6 +185,7 @@
>   
>   #include <linux/anon_inodes.h>
>   #include <linux/dma-fence-unwrap.h>
> +#include <linux/eventfd.h>
>   #include <linux/file.h>
>   #include <linux/fs.h>
>   #include <linux/sched/signal.h>
> @@ -212,6 +213,17 @@ struct syncobj_wait_entry {
>   static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
>   				      struct syncobj_wait_entry *wait);
>   
> +struct syncobj_eventfd_entry {
> +	struct list_head node;
> +	struct dma_fence_cb fence_cb;
> +	struct eventfd_ctx *ev_fd_ctx;
> +	u64 point;
> +};
> +
> +static void
> +syncobj_eventfd_entry_func(struct drm_syncobj *syncobj,
> +			   struct syncobj_eventfd_entry *entry);
> +
>   /**
>    * drm_syncobj_find - lookup and reference a sync object.
>    * @file_private: drm file private pointer
> @@ -274,6 +286,25 @@ static void drm_syncobj_remove_wait(struct drm_syncobj *syncobj,
>   	spin_unlock(&syncobj->lock);
>   }
>   
> +static void
> +syncobj_eventfd_entry_free(struct syncobj_eventfd_entry *entry)
> +{
> +	eventfd_ctx_put(entry->ev_fd_ctx);
> +	/* This happens inside the syncobj lock */
> +	list_del(&entry->node);
> +	kfree(entry);
> +}
> +
> +static void
> +drm_syncobj_add_eventfd(struct drm_syncobj *syncobj,
> +			struct syncobj_eventfd_entry *entry)
> +{
> +	spin_lock(&syncobj->lock);
> +	list_add_tail(&entry->node, &syncobj->cb_list);
> +	syncobj_eventfd_entry_func(syncobj, entry);
> +	spin_unlock(&syncobj->lock);
> +}
> +
>   /**
>    * drm_syncobj_add_point - add new timeline point to the syncobj
>    * @syncobj: sync object to add timeline point do
> @@ -288,7 +319,8 @@ void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>   			   struct dma_fence *fence,
>   			   uint64_t point)
>   {
> -	struct syncobj_wait_entry *cur, *tmp;
> +	struct syncobj_wait_entry *wait_cur, *wait_tmp;
> +	struct syncobj_eventfd_entry *ev_fd_cur, *ev_fd_tmp;
>   	struct dma_fence *prev;
>   
>   	dma_fence_get(fence);
> @@ -302,8 +334,10 @@ void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>   	dma_fence_chain_init(chain, prev, fence, point);
>   	rcu_assign_pointer(syncobj->fence, &chain->base);
>   
> -	list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node)
> -		syncobj_wait_syncobj_func(syncobj, cur);
> +	list_for_each_entry_safe(wait_cur, wait_tmp, &syncobj->cb_list, node)
> +		syncobj_wait_syncobj_func(syncobj, wait_cur);
> +	list_for_each_entry_safe(ev_fd_cur, ev_fd_tmp, &syncobj->ev_fd_list, node)
> +		syncobj_eventfd_entry_func(syncobj, ev_fd_cur);
>   	spin_unlock(&syncobj->lock);
>   
>   	/* Walk the chain once to trigger garbage collection */
> @@ -323,7 +357,8 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>   			       struct dma_fence *fence)
>   {
>   	struct dma_fence *old_fence;
> -	struct syncobj_wait_entry *cur, *tmp;
> +	struct syncobj_wait_entry *wait_cur, *wait_tmp;
> +	struct syncobj_eventfd_entry *ev_fd_cur, *ev_fd_tmp;
>   
>   	if (fence)
>   		dma_fence_get(fence);
> @@ -335,8 +370,10 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>   	rcu_assign_pointer(syncobj->fence, fence);
>   
>   	if (fence != old_fence) {
> -		list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node)
> -			syncobj_wait_syncobj_func(syncobj, cur);
> +		list_for_each_entry_safe(wait_cur, wait_tmp, &syncobj->cb_list, node)
> +			syncobj_wait_syncobj_func(syncobj, wait_cur);
> +		list_for_each_entry_safe(ev_fd_cur, ev_fd_tmp, &syncobj->ev_fd_list, node)
> +			syncobj_eventfd_entry_func(syncobj, ev_fd_cur);
>   	}
>   
>   	spin_unlock(&syncobj->lock);
> @@ -472,7 +509,13 @@ void drm_syncobj_free(struct kref *kref)
>   	struct drm_syncobj *syncobj = container_of(kref,
>   						   struct drm_syncobj,
>   						   refcount);
> +	struct syncobj_eventfd_entry *ev_fd_cur, *ev_fd_tmp;
> +
>   	drm_syncobj_replace_fence(syncobj, NULL);
> +
> +	list_for_each_entry_safe(ev_fd_cur, ev_fd_tmp, &syncobj->ev_fd_list, node)
> +		syncobj_eventfd_entry_free(ev_fd_cur);
> +
>   	kfree(syncobj);
>   }
>   EXPORT_SYMBOL(drm_syncobj_free);
> @@ -501,6 +544,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>   
>   	kref_init(&syncobj->refcount);
>   	INIT_LIST_HEAD(&syncobj->cb_list);
> +	INIT_LIST_HEAD(&syncobj->ev_fd_list);
>   	spin_lock_init(&syncobj->lock);
>   
>   	if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
> @@ -1304,6 +1348,63 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
>   	return ret;
>   }
>   
> +static void
> +syncobj_eventfd_entry_func(struct drm_syncobj *syncobj,
> +			   struct syncobj_eventfd_entry *entry)
> +{
> +	int ret;
> +	struct dma_fence *fence;
> +
> +	/* This happens inside the syncobj lock */
> +	fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, 1));
> +	ret = dma_fence_chain_find_seqno(&fence, entry->point);
> +	dma_fence_put(fence);
> +
> +	if (ret != 0)
> +		return;
> +
> +	eventfd_signal(entry->ev_fd_ctx, 1);
> +	syncobj_eventfd_entry_free(entry);
> +}
> +
> +int
> +drm_syncobj_timeline_register_eventfd_ioctl(struct drm_device *dev,
> +					    void *data,
> +					    struct drm_file *file_private)
> +{
> +	struct drm_syncobj_timeline_register_eventfd *args = data;
> +	struct drm_syncobj *syncobj;
> +	struct eventfd_ctx *ev_fd_ctx;
> +	struct syncobj_eventfd_entry *entry;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
> +		return -EOPNOTSUPP;
> +
> +	/* TODO: support other flags? */
> +	if (args->flags != DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)
> +		return -EINVAL;
> +
> +	syncobj = drm_syncobj_find(file_private, args->handle);
> +	if (!syncobj)
> +		return -ENOENT;
> +
> +	ev_fd_ctx = eventfd_ctx_fdget(args->fd);
> +	if (IS_ERR(ev_fd_ctx))
> +		return PTR_ERR(ev_fd_ctx);
> +
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry) {
> +		eventfd_ctx_put(ev_fd_ctx);
> +		return -ENOMEM;
> +	}
> +	entry->ev_fd_ctx = ev_fd_ctx;
> +	entry->point = args->point;
> +
> +	drm_syncobj_add_eventfd(syncobj, entry);
> +	drm_syncobj_put(syncobj);
> +
> +	return 0;
> +}
>   
>   int
>   drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> index 6cf7243a1dc5..b40052132e52 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -54,7 +54,11 @@ struct drm_syncobj {
>   	 */
>   	struct list_head cb_list;
>   	/**
> -	 * @lock: Protects &cb_list and write-locks &fence.
> +	 * @ev_fd_list: List of registered eventfd.
> +	 */
> +	struct list_head ev_fd_list;
> +	/**
> +	 * @lock: Protects &cb_list and &ev_fd_list, and write-locks &fence.
>   	 */
>   	spinlock_t lock;
>   	/**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 642808520d92..359e21414196 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -909,6 +909,20 @@ struct drm_syncobj_timeline_wait {
>   	__u32 pad;
>   };
>   
> +/**
> + * struct drm_syncobj_timeline_register_eventfd
> + *
> + * Register an eventfd to be signalled when a timeline point completes. The
> + * eventfd counter will be incremented by one.
> + */
> +struct drm_syncobj_timeline_register_eventfd {
> +	__u32 handle;
> +	__u32 flags;
> +	__u64 point;
> +	__s32 fd;
> +	__u32 pad;
> +};
> +
>   
>   struct drm_syncobj_array {
>   	__u64 handles;
> @@ -1095,6 +1109,7 @@ extern "C" {
>   #define DRM_IOCTL_SYNCOBJ_QUERY		DRM_IOWR(0xCB, struct drm_syncobj_timeline_array)
>   #define DRM_IOCTL_SYNCOBJ_TRANSFER	DRM_IOWR(0xCC, struct drm_syncobj_transfer)
>   #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL	DRM_IOWR(0xCD, struct drm_syncobj_timeline_array)
> +#define DRM_IOCTL_SYNCOBJ_TIMELINE_REGISTER_EVENTFD	DRM_IOWR(0xCE, struct drm_syncobj_timeline_register_eventfd)
>   
>   /**
>    * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata.



More information about the dri-devel mailing list