[PATCH libevdev 4/6] Drop hardcoded MAX_SLOTS in favour of pre-allocated memory
Benjamin Tissoires
benjamin.tissoires at gmail.com
Wed Apr 2 16:24:31 PDT 2014
On Tue, Apr 1, 2014 at 10:17 PM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> We can't allocate in sync_mt_state since it may be called in the signal
> handler. So pre-allocate based on the device's number of slots, store that in
> the libevdev struct and use it for the sync process.
>
> This fixes a remaining bug with the handling of ABS_MT_TRACKING_ID. If a
> device had > MAX_SLOTS and a slot above that limit would start or stop during
> a SYN_DROPPED event, the slot would not be synced, and a subsequent touch in
> that slot may double-terminate or double-open a touchpoint in the client.
> For the effects of that see
>
> commit 41334b5b40cd5456f5f584b55d8888aaafa1f26e
> Author: Peter Hutterer <peter.hutterer at who-t.net>
> Date: Thu Mar 6 11:54:00 2014 +1000
>
> If the tracking ID changes during SYN_DROPPED, terminate the touch first
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> libevdev/libevdev-int.h | 15 ++++++-
> libevdev/libevdev.c | 53 +++++++++++++++-------
> libevdev/libevdev.h | 5 ---
> test/test-libevdev-events.c | 107 --------------------------------------------
> 4 files changed, 52 insertions(+), 128 deletions(-)
>
> diff --git a/libevdev/libevdev-int.h b/libevdev/libevdev-int.h
> index 5ff6026..98c75ce 100644
> --- a/libevdev/libevdev-int.h
> +++ b/libevdev/libevdev-int.h
> @@ -32,7 +32,6 @@
> #include "libevdev-util.h"
>
> #define MAX_NAME 256
> -#define MAX_SLOTS 60
> #define ABS_MT_MIN ABS_MT_SLOT
> #define ABS_MT_MAX ABS_MT_TOOL_Y
> #define ABS_MT_CNT (ABS_MT_MAX - ABS_MT_MIN + 1)
> @@ -57,6 +56,11 @@ enum SyncState {
> SYNC_IN_PROGRESS,
> };
>
> +struct mt_sync_state {
> + int code;
> + int val[];
> +};
> +
> struct libevdev {
> int fd;
> bool initialized;
> @@ -94,6 +98,15 @@ struct libevdev {
> size_t queue_nsync; /**< number of sync events */
>
> struct timeval last_event_time;
> +
> + struct {
> + struct mt_sync_state *mt_state;
> + size_t mt_state_sz; /* in bytes */
> + unsigned long *slot_update;
> + size_t slot_update_sz; /* in bytes */
> + unsigned long *tracking_id_changes;
> + size_t tracking_id_changes_sz; /* in bytes */
> + } mt_sync;
> };
>
> struct logdata {
> diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> index e2070d4..ab67002 100644
> --- a/libevdev/libevdev.c
> +++ b/libevdev/libevdev.c
> @@ -152,6 +152,9 @@ libevdev_reset(struct libevdev *dev)
> free(dev->phys);
> free(dev->uniq);
> free(dev->mt_slot_vals);
> + free(dev->mt_sync.mt_state);
> + free(dev->mt_sync.tracking_id_changes);
> + free(dev->mt_sync.slot_update);
> memset(dev, 0, sizeof(*dev));
> dev->fd = -1;
> dev->initialized = false;
> @@ -389,8 +392,27 @@ libevdev_set_fd(struct libevdev* dev, int fd)
> goto out;
> }
> dev->current_slot = abs_info.value;
> +
> + dev->mt_sync.mt_state_sz = sizeof(*dev->mt_sync.mt_state) +
> + (dev->num_slots) * sizeof(int);
> + dev->mt_sync.mt_state = calloc(1, dev->mt_sync.mt_state_sz);
> + if (!dev->mt_sync.mt_state) {
> + rc = -ENOMEM;
> + goto out;
> + }
ok, just to try to say something:
"this test could go into the later if, so you will keep 3 lines of code".
Wonderful isn't it?
I can't say much more on this one, plus the stats makes me love it:
52 insertions(+), 128 deletions(-) \o/
Cheers,
Benjamin
> +
> + dev->mt_sync.tracking_id_changes_sz = NLONGS(dev->num_slots) * sizeof(long);
> + dev->mt_sync.tracking_id_changes = malloc(dev->mt_sync.tracking_id_changes_sz);
> +
> + dev->mt_sync.slot_update_sz = NLONGS(dev->num_slots * ABS_MT_CNT) * sizeof(long);
> + dev->mt_sync.slot_update = malloc(dev->mt_sync.slot_update_sz);
> +
> + if (!dev->mt_sync.tracking_id_changes ||
> + !dev->mt_sync.slot_update) {
> + rc = -ENOMEM;
> + goto out;
> + }
> }
> -
> }
> }
>
> @@ -556,14 +578,15 @@ sync_mt_state(struct libevdev *dev, int create_events)
> int axis, slot;
> int ioctl_success = 0;
> int last_reported_slot = 0;
> - struct mt_state {
> - int code;
> - int val[MAX_SLOTS];
> - } mt_state;
> - unsigned long slot_update[NLONGS(MAX_SLOTS * ABS_MT_CNT)] = {0};
> - unsigned long tracking_id_changes[NLONGS(MAX_SLOTS)] = {0};
> + struct mt_sync_state *mt_state = dev->mt_sync.mt_state;
> + unsigned long *slot_update = dev->mt_sync.slot_update;
> + unsigned long *tracking_id_changes = dev->mt_sync.tracking_id_changes;
> int need_tracking_id_changes = 0;
>
> + memset(dev->mt_sync.slot_update, 0, dev->mt_sync.slot_update_sz);
> + memset(dev->mt_sync.tracking_id_changes, 0,
> + dev->mt_sync.tracking_id_changes_sz);
> +
> #define AXISBIT(_slot, _axis) (_slot * ABS_MT_CNT + _axis - ABS_MT_MIN)
>
> for (axis = ABS_MT_MIN; axis <= ABS_MT_MAX; axis++) {
> @@ -573,8 +596,8 @@ sync_mt_state(struct libevdev *dev, int create_events)
> if (!libevdev_has_event_code(dev, EV_ABS, axis))
> continue;
>
> - mt_state.code = axis;
> - rc = ioctl(dev->fd, EVIOCGMTSLOTS(sizeof(struct mt_state)), &mt_state);
> + mt_state->code = axis;
> + rc = ioctl(dev->fd, EVIOCGMTSLOTS(dev->mt_sync.mt_state_sz), mt_state);
> if (rc < 0) {
> /* if the first ioctl fails with -EINVAL, chances are the kernel
> doesn't support the ioctl. Simply continue */
> @@ -586,19 +609,19 @@ sync_mt_state(struct libevdev *dev, int create_events)
> if (ioctl_success == 0)
> ioctl_success = 1;
>
> - for (slot = 0; slot < min(dev->num_slots, MAX_SLOTS); slot++) {
> + for (slot = 0; slot < dev->num_slots; slot++) {
>
> - if (*slot_value(dev, slot, axis) == mt_state.val[slot])
> + if (*slot_value(dev, slot, axis) == mt_state->val[slot])
> continue;
>
> if (axis == ABS_MT_TRACKING_ID &&
> *slot_value(dev, slot, axis) != -1 &&
> - mt_state.val[slot] != -1) {
> + mt_state->val[slot] != -1) {
> set_bit(tracking_id_changes, slot);
> need_tracking_id_changes = 1;
> }
>
> - *slot_value(dev, slot, axis) = mt_state.val[slot];
> + *slot_value(dev, slot, axis) = mt_state->val[slot];
>
> set_bit(slot_update, AXISBIT(slot, axis));
> /* note that this slot has updates */
> @@ -615,7 +638,7 @@ sync_mt_state(struct libevdev *dev, int create_events)
> }
>
> if (need_tracking_id_changes) {
> - for (slot = 0; slot < min(dev->num_slots, MAX_SLOTS); slot++) {
> + for (slot = 0; slot < dev->num_slots; slot++) {
> if (!bit_is_set(tracking_id_changes, slot))
> continue;
>
> @@ -631,7 +654,7 @@ sync_mt_state(struct libevdev *dev, int create_events)
> init_event(dev, ev, EV_SYN, SYN_REPORT, 0);
> }
>
> - for (slot = 0; slot < min(dev->num_slots, MAX_SLOTS); slot++) {
> + for (slot = 0; slot < dev->num_slots; slot++) {
> if (!bit_is_set(slot_update, AXISBIT(slot, ABS_MT_SLOT)))
> continue;
>
> diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> index 77ee08a..aa7b90d 100644
> --- a/libevdev/libevdev.h
> +++ b/libevdev/libevdev.h
> @@ -914,11 +914,6 @@ enum libevdev_read_status {
> * have been synced. For more details on what libevdev does to sync after a
> * SYN_DROPPED event, see @ref syn_dropped.
> *
> - * @note The implementation of libevdev limits the maximum number of slots
> - * that can be synched. If your device exceeds the number of slots
> - * (currently 60), slot indices equal and above this maximum are ignored and
> - * their value will not update until the next event in that slot.
> - *
> * If a device needs to be synced by the caller but the caller does not call
> * with the @ref LIBEVDEV_READ_FLAG_SYNC flag set, all events from the diff are
> * dropped after libevdev updates its internal state and event processing
> diff --git a/test/test-libevdev-events.c b/test/test-libevdev-events.c
> index 412db71..fda7617 100644
> --- a/test/test-libevdev-events.c
> +++ b/test/test-libevdev-events.c
> @@ -30,8 +30,6 @@
>
> #include "test-common.h"
>
> -#define MAX_SLOTS 60 /* as in libevdev-int.h */
> -
> START_TEST(test_next_event)
> {
> struct uinput_device* uidev;
> @@ -662,110 +660,6 @@ START_TEST(test_syn_delta_mt_reset_slot)
> }
> END_TEST
>
> -START_TEST(test_syn_delta_mt_too_many)
> -{
> - struct uinput_device* uidev;
> - struct libevdev *dev;
> - int rc;
> - struct input_event ev;
> - struct input_absinfo abs[6];
> - int i;
> - int num_slots = MAX_SLOTS + 20;
> -
> - 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 = num_slots;
> - abs[5].value = ABS_MT_TOOL_Y;
> - abs[5].maximum = 500;
> -
> - rc = test_create_abs_device(&uidev, &dev,
> - 6, abs,
> - EV_SYN, SYN_REPORT,
> - -1);
> - ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> -
> - for (i = num_slots; i >= 0; i--) {
> - uinput_device_event(uidev, EV_ABS, ABS_MT_SLOT, 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_ABS, ABS_MT_TOOL_Y, 1 + i);
> - uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
> - }
> -
> - /* drain the fd, so libevdev_next_event doesn't pick up any events
> - before the FORCE_SYNC */
> - do {
> - rc = read(libevdev_get_fd(dev), &ev, sizeof(ev));
> - } while (rc > 0);
> -
> - rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_FORCE_SYNC, &ev);
> - ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
> -
> - i = 0;
> - while (i < num_slots) {
> - int slot;
> -
> - rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev);
> - ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
> -
> - if (libevdev_event_is_code(&ev, EV_SYN, SYN_REPORT))
> - break;
> -
> - if (libevdev_event_is_code(&ev, EV_ABS, ABS_X) ||
> - libevdev_event_is_code(&ev, EV_ABS, ABS_Y))
> - continue;
> -
> - ck_assert_int_eq(ev.type, EV_ABS);
> - ck_assert_int_eq(ev.code, ABS_MT_SLOT);
> - slot = ev.value;
> - ck_assert_int_lt(slot, MAX_SLOTS);
> -
> - rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev);
> -
> - /* last MT_SLOT event is on its own */
> - if (libevdev_event_is_code(&ev, EV_SYN, SYN_REPORT))
> - break;
> -
> - ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
> - ck_assert_int_eq(ev.type, EV_ABS);
> - ck_assert_int_eq(ev.code, ABS_MT_POSITION_X);
> - ck_assert_int_eq(ev.value, 100 + slot);
> - rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev);
> - ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
> - ck_assert_int_eq(ev.type, EV_ABS);
> - ck_assert_int_eq(ev.code, ABS_MT_POSITION_Y);
> - ck_assert_int_eq(ev.value, 500 + slot);
> - rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev);
> - ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SYNC);
> - ck_assert_int_eq(ev.type, EV_ABS);
> - ck_assert_int_eq(ev.code, ABS_MT_TOOL_Y);
> - ck_assert_int_eq(ev.value, 1 + slot);
> -
> - i++;
> - }
> -
> - /* we expect eactly MAX_SLOTS to be synced */
> - ck_assert_int_eq(i, MAX_SLOTS);
> - rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev);
> - ck_assert_int_eq(rc, -EAGAIN);
> -
> - uinput_device_free(uidev);
> - libevdev_free(dev);
> -}
> -END_TEST
> -
> START_TEST(test_syn_delta_led)
> {
> struct uinput_device* uidev;
> @@ -1806,7 +1700,6 @@ libevdev_events(void)
> tcase_add_test(tc, test_syn_delta_button);
> tcase_add_test(tc, test_syn_delta_abs);
> tcase_add_test(tc, test_syn_delta_mt);
> - tcase_add_test(tc, test_syn_delta_mt_too_many);
> tcase_add_test(tc, test_syn_delta_mt_reset_slot);
> tcase_add_test(tc, test_syn_delta_led);
> tcase_add_test(tc, test_syn_delta_sw);
> --
> 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