x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling [chromiumos/overlays/chromiumos-overlay : master]

[Chung-yih Wang (Code Review)]

Chung-yih Wang has uploaded a new change for review.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling

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.

exec nice -n 19 /usr/bin/X -noreset -nolisten tcp vt01 -auth $1 2> /dev/null

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.patch
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(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/overlays/chromiumos-overlay refs/changes/87/37987/1
--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Andrew de los Reyes (Code Review)]

Andrew de los Reyes has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 2: Looks good to me, approved

This works fine w/ the MS mouse now. Thanks for the fix!

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

Gerrit-Reviewer: Andrew de los Reyes <ad...@chromium.org>
Gerrit-Reviewer: Chung-yih Wang <cyw...@chromium.org>
Gerrit-Reviewer: Daniel Kurtz <djk...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>

[Chung-yih Wang (Code Review)]

Chung-yih Wang has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 2: Verified; Looks good to me, approved; Ready

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 2: Do not submit

(7 inline comments)

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.patch
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:

EV_KEY: EVIOCGKEY(len)
EV_LED: EVIOCGLED(len)
EV_SND: EVIOCGSND(len)
EV_SW: EVIOCGSW(len)
EV_ABS: EVIOCGABS(abs)
EV_ABS/ABS_MT: EVIOCGMTSLOTS(len)

Line 187: +
This is just a special case of the EV_ABS axes and would better if moved to EvdevProbeAbsState() which syncs all abs axes.

Line 297: + unsigned code;
This is wrong.

unsigned and int must both be 32-bits, which isn't guaranteed when using unsigned and int. This only works when compiling for 32-bit.

Use something like
uint32_t code;
int32_t values[MAX_SLOT_COUNT];

// alternatively, you could declare it like this:
typedef struct {
uint32_t code;
int32_t values[];
};

// And create it like this
MTSlotInfoPtr slotInfo = malloc(sizeof(uint32_t) + num_slots * sizeof(int32_t));

// And access the result like this:

for (i = 0; i < num_slots; i++)
printf("slot[%u] = %d\n", slotInfo.code[i], slotInfo.values[i]);

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Chung-yih Wang (Code Review)]

Chung-yih Wang has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 2: (7 inline comments)

Thanks for the great comments!

....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.patch
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.

Line 187: +
Done

Line 297: + unsigned code;

Done

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 3: (15 inline comments)

Getting better, thanks!

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.patch
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.

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Chung-yih Wang (Code Review)]

Chung-yih Wang has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 3: (15 inline comments)

....................................................
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.patch
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

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

Gerrit-Reviewer: Andrew de los Reyes <ad...@chromium.org>
Gerrit-Reviewer: Chung-yih Wang <cyw...@chromium.org>
Gerrit-Reviewer: Daniel Kurtz <djk...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>

Gerrit-Reviewer: Yufeng Shen <mil...@chromium.org>

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

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.patch
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;
}
}

if (curr_slot != new_curr_slot) {
EvdevInjectSyncEvent(EV_ABS, ABS_MT_SLOT, new_curr_slot);
num_injected_events += 1;
}

return num_injected_events;

Line 226: + struct input_absinfo* absinfo;
Use a temporary input_absinfo

Line 236: + /* TODO: send ABS states out to clients */
(1) Use EvdevInjectSyncInputEvent() for all ABS_ events up to ABS_MT_SLOT.

Line 363: +static inline int TestBit(int bit, unsigned long* array)
Is this EvdevBitIsSet() ?

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 8: (4 inline comments)

....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.patch
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

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 8

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 10: (10 inline comments)

....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.patch
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.

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 10

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Yufeng Shen (Code Review)]

Yufeng Shen has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 12:

Ping.

We have found a real repro here

https://code.google.com/p/chromium-os/issues/detail?id=37595

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 12

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

Gerrit-Reviewer: Andrew de los Reyes <ad...@chromium.org>

Gerrit-Reviewer: ChromeBot <chrom...@google.com>

Gerrit-Reviewer: Chung-yih Wang <cyw...@chromium.org>
Gerrit-Reviewer: Daniel Kurtz <djk...@chromium.org>

Gerrit-Reviewer: Yufeng Shen <mil...@chromium.org>

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

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.patch
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.3-Add-SYN_DROPPED-handling.patch
Line 54: + array[bit / LONG_BITS] &= ~(1LL << (bit % LONG_BITS));
indent looks funny.


Line 207: + if (i > ABS_MT_SLOT && i < _ABS_MT_LAST)
i >= ABS_MT_SLOT && i <= LAST?


Line 230: + ev_count += EvdevInjectEvent(pInfo, EV_ABS, ABS_MT_SLOT, slot_index);
Only if device->cur_slot != slot_index


Line 239: + ABS_MT_TRACKING_ID, curr_tid);
...
/* When tracking ID changes, first remove old finger */
if (curr_tid != -1 && orig_tid != -1) {
ev_count += EvdevInjectEvent(pInfo, EV_ABS, ABS_MT_TRACKING_ID, -1);
ev_count += EvdevInjectEvent(pInfo, EV_SYN, SYN_REPORT, 0);
}
ev_count += EvdevInjectEvent(pInfo, EV_ABS, ABS_MT_TRACKING_ID, curr_tid);


