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(-)
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Feel free to add
Signed-off-by: Jiri Kosina <[email protected]>
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.
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 <[email protected]> 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 <[email protected]>
> ---
> ?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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
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
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
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