[PATCH libinput 1/3] Add a timer subsystem
Hans de Goede
hdegoede at redhat.com
Fri Jun 6 07:49:31 PDT 2014
Hi,
On 06/05/2014 12:18 AM, Jonas Ådahl wrote:
> 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 <hdegoede at redhat.com>
>> ---
>> 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 };
Nope, that won't work because it has only sub-structs, the shortest I can make it
without gcc becoming unhappy is:
struct itimerspec its = { { 0 }, { 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().
Fixed.
>
>> +{
>> + /* 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.
Fixed for v2.
>
>> + 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.
Thanks for the review.
Regards,
Hans
More information about the wayland-devel
mailing list