[PATCH] evdev: flush ABS_* events during EVIOCGABS

[PATCH] evdev: flush ABS_* events during EVIOCGABS

[David Herrmann]

We currently flush input events in the outgoing client queue on most
EVIOCG* ioctls so userspace doesn't get events twice (once during the
ioctl and once during a later read()).

We introduced this in:
  commit 483180281f0ac60d1138710eb21f4b9961901294
  Author: David Herrmann <dh.herrmann@gmail.com>
  Date:   Sun Apr 7 21:13:19 2013 -0700

      Input: evdev - flush queues during EVIOCGKEY-like ioctls

However, we didn't modify the EVIOCGABS handler as we considered ABS
values to change fast enough that flushing the queue seems irrelevant. But
as it turns out, the ABS SLOT events suffer from the same issues. Hence,
also flush the input queue from ABS values on EVIOCGABS.

Reported-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Hi

This is untested as I don't have any multitouch hardware right now. Peter, can
you give this a try?

Thanks
David

 drivers/input/evdev.c | 63 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 18 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index a06e125..fc55c19 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -55,8 +55,11 @@ struct evdev_client {
 	struct input_event buffer[];
 };
 
-/* flush queued events of type @type, caller must hold client->buffer_lock */
-static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
+/* Flush queued events of given type @type and code @code. A negative code
+ * is interpreted as catch-all. Caller must hold client->buffer_lock. */
+static void __evdev_flush_queue(struct evdev_client *client,
+				unsigned int type,
+				int code)
 {
 	unsigned int i, head, num;
 	unsigned int mask = client->bufsize - 1;
@@ -75,7 +78,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 		ev = &client->buffer[i];
 		is_report = ev->type == EV_SYN && ev->code == SYN_REPORT;
 
-		if (ev->type == type) {
+		if (ev->type == type && (code < 0 || ev->code == code)) {
 			/* drop matched entry */
 			continue;
 		} else if (is_report && !num) {
@@ -774,7 +777,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
 
 	spin_unlock(&dev->event_lock);
 
-	__evdev_flush_queue(client, type);
+	__evdev_flush_queue(client, type, -1);
 
 	spin_unlock_irq(&client->buffer_lock);
 
@@ -787,6 +790,40 @@ static int evdev_handle_get_val(struct evdev_client *client,
 	return ret;
 }
 
+static int evdev_handle_get_abs(struct evdev_client *client,
+				struct input_dev *dev,
+				unsigned int code,
+				unsigned int size,
+				void __user *p)
+{
+	struct input_absinfo abs;
+	int ret;
+
+	if (!dev->absinfo)
+		return -EINVAL;
+
+	/* see evdev_handle_get_val() for locking order */
+
+	spin_lock_irq(&dev->event_lock);
+	spin_lock(&client->buffer_lock);
+
+	abs = dev->absinfo[code];
+
+	spin_unlock(&dev->event_lock);
+
+	__evdev_flush_queue(client, EV_ABS, code);
+
+	spin_unlock_irq(&client->buffer_lock);
+
+	ret = copy_to_user(p, &abs, min_t(size_t,
+					  size,
+					  sizeof(struct input_absinfo)));
+	if (ret < 0)
+		evdev_queue_syn_dropped(client);
+
+	return ret;
+}
+
 static int evdev_handle_mt_request(struct input_dev *dev,
 				   unsigned int size,
 				   int __user *ip)
@@ -972,20 +1009,10 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 						_IOC_NR(cmd) & EV_MAX, size,
 						p, compat_mode);
 
-		if ((_IOC_NR(cmd) & ~ABS_MAX) == _IOC_NR(EVIOCGABS(0))) {
-
-			if (!dev->absinfo)
-				return -EINVAL;
-
-			t = _IOC_NR(cmd) & ABS_MAX;
-			abs = dev->absinfo[t];
-
-			if (copy_to_user(p, &abs, min_t(size_t,
-					size, sizeof(struct input_absinfo))))
-				return -EFAULT;
-
-			return 0;
-		}
+		if ((_IOC_NR(cmd) & ~ABS_MAX) == _IOC_NR(EVIOCGABS(0)))
+			return evdev_handle_get_abs(client, dev,
+						    _IOC_NR(cmd) & ABS_MAX,
+						    size, p);
 	}
 
 	if (_IOC_DIR(cmd) == _IOC_WRITE) {
-- 
1.9.1

Re: [PATCH] evdev: flush ABS_* events during EVIOCGABS

[Peter Hutterer]

On Thu, Apr 10, 2014 at 09:09:04PM +0200, David Herrmann wrote:
> We currently flush input events in the outgoing client queue on most
> EVIOCG* ioctls so userspace doesn't get events twice (once during the
> ioctl and once during a later read()).
> 
> We introduced this in:
>   commit 483180281f0ac60d1138710eb21f4b9961901294
>   Author: David Herrmann <dh.herrmann@gmail.com>
>   Date:   Sun Apr 7 21:13:19 2013 -0700
> 
>       Input: evdev - flush queues during EVIOCGKEY-like ioctls
> 
> However, we didn't modify the EVIOCGABS handler as we considered ABS
> values to change fast enough that flushing the queue seems irrelevant. But
> as it turns out, the ABS SLOT events suffer from the same issues. Hence,
> also flush the input queue from ABS values on EVIOCGABS.
> 
> Reported-by: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> Hi
> 
> This is untested as I don't have any multitouch hardware right now. Peter, can
> you give this a try?

How are you planning to handle the slot-based events? We'd either need to
add something similar (but more complex) to evdev_handle_mt_request or rely
on the caller to call the whole EV_ABS range and ditch anything ABS_MT_.
I'd prefer the former, the latter is yet more behaviour that's easy to get
wrong.

Cheers,
   Peter

> 
> Thanks
> David
> 
>  drivers/input/evdev.c | 63 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index a06e125..fc55c19 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -55,8 +55,11 @@ struct evdev_client {
>  	struct input_event buffer[];
>  };
>  
> -/* flush queued events of type @type, caller must hold client->buffer_lock */
> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
> +/* Flush queued events of given type @type and code @code. A negative code
> + * is interpreted as catch-all. Caller must hold client->buffer_lock. */
> +static void __evdev_flush_queue(struct evdev_client *client,
> +				unsigned int type,
> +				int code)
>  {
>  	unsigned int i, head, num;
>  	unsigned int mask = client->bufsize - 1;
> @@ -75,7 +78,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>  		ev = &client->buffer[i];
>  		is_report = ev->type == EV_SYN && ev->code == SYN_REPORT;
>  
> -		if (ev->type == type) {
> +		if (ev->type == type && (code < 0 || ev->code == code)) {
>  			/* drop matched entry */
>  			continue;
>  		} else if (is_report && !num) {
> @@ -774,7 +777,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>  
>  	spin_unlock(&dev->event_lock);
>  
> -	__evdev_flush_queue(client, type);
> +	__evdev_flush_queue(client, type, -1);
>  
>  	spin_unlock_irq(&client->buffer_lock);
>  
> @@ -787,6 +790,40 @@ static int evdev_handle_get_val(struct evdev_client *client,
>  	return ret;
>  }
>  
> +static int evdev_handle_get_abs(struct evdev_client *client,
> +				struct input_dev *dev,
> +				unsigned int code,
> +				unsigned int size,
> +				void __user *p)
> +{
> +	struct input_absinfo abs;
> +	int ret;
> +
> +	if (!dev->absinfo)
> +		return -EINVAL;
> +
> +	/* see evdev_handle_get_val() for locking order */
> +
> +	spin_lock_irq(&dev->event_lock);
> +	spin_lock(&client->buffer_lock);
> +
> +	abs = dev->absinfo[code];
> +
> +	spin_unlock(&dev->event_lock);
> +
> +	__evdev_flush_queue(client, EV_ABS, code);
> +
> +	spin_unlock_irq(&client->buffer_lock);
> +
> +	ret = copy_to_user(p, &abs, min_t(size_t,
> +					  size,
> +					  sizeof(struct input_absinfo)));
> +	if (ret < 0)
> +		evdev_queue_syn_dropped(client);
> +
> +	return ret;
> +}
> +
>  static int evdev_handle_mt_request(struct input_dev *dev,
>  				   unsigned int size,
>  				   int __user *ip)
> @@ -972,20 +1009,10 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  						_IOC_NR(cmd) & EV_MAX, size,
>  						p, compat_mode);
>  
> -		if ((_IOC_NR(cmd) & ~ABS_MAX) == _IOC_NR(EVIOCGABS(0))) {
> -
> -			if (!dev->absinfo)
> -				return -EINVAL;
> -
> -			t = _IOC_NR(cmd) & ABS_MAX;
> -			abs = dev->absinfo[t];
> -
> -			if (copy_to_user(p, &abs, min_t(size_t,
> -					size, sizeof(struct input_absinfo))))
> -				return -EFAULT;
> -
> -			return 0;
> -		}
> +		if ((_IOC_NR(cmd) & ~ABS_MAX) == _IOC_NR(EVIOCGABS(0)))
> +			return evdev_handle_get_abs(client, dev,
> +						    _IOC_NR(cmd) & ABS_MAX,
> +						    size, p);
>  	}
>  
>  	if (_IOC_DIR(cmd) == _IOC_WRITE) {
> -- 
> 1.9.1
> 

Re: [PATCH] evdev: flush ABS_* events during EVIOCGABS

[David Herrmann]

Hi Peter

On Tue, Apr 22, 2014 at 6:15 AM, Peter Hutterer
<peter.hutterer@who-t.net> wrote:
> How are you planning to handle the slot-based events? We'd either need to
> add something similar (but more complex) to evdev_handle_mt_request or rely
> on the caller to call the whole EV_ABS range and ditch anything ABS_MT_.
> I'd prefer the former, the latter is yet more behaviour that's easy to get
> wrong.

This is all racy..

We _really_ need an ioctl to receive _all_ ABS information atomically.
I mean, there's no way we can know the user's state from the kernel.
Even if the user resyncs via EVIOCGMTSLOTS, we can never flush the
whole ABS queue. Problem is, the user has to call the ioctl for _each_
available MT code and events might get queued in between. So yeah,
this patch doesn't help much..

I have no better idea than adding a new EVIOCGABS call that retrieves
ABS values for all slots atomically (and for all other axes..). No
idea how to properly fix the old ioctls.

Thanks
David

Re: [PATCH] evdev: flush ABS_* events during EVIOCGABS

[Peter Hutterer]

On Tue, Apr 22, 2014 at 08:21:54AM +0200, David Herrmann wrote:
> Hi Peter
> 
> On Tue, Apr 22, 2014 at 6:15 AM, Peter Hutterer
> <peter.hutterer@who-t.net> wrote:
> > How are you planning to handle the slot-based events? We'd either need to
> > add something similar (but more complex) to evdev_handle_mt_request or rely
> > on the caller to call the whole EV_ABS range and ditch anything ABS_MT_.
> > I'd prefer the former, the latter is yet more behaviour that's easy to get
> > wrong.
> 
> This is all racy..
> 
> We _really_ need an ioctl to receive _all_ ABS information atomically.
> I mean, there's no way we can know the user's state from the kernel.
> Even if the user resyncs via EVIOCGMTSLOTS, we can never flush the
> whole ABS queue. Problem is, the user has to call the ioctl for _each_
> available MT code and events might get queued in between. So yeah,
> this patch doesn't help much..
> 
> I have no better idea than adding a new EVIOCGABS call that retrieves
> ABS values for all slots atomically (and for all other axes..). No
> idea how to properly fix the old ioctls.

bonus points for making that ioctl fetch the state of the last SYN_DROPPED
and leave the events since in the client buffer. That way we can smooth over
SYN_DROPPED and lose less information.

Cheers,
   Peter

Re: [PATCH] evdev: flush ABS_* events during EVIOCGABS

[Peter Hutterer]

On Wed, Apr 23, 2014 at 10:21:03AM +1000, Peter Hutterer wrote:
> On Tue, Apr 22, 2014 at 08:21:54AM +0200, David Herrmann wrote:
> > Hi Peter
> > 
> > On Tue, Apr 22, 2014 at 6:15 AM, Peter Hutterer
> > <peter.hutterer@who-t.net> wrote:
> > > How are you planning to handle the slot-based events? We'd either need to
> > > add something similar (but more complex) to evdev_handle_mt_request or rely
> > > on the caller to call the whole EV_ABS range and ditch anything ABS_MT_.
> > > I'd prefer the former, the latter is yet more behaviour that's easy to get
> > > wrong.
> > 
> > This is all racy..
> > 
> > We _really_ need an ioctl to receive _all_ ABS information atomically.
> > I mean, there's no way we can know the user's state from the kernel.
> > Even if the user resyncs via EVIOCGMTSLOTS, we can never flush the
> > whole ABS queue. Problem is, the user has to call the ioctl for _each_
> > available MT code and events might get queued in between. So yeah,
> > this patch doesn't help much..
> > 
> > I have no better idea than adding a new EVIOCGABS call that retrieves
> > ABS values for all slots atomically (and for all other axes..). No
> > idea how to properly fix the old ioctls.
> 
> bonus points for making that ioctl fetch the state of the last SYN_DROPPED
> and leave the events since in the client buffer. That way we can smooth over
> SYN_DROPPED and lose less information.

to expand on this, something like the below would work from userspace:
 
1. userspace opens fd, EVIOCGBIT for everything
2. userspace calls EVIOCGABSATOMIC
3. kernel empties the event queue, flags the client as capable
4. kernel copies current device state into client-specific struct
5. kernel replies with that device state to the ioctl
6. client reads events 
..
7. kernel sees a SYN_DROPPED for this client. Takes a snapshot of the device
for the client, empties the buffer, leaves SYN_DROPPED in the buffer
(current behaviour)
8. client reads SYN_DROPPED, calls EVIOCGABSATOMIC
9. kernel replies with the snapshot state, leaves the event buffer otherwise
unmodified
10. client reads all events after SYN_DROPPED, gets a smooth continuation
11. goto 6

if the buffer overflows multiple times, repeat 7 so that the snapshot state
is always the last SYN_DROPPED state. well, technically the state should be
the state of the device at the first SYN_REPORT after the last SYN_DROPPED,
since the current API says that interrupted event is incomplete.

there are two oddities here:
1. the first ioctl will have to flush the buffer to guarantee consistent state,
though you could even avoid that by taking a snapshot of the device on
open(). though that comes with a disadvantage, you don't know if the client
supports the new approach so you're wasting effort and memory here.
2. I'm not quite sure how to handle multiple repeated calls short of
updating the client-specific snapshot with every event as it is read
successfully.

any comments?

Cheers,
   Peter

Re: [PATCH] evdev: flush ABS_* events during EVIOCGABS

[Dmitry Torokhov]

On Wed, Apr 23, 2014 at 03:38:49PM +1000, Peter Hutterer wrote:
> On Wed, Apr 23, 2014 at 10:21:03AM +1000, Peter Hutterer wrote:
> > On Tue, Apr 22, 2014 at 08:21:54AM +0200, David Herrmann wrote:
> > > Hi Peter
> > > 
> > > On Tue, Apr 22, 2014 at 6:15 AM, Peter Hutterer
> > > <peter.hutterer@who-t.net> wrote:
> > > > How are you planning to handle the slot-based events? We'd either need to
> > > > add something similar (but more complex) to evdev_handle_mt_request or rely
> > > > on the caller to call the whole EV_ABS range and ditch anything ABS_MT_.
> > > > I'd prefer the former, the latter is yet more behaviour that's easy to get
> > > > wrong.
> > > 
> > > This is all racy..
> > > 
> > > We _really_ need an ioctl to receive _all_ ABS information atomically.
> > > I mean, there's no way we can know the user's state from the kernel.
> > > Even if the user resyncs via EVIOCGMTSLOTS, we can never flush the
> > > whole ABS queue. Problem is, the user has to call the ioctl for _each_
> > > available MT code and events might get queued in between. So yeah,
> > > this patch doesn't help much..
> > > 
> > > I have no better idea than adding a new EVIOCGABS call that retrieves
> > > ABS values for all slots atomically (and for all other axes..). No
> > > idea how to properly fix the old ioctls.
> > 
> > bonus points for making that ioctl fetch the state of the last SYN_DROPPED
> > and leave the events since in the client buffer. That way we can smooth over
> > SYN_DROPPED and lose less information.
> 
> to expand on this, something like the below would work from userspace:
>  
> 1. userspace opens fd, EVIOCGBIT for everything
> 2. userspace calls EVIOCGABSATOMIC
> 3. kernel empties the event queue, flags the client as capable
> 4. kernel copies current device state into client-specific struct
> 5. kernel replies with that device state to the ioctl
> 6. client reads events 
> ..
> 7. kernel sees a SYN_DROPPED for this client. Takes a snapshot of the device
> for the client, empties the buffer, leaves SYN_DROPPED in the buffer
> (current behaviour)
> 8. client reads SYN_DROPPED, calls EVIOCGABSATOMIC
> 9. kernel replies with the snapshot state, leaves the event buffer otherwise
> unmodified
> 10. client reads all events after SYN_DROPPED, gets a smooth continuation
> 11. goto 6
> 
> if the buffer overflows multiple times, repeat 7 so that the snapshot state
> is always the last SYN_DROPPED state. well, technically the state should be
> the state of the device at the first SYN_REPORT after the last SYN_DROPPED,
> since the current API says that interrupted event is incomplete.
> 
> there are two oddities here:
> 1. the first ioctl will have to flush the buffer to guarantee consistent state,
> though you could even avoid that by taking a snapshot of the device on
> open(). though that comes with a disadvantage, you don't know if the client
> supports the new approach so you're wasting effort and memory here.
> 2. I'm not quite sure how to handle multiple repeated calls short of
> updating the client-specific snapshot with every event as it is read
> successfully.
> 
> any comments?

Do we really need to optimize the case when we are dropping events?

Thanks.

-- 
Dmitry

Re: [PATCH] evdev: flush ABS_* events during EVIOCGABS

[Peter Hutterer]

On Tue, Apr 22, 2014 at 10:46:47PM -0700, Dmitry Torokhov wrote:
> On Wed, Apr 23, 2014 at 03:38:49PM +1000, Peter Hutterer wrote:
> > On Wed, Apr 23, 2014 at 10:21:03AM +1000, Peter Hutterer wrote:
> > > On Tue, Apr 22, 2014 at 08:21:54AM +0200, David Herrmann wrote:
> > > > Hi Peter
> > > > 
> > > > On Tue, Apr 22, 2014 at 6:15 AM, Peter Hutterer
> > > > <peter.hutterer@who-t.net> wrote:
> > > > > How are you planning to handle the slot-based events? We'd either need to
> > > > > add something similar (but more complex) to evdev_handle_mt_request or rely
> > > > > on the caller to call the whole EV_ABS range and ditch anything ABS_MT_.
> > > > > I'd prefer the former, the latter is yet more behaviour that's easy to get
> > > > > wrong.
> > > > 
> > > > This is all racy..
> > > > 
> > > > We _really_ need an ioctl to receive _all_ ABS information atomically.
> > > > I mean, there's no way we can know the user's state from the kernel.
> > > > Even if the user resyncs via EVIOCGMTSLOTS, we can never flush the
> > > > whole ABS queue. Problem is, the user has to call the ioctl for _each_
> > > > available MT code and events might get queued in between. So yeah,
> > > > this patch doesn't help much..
> > > > 
> > > > I have no better idea than adding a new EVIOCGABS call that retrieves
> > > > ABS values for all slots atomically (and for all other axes..). No
> > > > idea how to properly fix the old ioctls.
> > > 
> > > bonus points for making that ioctl fetch the state of the last SYN_DROPPED
> > > and leave the events since in the client buffer. That way we can smooth over
> > > SYN_DROPPED and lose less information.
> > 
> > to expand on this, something like the below would work from userspace:
> >  
> > 1. userspace opens fd, EVIOCGBIT for everything
> > 2. userspace calls EVIOCGABSATOMIC
> > 3. kernel empties the event queue, flags the client as capable
> > 4. kernel copies current device state into client-specific struct
> > 5. kernel replies with that device state to the ioctl
> > 6. client reads events 
> > ..
> > 7. kernel sees a SYN_DROPPED for this client. Takes a snapshot of the device
> > for the client, empties the buffer, leaves SYN_DROPPED in the buffer
> > (current behaviour)
> > 8. client reads SYN_DROPPED, calls EVIOCGABSATOMIC
> > 9. kernel replies with the snapshot state, leaves the event buffer otherwise
> > unmodified
> > 10. client reads all events after SYN_DROPPED, gets a smooth continuation
> > 11. goto 6
> > 
> > if the buffer overflows multiple times, repeat 7 so that the snapshot state
> > is always the last SYN_DROPPED state. well, technically the state should be
> > the state of the device at the first SYN_REPORT after the last SYN_DROPPED,
> > since the current API says that interrupted event is incomplete.
> > 
> > there are two oddities here:
> > 1. the first ioctl will have to flush the buffer to guarantee consistent state,
> > though you could even avoid that by taking a snapshot of the device on
> > open(). though that comes with a disadvantage, you don't know if the client
> > supports the new approach so you're wasting effort and memory here.
> > 2. I'm not quite sure how to handle multiple repeated calls short of
> > updating the client-specific snapshot with every event as it is read
> > successfully.
> > 
> > any comments?
> 
> Do we really need to optimize the case when we are dropping events?

It happens frequently, to the point where on some laptops you're pretty much
guaranteed to get SYN_DROPPED events on resume and sometimes even during
normal multi-finger user.

I don't have any measurements on how many events are dropped on average.
Could be one or two, could be several buffer sizes, I honestly don't know.

Cheers,
   Peter

Re: [PATCH] evdev: flush ABS_* events during EVIOCGABS

[Dmitry Torokhov]

On Wed, Apr 23, 2014 at 03:55:28PM +1000, Peter Hutterer wrote:
> On Tue, Apr 22, 2014 at 10:46:47PM -0700, Dmitry Torokhov wrote:
> > On Wed, Apr 23, 2014 at 03:38:49PM +1000, Peter Hutterer wrote:
> > > On Wed, Apr 23, 2014 at 10:21:03AM +1000, Peter Hutterer wrote:
> > > > On Tue, Apr 22, 2014 at 08:21:54AM +0200, David Herrmann wrote:
> > > > > Hi Peter
> > > > > 
> > > > > On Tue, Apr 22, 2014 at 6:15 AM, Peter Hutterer
> > > > > <peter.hutterer@who-t.net> wrote:
> > > > > > How are you planning to handle the slot-based events? We'd either need to
> > > > > > add something similar (but more complex) to evdev_handle_mt_request or rely
> > > > > > on the caller to call the whole EV_ABS range and ditch anything ABS_MT_.
> > > > > > I'd prefer the former, the latter is yet more behaviour that's easy to get
> > > > > > wrong.
> > > > > 
> > > > > This is all racy..
> > > > > 
> > > > > We _really_ need an ioctl to receive _all_ ABS information atomically.
> > > > > I mean, there's no way we can know the user's state from the kernel.
> > > > > Even if the user resyncs via EVIOCGMTSLOTS, we can never flush the
> > > > > whole ABS queue. Problem is, the user has to call the ioctl for _each_
> > > > > available MT code and events might get queued in between. So yeah,
> > > > > this patch doesn't help much..
> > > > > 
> > > > > I have no better idea than adding a new EVIOCGABS call that retrieves
> > > > > ABS values for all slots atomically (and for all other axes..). No
> > > > > idea how to properly fix the old ioctls.
> > > > 
> > > > bonus points for making that ioctl fetch the state of the last SYN_DROPPED
> > > > and leave the events since in the client buffer. That way we can smooth over
> > > > SYN_DROPPED and lose less information.
> > > 
> > > to expand on this, something like the below would work from userspace:
> > >  
> > > 1. userspace opens fd, EVIOCGBIT for everything
> > > 2. userspace calls EVIOCGABSATOMIC
> > > 3. kernel empties the event queue, flags the client as capable
> > > 4. kernel copies current device state into client-specific struct
> > > 5. kernel replies with that device state to the ioctl
> > > 6. client reads events 
> > > ..
> > > 7. kernel sees a SYN_DROPPED for this client. Takes a snapshot of the device
> > > for the client, empties the buffer, leaves SYN_DROPPED in the buffer
> > > (current behaviour)
> > > 8. client reads SYN_DROPPED, calls EVIOCGABSATOMIC
> > > 9. kernel replies with the snapshot state, leaves the event buffer otherwise
> > > unmodified
> > > 10. client reads all events after SYN_DROPPED, gets a smooth continuation
> > > 11. goto 6
> > > 
> > > if the buffer overflows multiple times, repeat 7 so that the snapshot state
> > > is always the last SYN_DROPPED state. well, technically the state should be
> > > the state of the device at the first SYN_REPORT after the last SYN_DROPPED,
> > > since the current API says that interrupted event is incomplete.
> > > 
> > > there are two oddities here:
> > > 1. the first ioctl will have to flush the buffer to guarantee consistent state,
> > > though you could even avoid that by taking a snapshot of the device on
> > > open(). though that comes with a disadvantage, you don't know if the client
> > > supports the new approach so you're wasting effort and memory here.
> > > 2. I'm not quite sure how to handle multiple repeated calls short of
> > > updating the client-specific snapshot with every event as it is read
> > > successfully.
> > > 
> > > any comments?
> > 
> > Do we really need to optimize the case when we are dropping events?
> 
> It happens frequently, to the point where on some laptops you're pretty much
> guaranteed to get SYN_DROPPED events on resume and sometimes even during
> normal multi-finger user.
> 
> I don't have any measurements on how many events are dropped on average.
> Could be one or two, could be several buffer sizes, I honestly don't know.

I think we need to figure this out. The idea is that dropping events
should be an exception, not a rule.

-- 
Dmitry

Re: [PATCH] evdev: flush ABS_* events during EVIOCGABS

[Peter Hutterer]

On Tue, Apr 22, 2014 at 11:07:47PM -0700, Dmitry Torokhov wrote:
> On Wed, Apr 23, 2014 at 03:55:28PM +1000, Peter Hutterer wrote:
> > On Tue, Apr 22, 2014 at 10:46:47PM -0700, Dmitry Torokhov wrote:
> > > On Wed, Apr 23, 2014 at 03:38:49PM +1000, Peter Hutterer wrote:
> > > > On Wed, Apr 23, 2014 at 10:21:03AM +1000, Peter Hutterer wrote:
> > > > > On Tue, Apr 22, 2014 at 08:21:54AM +0200, David Herrmann wrote:

..

> > > > 
> > > > any comments?
> > > 
> > > Do we really need to optimize the case when we are dropping events?
> > 
> > It happens frequently, to the point where on some laptops you're pretty much
> > guaranteed to get SYN_DROPPED events on resume and sometimes even during
> > normal multi-finger user.
> > 
> > I don't have any measurements on how many events are dropped on average.
> > Could be one or two, could be several buffer sizes, I honestly don't know.
> 
> I think we need to figure this out. The idea is that dropping events
> should be an exception, not a rule.

increase the buffer size for the devices I guess, with some heuristics
maybe. you could dynamically grow the buffer in the kernel, if the buffer
gets full or close to full, grow it.

but really, this is a moving target, eventually you will get the
SYN_DROPPED. if a client sleeps for a second or more, the events from a
second ago are likely useless anyway, so the need for an atomic sync exists
regardless. fwiw, I can live with a atomic sync that clears the client
buffer provided I get the correct state. any optimisation on top of that can
be done afterwards.

Cheers,
   Peter