Re: [RFC PATCH] drm/syncobj: add IOCTL to register an eventfd for a timeline
On Monday, October 10th, 2022 at 10:19, Pekka Paalanen
wrote:
> I'm completely clueless about this API.
No worries!
> > +/**
> > + * 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.
>
> Sounds nice.
>
> Since the action is to increment the counter by one, does it mean it
> will be possible to wait for a bunch of completions and have the
> eventfd poll return only when they have all signaled?
It is possible to perform the IOCTL multiple times with the same eventfd, but
eventfd semnatics would wake up user-space each time any timeline point
completes.
> > + */
> > +struct drm_syncobj_timeline_register_eventfd {
> > + __u32 handle;
>
> Handle of what?
drm_syncobj handle
> > + __u32 flags;
>
> What flags are allowed? Must be zero for now?
Same flags as the wait IOCTL.
Must be WAIT_AVAILABLE for now, but I'll implement the zero case as well (see
TODO).
> > + __u64 point;
>
> Is this some Vulkan thingy?
It's a drm_syncobj timeline thing. The timeline contains multiple sync points.
> > + __s32 fd;
>
> I guess the userspace needs to create an eventfd first, and pass it as
> the argument here? This is not creating a new eventfd itself?
Correct
> > + __u32 pad;
>
> Must be zero?
Indeed.
I'll spell out these requirements explicitly in the next version.
Re: [RFC PATCH] drm/syncobj: add IOCTL to register an eventfd for a timeline
On Sunday, October 9th, 2022 at 20:00, Christian König wrote: > 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. Hm what do you mean exactly? I'm not sure I'm following. > 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. Agreed. > 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). This makes sense to me.
[ANNOUNCE] wayland-protocols 1.27
wayland-protocols 1.27 is now available. This release includes two new staging protocols: * Content type hint This protocol enables clients to provide hints to the compositor about what kind of content it provides, allowing compositors to optionally adapt its behavior accordingly. * Idle notify This extension allows compositors to notify clients about when the user is idle. Apart from these two new extensions, this release also brings the usual clarifications, cleanups and fixes. Enjoy! Daniel Stone (1): xdg-shell: ack_configure must be strictly monotonic Emmanuel Gil Peyrot (1): staging/content-type: Content type hint support Isaac Freund (1): ext-session-lock: add note on client termination Jonas Ådahl (1): build: Bump version to 1.27 Simon Ser (3): xdg-shell: forbid loops in set_parent ext-idle-notify: new protocol build: alphabetically sort list of staging protocols git tag: 1.27 https://gitlab.freedesktop.org/wayland/wayland-protocols/-/releases/1.27/downloads/wayland-protocols-1.27.tar.xz SHA256: 9046f10a425d4e2a00965a03acfb6b3fb575a56503ac72c2b86821c69653375c wayland-protocols-1.27.tar.xz SHA512: c0a49bc46c663c9f602998dfe2e184c09756790fbcc7acbc2bf9d9cf8f7d6dcdd00259b768222a30e5d134e6f97f7f4faf252947b544e8b32f53278b70da0390 wayland-protocols-1.27.tar.xz PGP: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/releases/1.27/downloads/wayland-protocols-1.27.tar.xz.sig signature.asc Description: PGP signature
Re: [RFC PATCH] drm/syncobj: add IOCTL to register an eventfd for a timeline
On Sun, 09 Oct 2022 14:40:14 +
Simon Ser wrote:
> Introduce a new DRM_IOCTL_SYNCOBJ_TIMELINE_REGISTER_EVENTFD IOCTL
> which signals an eventfd when a timeline point completes.
>
> 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
> Cc: Jason Ekstrand
> Cc: Daniel Vetter
> Cc: Christian König
> Cc: Bas Nieuwenhuizen
> Cc: Daniel Stone
> Cc: Pekka Paalanen
> Cc: James Jones
> ---
> 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/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;
> };
>
Hi,
I'm completely clueless about this API.
> +/**
> + * 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.
Sounds nice.
Since the action is to increment the counter by one, does it mean it
will be possible to wait for a bunch of completions and have the
eventfd poll return only when they have all signaled?
> + */
> +struct drm_syncobj_timeline_register_eventfd {
> + __u32 handle;
Handle of what?
> + __u32 flags;
What flags are allowed? Must be zero for now?
> + __u64 point;
Is this some Vulkan thingy?
> + __s32 fd;
I guess the userspace needs to create an eventfd first, and pass it as
the argument here? This is not creating a new eventfd itself?
> + __u32 pad;
Must be zero?
Thanks,
pq
> +};
> +
>
> 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_SIGNALDRM_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.
pgpOPOXVs7Q2J.pgp
Description: OpenPGP digital signature
Re: [RFC PATCH] drm/syncobj: add IOCTL to register an eventfd for a timeline
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
Cc: Jason Ekstrand
Cc: Daniel Vetter
Cc: Christian König
Cc: Bas Nieuwenhuizen
Cc: Daniel Stone
Cc: Pekka Paalanen
Cc: James Jones
---
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
#include
+#include
#include
#include
#include
@@ -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(>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(>node);
+ kfree(entry);
+}
+
+static void
+drm_syncobj_add_eventfd(struct drm_syncobj *syncobj,
+ struct syncobj_eventfd_entry *entry)
+{
+ spin_lock(>lock);
+ list_add_tail(>node, >cb_list);
+ syncobj_eventfd_entry_func(syncobj, entry);
+ spin_unlock(>lock);
+}
+
/**
* drm_syncobj_add_point - add new timeline point to the syncobj
* @syncobj:
