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

[Jonas Ådahl]

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 (

> > > +				struct input_event e;
> > > +				mtdev_get_event(device->mtdev, &e);
> > > +				evdev_process_event(device, &e);
> > > +			}
> > > +		}
> > >  	}
> > >  }
> > >  
> > > +static int
> > > +evdev_sync_device(struct evdev_device *device)
> > > +{
> > > +	struct input_event ev;
> > > +	int rc;
> > > +
> > > +	do {
> > > +		rc = libevdev_next_event(device->evdev,
> > > +					 LIBEVDEV_READ_FLAG_SYNC, &ev);
> > > +		if (rc < 0)
> > > +			break;
> > > +		evdev_device_dispatch_one(device, &ev);
> > > +	} while (rc == LIBEVDEV_READ_STATUS_SYNC);
> > > +
> > > +	return rc == -EAGAIN ? 0 : rc;
> > > +}
> > > +
> > >  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.

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

[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