Input: atkbd - speed up setting leds/repeat state
Changing led state is pretty slow operation; when there are multiple
requests coming at a high rate they may interfere with normal typing.
Try optimize (skip) changing hardware state when multiple requests
are coming back-to-back.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/input/keyboard/atkbd.c | 99 ++++++++++++++++++++++++++++-------------
1 files changed, 68 insertions(+), 31 deletions(-)
Index: work/drivers/input/keyboard/atkbd.c
===================================================================
--- work.orig/drivers/input/keyboard/atkbd.c
+++ work/drivers/input/keyboard/atkbd.c
@@ -166,6 +166,9 @@ static unsigned char atkbd_unxlate_table
#define ATKBD_SPECIAL 248
+#define ATKBD_LED_EVENT_BIT 0
+#define ATKBD_REP_EVENT_BIT 1
+
static struct {
unsigned char keycode;
unsigned char set2;
@@ -211,6 +214,10 @@ struct atkbd {
unsigned char err_xl;
unsigned int last;
unsigned long time;
+
+ struct work_struct event_work;
+ struct semaphore event_sem;
+ unsigned long event_mask;
};
static ssize_t atkbd_attr_show_helper(struct device *dev, char *buf,
@@ -424,58 +431,86 @@ out:
}
/*
- * Event callback from the input module. Events that change the state of
- * the hardware are processed here.
+ * atkbd_event_work() is used to complete processing of events that
+ * can not be processed by input_event() which is often called from
+ * interrupt context.
*/
-static int atkbd_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
+static void atkbd_event_work(void *data)
{
- struct atkbd *atkbd = dev->private;
const short period[32] =
{ 33, 37, 42, 46, 50, 54, 58, 63, 67, 75, 83, 92, 100, 109, 116, 125,
133, 149, 167, 182, 200, 217, 232, 250, 270, 303, 333, 370, 400, 435, 470, 500 };
const short delay[4] =
{ 250, 500, 750, 1000 };
+
+ struct atkbd *atkbd = data;
+ struct input_dev *dev = atkbd->dev;
unsigned char param[2];
int i, j;
+ down(&atkbd->event_sem);
+
+ if (test_and_clear_bit(ATKBD_LED_EVENT_BIT, &atkbd->event_mask)) {
+ param[0] = (test_bit(LED_SCROLLL, dev->led) ? 1 : 0)
+ | (test_bit(LED_NUML, dev->led) ? 2 : 0)
+ | (test_bit(LED_CAPSL, dev->led) ? 4 : 0);
+ ps2_command(&atkbd->ps2dev, param, ATKBD_CMD_SETLEDS);
+
+ if (atkbd->extra) {
+ param[0] = 0;
+ param[1] = (test_bit(LED_COMPOSE, dev->led) ? 0x01 : 0)
+ | (test_bit(LED_SLEEP, dev->led) ? 0x02 : 0)
+ | (test_bit(LED_SUSPEND, dev->led) ? 0x04 : 0)
+ | (test_bit(LED_MISC, dev->led) ? 0x10 : 0)
+ | (test_bit(LED_MUTE, dev->led) ? 0x20 : 0);
+ ps2_command(&atkbd->ps2dev, param, ATKBD_CMD_EX_SETLEDS);
+ }
+ }
+
+ if (test_and_clear_bit(ATKBD_REP_EVENT_BIT, &atkbd->event_mask)) {
+ i = j = 0;
+ while (i < 31 && period[i] < dev->rep[REP_PERIOD])
+ i++;
+ while (j < 3 && delay[j] < dev->rep[REP_DELAY])
+ j++;
+ dev->rep[REP_PERIOD] = period[i];
+ dev->rep[REP_DELAY] = delay[j];
+ param[0] = i | (j << 5);
+ ps2_command(&atkbd->ps2dev, param, ATKBD_CMD_SETREP);
+ }
+
+ up(&atkbd->event_sem);
+}
+
+/*
+ * Event callback from the input module. Events that change the state of
+ * the hardware are processed here. If action can not be performed in
+ * interrupt context it is offloaded to atkbd_event_work.
+ */
+
+static int atkbd_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
+{
+ struct atkbd *atkbd = dev->private;
+
if (!atkbd->write)
return -1;
switch (type) {
case EV_LED:
-
- param[0] = (test_bit(LED_SCROLLL, dev->led) ? 1 : 0)
- | (test_bit(LED_NUML, dev->led) ? 2 : 0)
- | (test_bit(LED_CAPSL, dev->led) ? 4 : 0);
- ps2_schedule_command(&atkbd->ps2dev, param, ATKBD_CMD_SETLEDS);
-
- if (atkbd->extra) {
- param[0] = 0;
- param[1] = (test_bit(LED_COMPOSE, dev->led) ? 0x01 : 0)
- | (test_bit(LED_SLEEP, dev->led) ? 0x02 : 0)
- | (test_bit(LED_SUSPEND, dev->led) ? 0x04 : 0)
- | (test_bit(LED_MISC, dev->led) ? 0x10 : 0)
- | (test_bit(LED_MUTE, dev->led) ? 0x20 : 0);
- ps2_schedule_command(&atkbd->ps2dev, param, ATKBD_CMD_EX_SETLEDS);
- }
-
+ set_bit(ATKBD_LED_EVENT_BIT, &atkbd->event_mask);
+ wmb();
+ schedule_work(&atkbd->event_work);
return 0;
case EV_REP:
- if (atkbd->softrepeat) return 0;
-
- i = j = 0;
- while (i < 31 && period[i] < dev->rep[REP_PERIOD])
- i++;
- while (j < 3 && delay[j] < dev->rep[REP_DELAY])
- j++;
- dev->rep[REP_PERIOD] = period[i];
- dev->rep[REP_DELAY] = delay[j];
- param[0] = i | (j << 5);
- ps2_schedule_command(&atkbd->ps2dev, param, ATKBD_CMD_SETREP);
+ if (!atkbd->softrepeat) {
+ set_bit(ATKBD_REP_EVENT_BIT, &atkbd->event_mask);
+ wmb();
+ schedule_work(&atkbd->event_work);
+ }
return 0;
}
@@ -810,6 +845,8 @@ static int atkbd_connect(struct serio *s
atkbd->dev = dev;
ps2_init(&atkbd->ps2dev, serio);
+ INIT_WORK(&atkbd->event_work, atkbd_event_work, atkbd);
+ init_MUTEX(&atkbd->event_sem);
switch (serio->id.type) {
On Mon, Oct 31, 2005 at 02:24:02AM -0500, Dmitry Torokhov wrote:
> Input: atkbd - speed up setting leds/repeat state
>
> Changing led state is pretty slow operation; when there are multiple
> requests coming at a high rate they may interfere with normal typing.
> Try optimize (skip) changing hardware state when multiple requests
> are coming back-to-back.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
It looks good - just two comments:
1) wmb() shouldn't be needed after set_bit()
2) maybe we want to enforce the delay before we send the
next SET_LED command.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
On Monday 31 October 2005 07:47, Vojtech Pavlik wrote:
> On Mon, Oct 31, 2005 at 02:24:02AM -0500, Dmitry Torokhov wrote:
> > Input: atkbd - speed up setting leds/repeat state
> >
> > Changing led state is pretty slow operation; when there are multiple
> > requests coming at a high rate they may interfere with normal typing.
> > Try optimize (skip) changing hardware state when multiple requests
> > are coming back-to-back.
> >
> > Signed-off-by: Dmitry Torokhov <[email protected]>
>
> It looks good - just two comments:
>
> 1) wmb() shouldn't be needed after set_bit()
>
Judging by the comments in bitops only i386 implementation of set_bit
implies memory barrier, other arches do not guarantee it. That's why
I added wmb() there.
> 2) maybe we want to enforce the delay before we send the
> next SET_LED command.
>
Well, with this patch "while true; do xset led 3; xset led -3; done"
does not interfere with typing on my box and system load is staying
low which means we don't have too many outstanding requests.
--
Dmitry
On Mon, Oct 31, 2005 at 09:05:31AM -0500, Dmitry Torokhov wrote:
> On Monday 31 October 2005 07:47, Vojtech Pavlik wrote:
> > On Mon, Oct 31, 2005 at 02:24:02AM -0500, Dmitry Torokhov wrote:
> > > Input: atkbd - speed up setting leds/repeat state
> > >
> > > Changing led state is pretty slow operation; when there are multiple
> > > requests coming at a high rate they may interfere with normal typing.
> > > Try optimize (skip) changing hardware state when multiple requests
> > > are coming back-to-back.
> > >
> > > Signed-off-by: Dmitry Torokhov <[email protected]>
> >
> > It looks good - just two comments:
> >
> > 1) wmb() shouldn't be needed after set_bit()
> >
>
> Judging by the comments in bitops only i386 implementation of set_bit
> implies memory barrier, other arches do not guarantee it. That's why
> I added wmb() there.
Hmm, OK. I always forget that atomicity doesn't imply a memory barrier.
> > 2) maybe we want to enforce the delay before we send the
> > next SET_LED command.
> Well, with this patch "while true; do xset led 3; xset led -3; done"
> does not interfere with typing on my box and system load is staying
> low which means we don't have too many outstanding requests.
OK. I'll give it a spin if I find the time to.
--
Vojtech Pavlik
SuSE Labs, SuSE CR