Bug 67563

Was this fixed by commit 92e83929f0485e739ecd57d8b04e176259b296d1 ?

No, this bug still exist on the tip of today's master branch

Created attachment 84292 [details]
evtest log

The following is output from evtest on my 10 point touchscreen

Not sure if it's related, but I can trigger weston to stop processing touch events with a single touch device, too.  It's harder to trigger, but by rapidly tapping the screen in random places, Weston eventually gets into a state where it stops sending touch events to the clients.

After a little debugging, I found that input.c::notify_touch() gets invoked with a value of WL_TOUCH_UP by evdev.c::evdev_process_key() twice in a row when seat->num_tp==1 prior to the first WL_TOUCH_UP.  This results in seat->num_tp getting decremented too many times, hence halting client touch notifications.  In other words, the number of invocations of notify_touch(WL_TOUCH_DOWN) and notify_touch(WL_TOUCH_UP) are not symmetric.  Beyond this, I'm not sure what triggers evdev_process_key() to handle two BTN_TOUCH 0 events in a row... since evtest BTN_TOUCH 1 and BTN_TOUCH 0 events appear to be symmetric.

I think this bug is caused by the patch in bug 51909. As you can see from the last section of Rusty's evtest log, it is getting multiple touch up events in-between SYN events. After that patch the touch up events are only flushed once the SYN event is received. However the evdev_device struct only keeps track of a single pending EVDEV_ABSOLUTE_MT_UP event so the earlier touch up event gets overridden by the second one and so it never gets emitted.

I can reliably replicate the bug by removing my fingers at the same time. If I revert the patch from that bug it seems to fix the problem.

Maybe we could fix this bug without breaking bug 51909 by setting the pending sync flag whenever we get a slot event as well?

Created attachment 86180 [details] [review]
evdev: Flush motion events when the slot changes, not just after sync

Here is a patch to make the change mentioned above.

It does seem to fix the basic problem but I can still get it into a confused state if I rapidly hit and release the screen with four fingers. I wonder if this is a separate problem and is related to what Artie describes.

(In reply to comment #6)
> Created attachment 86180 [details] [review] [review]
> evdev: Flush motion events when the slot changes, not just after sync
> 
> Here is a patch to make the change mentioned above.
> 
> It does seem to fix the basic problem but I can still get it into a confused
> state if I rapidly hit and release the screen with four fingers. I wonder if
> this is a separate problem and is related to what Artie describes.

Good catch.  This must have blocked a lot of motion events too, since we only report motion events on SYN too and only the most recently received slot then.

I think this should fix this bug (ie what Rusty reports), but I think it's different from Arties problem in comment 4.  Arties case is where we have a old-fashioned single-touch touchscreen that only reports BTN_TOUCH for touch up/down and ABS_X/Y for position.  Patch applied, but we have to keep looking for the single touch problem.

Experimenting with the touch events a bit more, I think I've found three separate problems:

1- I was a bit too hasty with my patch and it doesn't work quite right. When it gets the sync event it immediately changes the slot number and then sets the pending event. Then when it comes to flush the events it will use the new slot number to flush the old pending event so the events can have the wrong finger.

2- If you get more than 32 events in one read then it resets the pending events before processing the next batch in evdev_process_events. If I press all four fingers down at once then it ends up with more than 32 events and the sync message is in the second batch. The pending flag for the last finger gets cleared so it is never emitted. I think this is what I was seeing when bashing the screen with four fingers as described in comment #6.

3- If you release a finger and press it down again quickly you can get get the up and down events in the same batch. However the pending events are always processed in the order down then up so it will end up notifying two down events and then an up. This could be what Artie is seeing with a single finger, although I think that would only make sense if Artie's device is reporting the multi-touch events and not the ABS_X/Y events.

I think it would be a good idea to rethink the batching mechanism a bit so that it really only batches the motion events and anything else should cause a flush. Perhaps the the pending motion events should be queued per-slot as well rather than trying to have a single flag. I will experiment with a patch.

Created attachment 86455 [details] [review]
evdev: Only track one pending event

This patch seems to fix the three problems for me.

Artie, it would be great if you could confirm that your touch device is only reporting the ABS_X/Y events and not also the ABS_MT_* events. If that's the case then there is still another bug which I'm not able to reproduce so far.

(In reply to comment #9)
> Created attachment 86455 [details] [review] [review]
> evdev: Only track one pending event
> 
> This patch seems to fix the three problems for me.
> 
> Artie, it would be great if you could confirm that your touch device is only
> reporting the ABS_X/Y events and not also the ABS_MT_* events. If that's the
> case then there is still another bug which I'm not able to reproduce so far.

Neil, my touch device does not report MT events.  I'll open a new bug for the single touch bug (since it doesn't appear related to this MT issue) and attach my evtest output to it.

(In reply to comment #10)
> Neil, my touch device does not report MT events.  I'll open a new bug for
> the single touch bug (since it doesn't appear related to this MT issue) and
> attach my evtest output to it.

(In reply to comment #4)
> Not sure if it's related, but I can trigger weston to stop processing touch
> events with a single touch device, too.  It's harder to trigger, but by
> rapidly tapping the screen in random places, Weston eventually gets into a
> state where it stops sending touch events to the clients.
> 
> After a little debugging, I found that input.c::notify_touch() gets invoked
> with a value of WL_TOUCH_UP by evdev.c::evdev_process_key() twice in a row
> when seat->num_tp==1 prior to the first WL_TOUCH_UP.  This results in
> seat->num_tp getting decremented too many times, hence halting client touch
> notifications.  In other words, the number of invocations of
> notify_touch(WL_TOUCH_DOWN) and notify_touch(WL_TOUCH_UP) are not symmetric.
> Beyond this, I'm not sure what triggers evdev_process_key() to handle two
> BTN_TOUCH 0 events in a row... since evtest BTN_TOUCH 1 and BTN_TOUCH 0
> events appear to be symmetric.

The single touch issue is now being tracked in Bug 69759

Yeah, I agree with the patch, it cleans up the event reporting nicely.  There's no benefit to batching event delivery into weston core, beyond making sure we get both X and Y coordinates for touch down events or relative motion etc.  Inside weston we eventually write all outgoing events into the clients protocol buffer, so there's really no downside to calling notify_* for each event.

Patch applied:

commit daf7d4774beb5959d69eb6e84c0602872746d5a5
Author: Neil Roberts <neil@linux.intel.com>
Date:   Tue Sep 24 12:09:03 2013 +0100

    evdev: Only track one pending event

    ...

Thanks!