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]>
> ---
[...]
> +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);
would it make sense to pass the timer itself into the timer func, for the
use-cases where the func handles multiple timers and only needs to e.g.
print and re-arm that timer?
this is an internal API so we can add it when needed later.
> + }
> + }
> +}
> +
> +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)
> +{
> + /* All timer uses should have destroyed their timers now */
typo: "users"
> +struct libinput_timer {
> + struct libinput *libinput;
> + struct list list;
> + uint64_t expire;
do me a favour and add a comment here and in
libinput_timer_set that this is in abstime, not reltime.
> + void (*timer_func)(uint64_t now, void *timer_func_data);
> + void *timer_func_data;
> +};
series:
Reviewed-by: Peter Hutterer <[email protected]>
with Jonas' comments and the above addressed.
two other things that you may or may not want to add because they help
debugging but require some more effort:
- put a log_bug_libinput() into libinput_timer_set if the timer is <
current time
- put a log_bug_libinput() into libinput_timer_set if the timer is
more than say 5s into the future. this is arbitrary but a
good safety guard that can save on some debugging time.
Cheers,
Peter
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
