[PATCH 1/2] Handle SYN_DROPPED, filter keys with a bitmap
David Herrmann
dh.herrmann at gmail.com
Fri Jun 21 02:44:17 PDT 2013
Hi
On Tue, Jun 4, 2013 at 8:47 PM, Martin Minarik
<minarik11 at student.fiit.stuba.sk> wrote:
> Since evdev keys are unreliable, they might randomly get dropped, such
> as, on SYN_DROPPED. Even SYN_DROPPED is sometimes not delivered.
Ugh, if events are dropped but SYN_DROPPED is missing, it's a kernel
bug. Please report it to linux-input at vger.kernel.org if you see it. We
shouldn't assume this broken behavior in user-space!
There is also a fix pending on linux-input at vger which fixes duplicate
press/release events on EVIOCGKEY. So lets fix such bugs upstream
instead of introducing hacks.
Nevertheless, SYN_DROPPED handling is needed, indeed.
> Clients, compositor are not able to recover from duplicate press/release.
> This fixes this bug, thereby making the compositor and clients useable
> even under critical system conditions such as, but not limited to, high
> system load, device malfunction. This improves reliability.
>
> Every device contains a key bitmap. Each passing key press/release
> is checked and recorded. Duplicated presses, releases are handled by
> emitting the opposite type event in between (the time is faked -1 time unit).
> This is a last resort solution and happens only very rarely.
>
> When a SYN_DROPPED is detected:
> 1 wait until after the next SYN_REPORT.
Why wait for SYN_REPORT? Re-reading system state should be a "silent"
operation. But see below
> 2 Ask the kernel for the actual state of buttons
> 3 Compare the buttons with the internal key bitmap, to see what changed.
> 4 Send each different key via notify to the compositor, with timestamp.
> 5 Update the internal key bitmap to match the kernel's
>
> Disadvantages of this approach
> Each evdev device consumes 96 bytes more memory.
> Table lookup+bit toggle for almost key.
> Don't know the timestamp of missing keys.
> The key_notify from kernel bitmap compare occurs later the real missing event.
> That's a timestamp issue.
>
> ---
> src/evdev-touchpad.c | 51 ++++++++++++++-
> src/evdev.c | 176 ++++++++++++++++++++++++++++++++++++++------------
> src/evdev.h | 35 +++++++---
> 3 files changed, 208 insertions(+), 54 deletions(-)
>
> diff --git a/src/evdev-touchpad.c b/src/evdev-touchpad.c
> index a21ae0b..890ddad 100644
> --- a/src/evdev-touchpad.c
> +++ b/src/evdev-touchpad.c
> @@ -533,6 +533,26 @@ on_release(struct touchpad_dispatch *touchpad)
> push_fsm_event(touchpad, FSM_EVENT_RELEASE);
> }
>
> +struct evdev_dispatch_interface touchpad_interface;
> +struct evdev_dispatch_interface touchpad_syn_drop_interface;
> +
> +static inline void
> +process_syn(struct touchpad_dispatch *touchpad,
> + struct evdev_device *device,
> + struct input_event *e, uint32_t time)
> +{
> + switch (e->code) {
> + case SYN_DROPPED:
> + if (device->dispatch->interface == &touchpad_interface)
> + device->dispatch->interface = &touchpad_syn_drop_interface;
> + break;
> + case SYN_REPORT:
> + default:
> + touchpad->event_mask |= TOUCHPAD_EVENT_REPORT;
> + break;
> + }
> +}
> +
> static inline void
> process_absolute(struct touchpad_dispatch *touchpad,
> struct evdev_device *device,
> @@ -588,7 +608,7 @@ process_key(struct touchpad_dispatch *touchpad,
> case BTN_FORWARD:
> case BTN_BACK:
> case BTN_TASK:
> - notify_button(device->seat,
> + notify_button(device->seat,
> time, e->code,
> e->value ? WL_POINTER_BUTTON_STATE_PRESSED :
> WL_POINTER_BUTTON_STATE_RELEASED);
> @@ -634,8 +654,7 @@ touchpad_process(struct evdev_dispatch *dispatch,
>
> switch (e->type) {
> case EV_SYN:
> - if (e->code == SYN_REPORT)
> - touchpad->event_mask |= TOUCHPAD_EVENT_REPORT;
> + process_syn(touchpad, device, e, time);
> break;
> case EV_ABS:
> process_absolute(touchpad, device, e);
> @@ -664,6 +683,32 @@ struct evdev_dispatch_interface touchpad_interface = {
> touchpad_destroy
> };
>
> +static void
> +touchpad_syn_drop_process(struct evdev_dispatch *dispatch,
> + struct evdev_device *device,
> + struct input_event *event,
> + uint32_t time)
> +{
> + if ((event->code != EV_SYN) || (event->code != SYN_REPORT))
> + return;
> +
> + if (device->dispatch->interface == &touchpad_syn_drop_interface)
> + device->dispatch->interface = &touchpad_interface;
> +
> + evdev_keys_state_sync(device, time);
> +}
> +
> +static void
> +touchpad_syn_drop_destroy(struct evdev_dispatch *dispatch)
> +{
> + free(dispatch);
> +}
> +
> +struct evdev_dispatch_interface touchpad_syn_drop_interface = {
> + touchpad_syn_drop_process,
> + touchpad_syn_drop_destroy
> +};
> +
> static int
> touchpad_init(struct touchpad_dispatch *touchpad,
> struct evdev_device *device)
> diff --git a/src/evdev.c b/src/evdev.c
> index 122a2d9..8058c8f 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -62,13 +62,29 @@ evdev_led_update(struct evdev_device *device, enum weston_led leds)
> (void)i; /* no, we really don't care about the return value */
> }
>
> +struct evdev_dispatch_interface fallback_interface;
> +struct evdev_dispatch_interface syn_drop_interface;
> +
> static inline void
> -evdev_process_key(struct evdev_device *device, struct input_event *e, int time)
> +evdev_process_syn(struct evdev_device *device, struct input_event *e)
> {
> - if (e->value == 2)
> - return;
> -
> switch (e->code) {
> + case SYN_DROPPED:
> + if (device->dispatch->interface == &fallback_interface)
> + device->dispatch->interface = &syn_drop_interface;
> + break;
> + case SYN_REPORT:
> + default:
> + device->pending_events |= EVDEV_SYN;
> + break;
> + }
> +}
> +
> +static void
> +evdev_process_key(struct evdev_device *device,
> + unsigned int time, unsigned int code, int value)
> +{
> + switch (code) {
> case BTN_LEFT:
> case BTN_RIGHT:
> case BTN_MIDDLE:
> @@ -78,21 +94,56 @@ evdev_process_key(struct evdev_device *device, struct input_event *e, int time)
> case BTN_BACK:
> case BTN_TASK:
> notify_button(device->seat,
> - time, e->code,
> - e->value ? WL_POINTER_BUTTON_STATE_PRESSED :
> + time, code,
> + value ? WL_POINTER_BUTTON_STATE_PRESSED :
> WL_POINTER_BUTTON_STATE_RELEASED);
> break;
>
> default:
> notify_key(device->seat,
> - time, e->code,
> - e->value ? WL_KEYBOARD_KEY_STATE_PRESSED :
> + time, code,
> + value ? WL_KEYBOARD_KEY_STATE_PRESSED :
> WL_KEYBOARD_KEY_STATE_RELEASED,
> STATE_UPDATE_AUTOMATIC);
> break;
> }
> }
>
> +int
> +evdev_keys_state_sync(struct evdev_device *d, unsigned int time)
> +{
> + unsigned long kernel_keys[NBITS(KEY_CNT)];
> + unsigned int i, j;
> +
> + if (d->deliv_state.keys == NULL)
> + return -1;
> +
> + memset(kernel_keys, 0, sizeof(kernel_keys));
> +
> + if (ioctl(d->fd, EVIOCGKEY(KEY_CNT), kernel_keys) < 0)
> + return -1;
> +
> + for (i = 0; i < ARRAY_LENGTH(kernel_keys); i++) {
> + long changed_keys = kernel_keys[i] ^ d->deliv_state.keys[i];
> +
> + if (changed_keys == 0)
> + continue;
> +
> + d->deliv_state.keys[i] = kernel_keys[i];
> +
> + for (j = 0; j < BITS_PER_LONG; j++) {
> + if (changed_keys & BIT(j)) {
> + int key = BITS_PER_LONG * i + j;
> + int val = TEST_BIT(kernel_keys, key);
> +
> + evdev_process_key(d, time, key, val);
You cannot do that. Assume the user pressed "A", then presses "ctrl"
but the compositor didn't read any of them but instead got a
SYN_DROPPED. Now it re-reads keyboard state via EVIOCGKEY and it turns
out the code for "ctrl" is lower than "a" (let's assume that). If the
compositor or a client has a shortcut on this key, it will get
triggered.
However, the user clearly pressed "A" _first_. So this shouldn't
trigger any shortcut.
So the correct thing to do is to update keyboard state but let XKB
know that it should do that silently.
Obviously, this only applies to keys. For ABS or REL events, this is
irrelevant (well, at least for trivial drivers).
Cheers
David
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> static void
> evdev_process_touch(struct evdev_device *device, struct input_event *e)
> {
> @@ -307,10 +358,19 @@ fallback_process(struct evdev_dispatch *dispatch,
> evdev_process_absolute(device, event);
> break;
> case EV_KEY:
> - evdev_process_key(device, event, time);
> + if (event->value == 2)
> + break;
> +
> + if (event->value == !TEST_BIT(device->deliv_state.keys, event->code)) {
> + device->deliv_state.keys[LONG(event->code)] ^= BIT(event->code);
> + } else {
> + evdev_process_key(device, time - 1, event->code, !event->value);
> + }
> +
> + evdev_process_key(device, time, event->code, event->value);
> break;
> case EV_SYN:
> - device->pending_events |= EVDEV_SYN;
> + evdev_process_syn(device, event);
> break;
> }
> }
> @@ -326,6 +386,32 @@ struct evdev_dispatch_interface fallback_interface = {
> fallback_destroy
> };
>
> +static void
> +syn_drop_process(struct evdev_dispatch *dispatch,
> + struct evdev_device *device,
> + struct input_event *event,
> + uint32_t time)
> +{
> + if ((event->code != EV_SYN) || (event->code != SYN_REPORT))
> + return;
> +
> + if (device->dispatch->interface == &syn_drop_interface)
> + device->dispatch->interface = &fallback_interface;
> +
> + evdev_keys_state_sync(device, time);
> +}
> +
> +static void
> +syn_drop_destroy(struct evdev_dispatch *dispatch)
> +{
> + free(dispatch);
> +}
> +
> +struct evdev_dispatch_interface syn_drop_interface = {
> + syn_drop_process,
> + syn_drop_destroy
> +};
> +
> static struct evdev_dispatch *
> fallback_dispatch_create(void)
> {
> @@ -590,11 +676,26 @@ err1:
> }
>
> void
> +evdev_keys_down_release(struct evdev_device *device)
> +{
> + int i;
> + for (i = 0; i < KEY_CNT; i++) {
> + if (TEST_BIT(device->deliv_state.keys, i)) {
> + notify_key(device->seat,
> + 0xFFFF, i,
> + WL_KEYBOARD_KEY_STATE_RELEASED,
> + STATE_UPDATE_AUTOMATIC);
> + }
> + }
> +}
> +
> +void
> evdev_device_destroy(struct evdev_device *device)
> {
> - struct evdev_dispatch *dispatch;
> + struct evdev_dispatch *dispatch = device->dispatch;
> +
> + evdev_keys_down_release(device);
>
> - dispatch = device->dispatch;
> if (dispatch)
> dispatch->interface->destroy(dispatch);
>
> @@ -608,45 +709,38 @@ evdev_device_destroy(struct evdev_device *device)
> free(device);
> }
>
> +inline void
> +evdev_keys_down_array(struct wl_list *evdev_devices, struct wl_array *keys)
> +{
> + struct evdev_device *device;
> + uint32_t i;
> + uint32_t *k;
> +
> + for (i = 0; i < KEY_CNT; i++) {
> + wl_list_for_each(device, evdev_devices, link) {
> + if (TEST_BIT(device->deliv_state.keys, i)) {
> + k = wl_array_add(keys, sizeof *k);
> + *k = i;
> + break;
> + /* XXX:Perhaps deliver duplicate keys too */
> + }
> + }
> + }
> +}
> +
> void
> evdev_notify_keyboard_focus(struct weston_seat *seat,
> struct wl_list *evdev_devices)
> {
> - struct evdev_device *device;
> struct wl_array keys;
> - unsigned int i, set;
> - char evdev_keys[(KEY_CNT + 7) / 8];
> - char all_keys[(KEY_CNT + 7) / 8];
> - uint32_t *k;
> - int ret;
> -
> - if (!seat->keyboard)
> - return;
> + struct evdev_device *device;
>
> - memset(all_keys, 0, sizeof all_keys);
> wl_list_for_each(device, evdev_devices, link) {
> - memset(evdev_keys, 0, sizeof evdev_keys);
> - ret = ioctl(device->fd,
> - EVIOCGKEY(sizeof evdev_keys), evdev_keys);
> - if (ret < 0) {
> - weston_log("failed to get keys for device %s\n",
> - device->devnode);
> - continue;
> - }
> - for (i = 0; i < ARRAY_LENGTH(evdev_keys); i++)
> - all_keys[i] |= evdev_keys[i];
> + evdev_keys_state_sync(device, 0);
> }
>
> wl_array_init(&keys);
> - for (i = 0; i < KEY_CNT; i++) {
> - set = all_keys[i >> 3] & (1 << (i & 7));
> - if (set) {
> - k = wl_array_add(&keys, sizeof *k);
> - *k = i;
> - }
> - }
> -
> + evdev_keys_down_array(evdev_devices, &keys);
> notify_keyboard_focus_in(seat, &keys, STATE_UPDATE_AUTOMATIC);
> -
> wl_array_release(&keys);
> }
> diff --git a/src/evdev.h b/src/evdev.h
> index eb5c868..e6a8e4c 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -26,6 +26,16 @@
> #include <linux/input.h>
> #include <wayland-util.h>
>
> +/* copied from udev/extras/input_id/input_id.c */
> +/* we must use this kernel-compatible implementation */
> +#define BITS_PER_LONG (sizeof(unsigned long) * 8)
> +#define NBITS(x) ((((x)-1)/BITS_PER_LONG)+1)
> +#define OFF(x) ((x)%BITS_PER_LONG)
> +#define BIT(x) (1UL<<OFF(x))
> +#define LONG(x) ((x)/BITS_PER_LONG)
> +#define TEST_BIT(array, bit) ((array[LONG(bit)] >> OFF(bit)) & 1)
> +/* end copied */
> +
> #define MAX_SLOTS 16
>
> enum evdev_event_type {
> @@ -45,6 +55,10 @@ enum evdev_device_capability {
> EVDEV_TOUCH = (1 << 4),
> };
>
> +struct evdev_bitmap {
> + unsigned long keys[NBITS(KEY_CNT)];
> +};
> +
> struct evdev_device {
> struct weston_seat *seat;
> struct wl_list link;
> @@ -77,17 +91,9 @@ struct evdev_device {
> enum evdev_device_capability caps;
>
> int is_mt;
> -};
>
> -/* copied from udev/extras/input_id/input_id.c */
> -/* we must use this kernel-compatible implementation */
> -#define BITS_PER_LONG (sizeof(unsigned long) * 8)
> -#define NBITS(x) ((((x)-1)/BITS_PER_LONG)+1)
> -#define OFF(x) ((x)%BITS_PER_LONG)
> -#define BIT(x) (1UL<<OFF(x))
> -#define LONG(x) ((x)/BITS_PER_LONG)
> -#define TEST_BIT(array, bit) ((array[LONG(bit)] >> OFF(bit)) & 1)
> -/* end copied */
> + struct evdev_bitmap deliv_state;
> +};
>
> #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1)
>
> @@ -121,6 +127,15 @@ void
> evdev_device_destroy(struct evdev_device *device);
>
> void
> +evdev_keys_down_release(struct evdev_device *device);
> +
> +void
> +evdev_keys_down_array(struct wl_list *evdev_devices,
> + struct wl_array *keys);
> +int
> +evdev_keys_state_sync(struct evdev_device *d, unsigned int time);
> +
> +void
> evdev_notify_keyboard_focus(struct weston_seat *seat,
> struct wl_list *evdev_devices);
>
> --
> 1.7.10.4
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list