[PATCH libinput 2/2] Hook up event processing to libevdev

[Peter Hutterer]

On Mon, Feb 24, 2014 at 10:03:59PM +0100, Jonas Ådahl wrote:
> On Mon, Feb 24, 2014 at 09:44:34AM +1000, Peter Hutterer wrote:
> > On Sat, Feb 22, 2014 at 04:44:06PM +0100, Jonas Ådahl wrote:
> > > On Tue, Feb 18, 2014 at 04:09:10PM +1000, Peter Hutterer wrote:
> > > > This gives us the ability to handle SYN_DROPPED transparently to the caller.
> > > > 
> > > > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > > > ---
> > > >  src/evdev.c | 90 +++++++++++++++++++++++++++++++++++++++----------------------
> > > >  1 file changed, 58 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/src/evdev.c b/src/evdev.c
> > > > index ba28fc6..836d0af 100644
> > > > --- a/src/evdev.c
> > > > +++ b/src/evdev.c
> > > > @@ -29,7 +29,7 @@
> > > >  #include <linux/input.h>
> > > >  #include <unistd.h>
> > > >  #include <fcntl.h>
> > > > -#include <mtdev.h>
> > > > +#include <mtdev-plumbing.h>
> > > >  #include <assert.h>
> > > >  
> > > >  #include "libinput.h"
> > > > @@ -436,56 +436,82 @@ fallback_dispatch_create(void)
> > > >  	return dispatch;
> > > >  }
> > > >  
> > > > -static void
> > > > -evdev_process_events(struct evdev_device *device,
> > > > -		     struct input_event *ev, int count)
> > > > +static inline void
> > > > +evdev_process_event(struct evdev_device *device, struct input_event *e)
> > > >  {
> > > >  	struct evdev_dispatch *dispatch = device->dispatch;
> > > > -	struct input_event *e, *end;
> > > > -	uint32_t time = 0;
> > > > +	uint32_t time = e->time.tv_sec * 1000 + e->time.tv_usec / 1000;
> > > >  
> > > > -	e = ev;
> > > > -	end = e + count;
> > > > -	for (e = ev; e < end; e++) {
> > > > -		time = e->time.tv_sec * 1000 + e->time.tv_usec / 1000;
> > > > +	dispatch->interface->process(dispatch, device, e, time);
> > > > +}
> > > >  
> > > > -		dispatch->interface->process(dispatch, device, e, time);
> > > > +static inline void
> > > > +evdev_device_dispatch_one(struct evdev_device *device,
> > > > +			  struct input_event *ev)
> > > > +{
> > > > +	if (!device->mtdev) {
> > > > +		evdev_process_event(device, ev);
> > > > +	} else {
> > > > +		mtdev_put_event(device->mtdev, ev);
> > > > +		if (libevdev_event_is_code(ev, EV_SYN, SYN_REPORT)) {
> > > > +			while(!mtdev_empty(device->mtdev)) {
> 
> Nit (coding style): space between while and (

amended, thanks.

..

> > > >  static void
> > > >  evdev_device_dispatch(void *data)
> > > >  {
> > > >  	struct evdev_device *device = data;
> > > >  	struct libinput *libinput = device->base.seat->libinput;
> > > > -	int fd = device->fd;
> > > > -	struct input_event ev[32];
> > > > -	int len;
> > > > +	struct input_event ev;
> > > > +	int rc;
> > > >  
> > > >  	/* If the compositor is repainting, this function is called only once
> > > >  	 * per frame and we have to process all the events available on the
> > > >  	 * fd, otherwise there will be input lag. */
> > > >  	do {
> > > > -		if (device->mtdev)
> > > > -			len = mtdev_get(device->mtdev, fd, ev,
> > > > -					ARRAY_LENGTH(ev)) *
> > > > -				sizeof (struct input_event);
> > > > -		else
> > > > -			len = read(fd, &ev, sizeof ev);
> > > > +		rc = libevdev_next_event(device->evdev,
> > > > +					 LIBEVDEV_READ_FLAG_NORMAL, &ev);
> > > > +		if (rc == LIBEVDEV_READ_STATUS_SYNC) {
> > > > +			/* send one more sync event so we handle all
> > > > +			   currently pending events before we sync up
> > > > +			   to the current state */
> > > > +			ev.code = SYN_REPORT;
> > > > +			evdev_device_dispatch_one(device, &ev);
> > > 
> > > Is this really correct? Shouldn't calling libevdev_next_event() in SYNC
> > > mode return a SYN_REPORT event as part of the synchronization? If we do
> > > like this it looks like we might "cut" one series of events that would
> > > otherwise be grouped using SYN_REPORT in half.
> > 
> > I think you may have misread the diff, this code is only ever called once,
> > on the SYN_DROPPED, all the other events are handled in evdev_sync_device().
> > 
> > libevdev guarantees that the event you pass in when you get
> > LIBEVDEV_READ_STATUS_SYNC is the SYN_DROPPED event that triggered the sync.
> > All we do here is change from EV_SYN/SYN_DROPPED to EV_SYN/SYN_REPORT and
> > process that normally. That finishes the current event sequence.
> > 
> > We then call evdev_sync_device() which empties and processes the sync queue
> > until -EAGAIN. That queue is also guaranteed to end with a
> > EV_SYN/SYN_REPORT. Once that is done, we jump back here and continue with
> > the loop, which now hopefully has SUCCESS on the next read.
> 
> My concern is not that we would add several extra SYN_REPORT splitting
> event series when synchronizing, but that when we have reached the state
> where libevdev_next_event() returns LIBEVDEV_READ_STATUS_SYNC, we might
> be in progress of queuing up a series of events waiting for the next
> SYN_REPORT. What the documentation[0] states regarding this is that all
> events up and including the next SYN_REPORT should be ignored and the
> device should be synchronized. The way I'm reading that is when we reach
> SYN_DROPPED, we should wait with flushing the queue until we can
> synchronize and can provide a more complete state. However, we could as
> well in this case end up with multiple events with the same time in the
> same series (for example if we already queued ABS_X but x has since then
> changed, which would mean we'd have two ABS_X in the same series) so I
> guess its not really better to not flush there.

I think the documentation is a bit off here. As of 3.7
(4369c64c79a22b98d3b7eff9d089196cd878a10a) events are now
atomic, the kernel doesn't write until the SYN_REPORT is ready so there
aren't any events left that we haven't read yet. the documentation predates
that commit (2.6.39).

The only exception is SYN_REPORT with value 1 which I've been told doesn't
really happen anymore and in any way should be handled within libevdev, so
we don't need to cater for it here.

> So, in other words, Reviewed-by: Jonas Ådahl <jadahl at gmail.com>

thanks, much appreciated.

Cheers,
   Peter

> 
> [0] https://www.kernel.org/doc/Documentation/input/event-codes.txt
> 
> > 
> > > >  
> > > > -		if (len < 0 || len % sizeof ev[0] != 0) {
> > > > -			if (len < 0 && errno != EAGAIN && errno != EINTR) {
> > > > -				libinput_remove_source(libinput,
> > > > -						       device->source);
> > > > -				device->source = NULL;
> > > > -			}
> > > > +			rc = evdev_sync_device(device);
> > 
> >                                 ^^^ this is where the actual sync happens
> > 
> > > > +			if (rc == 0)
> > > > +				rc = LIBEVDEV_READ_STATUS_SUCCESS;
> > 
> > 
> > > > +		} else if (rc == LIBEVDEV_READ_STATUS_SUCCESS)
> > > > +			evdev_device_dispatch_one(device, &ev);
> > > 
> > > nit: if any branch of an if have braces, all should.
> > 
> > amended, thanks.
> > 
> > Cheers,
> >    Peter
> > 
> > > > +	} while (rc == LIBEVDEV_READ_STATUS_SUCCESS);
> > > >  
> > > > -			return;
> > > > -		}
> > > > -
> > > > -		evdev_process_events(device, ev, len / sizeof ev[0]);
> > > > -
> > > > -	} while (len > 0);
> > > > +	if (rc != -EAGAIN && rc != -EINTR) {
> > > > +		libinput_remove_source(libinput, device->source);
> > > > +		device->source = NULL;
> > > > +	}
> > > >  }
> > > >  
> > > >  static int
> > > > -- 
> > > > 1.8.4.2
> > > 
> > > 
> > > Jonas