On Wed, Jun 04, 2014 at 05:26:34PM +0200, Hans de Goede wrote:
> Currently we are using DIY timers in the touchpad softbutton and tap handling
> code, and at least the softbutton code gets its wrong. It uses one timer-fd
> per touchpad to set a timeout per touch, which means that if a timeout is
> set for 100ms from now for touch 1, and then a timeout gets set 50 ms later
> for 200 ms from now, then the timeout for touch 1 will come 150 ms too late.
>
> This commits adds a proper timer subsystem so that we've one place to deal
> with timer handling, and so that we can only get it wrong (well hopefully
> we get it right) in one place.
>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> src/Makefile.am | 2 +
> src/libinput-private.h | 9 ++-
> src/libinput.c | 11 +++-
> src/timer.c | 149
> +++++++++++++++++++++++++++++++++++++++++++++++++
> src/timer.h | 56 +++++++++++++++++++
> 5 files changed, 224 insertions(+), 3 deletions(-)
> create mode 100644 src/timer.c
> create mode 100644 src/timer.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index efb089a..8b56348 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -22,6 +22,8 @@ libinput_la_SOURCES = \
> path.c \
> udev-seat.c \
> udev-seat.h \
> + timer.c \
> + timer.h \
> ../include/linux/input.h
>
> libinput_la_LIBADD = $(MTDEV_LIBS) \
> diff --git a/src/libinput-private.h b/src/libinput-private.h
> index 0e92e68..ed8190d 100644
> --- a/src/libinput-private.h
> +++ b/src/libinput-private.h
> @@ -28,6 +28,8 @@
> #include "libinput.h"
> #include "libinput-util.h"
>
> +struct libinput_source;
> +
> struct libinput_interface_backend {
> int (*resume)(struct libinput *libinput);
> void (*suspend)(struct libinput *libinput);
> @@ -40,6 +42,12 @@ struct libinput {
>
> struct list seat_list;
>
> + struct {
> + struct list list;
> + struct libinput_source *source;
> + int fd;
> + } timer;
> +
> struct libinput_event **events;
> size_t events_count;
> size_t events_len;
> @@ -79,7 +87,6 @@ struct libinput_device {
>
> typedef void (*libinput_source_dispatch_t)(void *data);
>
> -struct libinput_source;
>
> #define log_debug(...) log_msg(LIBINPUT_LOG_PRIORITY_DEBUG, __VA_ARGS__)
> #define log_info(...) log_msg(LIBINPUT_LOG_PRIORITY_INFO, __VA_ARGS__)
> diff --git a/src/libinput.c b/src/libinput.c
> index 6b7e8b8..e74d29f 100644
> --- a/src/libinput.c
> +++ b/src/libinput.c
> @@ -33,6 +33,7 @@
> #include "libinput.h"
> #include "libinput-private.h"
> #include "evdev.h"
> +#include "timer.h"
>
> struct libinput_source {
> libinput_source_dispatch_t dispatch;
> @@ -485,6 +486,12 @@ libinput_init(struct libinput *libinput,
> list_init(&libinput->source_destroy_list);
> list_init(&libinput->seat_list);
>
> + if (libinput_timer_subsys_init(libinput) != 0) {
> + free(libinput->events);
> + close(libinput->epoll_fd);
> + return -1;
> + }
> +
> return 0;
> }
>
> @@ -521,8 +528,6 @@ libinput_destroy(struct libinput *libinput)
> while ((event = libinput_get_event(libinput)))
> libinput_event_destroy(event);
>
> - libinput_drop_destroyed_sources(libinput);
> -
> free(libinput->events);
>
> list_for_each_safe(seat, next_seat, &libinput->seat_list, link) {
> @@ -534,6 +539,8 @@ libinput_destroy(struct libinput *libinput)
> libinput_seat_destroy(seat);
> }
>
> + libinput_timer_subsys_exit(libinput);
> + libinput_drop_destroyed_sources(libinput);
> close(libinput->epoll_fd);
> free(libinput);
> }
> diff --git a/src/timer.c b/src/timer.c
> new file mode 100644
> index 0000000..349a6c9
> --- /dev/null
> +++ b/src/timer.c
> @@ -0,0 +1,149 @@
> +/*
> + * Copyright © 2014 Red Hat, Inc.
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission. The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose. It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <assert.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <sys/timerfd.h>
> +#include <unistd.h>
> +
> +#include "libinput-private.h"
> +#include "timer.h"
> +
> +void
> +libinput_timer_init(struct libinput_timer *timer, struct libinput *libinput,
> + void (*timer_func)(uint64_t now, void *timer_func_data),
> + void *timer_func_data)
> +{
> + timer->libinput = libinput;
> + timer->timer_func = timer_func;
> + timer->timer_func_data = timer_func_data;
> +}
> +
> +static void
> +libinput_timer_arm_timer_fd(struct libinput *libinput)
> +{
> + int r;
> + struct libinput_timer *timer;
> + struct itimerspec its = {
> + .it_interval.tv_sec = 0,
> + .it_interval.tv_nsec = 0,
> + .it_value.tv_sec = 0,
> + .it_value.tv_nsec = 0,
> + };
This could be shortened to: struct itimerspec its = { 0 };
> + uint64_t earliest_expire = UINT64_MAX;
> +
> + list_for_each(timer, &libinput->timer.list, list) {
> + if (timer->expire < earliest_expire)
> + earliest_expire = timer->expire;
> + }
> +
> + if (earliest_expire != UINT64_MAX) {
> + its.it_value.tv_sec = earliest_expire / 1000;
> + its.it_value.tv_nsec = (earliest_expire % 1000) * 1000 * 1000;
> + }
> +
> + r = timerfd_settime(libinput->timer.fd, TFD_TIMER_ABSTIME, &its, NULL);
> + if (r)
> + log_error("timerfd_settime error: %s\n", strerror(errno));
> +}
> +
> +void
> +libinput_timer_set(struct libinput_timer *timer, uint64_t expire)
> +{
> + assert(expire);
> +
> + if (!timer->expire)
> + list_insert(&timer->libinput->timer.list, &timer->list);
> +
> + timer->expire = expire;
> + libinput_timer_arm_timer_fd(timer->libinput);
> +}
> +
> +void
> +libinput_timer_cancel(struct libinput_timer *timer)
> +{
> + if (!timer->expire)
> + return;
> +
> + timer->expire = 0;
> + list_remove(&timer->list);
> + libinput_timer_arm_timer_fd(timer->libinput);
> +}
> +
> +static void
> +libinput_timer_handler(void *data)
> +{
> + struct libinput *libinput = data;
> + struct libinput_timer *timer, *tmp;
> + struct timespec ts;
> + uint64_t now;
> + int r;
> +
> + r = clock_gettime(CLOCK_MONOTONIC, &ts);
> + if (r) {
> + log_error("clock_gettime error: %s\n", strerror(errno));
> + return;
> + }
> +
> + now = ts.tv_sec * 1000ULL + ts.tv_nsec / 1000000;
> +
> + list_for_each_safe(timer, tmp, &libinput->timer.list, list) {
> + if (timer->expire <= now) {
> + /* Clear the timer before calling timer_func,
> + as timer_func may re-arm it */
> + libinput_timer_cancel(timer);
> + timer->timer_func(now, timer->timer_func_data);
> + }
> + }
> +}
> +
> +int
> +libinput_timer_subsys_init(struct libinput *libinput)
> +{
> + libinput->timer.fd = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
> + if (libinput->timer.fd < 0)
> + return -1;
> +
> + list_init(&libinput->timer.list);
> +
> + libinput->timer.source = libinput_add_fd(libinput,
> + libinput->timer.fd,
> + libinput_timer_handler,
> + libinput);
> + if (!libinput->timer.source) {
> + close(libinput->timer.fd);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +void
> +libinput_timer_subsys_exit(struct libinput *libinput)
For consistency, this should be _destroy() instead of _exit().
> +{
> + /* All timer uses should have destroyed their timers now */
> + assert(list_empty(&libinput->timer.list));
> +
> + libinput_remove_source(libinput, libinput->timer.source);
> + close(libinput->timer.fd);
> +}
> diff --git a/src/timer.h b/src/timer.h
> new file mode 100644
> index 0000000..fb2e9b2
> --- /dev/null
> +++ b/src/timer.h
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright © 2014 Red Hat, Inc.
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission. The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose. It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#ifndef TIMER_H
> +#define TIMER_H
> +
> +#include <stdint.h>
> +
> +struct libinput;
> +
> +struct libinput_timer {
> + struct libinput *libinput;
> + struct list list;
This should probably be called "link", as its not a list, but a link in
a linked list.
> + uint64_t expire;
> + void (*timer_func)(uint64_t now, void *timer_func_data);
> + void *timer_func_data;
> +};
> +
> +void
> +libinput_timer_init(struct libinput_timer *timer, struct libinput *libinput,
> + void (*timer_func)(uint64_t now, void *timer_func_data),
> + void *timer_func_data);
> +
> +/* Set timer expire time, in ms CLOCK_MONOTONIC */
> +void
> +libinput_timer_set(struct libinput_timer *timer, uint64_t expire);
> +
> +void
> +libinput_timer_cancel(struct libinput_timer *timer);
> +
> +int
> +libinput_timer_subsys_init(struct libinput *libinput);
> +
> +void
> +libinput_timer_subsys_exit(struct libinput *libinput);
> +
> +#endif
> --
> 2.0.0
>
Except for those comments above, this looks good to me.
Jonas
> _______________________________________________
> wayland-devel mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
