[PATCH libinput 1/3] Add a timer subsystem

[Hans de Goede]

Hi,

On 06/05/2014 05:40 AM, Peter Hutterer 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>
>> ---
> 
> [...]
> 
>> +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?

Maybe, if this were a public API I would agree with adding it now, just to
be sure. But since it is not, I vote for leaving it out until we actually
need it.

> 
> 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"
> 

Fixed.

>> +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.
> 

Done.

>> +       void (*timer_func)(uint64_t now, void *timer_func_data);
>> +       void *timer_func_data;
>> +};
> 
> 
> series:
> Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
> 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.

An interesting idea, the kernel developer in me objects, as doing
these checks means making a gettimeofday call, and those calls are
far from cheap. But we should not be setting timers that often, so
I'll put adding these checks on my todo list and I'll do an addon
patch with them at some later date :)

Regards,

Hans