Line 245: +EvdevCheckAbsMtAxesChange(InputInfoPtr pInfo, MTSlotInfo *slots)
MTSlotInfoPtr, here and elsewhere?


Line 254: + continue;
No! We still must update the state of the other axes!

Imagine this scenario:

Finger moves slowly from 100,100 -> 200, 200, then lifts.

SYN_DROPPED happens after receiving 150,150
We sync and find tid is now -1, but keep slots[axis].values[{x,y}] is 150,150.

Then a new finger arrives at 100, 200 -> but kernel only sends us:
tid: 400
x: 100

Because the kernel thought it already sent us y:200

So, we must still update the values for all axes, even for slots with sync'ed curr_tid == -1.

This could be one of your test cases...


Line 255: + for (j = _ABS_MT_FIRST; j < _ABS_MT_LAST; j++) {
j <= _ABS_MT_LAST


Line 263: + (j != ABS_MT_TRACKING_ID))
j always != ABS_MT_TRACKING_ID here.

Maybe just || these "continue" conditions together.


Line 285: + for (i = _ABS_MT_FIRST; i < _ABS_MT_LAST; i++) {
<= _ABS_MT_LAST


Line 304: + MTSlotInfo slots[_ABS_MT_LAST - _ABS_MT_FIRST + 1];
_ABS_MT_CNT


Line 327: + ev_count += EvdevAbsAxesSync(pInfo);
? Won't this sync all Abs Axes again?


Line 340: + EvdevPtr origin = pInfo->private;
usual you call your EvdevPtr "device", probably use device here to be consistent.


Line 367: + origin->after_sync_time.tv_usec);
Print ev_count, too.


Line 448: + int32_t cached_tid[64];
MAX_SLOT_COUNT?

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 12

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Chung-yih Wang (Code Review)]

Chung-yih Wang has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 12: (17 inline comments)

....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-add-block-reading-support.patch
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.3-Add-SYN_DROPPED-handling.patch
Line 54: + array[bit / LONG_BITS] &= ~(1LL << (bit % LONG_BITS));

Done

Line 207: + if (i > ABS_MT_SLOT && i < _ABS_MT_LAST)

We need to update the ABS_MT_SLOT here as it is not a per-slot axes.

Line 230: + ev_count += EvdevInjectEvent(pInfo, EV_ABS, ABS_MT_SLOT, slot_index);

Done

Line 239: + ABS_MT_TRACKING_ID, curr_tid);

Have listed all conditions of slot state changes and processing.

Line 245: +EvdevCheckAbsMtAxesChange(InputInfoPtr pInfo, MTSlotInfo *slots)

Done


Line 254: + continue;
Yes, I have listed all conditions, please review again.

Line 255: + for (j = _ABS_MT_FIRST; j < _ABS_MT_LAST; j++) {

Done

Line 263: + (j != ABS_MT_TRACKING_ID))

Done

Line 285: + for (i = _ABS_MT_FIRST; i < _ABS_MT_LAST; i++) {

Done

Line 304: + MTSlotInfo slots[_ABS_MT_LAST - _ABS_MT_FIRST + 1];

Done

Line 327: + ev_count += EvdevAbsAxesSync(pInfo);

Done. Change the order of the ABS_ and ABS_MT syncs.

Line 340: + EvdevPtr origin = pInfo->private;

Done

Line 367: + origin->after_sync_time.tv_usec);

Done

Line 448: + int32_t cached_tid[64];

Done

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 12

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 13: (7 inline comments)

....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-add-block-reading-support.patch
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.patch
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.

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 13

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Chung-yih Wang (Code Review)]

Chung-yih Wang has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 13: (7 inline comments)

....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.0-add-block-reading-support.patch
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.patch
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


Line 256: + *
Done


Line 462: + EvdevSyncState(pInfo);
Done

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 13

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 15: (5 inline comments)

....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.patch
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.

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 15

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Chung-yih Wang (Code Review)]

Chung-yih Wang has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 15: (5 inline comments)

....................................................
File x11-drivers/xf86-input-evdev/files/evdev-2.7.3-Add-SYN_DROPPED-handling.patch
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

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 15

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Daniel Kurtz (Code Review)]

Daniel Kurtz has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 18: Looks good to me, approved

Great!

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 18

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>

[Chung-yih Wang (Code Review)]

Chung-yih Wang has posted comments on this change.

Change subject: x11-drivers/xf86-input-evdev: Add SYN_DROPPED handling
......................................................................

Patch Set 18: Ready; Verified

Thanks Daniel for great reviews!

--
To view, visit https://gerrit.chromium.org/gerrit/37987
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6aa3ad9a3708563181553198b0264052e0da9dd
Gerrit-PatchSet: 18

Gerrit-Project: chromiumos/overlays/chromiumos-overlay
Gerrit-Branch: master
Gerrit-Owner: Chung-yih Wang <cyw...@chromium.org>