[RFC weston 1/2] libweston: Add event loop abstraction API
Pekka Paalanen
ppaalanen at gmail.com
Wed Feb 21 09:54:43 UTC 2018
On Mon, 19 Feb 2018 20:19:04 +0100
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:
> From: Quentin Glidic <sardemff7+git at sardemff7.net>
>
> This will allow compositors to pick any event loop implementation they
> want, and integrating the libweston components with it.
> Specifically, idle and timeout sources can now have proper priorities to
> avoid being ghosted by client event processing.
>
> Signed-off-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> ---
> compositor/main.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++--
> libweston/compositor.c | 86 +++++++++++++++++++++++
> libweston/compositor.h | 67 ++++++++++++++++++
> 3 files changed, 329 insertions(+), 6 deletions(-)
>
Hi,
in principle, the idea of making an interface for this is ok by me, but
I would like to hear others' opinions as well.
The comments below are about the API details.
> diff --git a/compositor/main.c b/compositor/main.c
> index 18810f288..c0a5ba506 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> @@ -76,6 +76,180 @@ struct wet_compositor {
> bool drm_use_current_mode;
> };
>
> +static struct wet_compositor *
> +to_wet_compositor(struct weston_compositor *compositor)
> +{
> + return weston_compositor_get_user_data(compositor);
> +}
> +
> +struct wet_event_source {
> + struct wl_event_loop *loop;
> + struct wl_event_source *source;
> + weston_event_source_callback cb;
> + weston_event_source_fd_callback fd_cb;
> + void *user_data;
> +};
> +
> +static void
> +wet_event_source_idle_cb(void *data)
> +{
> + struct wet_event_source *wsource = data;
> + enum weston_event_source_status ret;
> +
> + ret = wsource->cb(wsource->user_data);
> + if (ret == WESTON_EVENT_SOURCE_REMOVE)
> + return;
> +
> + wsource->source = wl_event_loop_add_idle(wsource->loop, wet_event_source_idle_cb, wsource);
> + if (wsource->source == NULL)
> + free(wsource);
Do we actually want an automatic re-add mode? I don't remember
libweston having any need for it. This implementation is also silent
about failures, it has no way to signal them.
If you modelled this after the "must check" return value in
libwayland-server, I don't think it means this.
> +}
> +
> +static void *
> +wet_event_source_add_idle(struct weston_compositor *compositor,
> + enum weston_event_source_priority priority,
> + weston_event_source_callback cb, void *user_data)
> +{
> + struct wet_event_source *wsource = malloc(sizeof(struct wet_event_source));
> + if (wsource == NULL)
> + return NULL;
> +
> + wsource->loop = wl_display_get_event_loop(compositor->wl_display);
> + wsource->cb = cb;
> + wsource->user_data = user_data;
> +
> + wsource->source = wl_event_loop_add_idle(wsource->loop, wet_event_source_idle_cb, wsource);
> + if (wsource->source == NULL) {
> + free(wsource);
> + return NULL;
> + }
> +
> + return wsource;
> +}
> +
> +static int
> +wet_event_source_timeout_cb(void *data)
> +{
> + struct wet_event_source *wsource = data;
> + enum weston_event_source_status ret;
> +
> + ret = wsource->cb(wsource->user_data);
> + if (ret == WESTON_EVENT_SOURCE_CONTINUE)
libwayland-server timers are always single-shot, there is no repeat
mode.
> + return 0;
> +
> + wl_event_source_remove(wsource->source);
> + free(wsource);
> + return 0;
> +}
> +
> +static void *
> +wet_event_source_add_timeout(struct weston_compositor *compositor,
> + enum weston_event_source_priority priority,
> + uint32_t milliseconds,
> + weston_event_source_callback cb, void *user_data)
I know the libwayland-server timers use an integer milliseconds delay,
but often we actually compute an absolute time when we want be woken up
and convert it to a delay. How about making the API use an absolute
timestamp instead? That means it would also need to specify the clock
to use, but I think it would be more robust against system load when
setting up a timer, if the event loop actually supports absolute times.
> +{
> + struct wet_event_source *wsource = malloc(sizeof(struct wet_event_source));
> + if (wsource == NULL)
> + return NULL;
> +
> + wsource->loop = wl_display_get_event_loop(compositor->wl_display);
> + wsource->cb = cb;
> + wsource->user_data = user_data;
> +
> + wsource->source = wl_event_loop_add_timer(wsource->loop, wet_event_source_timeout_cb, wsource);
> + if (wsource->source == NULL) {
> + free(wsource);
> + return NULL;
> + }
> +
> + if (milliseconds > 0)
> + wl_event_source_timer_update(wsource->source, milliseconds);
> +
> + return wsource;
> +}
> +
> +static void
> +wet_event_source_update_timeout(struct weston_compositor *compositor,
> + void *source, uint32_t milliseconds)
> +{
> + struct wet_event_source *wsource = source;
> +
> + wl_event_source_timer_update(wsource->source, milliseconds);
> +}
> +
> +static int
> +wet_event_source_fd_cb(int fd, uint32_t mask, void *data)
> +{
> + struct wet_event_source *wsource = data;
> + enum weston_event_source_status ret;
> +
> + ret = wsource->fd_cb(fd, mask, wsource->user_data);
> + if (ret == WESTON_EVENT_SOURCE_CONTINUE)
> + return 0;
> +
> + wl_event_source_remove(wsource->source);
> + free(wsource);
What's with this automatic removal? Where do we use it?
For fds, it would be common to change the events to watch for (polling
for writable is usually temporary, polling for readable is practically
always on).
> + return 0;
> +}
> +
> +static void *
> +wet_event_source_add_fd(struct weston_compositor *compositor,
> + enum weston_event_source_priority priority,
> + int fd, enum weston_event_source_fd_events events,
> + weston_event_source_fd_callback cb, void *user_data)
> +{
> + struct wet_event_source *wsource = malloc(sizeof(struct wet_event_source));
> + if (wsource == NULL)
> + return NULL;
> +
> + wsource->loop = wl_display_get_event_loop(compositor->wl_display);
> + wsource->fd_cb = cb;
> + wsource->user_data = user_data;
> +
> + wsource->source = wl_event_loop_add_fd(wsource->loop, fd, events, wet_event_source_fd_cb, wsource);
> + if (wsource->source == NULL) {
> + free(wsource);
> + return NULL;
> + }
> +
> + return wsource;
> +}
> +
> +static void
> +wet_event_source_update_fd(struct weston_compositor *compositor, void *source,
> + enum weston_event_source_fd_events events)
> +{
> + struct wet_event_source *wsource = source;
> +
> + wl_event_source_fd_update(wsource->source, events);
> +}
> +
> +static bool
> +wet_event_source_is_valid(struct weston_compositor *compositor, void *source)
> +{
> + return (source != NULL);
In isolation, this looks a bit... unnecessary?
> +}
> +
> +static void
> +wet_event_source_remove(struct weston_compositor *compositor, void *source)
> +{
> + struct wet_event_source *wsource = source;
> +
> + wl_event_source_remove(wsource->source);
> + free(wsource);
> +}
> +
> +
> +static const struct weston_event_loop wet_event_loop = {
> + .add_idle = wet_event_source_add_idle,
> + .add_timeout = wet_event_source_add_timeout,
> + .update_timeout = wet_event_source_update_timeout,
> + .add_fd = wet_event_source_add_fd,
> + .update_fd = wet_event_source_update_fd,
> + .is_valid = wet_event_source_is_valid,
> + .remove = wet_event_source_remove,
> +};
> +
> static FILE *weston_logfile = NULL;
>
> static int cached_tm_mday = -1;
> @@ -346,12 +520,6 @@ log_uname(void)
> usys.version, usys.machine);
> }
>
> -static struct wet_compositor *
> -to_wet_compositor(struct weston_compositor *compositor)
> -{
> - return weston_compositor_get_user_data(compositor);
> -}
> -
> static void
> wet_set_pending_output_handler(struct weston_compositor *ec,
> wl_notify_func_t handler)
> @@ -1773,6 +1941,8 @@ int main(int argc, char *argv[])
> }
> segv_compositor = ec;
>
> + ec->event_loop = &wet_event_loop;
> +
> if (weston_compositor_init_config(ec, config) < 0)
> goto out;
>
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index aec937bb8..2f129da38 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -5732,3 +5732,89 @@ weston_compositor_load_xwayland(struct weston_compositor *compositor)
> return -1;
> return 0;
> }
> +
All this stuff could use a new file, IMO.
> +
> +WL_EXPORT void
> +*weston_compositor_event_source_add_idle(struct weston_compositor *compositor, enum weston_event_source_priority priority, weston_event_source_callback cb, void *user_data)
> +{
> + assert(compositor->event_loop != NULL);
> + assert(compositor->event_loop->add_idle != NULL);
> +
> + return compositor->event_loop->add_idle(compositor, priority, cb, user_data);
> +}
> +
> +WL_EXPORT void
> +*weston_compositor_event_source_add_timeout(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t milliseconds, weston_event_source_callback cb, void *user_data)
> +{
> + assert(compositor->event_loop != NULL);
> + assert(compositor->event_loop->add_timeout != NULL);
> +
> + return compositor->event_loop->add_timeout(compositor, priority, milliseconds, cb, user_data);
> +}
> +
> +WL_EXPORT void
> +weston_compositor_event_source_update_timeout(struct weston_compositor *compositor, void *source, uint32_t milliseconds)
> +{
> + assert(compositor->event_loop != NULL);
> + assert(compositor->event_loop->update_timeout != NULL);
> +
> + compositor->event_loop->update_timeout(compositor, source, milliseconds);
> +}
> +
> +WL_EXPORT void
> +*weston_compositor_event_source_add_timeout_seconds(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t seconds, weston_event_source_callback cb, void *user_data)
> +{
Why the _seconds variant?
> + assert(compositor->event_loop != NULL);
> +
> + if (compositor->event_loop->add_timeout_seconds == NULL)
> + return compositor->event_loop->add_timeout(compositor, priority, seconds * 1000, cb, user_data);
> + else
> + return compositor->event_loop->add_timeout_seconds(compositor, priority, seconds, cb, user_data);
> +}
> +
> +WL_EXPORT void
> +weston_compositor_event_source_update_timeout_seconds(struct weston_compositor *compositor, void *source, uint32_t seconds)
> +{
> + assert(compositor->event_loop != NULL);
> +
> + if (compositor->event_loop->update_timeout_seconds == NULL)
> + return compositor->event_loop->update_timeout(compositor, source, seconds * 1000);
> + else
> + return compositor->event_loop->update_timeout_seconds(compositor, source, seconds);
> +}
> +
> +WL_EXPORT void
> +*weston_compositor_event_source_add_fd(struct weston_compositor *compositor, enum weston_event_source_priority priority, int fd, enum weston_event_source_fd_events events, weston_event_source_fd_callback cb, void *user_data)
> +{
> + assert(compositor->event_loop != NULL);
> + assert(compositor->event_loop->add_fd != NULL);
> +
> + return compositor->event_loop->add_fd(compositor, priority, fd, events, cb, user_data);
> +}
> +
> +WL_EXPORT void
> +weston_compositor_event_source_update_fd(struct weston_compositor *compositor, void *source, enum weston_event_source_fd_events events)
> +{
> + assert(compositor->event_loop != NULL);
> + assert(compositor->event_loop->update_fd != NULL);
> +
> + return compositor->event_loop->update_fd(compositor, source, events);
> +}
> +
> +WL_EXPORT bool
> +weston_compositor_event_source_is_valid(struct weston_compositor *compositor, void *source)
> +{
> + assert(compositor->event_loop != NULL);
> + assert(compositor->event_loop->is_valid != NULL);
> +
> + return compositor->event_loop->is_valid(compositor, source);
> +}
> +
> +WL_EXPORT void
> +weston_compositor_event_source_remove(struct weston_compositor *compositor, void *source)
> +{
> + assert(compositor->event_loop != NULL);
> + assert(compositor->event_loop->remove != NULL);
> +
> + return compositor->event_loop->remove(compositor, source);
> +}
> diff --git a/libweston/compositor.h b/libweston/compositor.h
> index ca1acc605..9d662b9d5 100644
> --- a/libweston/compositor.h
> +++ b/libweston/compositor.h
> @@ -857,11 +857,13 @@ struct weston_backend {
>
> struct weston_desktop_xwayland;
> struct weston_desktop_xwayland_interface;
> +struct weston_event_loop;
>
> struct weston_compositor {
> struct wl_signal destroy_signal;
>
> struct wl_display *wl_display;
> + const struct weston_event_loop *event_loop;
> struct weston_desktop_xwayland *xwayland;
> const struct weston_desktop_xwayland_interface *xwayland_interface;
>
> @@ -1959,6 +1961,71 @@ weston_pending_output_coldplug(struct weston_compositor *compositor);
> struct weston_output *
> weston_output_from_resource(struct wl_resource *resource);
>
> +enum weston_event_source_priority {
> + WESTON_EVENT_SOURCE_PRIORITY_HIGH = -100,
> + WESTON_EVENT_SOURCE_PRIORITY_DEFAULT = 0,
> + WESTON_EVENT_SOURCE_PRIORITY_TIMEOUT = 0,
> + WESTON_EVENT_SOURCE_PRIORITY_FD = 0,
> + WESTON_EVENT_SOURCE_PRIORITY_IDLE = 200,
> + WESTON_EVENT_SOURCE_PRIORITY_LOW = 300,
> +};
There can be several kinds of fd sources etc., maybe the API functions
should take an int instead of enum as argument? This list will probably
change in any case to list not high/default/low, but
input-device/output-device/repaint-timing/shell-timeout/...
> +enum weston_event_source_status {
> + WESTON_EVENT_SOURCE_REMOVE,
> + WESTON_EVENT_SOURCE_CONTINUE,
> +};
This was the thing I didn't understand the need of.
> +enum weston_event_source_fd_events {
> + WESTON_EVENT_SOURCE_FD_IN = (1 << 0),
> + WESTON_EVENT_SOURCE_FD_OUT = (1 << 1),
> + WESTON_EVENT_SOURCE_FD_HUP = (1 << 2),
> + WESTON_EVENT_SOURCE_FD_ERR = (1 << 3),
> +};
I think ERR and HUP should be delivered always, but I suppose you need
them here for the callbacks more than the API funcs.
> +typedef enum weston_event_source_status (*weston_event_source_callback)(void *user_data);
> +typedef enum weston_event_source_status (*weston_event_source_fd_callback)(int fd, enum weston_event_source_fd_events events, void *user_data);
> +struct weston_event_loop {
> + void *(*add_idle)
> + (struct weston_compositor *compositor,
> + enum weston_event_source_priority priority,
> + weston_event_source_callback cb, void *user_data);
> + void *(*add_timeout)
> + (struct weston_compositor *compositor,
> + enum weston_event_source_priority priority,
> + uint32_t milliseconds,
> + weston_event_source_callback cb, void *user_data);
> + void (*update_timeout)
> + (struct weston_compositor *compositor, void *source,
> + uint32_t milliseconds);
> + void *(*add_timeout_seconds)
> + (struct weston_compositor *compositor,
> + enum weston_event_source_priority priority,
> + uint32_t seconds,
> + weston_event_source_callback cb, void *user_data);
> + void (*update_timeout_seconds)
> + (struct weston_compositor *compositor, void *source,
> + uint32_t seconds);
> + void *(*add_fd)
> + (struct weston_compositor *compositor,
> + enum weston_event_source_priority priority,
> + int fd, enum weston_event_source_fd_events events,
> + weston_event_source_fd_callback cb, void *user_data);
> + void (*update_fd)
> + (struct weston_compositor *compositor, void *source,
> + enum weston_event_source_fd_events events);
> + bool (*is_valid)
> + (struct weston_compositor *compositor, void *source);
> + void (*remove)
> + (struct weston_compositor *compositor, void *source);
> +};
> +
> +void *weston_compositor_event_source_add_idle(struct weston_compositor *compositor, enum weston_event_source_priority priority, weston_event_source_callback cb, void *user_data);
> +void *weston_compositor_event_source_add_timeout(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t milliseconds, weston_event_source_callback cb, void *user_data);
> +void weston_compositor_event_source_update_timeout(struct weston_compositor *compositor, void *source, uint32_t milliseconds);
> +void *weston_compositor_event_source_add_timeout_seconds(struct weston_compositor *compositor, enum weston_event_source_priority priority, uint32_t seconds, weston_event_source_callback cb, void *user_data);
> +void weston_compositor_event_source_update_timeout_seconds(struct weston_compositor *compositor, void *source, uint32_t seconds);
> +void *weston_compositor_event_source_add_fd(struct weston_compositor *compositor, enum weston_event_source_priority priority, int fd, enum weston_event_source_fd_events events, weston_event_source_fd_callback cb, void *user_data);
> +void weston_compositor_event_source_update_fd(struct weston_compositor *compositor, void *source, enum weston_event_source_fd_events events);
> +bool weston_compositor_event_source_is_valid(struct weston_compositor *compositor, void *source);
> +void weston_compositor_event_source_remove(struct weston_compositor *compositor, void *source);
> +
> #ifdef __cplusplus
> }
> #endif
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180221/70e17f16/attachment.sig>
More information about the wayland-devel
mailing list