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.hutte...@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 <jad...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel