[PATCH 0/4] input: evdev: Dynamic buffers (rev5)

[PATCH 0/4] input: evdev: Dynamic buffers (rev5)

[Henrik Rydberg]

Hi Dmitry,

Please find enclosed the fifth version of the evdev buffer patches.

In this version, the buffer scheme is left intact, leaving a single
patch to make the client buffers dynamic. Mostly harmless.

The rest of the patches are rebased versions of what has already been
sent, and summarizes what seems to be needed to fix the original
buffer problem. The last patch is a bonus which fixes an old bug.

Thanks,
Henrik

Henrik Rydberg (5):
  input: evdev: Convert to dynamic event buffer (rev5)
  input: Use driver hint to compute the evdev buffer size (rev3)
  input: bcm5974: Set the average number of events per MT event packet
  hid-input: Use a larger event buffer for MT devices
  input: evdev: Never leave the client buffer empty after write

 drivers/hid/hid-input.c       |    3 +++
 drivers/input/evdev.c         |   33 ++++++++++++++++++++++++++++-----
 drivers/input/mouse/bcm5974.c |    2 ++
 include/linux/input.h         |   17 +++++++++++++++++
 4 files changed, 50 insertions(+), 5 deletions(-)

[PATCH 1/5] input: evdev: Convert to dynamic event buffer (rev5)

[Henrik Rydberg]

Allocate the event buffer dynamically, and prepare to compute the
buffer size in a separate function. This patch defines the size
computation to be identical to the current code, and does not contain
any logical changes.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/input/evdev.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 2ee6c7a..5d84e59 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -10,7 +10,7 @@
 
 #define EVDEV_MINOR_BASE	64
 #define EVDEV_MINORS		32
-#define EVDEV_BUFFER_SIZE	64
+#define EVDEV_MIN_BUFFER_SIZE	64
 
 #include <linux/poll.h>
 #include <linux/sched.h>
@@ -36,7 +36,8 @@ struct evdev {
 };
 
 struct evdev_client {
-	struct input_event buffer[EVDEV_BUFFER_SIZE];
+	struct input_event *buffer;
+	int bufsize;
 	int head;
 	int tail;
 	spinlock_t buffer_lock; /* protects access to buffer, head and tail */
@@ -48,6 +49,11 @@ struct evdev_client {
 static struct evdev *evdev_table[EVDEV_MINORS];
 static DEFINE_MUTEX(evdev_table_mutex);
 
+static int evdev_compute_buffer_size(struct input_dev *dev)
+{
+	return EVDEV_MIN_BUFFER_SIZE;
+}
+
 static void evdev_pass_event(struct evdev_client *client,
 			     struct input_event *event)
 {
@@ -56,7 +62,7 @@ static void evdev_pass_event(struct evdev_client *client,
 	 */
 	spin_lock(&client->buffer_lock);
 	client->buffer[client->head++] = *event;
-	client->head &= EVDEV_BUFFER_SIZE - 1;
+	client->head &= client->bufsize - 1;
 	spin_unlock(&client->buffer_lock);
 
 	if (event->type == EV_SYN)
@@ -234,6 +240,7 @@ static int evdev_release(struct inode *inode, struct file *file)
 	mutex_unlock(&evdev->mutex);
 
 	evdev_detach_client(evdev, client);
+	kfree(client->buffer);
 	kfree(client);
 
 	evdev_close_device(evdev);
@@ -269,6 +276,14 @@ static int evdev_open(struct inode *inode, struct file *file)
 		goto err_put_evdev;
 	}
 
+	client->bufsize = evdev_compute_buffer_size(evdev->handle.dev);
+	client->buffer = kcalloc(client->bufsize, sizeof(struct input_event),
+				 GFP_KERNEL);
+	if (!client->buffer) {
+		error = -ENOMEM;
+		goto err_free_memory;
+	}
+
 	spin_lock_init(&client->buffer_lock);
 	client->evdev = evdev;
 	evdev_attach_client(evdev, client);
@@ -284,6 +299,8 @@ static int evdev_open(struct inode *inode, struct file *file)
 
  err_free_client:
 	evdev_detach_client(evdev, client);
+	kfree(client->buffer);
+ err_free_memory:
 	kfree(client);
  err_put_evdev:
 	put_device(&evdev->dev);
@@ -334,7 +351,7 @@ static int evdev_fetch_next_event(struct evdev_client *client,
 	have_event = client->head != client->tail;
 	if (have_event) {
 		*event = client->buffer[client->tail++];
-		client->tail &= EVDEV_BUFFER_SIZE - 1;
+		client->tail &= client->bufsize - 1;
 	}
 
 	spin_unlock_irq(&client->buffer_lock);
-- 
1.6.3.3

[PATCH 2/5] input: Use driver hint to compute the evdev buffer size (rev3)

[Henrik Rydberg]

Some devices, in particular MT devices, produce a lot of data.  This
leads to a high frequency of lost packets in evdev, which by default
uses a fairly small event buffer. Let the drivers hint the average
number of events per packet for the device by calling the
input_set_events_per_packet(), and use that information when computing
the evdev buffer size.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/input/evdev.c |    5 ++++-
 include/linux/input.h |   17 +++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 5d84e59..728802f 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -11,6 +11,7 @@
 #define EVDEV_MINOR_BASE	64
 #define EVDEV_MINORS		32
 #define EVDEV_MIN_BUFFER_SIZE	64
+#define EVDEV_BUF_PACKETS	8
 
 #include <linux/poll.h>
 #include <linux/sched.h>
@@ -51,7 +52,9 @@ static DEFINE_MUTEX(evdev_table_mutex);
 
 static int evdev_compute_buffer_size(struct input_dev *dev)
 {
-	return EVDEV_MIN_BUFFER_SIZE;
+	int nev = dev->hint_events_per_packet * EVDEV_BUF_PACKETS;
+	nev = max(nev, EVDEV_MIN_BUFFER_SIZE);
+	return roundup_pow_of_two(nev);
 }
 
 static void evdev_pass_event(struct evdev_client *client,
diff --git a/include/linux/input.h b/include/linux/input.h
index 20e4eac..9e024b6 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -1162,6 +1162,8 @@ struct input_dev {
 	unsigned long ffbit[BITS_TO_LONGS(FF_CNT)];
 	unsigned long swbit[BITS_TO_LONGS(SW_CNT)];
 
+	unsigned int hint_events_per_packet;
+
 	unsigned int keycodemax;
 	unsigned int keycodesize;
 	void *keycode;
@@ -1439,6 +1441,21 @@ static inline void input_mt_slot(struct input_dev *dev, int slot)
 
 void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int code);
 
+/**
+ * input_set_events_per_packet - tell handlers about the driver event rate
+ * @dev: the input device used by the driver
+ * @nev: the average number of events between calls to input_sync()
+ *
+ * If the event rate sent from a device is unusually large, use this
+ * function to set the expected event rate. This will allow handlers
+ * to set up an approriate buffer size for the event stream, in order
+ * to minimize information loss.
+ */
+static inline void input_set_events_per_packet(struct input_dev *dev, int nev)
+{
+	dev->hint_events_per_packet = nev;
+}
+
 static inline void input_set_abs_params(struct input_dev *dev, int axis, int min, int max, int fuzz, int flat)
 {
 	dev->absmin[axis] = min;
-- 
1.6.3.3

[PATCH 3/5] input: bcm5974: Set the average number of events per MT event packet

[Henrik Rydberg]

The MT devices produce a lot of data. Tell the underlying input device
approximately how many events will be sent per synchronization, to allow
for better buffering. The number is a template based on continuously
reporting details for each finger on a single hand.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/input/mouse/bcm5974.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 6dedded..84094bb 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -312,6 +312,8 @@ static void setup_events_to_report(struct input_dev *input_dev,
 	__set_bit(BTN_TOOL_TRIPLETAP, input_dev->keybit);
 	__set_bit(BTN_TOOL_QUADTAP, input_dev->keybit);
 	__set_bit(BTN_LEFT, input_dev->keybit);
+
+	input_set_events_per_packet(input_dev, 60);
 }
 
 /* report button data as logical button state */
-- 
1.6.3.3

[PATCH 4/5] hid-input: Use a larger event buffer for MT devices

[Henrik Rydberg]

The MT devices produce a lot of data. Tell the underlying input device
approximately how many events will be sent per synchronization, to allow
for better buffering. The number is a template based on continuously
reporting details for each finger on a single hand.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-input.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 7a0d2e4..69d152e 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -534,6 +534,9 @@ mapped:
 			input_set_abs_params(input, usage->code, a, b, (b - a) >> 8, (b - a) >> 4);
 		else	input_set_abs_params(input, usage->code, a, b, 0, 0);
 
+		/* use a larger default input buffer for MT devices */
+		if (usage->code == ABS_MT_POSITION_X && input->hint_events_per_packet == 0)
+			input_set_events_per_packet(input, 60);
 	}
 
 	if (usage->type == EV_ABS &&
-- 
1.6.3.3

[PATCH 5/5] input: evdev: Never leave the client buffer empty after write

[Henrik Rydberg]

When the client buffer is very small and wraps around a lot, it may
well be that a write increases the head such that head == tail. If
this happens between the point where a poll is triggered and the
actual data is being read, there will be no data to read. This is
confusing to applications, which might end up closing the file.

This patch solves the problem by making sure the client buffer is
never empty after writing to it.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/input/evdev.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 728802f..45eae19 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -62,10 +62,13 @@ static void evdev_pass_event(struct evdev_client *client,
 {
 	/*
 	 * Interrupts are disabled, just acquire the lock
+	 * Never leave the client buffer empty
 	 */
 	spin_lock(&client->buffer_lock);
-	client->buffer[client->head++] = *event;
-	client->head &= client->bufsize - 1;
+	do {
+		client->buffer[client->head++] = *event;
+		client->head &= client->bufsize - 1;
+	} while (client->head == client->tail);
 	spin_unlock(&client->buffer_lock);
 
 	if (event->type == EV_SYN)
-- 
1.6.3.3

Re: [PATCH 4/5] hid-input: Use a larger event buffer for MT devices

[Jiri Kosina]

On Wed, 23 Jun 2010, Henrik Rydberg wrote:

> The MT devices produce a lot of data. Tell the underlying input device
> approximately how many events will be sent per synchronization, to allow
> for better buffering. The number is a template based on continuously
> reporting details for each finger on a single hand.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

Feel free to add

	Signed-off-by: Jiri Kosina <jkosina@suse.cz>

to any other further patch respins (if needed), or if actually applying 
this patch to any tree together with the rest of the series.

Thanks.

> ---
>  drivers/hid/hid-input.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 7a0d2e4..69d152e 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -534,6 +534,9 @@ mapped:
>  			input_set_abs_params(input, usage->code, a, b, (b - a) >> 8, (b - a) >> 4);
>  		else	input_set_abs_params(input, usage->code, a, b, 0, 0);
>  
> +		/* use a larger default input buffer for MT devices */
> +		if (usage->code == ABS_MT_POSITION_X && input->hint_events_per_packet == 0)
> +			input_set_events_per_packet(input, 60);
>  	}
>  
>  	if (usage->type == EV_ABS &&
> -- 
> 1.6.3.3
> 

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 2/5] input: Use driver hint to compute the evdev buffer size (rev3)

[Ping Cheng]

Hi Henrik,

I like this patchset. Along with the mtdev patchset submitted to x.org
by Chase, I see a great collaboration for the MT support.  Keep up the
good work.

I have a minor comment inline.

Thank you.

Ping

On Wed, Jun 23, 2010 at 4:14 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Some devices, in particular MT devices, produce a lot of data.  This
> leads to a high frequency of lost packets in evdev, which by default
> uses a fairly small event buffer. Let the drivers hint the average
> number of events per packet for the device by calling the
> input_set_events_per_packet(), and use that information when computing
> the evdev buffer size.
>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/input/evdev.c |    5 ++++-
>  include/linux/input.h |   17 +++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 5d84e59..728802f 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -11,6 +11,7 @@
>  #define EVDEV_MINOR_BASE       64
>  #define EVDEV_MINORS           32
>  #define EVDEV_MIN_BUFFER_SIZE  64
> +#define EVDEV_BUF_PACKETS      8
>
>  #include <linux/poll.h>
>  #include <linux/sched.h>
> @@ -51,7 +52,9 @@ static DEFINE_MUTEX(evdev_table_mutex);
>
>  static int evdev_compute_buffer_size(struct input_dev *dev)
>  {
> -       return EVDEV_MIN_BUFFER_SIZE;
> +       int nev = dev->hint_events_per_packet * EVDEV_BUF_PACKETS;
> +       nev = max(nev, EVDEV_MIN_BUFFER_SIZE);
> +       return roundup_pow_of_two(nev);

I think we have a backward compatibility issue here. This routine will
return 7 when nev falls to the default value
(EVDEV_MIN_BUFFER_SIZE/64).  This could happen to those drivers that
don't report MT events or forget/don't feel the need to set
hint_events_per_packet since the old BUFFER_SIZE worked perfectly for
them.  We need to keep the return value for those drivers as 64 so we
could allocate the same space as it was in [PATCH 1/5].

>  }
>
>  static void evdev_pass_event(struct evdev_client *client,
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 20e4eac..9e024b6 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -1162,6 +1162,8 @@ struct input_dev {
>        unsigned long ffbit[BITS_TO_LONGS(FF_CNT)];
>        unsigned long swbit[BITS_TO_LONGS(SW_CNT)];
>
> +       unsigned int hint_events_per_packet;
> +
>        unsigned int keycodemax;
>        unsigned int keycodesize;
>        void *keycode;
> @@ -1439,6 +1441,21 @@ static inline void input_mt_slot(struct input_dev *dev, int slot)
>
>  void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int code);
>
> +/**
> + * input_set_events_per_packet - tell handlers about the driver event rate
> + * @dev: the input device used by the driver
> + * @nev: the average number of events between calls to input_sync()
> + *
> + * If the event rate sent from a device is unusually large, use this
> + * function to set the expected event rate. This will allow handlers
> + * to set up an approriate buffer size for the event stream, in order
> + * to minimize information loss.
> + */
> +static inline void input_set_events_per_packet(struct input_dev *dev, int nev)
> +{
> +       dev->hint_events_per_packet = nev;
> +}
> +
>  static inline void input_set_abs_params(struct input_dev *dev, int axis, int min, int max, int fuzz, int flat)
>  {
>        dev->absmin[axis] = min;
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] input: Use driver hint to compute the evdev buffer size (rev3)

[Henrik Rydberg]

Hi Ping,
[...]
>> @@ -51,7 +52,9 @@ static DEFINE_MUTEX(evdev_table_mutex);
>>
>>  static int evdev_compute_buffer_size(struct input_dev *dev)
>>  {
>> -       return EVDEV_MIN_BUFFER_SIZE;
>> +       int nev = dev->hint_events_per_packet * EVDEV_BUF_PACKETS;
>> +       nev = max(nev, EVDEV_MIN_BUFFER_SIZE);
>> +       return roundup_pow_of_two(nev);
> 
> I think we have a backward compatibility issue here. This routine will
> return 7 when nev falls to the default value
> (EVDEV_MIN_BUFFER_SIZE/64).  This could happen to those drivers that
> don't report MT events or forget/don't feel the need to set
> hint_events_per_packet since the old BUFFER_SIZE worked perfectly for
> them.  We need to keep the return value for those drivers as 64 so we
> could allocate the same space as it was in [PATCH 1/5].

Are you perhaps confusing EVDEV_BUF_PACKETS and EVDEV_MIN_BUFFER_SIZE? The last
line ensures that the value returned is a power of two (hence not 7). The
second-to-last line ensures the value is at least equal to 64 (hence not 7). The
default hint value for a driver that does not do anything is zero, which leads
to a return value of 64, just as it is today.

Thanks,
Henrik

Re: [PATCH 2/5] input: Use driver hint to compute the evdev buffer size (rev3)

[Dmitry Torokhov]

On Wed, Jun 23, 2010 at 07:07:44PM +0200, Henrik Rydberg wrote:
> Hi Ping,
> [...]
> >> @@ -51,7 +52,9 @@ static DEFINE_MUTEX(evdev_table_mutex);
> >>
> >>  static int evdev_compute_buffer_size(struct input_dev *dev)
> >>  {
> >> -       return EVDEV_MIN_BUFFER_SIZE;
> >> +       int nev = dev->hint_events_per_packet * EVDEV_BUF_PACKETS;
> >> +       nev = max(nev, EVDEV_MIN_BUFFER_SIZE);
> >> +       return roundup_pow_of_two(nev);
> > 
> > I think we have a backward compatibility issue here. This routine will
> > return 7 when nev falls to the default value
> > (EVDEV_MIN_BUFFER_SIZE/64).  This could happen to those drivers that
> > don't report MT events or forget/don't feel the need to set
> > hint_events_per_packet since the old BUFFER_SIZE worked perfectly for
> > them.  We need to keep the return value for those drivers as 64 so we
> > could allocate the same space as it was in [PATCH 1/5].
> 
> Are you perhaps confusing EVDEV_BUF_PACKETS and EVDEV_MIN_BUFFER_SIZE? The last
> line ensures that the value returned is a power of two (hence not 7). The
> second-to-last line ensures the value is at least equal to 64 (hence not 7). The
> default hint value for a driver that does not do anything is zero, which leads
> to a return value of 64, just as it is today.
> 

I think Ping might have confused roundup_pow_of_two() with
get_order()-type function...

Anyways, applied all 5, thanks Henrik.

-- 
Dmitry

Re: [PATCH 2/5] input: Use driver hint to compute the evdev buffer size (rev3)

[Henrik Rydberg]

Dmitry Torokhov wrote:
> On Wed, Jun 23, 2010 at 07:07:44PM +0200, Henrik Rydberg wrote:
>> Hi Ping,
>> [...]
>>>> @@ -51,7 +52,9 @@ static DEFINE_MUTEX(evdev_table_mutex);
>>>>
>>>>  static int evdev_compute_buffer_size(struct input_dev *dev)
>>>>  {
>>>> -       return EVDEV_MIN_BUFFER_SIZE;
>>>> +       int nev = dev->hint_events_per_packet * EVDEV_BUF_PACKETS;
>>>> +       nev = max(nev, EVDEV_MIN_BUFFER_SIZE);
>>>> +       return roundup_pow_of_two(nev);
>>> I think we have a backward compatibility issue here. This routine will
>>> return 7 when nev falls to the default value
>>> (EVDEV_MIN_BUFFER_SIZE/64).  This could happen to those drivers that
>>> don't report MT events or forget/don't feel the need to set
>>> hint_events_per_packet since the old BUFFER_SIZE worked perfectly for
>>> them.  We need to keep the return value for those drivers as 64 so we
>>> could allocate the same space as it was in [PATCH 1/5].
>> Are you perhaps confusing EVDEV_BUF_PACKETS and EVDEV_MIN_BUFFER_SIZE? The last
>> line ensures that the value returned is a power of two (hence not 7). The
>> second-to-last line ensures the value is at least equal to 64 (hence not 7). The
>> default hint value for a driver that does not do anything is zero, which leads
>> to a return value of 64, just as it is today.
>>
> 
> I think Ping might have confused roundup_pow_of_two() with
> get_order()-type function...
> 
> Anyways, applied all 5, thanks Henrik.
> 

Got them, thanks to you too.

Henrik