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

[Peter Hutterer]

On Tue, Feb 25, 2014 at 08:17:24AM +1000, Peter Hutterer wrote:
> > > > >  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).

I double-checked this and it's a bit different than I wrote above.
the kernel input code submits this as a single frame but we may still get a
buffer overflow and thus SYN_DROPPED (at random times) in evdev when we're
not reading fast enough. However, this is exactly the kind of problem that
libevdev is there for, so I'll add the handling there so that what comes out
of libevdev is consistent.

Cheers,
   Peter

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