[PATCH libevdev 3/3] Drain all events before synchronizing after SYN_DROPPED
Benjamin Tissoires
benjamin.tissoires at gmail.com
Tue Apr 8 07:08:30 PDT 2014
On Mon, Apr 7, 2014 at 8:23 PM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> The kernel ring buffer drops all events on SYN_DROPPED, but then continues to
> fill up again. So by the time we read the events, the kernel's client buffer is
> essentially like this:
> SYN_DROPPED, ev1, ev2, ev3, ...., evN
>
> The kernel's device state represents the device after evN, and that is what
> the ioctls return. So we can't actually sync while there are events on the
> wire because the events represent an earlier state. So simply discard all
> events in the kernel buffer, synchronize, and then start processing again. We
> lose some granularity but at least the events are correct.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> I thought about doing something fancier here but I think it'd probably be
> wasted effort since there is always some guesswork involved.
>
> libevdev/libevdev.c | 58 ++++++++++++-------
> libevdev/libevdev.h | 45 +++++++++++++++
> test/test-libevdev-events.c | 134 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 217 insertions(+), 20 deletions(-)
>
> diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> index 7e8d3db..a316831 100644
> --- a/libevdev/libevdev.c
> +++ b/libevdev/libevdev.c
> @@ -726,32 +726,50 @@ read_more_events(struct libevdev *dev)
> return 0;
> }
>
> +static inline void
> +drain_events(struct libevdev *dev)
> +{
> + int rc;
> + size_t nelem;
> + int iterations = 0;
> + const int max_iterations = 8; /* EVDEV_BUF_PACKETS in
> + kernel/drivers/input/evedev.c */
> +
> + queue_shift_multiple(dev, queue_num_elements(dev), NULL);
> +
> + do {
> + rc = read_more_events(dev);
> + if (rc == -EAGAIN)
> + return;
> +
> + if (rc < 0) {
> + log_error("Failed to drain events before sync.\n");
> + return;
> + }
> +
> + nelem = queue_num_elements(dev);
> + queue_shift_multiple(dev, nelem, NULL);
> + } while (iterations++ < max_iterations && nelem >= queue_size(dev));
> +
> + /* Our buffer should be roughly the same or bigger than the kernel
> + buffer in most cases, so we usually don't expect to recurse. If
> + we do, make sure we stop after max_iterations and proceed with
> + what we have. This could happen if events queue up faster than
> + we can drain them.
> + */
> + if (iterations >= max_iterations)
> + log_info("Unable to drain events, buffer size mismatch.\n");
> +}
> +
> static int
> sync_state(struct libevdev *dev)
> {
> - int i;
> int rc = 0;
> struct input_event *ev;
>
> - /* FIXME: if we have events in the queue after the SYN_DROPPED (which was
> - queue[0]) we need to shift this backwards. Except that chances are that the
> - queue may be either full or too full to prepend all the events needed for
> - SYNC_IN_PROGRESS.
> -
> - so we search for the last sync event in the queue and drop everything before
> - including that event and rely on the kernel to tell us the right value for that
> - bitfield during the sync process.
> - */
> -
> - for (i = queue_num_elements(dev) - 1; i >= 0; i--) {
> - struct input_event e = {{0,0}, 0, 0, 0};
> - queue_peek(dev, i, &e);
> - if (e.type == EV_SYN)
> - break;
> - }
> -
> - if (i > 0)
> - queue_shift_multiple(dev, i + 1, NULL);
> + /* see section "Discarding events before synchronizing" in
> + * libevdev/libevdev.h */
> + drain_events(dev);
>
> if (libevdev_has_event_type(dev, EV_KEY))
> rc = sync_key_state(dev);
> diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> index d79b191..95f1e19 100644
> --- a/libevdev/libevdev.h
> +++ b/libevdev/libevdev.h
> @@ -367,6 +367,51 @@ extern "C" {
> * The axis events do not reflect the position of a current touch point, a
> * client must take care not to generate a new touch point based on those
> * updates.
> + *
> + * Discarding events before synchronizing
> + * =====================================
> + *
> + * The kernel implements the client buffer as a ring buffer. SYN_DROPPED
> + * events are handled when the buffer is full and a new event is received
> + * from a device. All existing events are discarded, a SYN_DROPPED is added
> + * to the buffer followed by the actual device event. Further events will be
> + * appended to the buffer until it is either read by the client, or filled
> + * again, at which point the sequence repeats.
> + *
> + * When the client reads the buffer, the buffer will thus always consist of
> + * exactly one SYN_DROPPED event followed by an unspecified number of real
> + * events. The data the ioctls return is the current state of the device,
> + * i.e. the state after all these events have been processed. For example,
> + * assume the buffer contains the following sequence:
> + *
> + * @code
> + EV_SYN SYN_DROPPED
> + EV_ABS ABS_X 1
> + EV_SYN SYN_REPORT 0
> + EV_ABS ABS_X 2
> + EV_SYN SYN_REPORT 0
> + EV_ABS ABS_X 3
> + EV_SYN SYN_REPORT 0
> + EV_ABS ABS_X 4
> + EV_SYN SYN_REPORT 0
> + EV_ABS ABS_X 5
> + EV_SYN SYN_REPORT 0
> + EV_ABS ABS_X 6
> + EV_SYN SYN_REPORT 0
> + * @endcode
> + * An ioctl at any time in this sequence will return a value of 6 for ABS_X.
> + *
> + * libevdev discards all events after a SYN_DROPPED to ensure the events
> + * during @ref LIBEVDEV_READ_FLAG_SYNC represent the last known state of the
> + * device. This loses some granularity of the events especially as the time
> + * between the SYN_DROPPED and the sync process increases. It does however
> + * avoid spurious cursor movements. In the above example, the event sequence
> + * by libevdev is:
> + * @code
> + EV_SYN SYN_DROPPED
> + EV_ABS ABS_X 6
> + EV_SYN SYN_REPORT 0
> + @endcode
> */
>
> /**
> diff --git a/test/test-libevdev-events.c b/test/test-libevdev-events.c
> index dc412a7..9bbae35 100644
> --- a/test/test-libevdev-events.c
> +++ b/test/test-libevdev-events.c
> @@ -927,6 +927,139 @@ START_TEST(test_syn_delta_tracking_ids)
> }
> END_TEST
>
> +START_TEST(test_syn_delta_late_sync)
> +{
> + struct uinput_device* uidev;
> + struct libevdev *dev;
> + int rc;
> + struct input_event ev;
> + struct input_absinfo abs[6];
> + int i, slot;
> +
> + memset(abs, 0, sizeof(abs));
> + abs[0].value = ABS_X;
> + abs[0].maximum = 1000;
> + abs[1].value = ABS_MT_POSITION_X;
> + abs[1].maximum = 1000;
> +
> + abs[2].value = ABS_Y;
> + abs[2].maximum = 1000;
> + abs[3].value = ABS_MT_POSITION_Y;
> + abs[3].maximum = 1000;
> +
> + abs[4].value = ABS_MT_SLOT;
> + abs[4].maximum = 1;
> +
> + abs[5].minimum = -1;
> + abs[5].maximum = 255;
> + abs[5].value = ABS_MT_TRACKING_ID;
> +
> + test_create_abs_device(&uidev, &dev,
> + 6, abs,
> + EV_SYN, SYN_REPORT,
> + -1);
> +
> + /* emulate a touch down */
> + uinput_device_event(uidev, EV_ABS, ABS_MT_SLOT, 0);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_TRACKING_ID, 1);
> + uinput_device_event(uidev, EV_ABS, ABS_X, 100);
> + uinput_device_event(uidev, EV_ABS, ABS_Y, 500);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_POSITION_X, 100);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_POSITION_Y, 500);
> + uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
> + do {
> + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> + ck_assert_int_ne(rc, LIBEVDEV_READ_STATUS_SYNC);
> + } while (rc >= 0);
> +
> + /* force enough events to trigger a SYN_DROPPED */
> + for (i = 0; i < 100; i++) {
> + uinput_device_event(uidev, EV_ABS, ABS_X, 100 + i);
> + uinput_device_event(uidev, EV_ABS, ABS_Y, 500 + i);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_POSITION_X, 100 + i);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_POSITION_Y, 500 + i);
> + uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
> + }
> +
> + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
> +
> + /* trigger the tracking ID change after getting the SYN_DROPPED */
> + uinput_device_event(uidev, EV_ABS, ABS_MT_SLOT, 0);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_TRACKING_ID, -1);
> + uinput_device_event(uidev, EV_ABS, ABS_X, 200);
> + uinput_device_event(uidev, EV_ABS, ABS_Y, 600);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_POSITION_X, 200);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_POSITION_Y, 600);
> + uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
> +
> + slot = 0;
> +
> + /* Now sync the device, expect the data to be equal to the last event*/
> + while ((rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev)) != -EAGAIN) {
> + if (ev.type == EV_SYN)
> + continue;
> +
> + ck_assert_int_eq(ev.type, EV_ABS);
> + switch(ev.code) {
> + case ABS_MT_SLOT:
> + slot = 0;
Not 100% sure, but this seems wrong.
Shouldn't we use slot = ev.value; ?
Otherwise, slot is already initialized with 0 which makes this
statement not very useful.
> + break;
> + case ABS_MT_TRACKING_ID:
> + if (slot == 0)
> + ck_assert_int_eq(ev.value, -1);
> + break;
> + case ABS_X:
> + case ABS_MT_POSITION_X:
> + ck_assert_int_eq(ev.value, 200);
> + break;
> + case ABS_Y:
> + case ABS_MT_POSITION_Y:
> + ck_assert_int_eq(ev.value, 600);
> + break;
> + }
> + }
> +
> + /* And a new tracking ID */
> + uinput_device_event(uidev, EV_ABS, ABS_MT_SLOT, 0);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_TRACKING_ID, 2);
> + uinput_device_event(uidev, EV_ABS, ABS_X, 201);
> + uinput_device_event(uidev, EV_ABS, ABS_Y, 601);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_POSITION_X, 201);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_POSITION_Y, 601);
> + uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
If I read this correctly, you check the following:
events, bla bla, drop, tracking_id = -1, events, sync, read
The previous implementation would give tracking id == -1 during sync
and then tracking id == -1 while reading the events.
But, if the reported bug is a double tracking id, I guess we should
also test the other way around:
events, blabla, tracking_id = -1, blabla, drop, few events, tracking
id = X, few events, sync, read
This should give us the other way where the tracking id is set twice
for assigning slots.
Though I am not sure it will be that easy to trigger (maybe it can be
done by adding a drop after the tracking_id = -1).
Cheers,
Benjamin
> +
> + while ((rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev)) != -EAGAIN) {
> + ck_assert_int_ne(rc, LIBEVDEV_READ_STATUS_SYNC);
> +
> + if (ev.type == EV_SYN)
> + continue;
> +
> + ck_assert_int_eq(ev.type, EV_ABS);
> +
> + switch(ev.code) {
> + case ABS_MT_SLOT:
> + ck_assert_int_eq(ev.value, 0);
> + break;
> + case ABS_MT_TRACKING_ID:
> + ck_assert_int_eq(ev.value, 2);
> + break;
> + case ABS_X:
> + case ABS_MT_POSITION_X:
> + ck_assert_int_eq(ev.value, 201);
> + break;
> + case ABS_Y:
> + case ABS_MT_POSITION_Y:
> + ck_assert_int_eq(ev.value, 601);
> + break;
> + }
> + }
> +
> + uinput_device_free(uidev);
> + libevdev_free(dev);
> +}
> +END_TEST
> +
> START_TEST(test_syn_delta_fake_mt)
> {
> struct uinput_device* uidev;
> @@ -1863,6 +1996,7 @@ libevdev_events(void)
> tcase_add_test(tc, test_syn_delta_sw);
> tcase_add_test(tc, test_syn_delta_fake_mt);
> tcase_add_test(tc, test_syn_delta_tracking_ids);
> + tcase_add_test(tc, test_syn_delta_late_sync);
> suite_add_tcase(s, tc);
>
> tc = tcase_create("skipped syncs");
> --
> 1.9.0
>
> _______________________________________________
> Input-tools mailing list
> Input-tools at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/input-tools
More information about the Input-tools
mailing list