If an evdev client can not consume evdev events in its queue fast enough, the
evdev kernel driver will fill an SYN_DROPPED event and clear the queue
once the client's queue is full. Then the queue will be out of sync with
evdev's state. The patch tries to handle the SYN_DROPPED event by retrieving
evdev's state with specific ioctl commands in order to make the state of evdev
consistent between kernel and user-space evdev driver.
BUG=chromium-os:35291
TEST=on device; run the platform_TouchscreenSynDrop test
1. Modify the line in /sbin/xstart.sh to change the nice level from -20 to 19.
2. Restart the X server to slow it down.
restart ui
3. Execute the test script to genereate the SYN_DROPPED event and see if it passes the test.
cd /usr/local/autotest
bin/autotest tests/platform_TouchscreenSynDrop/control
4. Test if touchscreen still functions correctly.
Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
---
A x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
R x11-drivers/xf86-input-evdev/xf86-input-evdev-2.7.3-r11.ebuild
M x11-drivers/xf86-input-evdev/xf86-input-evdev-2.7.3.ebuild
3 files changed, 306 insertions(+), 0 deletions(-)
Two meta comments:
1) Some of these changes can be applied back to CMT
2) This patch seems like a general improvement to xf86-input-evdev w/out anything chrome OS specific. Please upstream! I think we will get very valuable feedback (including, perhaps, a reason why they don't do this already).
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 25: +/* SYN_DROPPED added in kernel v2.6.38-rc4 */
Let's just drop this and assume it isn't needed anymore.
With such old kernel headers this wouldn't compile anyway, since you would also need EVIOCGMTSLOTS.
Line 71: + if (ev->code == SYN_DROPPED) {
Can you push this into EvdevProcessSyncEvent(), and have that function return TRUE if SYN_DROPPED.
Line 93: +EvdevGetTime(struct timeval *current_time, BOOL use_monotonic) {
EvdevGetKernelTime maybe?
Line 108: + memset(device->key_state_bitmask, 0, len);
Why do you zero the buffer before fetching new contents?
Line 182: + EvdevGetTime(&device->before_sync_time, device->is_monotonic);
This is the generic evdev driver which handles all event types. Thus, you should try to sync all of the ones that have state.
I recommend just syncing all of the ones that have Get ioctls:
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 25: +/* SYN_DROPPED added in kernel v2.6.38-rc4 */
Done
Line 71: + if (ev->code == SYN_DROPPED) {
Done
Line 93: +EvdevGetTime(struct timeval *current_time, BOOL use_monotonic) {
Done
Line 108: + memset(device->key_state_bitmask, 0, len);
Just make sure the new state is clear if the ioctl command failed.
Line 182: + EvdevGetTime(&device->before_sync_time, device->is_monotonic);
Done. And add comments for SND and SW which the evdev driver does not handle those bits currently.
I just thought of something, though.
Syncing could take a while, and probably shouldn't be done in the signal handler.
It would take a lot more work, but what you probably want to do to get this upstreamed is define X Block/Wakeup Handlers to do the syncing in the main X thread. But, as that would delay this patch even more, let's get this signal handler patch in first!
....................................................
Commit Message
Line 9: If an evdev client can not consume evdev events in its queue fast enough, the
cannot
Line 10: evdev kernel driver will fill an SYN_DROPPED event and clear the queue
enqueue a SYN_DROPPED event
Line 11: once the client's queue is full. Then the queue will be out of sync with
The result is that the X driver will be out of sync with respect to the kernel driver state.
Line 13: evdev's state with specific ioctl commands in order to make the state of evdev
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.
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-feedback-log.patch
Line 164: }
Can you restore this blank line?
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 6: If an evdev client can not consume evdev events in its queue fast enough, the
Can you update the patch commit message to match the CL commit message?
Line 113: + int len = sizeof(device->key_state_bitmask);
Hmmm... I see what you mean about not updating the X client now.
xf86-input-evdev doesn't use key_state_bitmask to track kernel state. It just uses its own valuators, right? Take a look at where it copies the value when it detects a new button event has arrived. So syncing to your own array like this does not actually do anything.
I think what you want here is to:
1) use a temporary array for key_state_bitmask; 2) fetch new key_state_bitmask from kernel
3) run through all defined X key/button *valuators* and update their values
4) send (queue) X Button/Key events as necessary
Line 131: + if (ioctl(pInfo->fd, EVIOCGKEY(len), device->key_state_bitmask) < 0) {
EVIOCLED(), ->led_state_bitmask
Line 133: + "ioctl EVIOCGKEY failed: %s\n", strerror(errno));
LED
Line 171: + int map = pEvdev->axis_map[req->code];
range check req->code to ensure it doesn't overflow axis_map (kernel drivers are buggy sometimes).
Line 191: + if (key == ABS_MT_SLOT)
else not needed.
Line 192: + EvdevMTSlotSet(pInfo);
Add a TODO to send updated ABS state to X Client.
Line 212: + if (!EvdevMTSlotSync(pInfo, &req))
Just pass in i, and let EvdevMTslotSync own MTSlotInfo.
Line 229: + EvdevKeyStateSync(pInfo); /* sync global key state */
These comments don't add much, since your function names are so good :)
Line 321: + unsigned long key_state_bitmask[NLONGS(KEY_CNT)];
This isn't actually used by the driver; the driver stores its state in the valuators. See comment above.
....................................................
Commit Message
Line 9: If an evdev client can not consume evdev events in its queue fast enough, the
Done
Line 10: evdev kernel driver will fill an SYN_DROPPED event and clear the queue
Done
Line 11: once the client's queue is full. Then the queue will be out of sync with
Done
Line 13: evdev's state with specific ioctl commands in order to make the state of evdev
Done
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-feedback-log.patch
Line 164: }
Done
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 6: If an evdev client can not consume evdev events in its queue fast enough, the
Done
Line 113: + int len = sizeof(device->key_state_bitmask);
Done
Line 131: + if (ioctl(pInfo->fd, EVIOCGKEY(len), device->key_state_bitmask) < 0) {
Oops.. my bad... Anyway, let's remove it for now as all led events are ignored here.
Line 133: + "ioctl EVIOCGKEY failed: %s\n", strerror(errno));
Removed.
Line 171: + int map = pEvdev->axis_map[req->code];
Done
Line 191: + if (key == ABS_MT_SLOT)
Done
Line 192: + EvdevMTSlotSet(pInfo);
Done
Line 212: + if (!EvdevMTSlotSync(pInfo, &req))
Done
Line 229: + EvdevKeyStateSync(pInfo); /* sync global key state */
Done
Line 321: + unsigned long key_state_bitmask[NLONGS(KEY_CNT)];
Done
Patch Set 6: I would prefer that you didn't submit this
(8 inline comments)
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 49: -static inline void EvdevSetBit(unsigned long *array, int bit)
I'd prefer if you left this function alone, and just create a new one that did something like this:
if (val)
EvdevSetBit()
else
EvdevClrBit()
Line 141: +EvdevGenerateInputEvent(InputInfoPtr pInfo, uint16_t type, uint16_t code,
EvdevInjectSyncInputEvent()
Line 149: + ev.time = pEvdev->after_sync_time;
This is cheating a bit. Just pass in the time.
Line 153: +static int
I recommend returning:
-1 - on error
0 - no events generated
>0 - the number of events that were generated
You need to know how if any buttons changed so you know whether you need to generate an EV_SYN/SYN_REPORT.
Line 193: +EvdevMTSlotSync(InputInfoPtr pInfo, size_t key) {
Your current method syncs state one "key" at a time. But in fact, you will want to inject any state changes into X one *slot* at a time.
So, you will want to do this in two steps:
(1) read in the current ABS_MT_SLOT
(a) int curr_slot = pEvdev.curr_slot;
(b) Save EVIOCGABS(ABS_MT_SLOT) to new_curr_slot
(2) allocate enough MTSlotInfo reqs for all supported ABS_MT axes.
(3) use the ioctl to read in the current value of all of these axes.
(4) then, for each slot:
(a) for each req.code, compare the newly sync'ed value to the mapped valuator_mask value.
(b) if any value has changed, inject a kernel event
(i) Before the first value that changed, per slot, inject an extra "ABS_MT_SLOT" event to set the current slot.
int num_injected_events = 0;
for (i = min ; i < min + num_slots(pEvdev); i++) {
for (code = _ABS_MT_FIRST; code <= _ABS_MT_LAST; code++) {
int map = pEvdev->axis_map[code];
if (map == -1)
continue;
if (valuator_mask_get(pEvdev->last_mt_vals[i], map) != req[code].values[i]) {
if (i != curr_slot) {
EvdevInjectSyncEvent(EV_ABS, ABS_MT_SLOT, i);
num_injected_events += 1;
curr_slot = i;
}
EvdevInjectSyncEvent(EV_ABS, code, req[code].values[i]);
num_injected_events += 1;
}
}
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 166: + for (int i = 0, ev_count = 0; i < KEY_CNT; i++) {
don't declare a variable in the for loop.
Also, probably more traditional to just do ev_count = 0 before the for loop.
Line 171: + EvdevInjectSyncInputEvent(pInfo, EV_KEY, i, current_value);
Technically, this is probably ok, but you might want to do this:
if (!EvdevBitIsSet(pEvdev->key_bitmask, i)
continue;
orig_value = ;
curr_value = ;
if (orig_value != curr_value)
continue;
Inject()
// perhaps add a comment that Inject() will cause pEvdev->key_state_bitmask to be updated
Line 175: + EvdevInjectSyncInputEvent(pInfo, EV_SYN, SYN_REPORT, 0);
Save this for the end, after Key, Abs, MT etc.
Just return ev_count.
Line 198: +EvdevMTSlotSync(InputInfoPtr pInfo, size_t key) {
Please see comment from Patch Set 6
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 164: +static void
If you return 1 from this function, you can use it like this, everywhere:
ev_count += EvdevInjectSyncInputEvent();
Line 173: + EvdevGetKernelTime(&ev.time, pEvdev->is_monotonic);
Well, this actually causes lots of overhead with many system calls fetching the current time. It might be faster to just use the 'sync begin time' for all injected events. Just double check that this works ok for higher layers.
Line 188: + return !Success;
Does !Success == 0?
How can you distinguish between failure, and "no events sync'ed"?
Line 360: + ValuatorMask *mt_vals[num_slots(device)];
It's generally not a good idea to use stack-allocated variable length arrays, since you have no way of detected out-of-memory conditions; you just get strange stack corruption or crashes.
Line 369: + /* Get all current slots axes, then check if there is any update required */
It's probably better to fetch all of the MTSlotInfo structs first, and then parse them to see what has changed. This should save you from having to copy the valuators first:
(1) read in the current ABS_MT_SLOT
(a) int curr_slot = pEvdev.curr_slot;
(b) Save EVIOCGABS(ABS_MT_SLOT) to new_curr_slot
(2) allocate enough MTSlotInfo reqs for all supported ABS_MT axes.
(3) use the ioctl to read in the current value of all of these axes.
(4) then, for each slot:
(a) for each req.code, compare the newly sync'ed value to the mapped valuator_mask value.
(b) if any value has changed, inject a kernel event
(i) Before the first value that changed, per slot, inject an extra "ABS_MT_SLOT" event to set the current slot:
MTSlotInfo req[_ABS_MT_LAST - _ABS_MT_FIRST + 1];
int num_injected_events = 0;
int curr_slot = pEvdev.curr_slot;
int new_curr_slot;
struct input_absinfo abs_mt_slot_info;
// Save current 'current_slot'
curr_slot = pEvdev.curr_slot;
// Sync new 'current_slot'
ioctl(pInfo->fd, EVIOCGABS(key), &abs_mt_slot_info);
new_curr_slot = abs_mt_slot_info->value;
// Read in current state of all supported ABS_MT fields
for (i = _ABS_MT_FIRST; i <= _ABS_MT_LAST; i++) {
if (EvdevBitIsSet(device->abs_bitmask, i))
ioctl(pInfo->fd, EVIOCGMTSLOTS((sizeof(MTSlotInfo))), &req[i]);
}
// For each slot, sync its values
for (i = min ; i < min + num_slots(pEvdev); i++) {
for (code = _ABS_MT_FIRST; code <= _ABS_MT_LAST; code++) {
int map = pEvdev->axis_map[code];
if (map == -1)
continue;
if (valuator_mask_get(pEvdev->last_mt_vals[i], map) != req[code].values[i]) {
// If this slot is not curr_slot, inject an ABS_MT_SLOT update
if (i != curr_slot) {
EvdevInjectSyncEvent(EV_ABS, ABS_MT_SLOT, i);
num_injected_events += 1;
curr_slot = i;
}
EvdevInjectSyncEvent(EV_ABS, code, req[code].values[i]);
num_injected_events += 1;
}
}
// If curr_slot has changed, and we haven't updated it yet, inject the update now
if (curr_slot != new_curr_slot) {
EvdevInjectSyncEvent(EV_ABS, ABS_MT_SLOT, new_curr_slot);
num_injected_events += 1;
}
return num_injected_events;
Line 384: + /* Sync all ABS_ axes first */
/* (1) sync all non-MT ABS axes */
Line 387: + /* Sync ABS_MT_ axes for all slots if exists */
/* (2) sync ABS_MT_ axes, if device has any */
Line 400: + EvdevInjectSyncInputEvent(pInfo, EV_SYN, SYN_REPORT, 0);
I think you want to move this down to EvdevSyncState()
Line 411: + int ev_count = 0;
You don't use ev_count?
Line 425: +
Debug message w/ number of sync'ed events would be nice.
Patch Set 12: I would prefer that you didn't submit this
(17 inline comments)
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-add-block-reading-support.pa tch
Line 69: +
Why does this patch add a blank line?
Line 117: #endif
No need to remove this line with this patch.
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-Use-monotonic-timestamps-for -input-events-if-availab.patch
Line 9: And it corrects a previous similar patch by setting the clock source
Do you need to change this patch comment?
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-add-block-reading-support.pa tch
Line 69: +
Done
Line 117: #endif
Done
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-Use-monotonic-timestamps-for -input-events-if-availab.patch
Line 9: And it corrects a previous similar patch by setting the clock source
Reset.
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-add-block-reading-support.pa tch
Line 77: + * Finger changing (c): device->absinfo[i].valuefirst, inject finger leaving with tracking id -1,
Should these changes be in a different patch?
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 199: + if (i > ABS_MT_SLOT && i <= _ABS_MT_LAST)
nit:
>= ABS_MT_SLOT,
I think you want to let EvdevCheckAbsMtAxesChange() set device->cur_slot to the sync'ed value last, after processing all of the slots; this could potentially save us from changing the slot twice times:
Your implementation:
initial slot == old_value
set slot = sync_value
/* slot[old_value] has changes */
set slot = old_value
/* apply changes to old slot axes */
set slot = sync_value
What I am proposing (defer setting device->cur_slot to sync_value):
initial slot == old_value
/* slot[old_value] has changes */
set slot = old_value
/* apply changes to old slot axes */
set slot = sync_value
Line 214: +EvdevInjectTrackingIdChangeEvents(InputInfoPtr pInfo, int slot_index,
You use this function in two completely different ways.
(a) case b/c: cur_tid != -1
(b) case (a): cur_tid == -1
The only thing these two cases have in common is that they both use the slot-changed detection to inject MT_SLOT if needed.
This same MT_SLOT injection is also used later when injecting other axes changes.
Can you just refactor that part:
static int
EvdevInjectAbsMtAxisChangeEvent(InputInfoPtr pInfo, int slot_index, int code, int 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;
}
Line 229: + /* For finger chaning (c), inject the leaving event for original finger */
typo: finger changing
Line 253: + * d. Finger axes changing, i.e. tracking id remains -1, but axes values
"Fingers arrive and leave, i.e., tracking ID was -1, and is still -1, but some axes values have changed."
Line 256: + *
f. Same fingers, but axes change, i.e., no tracking id changes, but some axes values have changed.
Line 462: + EvdevSyncState(pInfo);
nit: Can you move this up a line, (after SLOTSTATE_EMPTY), and leave a blank line before return.
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-add-block-reading-support.pa tch
Line 77: + * Finger changing (c): device->absinfo[i].valuefirst, inject finger leaving with tracking id -1,
Done
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 199: + if (i > ABS_MT_SLOT && i <= _ABS_MT_LAST)
Done
Line 214: +EvdevInjectTrackingIdChangeEvents(InputInfoPtr pInfo, int slot_index,
Done
Line 229: + /* For finger chaning (c), inject the leaving event for original finger */
Done
Line 253: + * d. Finger axes changing, i.e. tracking id remains -1, but axes values
Done
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 116: + * TRUE if an SYN_DROPPED event is received, false otherwise.
if a SYN_DROPPED ... FALSE otherwise.
Line 222: + strerror(errno));
return 0;
Line 223: + } else if (device->cur_slot != absinfo.value) {
nit: I think it is slightly nicer to remove the else, and just exit immediately if the ioctl fails.
Line 292: + ev_count += EvdevInjectAbsMtAxisChangeEvent(pInfo,
/* For (b) and (c), set the new tid before updating axes */
Line 336: + ev_count += EvdevInjectEvent(pInfo, EV_SYN, SYN_REPORT, 0);
I don't think this is needed here. Besides, if the first slot changed, ev_count will be != 0 for all subsequent slots, even if the other slots don't change, so there will be a lot of extra SYN_REPORTS injected.
....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.pat ch
Line 116: + * TRUE if an SYN_DROPPED event is received, false otherwise.
Done
Line 222: + strerror(errno));
Done
Line 223: + } else if (device->cur_slot != absinfo.value) {
Done
Line 292: + ev_count += EvdevInjectAbsMtAxisChangeEvent(pInfo,
Done
Line 336: + ev_count += EvdevInjectEvent(pInfo, EV_SYN, SYN_REPORT, 0);
Done
