On Wed, Jan 23, 2013 at 3:19 AM, Chung-yih Wang <cyw...@chromium.org> wrote: > If an evdev client cannot consume evdev events in its queue fast enough, the > evdev kernel driver will enqueue a SYN_DROPPED event and clear the queue > once the client's queue is full. The result is that the X driver will be out > of sync with respect to the kernel driver state. The patch tries to handle the > SYN_DROPPED event by retrieving the kernel driver's state. Retrieving this > state is inherently non-atomic, since it requires a sequence of ioctls. We use > a simple before and after time stamping approach to deal with the race > condition between partially syncing state and any potentially stale events > that > arrive during synchronization.
Hi, just a little comment in addition to Peter's. > > Signed-off-by: Chung-yih Wang <cyw...@chromium.org> > --- > src/evdev.c | 371 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > src/evdev.h | 17 +++ > 2 files changed, 381 insertions(+), 7 deletions(-) > > diff --git a/src/evdev.c b/src/evdev.c > index 6564cd0..fc8a08d 100644 > --- a/src/evdev.c > +++ b/src/evdev.c > @@ -128,6 +128,8 @@ static void EvdevSetCalibration(InputInfoPtr pInfo, int > num_calibration, int cal > static int EvdevOpenDevice(InputInfoPtr pInfo); > static void EvdevCloseDevice(InputInfoPtr pInfo); > > +static int EvdevInjectEvent(InputInfoPtr pInfo, uint16_t type, > + uint16_t code, int32_t value); > static void EvdevInitAxesLabels(EvdevPtr pEvdev, int mode, int natoms, Atom > *atoms); > static void EvdevInitOneAxisLabel(EvdevPtr pEvdev, int axis, > const char **labels, int label_idx, Atom > *atoms); > @@ -135,6 +137,20 @@ static void EvdevInitButtonLabels(EvdevPtr pEvdev, int > natoms, Atom *atoms); > static void EvdevInitProperty(DeviceIntPtr dev); > static int EvdevSetProperty(DeviceIntPtr dev, Atom atom, > XIPropertyValuePtr val, BOOL checkonly); > +static void EvdevSyncState(InputInfoPtr pInfo); > +static void EvdevGetKernelTime(struct timeval *current_time, > + BOOL use_monotonic); > +static int EvdevKeyStateSync(InputInfoPtr pInfo); > +static int EvdevAbsAxesSync(InputInfoPtr pInfo); > +static int EvdevAbsMtSlotSync(InputInfoPtr pInfo); > +static int EvdevInjectAbsMtAxisChangeEvent(InputInfoPtr pInfo, int > slot_index, > + uint16_t code, int32_t value); > +static int EvdevCheckAbsMtAxesChange(InputInfoPtr pInfo, MTSlotInfoPtr slots, > + int *count_after_synreport); > +static int EvdevGetAllSlotVals(InputInfoPtr pInfo, MTSlotInfoPtr slots); > +static int EvdevAbsMtStateSync(InputInfoPtr pInfo, int > *count_after_synreport); > +static int EvdevAbsStateSync(InputInfoPtr pInfo, int *count_after_synreport); > + > static Atom prop_product_id; > static Atom prop_invert; > static Atom prop_calibration; > @@ -208,6 +224,11 @@ static inline void EvdevSetBit(unsigned long *array, int > bit) > array[bit / LONG_BITS] |= (1LL << (bit % LONG_BITS)); > } > > +static inline void EvdevClearBit(unsigned long *array, int bit) > +{ > + array[bit / LONG_BITS] &= ~(1LL << (bit % LONG_BITS)); > +} > + > static int > EvdevGetMajorMinor(InputInfoPtr pInfo) > { > @@ -649,6 +670,11 @@ EvdevProcessButtonEvent(InputInfoPtr pInfo, struct > input_event *ev) > /* Get the signed value, earlier kernels had this as unsigned */ > value = ev->value; > > + if (ev->value) > + EvdevSetBit(pEvdev->key_state_bitmask, ev->code); > + else > + EvdevClearBit(pEvdev->key_state_bitmask, ev->code); > + > /* Handle drag lock */ > if (EvdevDragLockFilterEvent(pInfo, button, value)) > return; > @@ -779,6 +805,7 @@ EvdevProcessTouchEvent(InputInfoPtr pInfo, struct > input_event *ev) > if (pEvdev->slot_state == SLOTSTATE_EMPTY) > pEvdev->slot_state = SLOTSTATE_UPDATE; > if (ev->code == ABS_MT_TRACKING_ID) { > + pEvdev->cached_tid[slot_index] = ev->value; > if (ev->value >= 0) { > pEvdev->slot_state = SLOTSTATE_OPEN; > > @@ -993,7 +1020,7 @@ static void EvdevPostQueuedEvents(InputInfoPtr pInfo, > int num_v, int first_v, > * Take the synchronization input event and process it accordingly; the > motion > * notify events are sent first, then any button/key press/release events. > */ > -static void > +static BOOL > EvdevProcessSyncEvent(InputInfoPtr pInfo, struct input_event *ev) > { > int i; > @@ -1001,6 +1028,11 @@ EvdevProcessSyncEvent(InputInfoPtr pInfo, struct > input_event *ev) > int v[MAX_VALUATORS] = {}; > EvdevPtr pEvdev = pInfo->private; > > + if (ev->code == SYN_DROPPED) { > + xf86IDrvMsg(pInfo, X_INFO, "+++ SYN_DROPPED +++\n"); > + return TRUE; > + } > + > EvdevProcessProximityState(pInfo); > > EvdevProcessValuators(pInfo); > @@ -1028,16 +1060,20 @@ EvdevProcessSyncEvent(InputInfoPtr pInfo, struct > input_event *ev) > pEvdev->abs_queued = 0; > pEvdev->rel_queued = 0; > pEvdev->prox_queued = 0; > - > + return FALSE; > } > > /** > * Process the events from the device; nothing is actually posted to the > server > - * until an EV_SYN event is received. > + * until an EV_SYN event is received. As the SYN_DROPPED event indicates > that the > + * state of evdev driver will be out of sync with the event queue, additional > + * handling is required for processing the SYN_DROPPED event. The function > returns > + * TRUE if a SYN_DROPPED event is received, FALSE otherwise. > */ > -static void > +static BOOL > EvdevProcessEvent(InputInfoPtr pInfo, struct input_event *ev) > { > + BOOL syn_dropped = FALSE; > switch (ev->type) { > case EV_REL: > EvdevProcessRelativeMotionEvent(pInfo, ev); > @@ -1049,9 +1085,10 @@ EvdevProcessEvent(InputInfoPtr pInfo, struct > input_event *ev) > EvdevProcessKeyEvent(pInfo, ev); > break; > case EV_SYN: > - EvdevProcessSyncEvent(pInfo, ev); > + syn_dropped = EvdevProcessSyncEvent(pInfo, ev); > break; > } > + return syn_dropped; > } > > #undef ABS_X_VALUE > @@ -1082,6 +1119,308 @@ EvdevFreeMasks(EvdevPtr pEvdev) > #endif > } > > +static void > +EvdevGetKernelTime(struct timeval *current_time, BOOL use_monotonic) { > + struct timespec now; > + clockid_t clockid = (use_monotonic) ? CLOCK_MONOTONIC : CLOCK_REALTIME; > + > + clock_gettime(clockid, &now); > + current_time->tv_sec = now.tv_sec; > + current_time->tv_usec = now.tv_nsec / 1000; > +} > + > +static int > +EvdevInjectEvent(InputInfoPtr pInfo, uint16_t type, uint16_t code, > + int32_t value) { > + EvdevPtr pEvdev = pInfo->private; > + struct input_event ev; > + > + ev.type = type; > + ev.code = code; > + ev.value = value; > + EvdevGetKernelTime(&ev.time, pEvdev->is_monotonic); > + /* Inject the event by processing it */ > + EvdevProcessEvent(pInfo, &ev); > + return 1; > +} > + > +static int > +EvdevKeyStateSync(InputInfoPtr pInfo) { > + EvdevPtr pEvdev = pInfo->private; > + unsigned long key_state_bitmask[NLONGS(KEY_CNT)]; > + int i, ev_count = 0; > + int len = sizeof(key_state_bitmask); > + > + if (ioctl(pInfo->fd, EVIOCGKEY(len), key_state_bitmask) < 0) { > + xf86IDrvMsg(pInfo, X_ERROR, > + "ioctl EVIOCGKEY failed: %s\n", strerror(errno)); > + return !Success; > + } > + for (i = 0; i < KEY_CNT; i++) { > + int orig_value, current_value; > + if (!EvdevBitIsSet(pEvdev->key_bitmask, i)) > + continue; > + orig_value = EvdevBitIsSet(pEvdev->key_state_bitmask, i); > + current_value = EvdevBitIsSet(key_state_bitmask, i); > + if (current_value == orig_value) > + continue; > + ev_count += EvdevInjectEvent(pInfo, EV_KEY, i, current_value); > + } > + return ev_count; > +} > + > +static int > +EvdevAbsAxesSync(InputInfoPtr pInfo) { > + EvdevPtr device = pInfo->private; > + struct input_absinfo absinfo; > + int i, ev_count = 0; > + > + /* Sync all ABS_ axes excluding ABS_MT_ axes */ > + for (i = ABS_X; i < ABS_MAX; i++) { > + if (i >= ABS_MT_SLOT && i <= _ABS_MT_LAST) > + continue; > + if (!EvdevBitIsSet(device->abs_bitmask, i)) > + continue; > + if (ioctl(pInfo->fd, EVIOCGABS(i), &absinfo) < 0) { > + xf86IDrvMsg(pInfo, X_ERROR, "ioctl EVIOCGABS(%zu) failed: %s\n", > + i, strerror(errno)); > + } else if (absinfo.value != device->absinfo[i].value) { > + ev_count += EvdevInjectEvent(pInfo, EV_ABS, i, absinfo.value); > + } > + } > + return ev_count; > +} > + > +static int > +EvdevAbsMtSlotSync(InputInfoPtr pInfo) { > + EvdevPtr device = pInfo->private; > + struct input_absinfo absinfo; > + int ev_count = 0; > + > + if (ioctl(pInfo->fd, EVIOCGABS(ABS_MT_SLOT), &absinfo) < 0) { > + xf86IDrvMsg(pInfo, X_ERROR, "ioctl EVIOCGABS(ABS_MT_SLOT) failed: > %s\n", > + strerror(errno)); > + return 0; > + } > + if (device->cur_slot != absinfo.value) > + ev_count = EvdevInjectEvent(pInfo, EV_ABS, ABS_MT_SLOT, > absinfo.value); > + return ev_count; > +} > + > +static int > +EvdevInjectAbsMtAxisChangeEvent(InputInfoPtr pInfo, int slot_index, > + uint16_t code, int32_t value) { > + EvdevPtr device = pInfo->private; > + int ev_count = 0; > + > + if (device->cur_slot != slot_index) > + ev_count += EvdevInjectEvent(pInfo, EV_ABS, ABS_MT_SLOT, slot_index); > + ev_count += EvdevInjectEvent(pInfo, EV_ABS, code, value); > + return ev_count; > +} > + > +static int > +EvdevCheckAbsMtAxesChange(InputInfoPtr pInfo, MTSlotInfoPtr slots, > + int *count_after_synreport) > +{ > + EvdevPtr device = pInfo->private; > + int i, j, ev_count = 0; > + int total_ev_count = 0; > + > + /* > + * There will be five conditions of a slot change after SYN_DROPPED: > + * a. Finger leaving, i.e., tracking id changes from a non-negative > + * number to -1. > + * b. Finger arriving, i.e., tracking id changes from -1 to a > + * non-negative number. > + * c. Finger changing, i.e., original finger leaving and new finger > + * arriving, tracking id changes from a non-negative number to > + * another one. > + * d. Same finger, but axes change, i.e., no tracking id changes, but > some > + * axes values have changed. > + * e. Fingers arrive and leave: tracking ID was -1, and is still -1, but > + * some axes values have changed. > + * f. nothing changed > + * > + * To have X server seamless of SYN_DROPPED event, additional event > + * injections will be required except for conditions e and f: > + * > + * Finger leaving (a): all axes of the slot should be updated first, then > + * followed with tracking id change (-1). > + * > + * Finger arriving (b): new tracking id should be injected first, > followed > + * with all axes updates. > + * > + * Finger changing (c): first, inject finger leaving with tracking id -1, > + * followed with new tracking id event, then update all axes data. > + * > + * Same finger, but axes change (d): all axes updates should be injected > + * > + */ > + > + for (i = 0; i < num_slots(device); i++) { > + int curr_tid = slots[ABS_MT_TRACKING_ID - _ABS_MT_FIRST].values[i]; > + int orig_tid = device->cached_tid[i]; > + > + /* For conditions b and c, inject the tracking id change events > first */ > + if (orig_tid != curr_tid && curr_tid != -1) { > + /* For (c), inject the leaving event for original finger */ > + if (orig_tid != -1) { > + ev_count += EvdevInjectAbsMtAxisChangeEvent(pInfo, > + i, > + > ABS_MT_TRACKING_ID, > + -1); > + ev_count += EvdevInjectEvent(pInfo, EV_SYN, SYN_REPORT, 0); > + /* Reset the count_after_synreport after SYN_REPORT event */ > + total_ev_count += ev_count; > + *count_after_synreport = ev_count = 0; > + } > + /* For (b) and (c), set the new tid before updating axes */ > + ev_count += EvdevInjectAbsMtAxisChangeEvent(pInfo, > + i, > + ABS_MT_TRACKING_ID, > + curr_tid); > + } > + > + > + for (j = _ABS_MT_FIRST; j <= _ABS_MT_LAST; j++) { > + int axis = j - _ABS_MT_FIRST; > + int map, orig_value, curr_value; > + if ((j == ABS_MT_TRACKING_ID) || > + ((map = device->axis_map[j]) == -1)) > + continue; > + if (!EvdevBitIsSet(device->abs_bitmask, j)) > + continue; > + > + orig_value = valuator_mask_get(device->last_mt_vals[i], map); > + curr_value = slots[axis].values[i]; > + > + if (orig_value == curr_value) > + continue; > + > + /* For condition e, internal axes values should be updated */ > + if (orig_tid == -1 && curr_tid == -1) { > + valuator_mask_set(device->last_mt_vals[i], map, curr_value); > + continue; > + } > + > + /* In addition to condition d, all axes updates will be injected > */ > + ev_count += EvdevInjectAbsMtAxisChangeEvent(pInfo, > + i, > + j, > + curr_value); > + } > + > + /* For condition a, inject finger leaving event */ > + if (orig_tid != -1 && curr_tid == -1) { > + ev_count += EvdevInjectAbsMtAxisChangeEvent(pInfo, > + i, > + ABS_MT_TRACKING_ID, > + -1); > + } > + } > + /* Update current slot index if it is different from cur_slot value */ > + ev_count += EvdevAbsMtSlotSync(pInfo); > + *count_after_synreport += ev_count; > + > + return total_ev_count + ev_count; > +} > + > +static int > +EvdevGetAllSlotVals(InputInfoPtr pInfo, MTSlotInfoPtr slots) > +{ > + EvdevPtr device = pInfo->private; > + int i; > + > + /* Retrieve current ABS_MT_ axes for all slots */ > + for (i = _ABS_MT_FIRST; i <= _ABS_MT_LAST; i++) { > + MTSlotInfoPtr req = &slots[i - _ABS_MT_FIRST]; > + if (!EvdevBitIsSet(device->abs_bitmask, i)) > + continue; > + req->code = i; > + if (ioctl(pInfo->fd, EVIOCGMTSLOTS((sizeof(*req))), req) < 0) { > + xf86IDrvMsg(pInfo, X_ERROR, > + "ioctl EVIOCGMTSLOTS(req.code=%d) failed: %s\n", > + req->code, strerror(errno)); > + return !Success; > + } Given the current status of the multitouch handling in evdev, this is not the exact reliable thing (but there is no exact reliable thing in fact). xf86-input-evdev uses mtdev to uniform multitouch protocol A and B, and mtdev does not has a way to retrieve the current slot state. Which means that devices following protocol A won't be fixed with your patch. It's not a very bad issue, as I'm working on a cleanup of multitouch handling in evdev, and that devices following protocol A are disappearing, but I just wanted to warn you this. Anyway, as long as mtdev is not fixed with the same kind of patch, devices following protocol A will be broken in case of a EV_SYN_DROPPED event. I think we should just mention this fact with a comment. Cheers, Benjamin > + } > + > + return Success; > +} > + > +static int > +EvdevAbsMtStateSync(InputInfoPtr pInfo, int *count_after_synreport) { > + MTSlotInfo slots[_ABS_MT_CNT]; > + int ev_count = 0; > + > + /* Get all current slots axes, then check if there is any update > required */ > + if (EvdevGetAllSlotVals(pInfo, slots) == Success) { > + ev_count = EvdevCheckAbsMtAxesChange(pInfo, slots, > + count_after_synreport); > + } > + > + return ev_count; > +} > + > +static int > +EvdevAbsStateSync(InputInfoPtr pInfo, int *count_after_synreport) { > + EvdevPtr device = pInfo->private; > + int ev_count; > + > + /* Sync all ABS_ axes */ > + ev_count = EvdevAbsAxesSync(pInfo); > + *count_after_synreport += ev_count; > + > + /* Sync ABS_MT_ axes for all slots if exists */ > + if (device->num_mt_vals) > + ev_count += EvdevAbsMtStateSync(pInfo, count_after_synreport); > + > + return ev_count; > +} > + > +/** > + * Synchronize the current state with kernel evdev driver. > + */ > +static void > +EvdevSyncState(InputInfoPtr pInfo) > +{ > + int ev_count = 0; > + int ev_count_after_synreport = 0; > + EvdevPtr device = pInfo->private; > + > + EvdevGetKernelTime(&device->before_sync_time, device->is_monotonic); > + > + ev_count = EvdevKeyStateSync(pInfo); > + ev_count_after_synreport += ev_count; > + > + /* > + * TODO: sync all led, switch and sound states as well. We probably need > + * to post events out actively if the new states are different from the > + * cached ones. > + */ > + > + /* sync abs and abs_mt value/limits */ > + ev_count += EvdevAbsStateSync(pInfo, &ev_count_after_synreport); > + > + /* > + * Push SYN_REPORT event out if there is any event injected > + * during the state synchronization. > + */ > + if (ev_count_after_synreport) > + ev_count += EvdevInjectEvent(pInfo, EV_SYN, SYN_REPORT, 0); > + > + EvdevGetKernelTime(&device->after_sync_time, device->is_monotonic); > + > + xf86IDrvMsg(pInfo, X_INFO, > + "Sync_State: before %ld.%ld after %ld.%ld injected > events=%d\n", > + device->before_sync_time.tv_sec, > + device->before_sync_time.tv_usec, > + device->after_sync_time.tv_sec, > + device->after_sync_time.tv_usec, > + ev_count); > +} > + > /* just a magic number to reduce the number of reads */ > #define NUM_EVENTS 16 > > @@ -1090,6 +1429,7 @@ EvdevReadInput(InputInfoPtr pInfo) > { > struct input_event ev[NUM_EVENTS]; > int i, len = sizeof(ev); > + BOOL sync_evdev_state = FALSE; > > while (len == sizeof(ev)) > { > @@ -1120,9 +1460,23 @@ EvdevReadInput(InputInfoPtr pInfo) > break; > } > > - for (i = 0; i < len/sizeof(ev[0]); i++) > - EvdevProcessEvent(pInfo, &ev[i]); > + for (i = 0; i < len/sizeof(ev[0]); i++) { > + if (sync_evdev_state) > + break; > + if (timercmp(&ev[i].time, &pEvdev->before_sync_time, <)) { > + /* Ignore events before last sync time */ > + continue; > + } else if (timercmp(&ev[i].time, &pEvdev->after_sync_time, >)) { > + /* Event_Process returns TRUE if SYN_DROPPED detected */ > + sync_evdev_state = EvdevProcessEvent(pInfo, &ev[i]); > + } else { > + /* If the event occurred during sync, then sync again */ > + sync_evdev_state = TRUE; > + } > + } > } > + if (sync_evdev_state) > + EvdevSyncState(pInfo); > } > > static void > @@ -1308,6 +1662,7 @@ EvdevAddAbsValuatorClass(DeviceIntPtr device) > } > > for (i = 0; i < num_slots(pEvdev); i++) { > + pEvdev->cached_tid[i] = -1; > pEvdev->last_mt_vals[i] = valuator_mask_new(num_mt_axes_total); > if (!pEvdev->last_mt_vals[i]) { > xf86IDrvMsg(pInfo, X_ERROR, > @@ -1833,6 +2188,8 @@ EvdevOn(DeviceIntPtr device) > Evdev3BEmuOn(pInfo); > pEvdev->flags |= EVDEV_INITIALIZED; > device->public.on = TRUE; > + pEvdev->slot_state = SLOTSTATE_EMPTY; > + EvdevSyncState(pInfo); > > return Success; > } > diff --git a/src/evdev.h b/src/evdev.h > index 58a3fa3..277eff5 100644 > --- a/src/evdev.h > +++ b/src/evdev.h > @@ -101,6 +101,12 @@ > /* Number of longs needed to hold the given number of bits */ > #define NLONGS(x) (((x) + LONG_BITS - 1) / LONG_BITS) > > +#define _ABS_MT_FIRST ABS_MT_TOUCH_MAJOR > +#define _ABS_MT_LAST ABS_MT_DISTANCE > +#define _ABS_MT_CNT (_ABS_MT_LAST - _ABS_MT_FIRST + 1) > + > +#define MAX_SLOT_COUNT 64 > + > /* Function key mode */ > enum fkeymode { > FKEYMODE_UNKNOWN = 0, > @@ -251,8 +257,19 @@ typedef struct { > EventQueueRec queue[EVDEV_MAXQUEUE]; > > enum fkeymode fkeymode; > + > + /* Sync timestamps */ > + unsigned long key_state_bitmask[NLONGS(KEY_CNT)]; > + struct timeval before_sync_time; > + struct timeval after_sync_time; > + int32_t cached_tid[MAX_SLOT_COUNT]; > } EvdevRec, *EvdevPtr; > > +typedef struct { > + uint32_t code; > + int32_t values[MAX_SLOT_COUNT]; > +} MTSlotInfo, *MTSlotInfoPtr; > + > /* Event posting functions */ > void EvdevQueueKbdEvent(InputInfoPtr pInfo, struct input_event *ev, int > value); > void EvdevQueueButtonEvent(InputInfoPtr pInfo, int button, int value); > -- > 1.7.7.3 > > _______________________________________________ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel