[Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Amos Kong]

Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
but ps2 backend doesn't process it and no auto-repeat implementation.
This patch adds support of auto-repeat feature.

Guest ps2 driver sets autorepeat to fastest possible in reset,
period: 250ms, delay: 33ms

Tested by 'sendkey' monitor command.

referenced: http://www.computer-engineering.org/ps2keyboard/

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/input/ps2.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 3412079..1cfe055 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -94,6 +94,9 @@ typedef struct {
     int translate;
     int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
     int ledstate;
+    int repeat_period; /* typematic period, ms */
+    int repeat_delay; /* typematic delay, ms */
+    int repeat_key; /* keycode to repeat */
 } PS2KbdState;
 
 typedef struct {
@@ -146,6 +149,22 @@ void ps2_queue(void *opaque, int b)
     s->update_irq(s->update_arg, 1);
 }
 
+static QEMUTimer *repeat_timer;
+static bool auto_repeat;
+
+static void repeat_ps2_queue(void *opaque)
+{
+    PS2KbdState *s = opaque;
+
+    if (auto_repeat) {
+        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
+                       muldiv64(get_ticks_per_sec(), s->repeat_period,
+                       1000));
+        ps2_queue(&s->common, s->repeat_key);
+    }
+}
+
+
 /*
    keycode is expressed as follow:
    bit 7    - 0 key pressed, 1 = key released
@@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
             keycode = ps2_raw_keycode_set3[keycode & 0x7f];
         }
       }
+
+    /* only auto-repeat press event */
+    auto_repeat = ~keycode & 0x80;
     ps2_queue(&s->common, keycode);
+
+    if (auto_repeat) {
+        s->repeat_key = keycode;
+        /* delay a while before first repeat */
+        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
+                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
+    }
 }
 
 uint32_t ps2_read_data(void *opaque)
@@ -213,6 +242,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
 
 void ps2_write_keyboard(void *opaque, int val)
 {
+    /* repeat period/delay table from kernel (drivers/input/keyboard/atkbd.c) */
+    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 };
     PS2KbdState *s = (PS2KbdState *)opaque;
 
     switch(s->common.write_cmd) {
@@ -288,6 +322,11 @@ void ps2_write_keyboard(void *opaque, int val)
         s->common.write_cmd = -1;
         break;
     case KBD_CMD_SET_RATE:
+       /* Bit0-4 specifies the repeat rate */
+        s->repeat_period = period[val & 0x1f];
+       /* Bit5-6 bit specifies the delay time */
+        s->repeat_delay = delay[(val & 0x60) >> 5];
+
         ps2_queue(&s->common, KBD_REPLY_ACK);
         s->common.write_cmd = -1;
         break;
@@ -536,6 +575,8 @@ static void ps2_kbd_reset(void *opaque)
     s->scan_enabled = 0;
     s->translate = 0;
     s->scancode_set = 0;
+    s->repeat_period = 92;
+    s->repeat_delay = 500;
 }
 
 static void ps2_mouse_reset(void *opaque)
@@ -660,6 +701,7 @@ void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
     vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
     qemu_add_kbd_event_handler(ps2_put_keycode, s);
     qemu_register_reset(ps2_kbd_reset, s);
+    repeat_timer = qemu_new_timer_ns(vm_clock, repeat_ps2_queue, s);
     return s;
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[li guang]

在 2013-05-16四的 12:30 +0800，Amos Kong写道：
> Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> but ps2 backend doesn't process it and no auto-repeat implementation.
> This patch adds support of auto-repeat feature.
> 
> Guest ps2 driver sets autorepeat to fastest possible in reset,
> period: 250ms, delay: 33ms
> 
> Tested by 'sendkey' monitor command.
> 
> referenced: http://www.computer-engineering.org/ps2keyboard/
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  hw/input/ps2.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 3412079..1cfe055 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -94,6 +94,9 @@ typedef struct {
>      int translate;
>      int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
>      int ledstate;
> +    int repeat_period; /* typematic period, ms */
> +    int repeat_delay; /* typematic delay, ms */
> +    int repeat_key; /* keycode to repeat */
>  } PS2KbdState;
>  
>  typedef struct {
> @@ -146,6 +149,22 @@ void ps2_queue(void *opaque, int b)
>      s->update_irq(s->update_arg, 1);
>  }
>  
> +static QEMUTimer *repeat_timer;
> +static bool auto_repeat;
> +
> +static void repeat_ps2_queue(void *opaque)
> +{
> +    PS2KbdState *s = opaque;
> +

theoretically, we have to check if the typematic key is in break
now, if so, we will not do repeat anymore.
don't you think we have a chance that new key can come in during
waiting?

> +    if (auto_repeat) {
> +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                       muldiv64(get_ticks_per_sec(), s->repeat_period,
> +                       1000));
> +        ps2_queue(&s->common, s->repeat_key);
> +    }
> +}
> +
> +
>  /*
>     keycode is expressed as follow:
>     bit 7    - 0 key pressed, 1 = key released
> @@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
>              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
>          }
>        }
> +
> +    /* only auto-repeat press event */
> +    auto_repeat = ~keycode & 0x80;
>      ps2_queue(&s->common, keycode);
> +
> +    if (auto_repeat) {
> +        s->repeat_key = keycode;
> +        /* delay a while before first repeat */
> +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
> +    }
>  }
>  
>  uint32_t ps2_read_data(void *opaque)
> @@ -213,6 +242,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
>  
>  void ps2_write_keyboard(void *opaque, int val)
>  {
> +    /* repeat period/delay table from kernel (drivers/input/keyboard/atkbd.c) */
> +    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 };
>      PS2KbdState *s = (PS2KbdState *)opaque;
>  
>      switch(s->common.write_cmd) {
> @@ -288,6 +322,11 @@ void ps2_write_keyboard(void *opaque, int val)
>          s->common.write_cmd = -1;
>          break;
>      case KBD_CMD_SET_RATE:
> +       /* Bit0-4 specifies the repeat rate */
> +        s->repeat_period = period[val & 0x1f];
> +       /* Bit5-6 bit specifies the delay time */
> +        s->repeat_delay = delay[(val & 0x60) >> 5];

s/(val & 0x60) >> 5/(val & 0x60) >> 5 & 0x4/

> +
>          ps2_queue(&s->common, KBD_REPLY_ACK);
>          s->common.write_cmd = -1;
>          break;
> @@ -536,6 +575,8 @@ static void ps2_kbd_reset(void *opaque)
>      s->scan_enabled = 0;
>      s->translate = 0;
>      s->scancode_set = 0;
> +    s->repeat_period = 92;
> +    s->repeat_delay = 500;
>  }
>  
>  static void ps2_mouse_reset(void *opaque)
> @@ -660,6 +701,7 @@ void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
>      vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
>      qemu_add_kbd_event_handler(ps2_put_keycode, s);
>      qemu_register_reset(ps2_kbd_reset, s);
> +    repeat_timer = qemu_new_timer_ns(vm_clock, repeat_ps2_queue, s);
>      return s;
>  }
>  

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Jason Wang]

On 05/16/2013 12:30 PM, Amos Kong wrote:
> Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> but ps2 backend doesn't process it and no auto-repeat implementation.
> This patch adds support of auto-repeat feature.
>
> Guest ps2 driver sets autorepeat to fastest possible in reset,
> period: 250ms, delay: 33ms
>
> Tested by 'sendkey' monitor command.
>
> referenced: http://www.computer-engineering.org/ps2keyboard/
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  hw/input/ps2.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 3412079..1cfe055 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -94,6 +94,9 @@ typedef struct {
>      int translate;
>      int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
>      int ledstate;
> +    int repeat_period; /* typematic period, ms */
> +    int repeat_delay; /* typematic delay, ms */
> +    int repeat_key; /* keycode to repeat */
>  } PS2KbdState;
>  
>  typedef struct {
> @@ -146,6 +149,22 @@ void ps2_queue(void *opaque, int b)
>      s->update_irq(s->update_arg, 1);
>  }
>  
> +static QEMUTimer *repeat_timer;
> +static bool auto_repeat;
> +
> +static void repeat_ps2_queue(void *opaque)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    if (auto_repeat) {
> +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                       muldiv64(get_ticks_per_sec(), s->repeat_period,
> +                       1000));
> +        ps2_queue(&s->common, s->repeat_key);
> +    }
> +}
> +
> +
>  /*
>     keycode is expressed as follow:
>     bit 7    - 0 key pressed, 1 = key released
> @@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
>              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
>          }
>        }
> +
> +    /* only auto-repeat press event */
> +    auto_repeat = ~keycode & 0x80;
>      ps2_queue(&s->common, keycode);
> +
> +    if (auto_repeat) {
> +        s->repeat_key = keycode;
> +        /* delay a while before first repeat */
> +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
> +    }
>  }

Not familiar with ps2, but don't we need something to make this safe
after migration?
>  
>  uint32_t ps2_read_data(void *opaque)
> @@ -213,6 +242,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
>  
>  void ps2_write_keyboard(void *opaque, int val)
>  {
> +    /* repeat period/delay table from kernel (drivers/input/keyboard/atkbd.c) */
> +    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 };
>      PS2KbdState *s = (PS2KbdState *)opaque;
>  
>      switch(s->common.write_cmd) {
> @@ -288,6 +322,11 @@ void ps2_write_keyboard(void *opaque, int val)
>          s->common.write_cmd = -1;
>          break;
>      case KBD_CMD_SET_RATE:
> +       /* Bit0-4 specifies the repeat rate */
> +        s->repeat_period = period[val & 0x1f];
> +       /* Bit5-6 bit specifies the delay time */
> +        s->repeat_delay = delay[(val & 0x60) >> 5];
> +
>          ps2_queue(&s->common, KBD_REPLY_ACK);
>          s->common.write_cmd = -1;
>          break;
> @@ -536,6 +575,8 @@ static void ps2_kbd_reset(void *opaque)
>      s->scan_enabled = 0;
>      s->translate = 0;
>      s->scancode_set = 0;
> +    s->repeat_period = 92;
> +    s->repeat_delay = 500;
>  }
>  
>  static void ps2_mouse_reset(void *opaque)
> @@ -660,6 +701,7 @@ void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
>      vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
>      qemu_add_kbd_event_handler(ps2_put_keycode, s);
>      qemu_register_reset(ps2_kbd_reset, s);
> +    repeat_timer = qemu_new_timer_ns(vm_clock, repeat_ps2_queue, s);
>      return s;
>  }
>  

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Peter Maydell]

On 16 May 2013 05:30, Amos Kong <akong@redhat.com> wrote:
> Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> but ps2 backend doesn't process it and no auto-repeat implementation.
> This patch adds support of auto-repeat feature.

> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 3412079..1cfe055 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -94,6 +94,9 @@ typedef struct {
>      int translate;
>      int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
>      int ledstate;
> +    int repeat_period; /* typematic period, ms */
> +    int repeat_delay; /* typematic delay, ms */
> +    int repeat_key; /* keycode to repeat */
>  } PS2KbdState;
>
>  typedef struct {
> @@ -146,6 +149,22 @@ void ps2_queue(void *opaque, int b)
>      s->update_irq(s->update_arg, 1);
>  }
>
> +static QEMUTimer *repeat_timer;
> +static bool auto_repeat;

These shouldn't be static -- what would happen
on a system with two ps2 keyboard models in it?

You need to reset your qemu_timer in the ps2 reset
handler, as well; otherwise it could go off unexpectedly
after a reset. (Though perhaps not if we're simulating
a human with their finger held down on the key...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Amos Kong]

On Thu, May 16, 2013 at 01:30:28PM +0800, li guang wrote:
> 在 2013-05-16四的 12:30 +0800，Amos Kong写道：
> > Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> > but ps2 backend doesn't process it and no auto-repeat implementation.
> > This patch adds support of auto-repeat feature.
> > 
> > Guest ps2 driver sets autorepeat to fastest possible in reset,
> > period: 250ms, delay: 33ms
> > 
> > Tested by 'sendkey' monitor command.
> > 
> > referenced: http://www.computer-engineering.org/ps2keyboard/
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  hw/input/ps2.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> > index 3412079..1cfe055 100644
> > --- a/hw/input/ps2.c
> > +++ b/hw/input/ps2.c
> > @@ -94,6 +94,9 @@ typedef struct {
> >      int translate;
> >      int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
> >      int ledstate;
> > +    int repeat_period; /* typematic period, ms */
> > +    int repeat_delay; /* typematic delay, ms */
> > +    int repeat_key; /* keycode to repeat */
> >  } PS2KbdState;
> >  
> >  typedef struct {
> > @@ -146,6 +149,22 @@ void ps2_queue(void *opaque, int b)
> >      s->update_irq(s->update_arg, 1);
> >  }
> >  
> > +static QEMUTimer *repeat_timer;
> > +static bool auto_repeat;
> > +
> > +static void repeat_ps2_queue(void *opaque)
> > +{
> > +    PS2KbdState *s = opaque;
> > +

Hi, Li guang
 
> theoretically, we have to check if the typematic key is in break
> now, if so, we will not do repeat anymore.

You mean key is released?  I checked by '~keycode & 0x80', stop repeat
when key is release.

> don't you think we have a chance that new key can come in during
> waiting?

When the new key (press) comes, repeat_timer will be modified by
qemu_mod_timer(), original repate will be end.
 
> > +    if (auto_repeat) {
> > +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> > +                       muldiv64(get_ticks_per_sec(), s->repeat_period,
> > +                       1000));
> > +        ps2_queue(&s->common, s->repeat_key);
> > +    }
> > +}
> > +
> > +
> >  /*
> >     keycode is expressed as follow:
> >     bit 7    - 0 key pressed, 1 = key released
> > @@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
> >              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
> >          }
> >        }
> > +
> > +    /* only auto-repeat press event */
> > +    auto_repeat = ~keycode & 0x80;

^^^

> >      ps2_queue(&s->common, keycode);
> > +
> > +    if (auto_repeat) {
> > +        s->repeat_key = keycode;
> > +        /* delay a while before first repeat */
> > +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> > +                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
> > +    }
> >  }
> >  
> >  uint32_t ps2_read_data(void *opaque)
> > @@ -213,6 +242,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
> >  
> >  void ps2_write_keyboard(void *opaque, int val)
> >  {
> > +    /* repeat period/delay table from kernel (drivers/input/keyboard/atkbd.c) */
> > +    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 };
> >      PS2KbdState *s = (PS2KbdState *)opaque;
> >  
> >      switch(s->common.write_cmd) {
> > @@ -288,6 +322,11 @@ void ps2_write_keyboard(void *opaque, int val)
> >          s->common.write_cmd = -1;
> >          break;
> >      case KBD_CMD_SET_RATE:
> > +       /* Bit0-4 specifies the repeat rate */
> > +        s->repeat_period = period[val & 0x1f];
> > +       /* Bit5-6 bit specifies the delay time */
> > +        s->repeat_delay = delay[(val & 0x60) >> 5];
> 
> s/(val & 0x60) >> 5/(val & 0x60) >> 5 & 0x4/
                                          ^^ 0x3 ?

    val >> 5 & 0x3

-- 
			Amos.

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[li guang]

在 2013-05-16四的 14:58 +0800，Amos Kong写道：
> On Thu, May 16, 2013 at 01:30:28PM +0800, li guang wrote:
> > 在 2013-05-16四的 12:30 +0800，Amos Kong写道：
> > > Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> > > but ps2 backend doesn't process it and no auto-repeat implementation.
> > > This patch adds support of auto-repeat feature.
> > > 
> > > Guest ps2 driver sets autorepeat to fastest possible in reset,
> > > period: 250ms, delay: 33ms
> > > 
> > > Tested by 'sendkey' monitor command.
> > > 
> > > referenced: http://www.computer-engineering.org/ps2keyboard/
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  hw/input/ps2.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > > 
> > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> > > index 3412079..1cfe055 100644
> > > --- a/hw/input/ps2.c
> > > +++ b/hw/input/ps2.c
> > > @@ -94,6 +94,9 @@ typedef struct {
> > >      int translate;
> > >      int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
> > >      int ledstate;
> > > +    int repeat_period; /* typematic period, ms */
> > > +    int repeat_delay; /* typematic delay, ms */
> > > +    int repeat_key; /* keycode to repeat */
> > >  } PS2KbdState;
> > >  
> > >  typedef struct {
> > > @@ -146,6 +149,22 @@ void ps2_queue(void *opaque, int b)
> > >      s->update_irq(s->update_arg, 1);
> > >  }
> > >  
> > > +static QEMUTimer *repeat_timer;
> > > +static bool auto_repeat;
> > > +
> > > +static void repeat_ps2_queue(void *opaque)
> > > +{
> > > +    PS2KbdState *s = opaque;
> > > +
> 
> Hi, Li guang
>  
> > theoretically, we have to check if the typematic key is in break
> > now, if so, we will not do repeat anymore.
> 
> You mean key is released?  I checked by '~keycode & 0x80', stop repeat
> when key is release.
> 
> > don't you think we have a chance that new key can come in during
> > waiting?
> 
> When the new key (press) comes, repeat_timer will be modified by
> qemu_mod_timer(), original repate will be end.
>  
> > > +    if (auto_repeat) {
> > > +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> > > +                       muldiv64(get_ticks_per_sec(), s->repeat_period,
> > > +                       1000));
> > > +        ps2_queue(&s->common, s->repeat_key);
> > > +    }
> > > +}
> > > +
> > > +
> > >  /*
> > >     keycode is expressed as follow:
> > >     bit 7    - 0 key pressed, 1 = key released
> > > @@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
> > >              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
> > >          }
> > >        }
> > > +
> > > +    /* only auto-repeat press event */
> > > +    auto_repeat = ~keycode & 0x80;
> 
> ^^^

case:

1. new key pressed
2. enter ps2_put_keycode
3. previous timer timeout
4. enter repeat_ps2_queue
5. put previous keycode in queue
6. back to ps2_put_keycode
7. check auto_repeat

so, an obsolete key comes up.
can timer interrupt ps2_put_keycode?


> 
> > >      ps2_queue(&s->common, keycode);
> > > +
> > > +    if (auto_repeat) {
> > > +        s->repeat_key = keycode;
> > > +        /* delay a while before first repeat */
> > > +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> > > +                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
> > > +    }
> > >  }
> > >  
> > >  uint32_t ps2_read_data(void *opaque)
> > > @@ -213,6 +242,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
> > >  
> > >  void ps2_write_keyboard(void *opaque, int val)
> > >  {
> > > +    /* repeat period/delay table from kernel (drivers/input/keyboard/atkbd.c) */
> > > +    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 };
> > >      PS2KbdState *s = (PS2KbdState *)opaque;
> > >  
> > >      switch(s->common.write_cmd) {
> > > @@ -288,6 +322,11 @@ void ps2_write_keyboard(void *opaque, int val)
> > >          s->common.write_cmd = -1;
> > >          break;
> > >      case KBD_CMD_SET_RATE:
> > > +       /* Bit0-4 specifies the repeat rate */
> > > +        s->repeat_period = period[val & 0x1f];
> > > +       /* Bit5-6 bit specifies the delay time */
> > > +        s->repeat_delay = delay[(val & 0x60) >> 5];
> > 
> > s/(val & 0x60) >> 5/(val & 0x60) >> 5 & 0x4/
>                                           ^^ 0x3 ?
> 
>     val >> 5 & 0x3

Oh, yes should be '& 0x3'

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Amos Kong]

On Thu, May 16, 2013 at 07:50:35AM +0100, Peter Maydell wrote:
> On 16 May 2013 05:30, Amos Kong <akong@redhat.com> wrote:
> > Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> > but ps2 backend doesn't process it and no auto-repeat implementation.
> > This patch adds support of auto-repeat feature.
> 
> > diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> > index 3412079..1cfe055 100644
> > --- a/hw/input/ps2.c
> > +++ b/hw/input/ps2.c
> > @@ -94,6 +94,9 @@ typedef struct {
> >      int translate;
> >      int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
> >      int ledstate;
> > +    int repeat_period; /* typematic period, ms */
> > +    int repeat_delay; /* typematic delay, ms */
> > +    int repeat_key; /* keycode to repeat */
> >  } PS2KbdState;
> >
> >  typedef struct {
> > @@ -146,6 +149,22 @@ void ps2_queue(void *opaque, int b)
> >      s->update_irq(s->update_arg, 1);
> >  }
> >
> > +static QEMUTimer *repeat_timer;
> > +static bool auto_repeat;
> 
> These shouldn't be static -- what would happen
> on a system with two ps2 keyboard models in it?

I will move them to PS2KbdState.


For the migrate issue mentioned by jason, the keyboard state
(repeat_period/repeat_delay/etc) are configured by guest, it
should be saved to vmstate and migrated to dest vm.

If we want the unfinished repeat continue, repeat-timer also
should be migrated, but another key_timer in ui/input.c for
send-key could not be migrated, it means the release event
will be lost.

Do we need to migrate the keyboard state?
 
> You need to reset your qemu_timer in the ps2 reset
> handler, as well; otherwise it could go off unexpectedly
> after a reset. (Though perhaps not if we're simulating
> a human with their finger held down on the key...)

Ok
 
> thanks
> -- PMM

-- 
			Amos.

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Lei Li]

On 05/16/2013 12:30 PM, Amos Kong wrote:
> Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> but ps2 backend doesn't process it and no auto-repeat implementation.
> This patch adds support of auto-repeat feature.
>
> Guest ps2 driver sets autorepeat to fastest possible in reset,
> period: 250ms, delay: 33ms
>
> Tested by 'sendkey' monitor command.
>
> referenced: http://www.computer-engineering.org/ps2keyboard/
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>   hw/input/ps2.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
>
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 3412079..1cfe055 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -94,6 +94,9 @@ typedef struct {
>       int translate;
>       int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
>       int ledstate;
> +    int repeat_period; /* typematic period, ms */
> +    int repeat_delay; /* typematic delay, ms */
> +    int repeat_key; /* keycode to repeat */
>   } PS2KbdState;
>
>   typedef struct {
> @@ -146,6 +149,22 @@ void ps2_queue(void *opaque, int b)
>       s->update_irq(s->update_arg, 1);
>   }
>
> +static QEMUTimer *repeat_timer;
> +static bool auto_repeat;
> +
> +static void repeat_ps2_queue(void *opaque)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    if (auto_repeat) {
> +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                       muldiv64(get_ticks_per_sec(), s->repeat_period,
> +                       1000));
> +        ps2_queue(&s->common, s->repeat_key);
> +    }
> +}
> +
> +
>   /*
>      keycode is expressed as follow:
>      bit 7    - 0 key pressed, 1 = key released
> @@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
>               keycode = ps2_raw_keycode_set3[keycode & 0x7f];
>           }
>         }
> +
> +    /* only auto-repeat press event */
> +    auto_repeat = ~keycode & 0x80;

Does this check allow to distinguish the difference between auto-repeat and
actual repeated entry by the user?
  

>       ps2_queue(&s->common, keycode);
> +
> +    if (auto_repeat) {
> +        s->repeat_key = keycode;
> +        /* delay a while before first repeat */
> +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
> +    }
>   }
>
>   uint32_t ps2_read_data(void *opaque)
> @@ -213,6 +242,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
>
>   void ps2_write_keyboard(void *opaque, int val)
>   {
> +    /* repeat period/delay table from kernel (drivers/input/keyboard/atkbd.c) */
> +    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 };
>       PS2KbdState *s = (PS2KbdState *)opaque;
>
>       switch(s->common.write_cmd) {
> @@ -288,6 +322,11 @@ void ps2_write_keyboard(void *opaque, int val)
>           s->common.write_cmd = -1;
>           break;
>       case KBD_CMD_SET_RATE:
> +       /* Bit0-4 specifies the repeat rate */
> +        s->repeat_period = period[val & 0x1f];
> +       /* Bit5-6 bit specifies the delay time */
> +        s->repeat_delay = delay[(val & 0x60) >> 5];
> +
>           ps2_queue(&s->common, KBD_REPLY_ACK);
>           s->common.write_cmd = -1;
>           break;
> @@ -536,6 +575,8 @@ static void ps2_kbd_reset(void *opaque)
>       s->scan_enabled = 0;
>       s->translate = 0;
>       s->scancode_set = 0;
> +    s->repeat_period = 92;
> +    s->repeat_delay = 500;
>   }
>
>   static void ps2_mouse_reset(void *opaque)
> @@ -660,6 +701,7 @@ void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
>       vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
>       qemu_add_kbd_event_handler(ps2_put_keycode, s);
>       qemu_register_reset(ps2_kbd_reset, s);
> +    repeat_timer = qemu_new_timer_ns(vm_clock, repeat_ps2_queue, s);
>       return s;
>   }
>


-- 
Lei

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Amos Kong]

On Thu, May 16, 2013 at 03:13:10PM +0800, li guang wrote:
> 在 2013-05-16四的 14:58 +0800，Amos Kong写道：
> > On Thu, May 16, 2013 at 01:30:28PM +0800, li guang wrote:
> > > 在 2013-05-16四的 12:30 +0800，Amos Kong写道：
> > > > Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> > > > but ps2 backend doesn't process it and no auto-repeat implementation.
> > > > This patch adds support of auto-repeat feature.
> > > > 
> > > > Guest ps2 driver sets autorepeat to fastest possible in reset,
> > > > period: 250ms, delay: 33ms
> > > > 
> > > > Tested by 'sendkey' monitor command.
> > > > 
> > > > referenced: http://www.computer-engineering.org/ps2keyboard/
> > > > 
> > > > Signed-off-by: Amos Kong <akong@redhat.com>

> > Hi, Li guang
> >  
> > > theoretically, we have to check if the typematic key is in break
> > > now, if so, we will not do repeat anymore.
> > 
> > You mean key is released?  I checked by '~keycode & 0x80', stop repeat
> > when key is release.
> > 
> > > don't you think we have a chance that new key can come in during
> > > waiting?
> > 
> > When the new key (press) comes, repeat_timer will be modified by
> > qemu_mod_timer(), original repate will be end.
> >  
> > > > +    if (auto_repeat) {
> > > > +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> > > > +                       muldiv64(get_ticks_per_sec(), s->repeat_period,
> > > > +                       1000));
> > > > +        ps2_queue(&s->common, s->repeat_key);
> > > > +    }
> > > > +}
> > > > +
> > > > +
> > > >  /*
> > > >     keycode is expressed as follow:
> > > >     bit 7    - 0 key pressed, 1 = key released
> > > > @@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
> > > >              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
> > > >          }
> > > >        }
> > > > +
> > > > +    /* only auto-repeat press event */
> > > > +    auto_repeat = ~keycode & 0x80;
> > 
> > ^^^
> 
> case:
> 
> 1. new key pressed
> 2. enter ps2_put_keycode
> 3. previous timer timeout

I guess it's repeat_timer

> 4. enter repeat_ps2_queue
> 5. put previous keycode in queue
> 6. back to ps2_put_keycode

back? repeat_ps_queue() is called in background.

> 7. check auto_repeat
> 
> so, an obsolete key comes up.
> can timer interrupt ps2_put_keycode?
 
no.

-- 
			Amos.

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Amos Kong]

On Thu, May 16, 2013 at 03:23:21PM +0800, Lei Li wrote:
> On 05/16/2013 12:30 PM, Amos Kong wrote:
> >Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> >but ps2 backend doesn't process it and no auto-repeat implementation.
> >This patch adds support of auto-repeat feature.
> >
> >Guest ps2 driver sets autorepeat to fastest possible in reset,
> >period: 250ms, delay: 33ms
> >
> >Tested by 'sendkey' monitor command.
> >
> >referenced: http://www.computer-engineering.org/ps2keyboard/
> >
> >Signed-off-by: Amos Kong <akong@redhat.com>


> >  /*
> >     keycode is expressed as follow:
> >     bit 7    - 0 key pressed, 1 = key released
> >@@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
> >              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
> >          }
> >        }
> >+
> >+    /* only auto-repeat press event */
> >+    auto_repeat = ~keycode & 0x80;

Hi Lei,
 
> Does this check allow to distinguish the difference between auto-repeat and
> actual repeated entry by the user?

Actual repeat by user:
  press event
  release event
  press event
  release event
  press event
  release event

Auto-repeat example:
  press event
  press event
  press event
  release event
 
so here we check if it's a press event, only set repeat_timer for
press event. When we get release event, we just stop repeat action.


> >      ps2_queue(&s->common, keycode);
> >+
> >+    if (auto_repeat) {
> >+        s->repeat_key = keycode;
> >+        /* delay a while before first repeat */
> >+        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> >+                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
> >+    }
> >  }

-- 
			Amos.

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Lei Li]

On 05/16/2013 03:35 PM, Amos Kong wrote:
> On Thu, May 16, 2013 at 03:23:21PM +0800, Lei Li wrote:
>> On 05/16/2013 12:30 PM, Amos Kong wrote:
>>> Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
>>> but ps2 backend doesn't process it and no auto-repeat implementation.
>>> This patch adds support of auto-repeat feature.
>>>
>>> Guest ps2 driver sets autorepeat to fastest possible in reset,
>>> period: 250ms, delay: 33ms
>>>
>>> Tested by 'sendkey' monitor command.
>>>
>>> referenced: http://www.computer-engineering.org/ps2keyboard/
>>>
>>> Signed-off-by: Amos Kong <akong@redhat.com>
>
>>>   /*
>>>      keycode is expressed as follow:
>>>      bit 7    - 0 key pressed, 1 = key released
>>> @@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
>>>               keycode = ps2_raw_keycode_set3[keycode & 0x7f];
>>>           }
>>>         }
>>> +
>>> +    /* only auto-repeat press event */
>>> +    auto_repeat = ~keycode & 0x80;
> Hi Lei,
>
>> Does this check allow to distinguish the difference between auto-repeat and
>> actual repeated entry by the user?
> Actual repeat by user:
>    press event
>    release event
>    press event
>    release event
>    press event
>    release event
>
> Auto-repeat example:
>    press event
>    press event
>    press event
>    release event

On what platform?

AFAIK, the Auto-repeat event is like below on some GTK-based
||||||||||||environments,||||||||||||

keydown
keypress
keyup
keydown
keypress
keyup|||||||||||||
...
as reference link:

https://developer.mozilla.org/zh-CN/docs/DOM/KeyboardEvent

And on Xwindows:

keypress
keyrelease
keypress
keyrelease
...
as reference link:

http://www.ypass.net/blog/2009/06/detecting-xlibs-keyboard-auto-repeat-functionality-and-how-to-fix-it/

This would cause it's hard to distinguish them. But looks like the links above is
a little out of time, and I am not sure if the auto-repeat behaviour on such platforms
has been changed. :)
|||||||||||||

>
> so here we check if it's a press event, only set repeat_timer for
> press event. When we get release event, we just stop repeat action.
>
>
>>>       ps2_queue(&s->common, keycode);
>>> +
>>> +    if (auto_repeat) {
>>> +        s->repeat_key = keycode;
>>> +        /* delay a while before first repeat */
>>> +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
>>> +                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
>>> +    }
>>>   }


-- 
Lei

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Paolo Bonzini]

Il 16/05/2013 06:30, Amos Kong ha scritto:
> Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> but ps2 backend doesn't process it and no auto-repeat implementation.
> This patch adds support of auto-repeat feature.
> 
> Guest ps2 driver sets autorepeat to fastest possible in reset,
> period: 250ms, delay: 33ms
> 
> Tested by 'sendkey' monitor command.
> 
> referenced: http://www.computer-engineering.org/ps2keyboard/
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  hw/input/ps2.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 3412079..1cfe055 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -94,6 +94,9 @@ typedef struct {
>      int translate;
>      int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
>      int ledstate;
> +    int repeat_period; /* typematic period, ms */
> +    int repeat_delay; /* typematic delay, ms */

These have to be migrated in a subsection.

> +    int repeat_key; /* keycode to repeat */
>  } PS2KbdState;
>  
>  typedef struct {
> @@ -146,6 +149,22 @@ void ps2_queue(void *opaque, int b)
>      s->update_irq(s->update_arg, 1);
>  }
>  
> +static QEMUTimer *repeat_timer;

In theory, this timer and repeat_key also need to be migrated.  But
perhaps not migrating anything is fine.  Migration will then stop the
autorepeat, which is not a particularly bad thing and may even avoid
stuck keys.  However, please add a comment, and move the timer into
Ps2KbdState instead of having a global.

> +static bool auto_repeat;

See below about removing this other global.

> +static void repeat_ps2_queue(void *opaque)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    if (auto_repeat) {
> +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                       muldiv64(get_ticks_per_sec(), s->repeat_period,
> +                       1000));
> +        ps2_queue(&s->common, s->repeat_key);
> +    }
> +}
> +
> +
>  /*
>     keycode is expressed as follow:
>     bit 7    - 0 key pressed, 1 = key released
> @@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
>              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
>          }
>        }
> +
> +    /* only auto-repeat press event */
> +    auto_repeat = ~keycode & 0x80;
>      ps2_queue(&s->common, keycode);
> +
> +    if (auto_repeat) {
> +        s->repeat_key = keycode;
> +        /* delay a while before first repeat */
> +        qemu_mod_timer(repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
> +    }

Please del_timer here so you can remove the if above.

Paolo

>  }
>  
>  uint32_t ps2_read_data(void *opaque)
> @@ -213,6 +242,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
>  
>  void ps2_write_keyboard(void *opaque, int val)
>  {
> +    /* repeat period/delay table from kernel (drivers/input/keyboard/atkbd.c) */
> +    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 };
>      PS2KbdState *s = (PS2KbdState *)opaque;
>  
>      switch(s->common.write_cmd) {
> @@ -288,6 +322,11 @@ void ps2_write_keyboard(void *opaque, int val)
>          s->common.write_cmd = -1;
>          break;
>      case KBD_CMD_SET_RATE:
> +       /* Bit0-4 specifies the repeat rate */
> +        s->repeat_period = period[val & 0x1f];
> +       /* Bit5-6 bit specifies the delay time */
> +        s->repeat_delay = delay[(val & 0x60) >> 5];
> +
>          ps2_queue(&s->common, KBD_REPLY_ACK);
>          s->common.write_cmd = -1;
>          break;
> @@ -536,6 +575,8 @@ static void ps2_kbd_reset(void *opaque)
>      s->scan_enabled = 0;
>      s->translate = 0;
>      s->scancode_set = 0;
> +    s->repeat_period = 92;
> +    s->repeat_delay = 500;
>  }
>  
>  static void ps2_mouse_reset(void *opaque)
> @@ -660,6 +701,7 @@ void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg)
>      vmstate_register(NULL, 0, &vmstate_ps2_keyboard, s);
>      qemu_add_kbd_event_handler(ps2_put_keycode, s);
>      qemu_register_reset(ps2_kbd_reset, s);
> +    repeat_timer = qemu_new_timer_ns(vm_clock, repeat_ps2_queue, s);
>      return s;
>  }
>  
> 

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Paolo Bonzini]

Il 16/05/2013 08:58, Amos Kong ha scritto:
>> > theoretically, we have to check if the typematic key is in break
>> > now, if so, we will not do repeat anymore.
> You mean key is released?  I checked by '~keycode & 0x80', stop repeat
> when key is release.
> 

BTW, !(keycode & 0x80) is more readable.

Paolo

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Paolo Bonzini]

Il 16/05/2013 22:37, Amos Kong ha scritto:
> On Thu, May 16, 2013 at 05:11:59PM +0800, Lei Li wrote:
>> On 05/16/2013 03:35 PM, Amos Kong wrote:
>>> On Thu, May 16, 2013 at 03:23:21PM +0800, Lei Li wrote:
>>>> On 05/16/2013 12:30 PM, Amos Kong wrote:
>>>>> Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
>>>>> but ps2 backend doesn't process it and no auto-repeat implementation.
>>>>> This patch adds support of auto-repeat feature.
>>>>>
>>>>> Guest ps2 driver sets autorepeat to fastest possible in reset,
>>>>> period: 250ms, delay: 33ms
>>>>>
>>>>> Tested by 'sendkey' monitor command.
>>>>>
>>>>> referenced: http://www.computer-engineering.org/ps2keyboard/
>>>>>
>>>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>>
>>>>>  /*
>>>>>     keycode is expressed as follow:
>>>>>     bit 7    - 0 key pressed, 1 = key released
>>>>> @@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
>>>>>              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
>>>>>          }
>>>>>        }
>>>>> +
>>>>> +    /* only auto-repeat press event */
>>>>> +    auto_repeat = ~keycode & 0x80;
>>> Hi Lei,
>>>
>>>> Does this check allow to distinguish the difference between auto-repeat and
>>>> actual repeated entry by the user?
>>> Actual repeat by user:
>>>   press event
>>>   release event
>>>   press event
>>>   release event
>>>   press event
>>>   release event
>>>
>>> Auto-repeat example:
>>>   press event
>>>   press event
>>>   press event
>>>   release event
> 
> Hi Lei,
>  
>> On what platform?
> 
> 
> Fedora 18 @ thinkpad t430s
> 
> [root@t430s amos]# showkey  (hold 'a')
> akeycode  30 press
> aaaaaaaaaaakeycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> keycode  30 press
> aakeycode  30 press
> keycode  30 press
> keycode  30 release   <----(one release event in the end)
> 
> 
> Qemu VM (rhel6, using vnc/ SDL) can get same result.
>  
>> AFAIK, the Auto-repeat event is like below on some GTK-based
>> ||||||||||||environments,||||||||||||
>>
>> keydown
>> keypress
>> keyup
>> keydown
>> keypress
>> keyup|||||||||||||
>> ...
>> as reference link:
>>
>> https://developer.mozilla.org/zh-CN/docs/DOM/KeyboardEvent
> 
> ===== Auto-repeat handling  (it's also mentioned in mozilla page)
> 
> When a key is pressed and held down, it begins to auto-repeat. This
> results in a sequence of events similar to the following being
> dispatched:
> 
> keydown
> keypress
> keydown
> keypress
> <<repeating until the user releases the key>>
> keyup  <----(only one up event in the end)
>  
>> And on Xwindows:
>>
>> keypress
>> keyrelease
>> keypress
>> keyrelease
>> ...
>> as reference link:
>>
>> http://www.ypass.net/blog/2009/06/detecting-xlibs-keyboard-auto-repeat-functionality-and-how-to-fix-it/
> 
> """Just what we’d expect, a bunch of KeyPress Events and one KeyRelease
> event. But that’s not how it works in X."""

...  In XWindows, you get a KeyRelease for every KeyPress Event. In X,
it looks something like this:

PRPRPRPRPRPRPRPR

Can you test your patch with all of VNC, SDL and GTK+?

Paolo

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Peter Maydell]

On 16 May 2013 16:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
> ...  In XWindows, you get a KeyRelease for every KeyPress Event. In X,
> it looks something like this:
>
> PRPRPRPRPRPRPRPR

Shouldn't we be abstracting this platform difference
out in the ui layer, rather than having to deal with it
in the ps2 device model? That is, we should define what
our key-repeat model is for the QEMU keyboard-event-handler
API, and then make sure all our UI frontends (gtk, sdl,
cocoa, etc) do what we require...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Paolo Bonzini]

Il 16/05/2013 17:17, Peter Maydell ha scritto:
> On 16 May 2013 16:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> ...  In XWindows, you get a KeyRelease for every KeyPress Event. In X,
>> it looks something like this:
>>
>> PRPRPRPRPRPRPRPR
> 
> Shouldn't we be abstracting this platform difference
> out in the ui layer, rather than having to deal with it
> in the ps2 device model? That is, we should define what
> our key-repeat model is for the QEMU keyboard-event-handler
> API, and then make sure all our UI frontends (gtk, sdl,
> cocoa, etc) do what we require...

Yes, I am asking Amos to check which of our frontends comply.

It needs to be checked in the host, because Linux guests emulate
autorepeat anyway.  Or you can test with FreeDOS.

Paolo

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Amos Kong]

On Thu, May 16, 2013 at 05:11:59PM +0800, Lei Li wrote:
> On 05/16/2013 03:35 PM, Amos Kong wrote:
> >On Thu, May 16, 2013 at 03:23:21PM +0800, Lei Li wrote:
> >>On 05/16/2013 12:30 PM, Amos Kong wrote:
> >>>Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE,
> >>>but ps2 backend doesn't process it and no auto-repeat implementation.
> >>>This patch adds support of auto-repeat feature.
> >>>
> >>>Guest ps2 driver sets autorepeat to fastest possible in reset,
> >>>period: 250ms, delay: 33ms
> >>>
> >>>Tested by 'sendkey' monitor command.
> >>>
> >>>referenced: http://www.computer-engineering.org/ps2keyboard/
> >>>
> >>>Signed-off-by: Amos Kong <akong@redhat.com>
> >
> >>>  /*
> >>>     keycode is expressed as follow:
> >>>     bit 7    - 0 key pressed, 1 = key released
> >>>@@ -167,7 +186,17 @@ static void ps2_put_keycode(void *opaque, int keycode)
> >>>              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
> >>>          }
> >>>        }
> >>>+
> >>>+    /* only auto-repeat press event */
> >>>+    auto_repeat = ~keycode & 0x80;
> >Hi Lei,
> >
> >>Does this check allow to distinguish the difference between auto-repeat and
> >>actual repeated entry by the user?
> >Actual repeat by user:
> >   press event
> >   release event
> >   press event
> >   release event
> >   press event
> >   release event
> >
> >Auto-repeat example:
> >   press event
> >   press event
> >   press event
> >   release event

Hi Lei,
 
> On what platform?


Fedora 18 @ thinkpad t430s

[root@t430s amos]# showkey  (hold 'a')
akeycode  30 press
aaaaaaaaaaakeycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
keycode  30 press
aakeycode  30 press
keycode  30 press
keycode  30 release   <----(one release event in the end)


Qemu VM (rhel6, using vnc/ SDL) can get same result.
 
> AFAIK, the Auto-repeat event is like below on some GTK-based
> ||||||||||||environments,||||||||||||
> 
> keydown
> keypress
> keyup
> keydown
> keypress
> keyup|||||||||||||
> ...
> as reference link:
> 
> https://developer.mozilla.org/zh-CN/docs/DOM/KeyboardEvent

===== Auto-repeat handling  (it's also mentioned in mozilla page)

When a key is pressed and held down, it begins to auto-repeat. This
results in a sequence of events similar to the following being
dispatched:

keydown
keypress
keydown
keypress
<<repeating until the user releases the key>>
keyup  <----(only one up event in the end)
 
> And on Xwindows:
> 
> keypress
> keyrelease
> keypress
> keyrelease
> ...
> as reference link:
> 
> http://www.ypass.net/blog/2009/06/detecting-xlibs-keyboard-auto-repeat-functionality-and-how-to-fix-it/

"""Just what we’d expect, a bunch of KeyPress Events and one KeyRelease
event. But that’s not how it works in X."""

So pppppppR is expected.



From: http://www.computer-engineering.org/ps2keyboard/

When you press and hold down a key, that key becomes typematic, which
means the keyboard will _keep_ sending that key's _make code_ until the
key is released or another key is pressed.

(repeatedly send the make code, only one break code in the end)


> This would cause it's hard to distinguish them. But looks like the links above is
> a little out of time, and I am not sure if the auto-repeat behaviour on such platforms
> has been changed. :)
> |||||||||||||

Both of them works, but the repeated release events are redundant (no risk).


-- 
			Amos.

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Amos Kong]

On Thu, May 16, 2013 at 05:20:37PM +0200, Paolo Bonzini wrote:
> Il 16/05/2013 17:17, Peter Maydell ha scritto:
> > On 16 May 2013 16:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> ...  In XWindows, you get a KeyRelease for every KeyPress Event. In X,
> >> it looks something like this:
> >>
> >> PRPRPRPRPRPRPRPR
> > 
> > Shouldn't we be abstracting this platform difference
> > out in the ui layer, rather than having to deal with it
> > in the ps2 device model? That is, we should define what
> > our key-repeat model is for the QEMU keyboard-event-handler
> > API, and then make sure all our UI frontends (gtk, sdl,
> > cocoa, etc) do what we require...
> 
> Yes, I am asking Amos to check which of our frontends comply.
> 
> It needs to be checked in the host, because Linux guests emulate
> autorepeat anyway.  Or you can test with FreeDOS.

Please correct me if something is wrong, thanks.

When we use VNC/SPICE/SDL, vm Window will captured the key events,
then qemu process the events and transfer to guest through emulated PS2
device.

When we hold the key in keyboard of host, real keyboard or host OS will
do auto-repeat. vm Window will transfer repeated events to guest.
In this case, it seems the auto-repeat of emulated PS2 device doesn't
needed.

But when keyboard/host os doesn't support auto-repeat, or we directly
send event from monitor by 'sendkey'. held key could not be repeated
without auto-repeat support of emulated PS2 device.


When I use (Lenovo t430s & Fedoar 18 host), when I hold the real key,
qemu will get PPPPPPPR style events queue from host.

Guest (RHEL6/Win7/WinXp) & (SDL & VNC & SPICE) works well.


Others:
1. I read the keyboard driver in linux kernel, soft_auto-repeat was
   implemented in linux ps2 driver is PPPPPPPR style.

2. PPPPPPPPR style is described in freescale hardware manual:
   http://www.freescale.com/files/microcontrollers/doc/ref_manual/DRM014.pdf
   """
     1.5.9  PS/2 Scan Codes
     
     Make code or break code is sent when any key is pressed or released.
     While a key is pressed, its _make code_ is sent out repeatedly and the
     rate depends on the typematic repeat value.
   """

I will update patches to fix other problem, keep using PPPPPPPR style.

-- 
			Amos.

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Paolo Bonzini]

Il 21/05/2013 10:33, Amos Kong ha scritto:
> On Thu, May 16, 2013 at 05:20:37PM +0200, Paolo Bonzini wrote:
>> Il 16/05/2013 17:17, Peter Maydell ha scritto:
>>> On 16 May 2013 16:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> ...  In XWindows, you get a KeyRelease for every KeyPress Event. In X,
>>>> it looks something like this:
>>>>
>>>> PRPRPRPRPRPRPRPR
>>>
>>> Shouldn't we be abstracting this platform difference
>>> out in the ui layer, rather than having to deal with it
>>> in the ps2 device model? That is, we should define what
>>> our key-repeat model is for the QEMU keyboard-event-handler
>>> API, and then make sure all our UI frontends (gtk, sdl,
>>> cocoa, etc) do what we require...
>>
>> Yes, I am asking Amos to check which of our frontends comply.
>>
>> It needs to be checked in the host, because Linux guests emulate
>> autorepeat anyway.  Or you can test with FreeDOS.
> 
> Please correct me if something is wrong, thanks.
> 
> When we use VNC/SPICE/SDL, vm Window will captured the key events,
> then qemu process the events and transfer to guest through emulated PS2
> device.
> 
> When we hold the key in keyboard of host, real keyboard or host OS will
> do auto-repeat. vm Window will transfer repeated events to guest.
> In this case, it seems the auto-repeat of emulated PS2 device doesn't
> needed.

If you can make emulated autorepeat work also with VNC/SDL/SPICE, it
would be much better, because then the guest can choose to enable or
disable the autorepeat as desired.

That's why I mentioned testing with FreeDOS, which does no emulation.
You can find DOS programs to change the typematic rate.

Paolo

> But when keyboard/host os doesn't support auto-repeat, or we directly
> send event from monitor by 'sendkey'. held key could not be repeated
> without auto-repeat support of emulated PS2 device.
> 
> 
> When I use (Lenovo t430s & Fedoar 18 host), when I hold the real key,
> qemu will get PPPPPPPR style events queue from host.
> 
> Guest (RHEL6/Win7/WinXp) & (SDL & VNC & SPICE) works well.
> 
> 
> Others:
> 1. I read the keyboard driver in linux kernel, soft_auto-repeat was
>    implemented in linux ps2 driver is PPPPPPPR style.
> 
> 2. PPPPPPPPR style is described in freescale hardware manual:
>    http://www.freescale.com/files/microcontrollers/doc/ref_manual/DRM014.pdf
>    """
>      1.5.9  PS/2 Scan Codes
>      
>      Make code or break code is sent when any key is pressed or released.
>      While a key is pressed, its _make code_ is sent out repeatedly and the
>      rate depends on the typematic repeat value.
>    """
> 
> I will update patches to fix other problem, keep using PPPPPPPR style.
> 

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Amos Kong]

On Tue, May 21, 2013 at 10:38:00AM +0200, Paolo Bonzini wrote:
> Il 21/05/2013 10:33, Amos Kong ha scritto:
> > On Thu, May 16, 2013 at 05:20:37PM +0200, Paolo Bonzini wrote:
> >> Il 16/05/2013 17:17, Peter Maydell ha scritto:
> >>> On 16 May 2013 16:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>>> ...  In XWindows, you get a KeyRelease for every KeyPress Event. In X,
> >>>> it looks something like this:
> >>>>
> >>>> PRPRPRPRPRPRPRPR
> >>>
> >>> Shouldn't we be abstracting this platform difference
> >>> out in the ui layer, rather than having to deal with it
> >>> in the ps2 device model? That is, we should define what
> >>> our key-repeat model is for the QEMU keyboard-event-handler
> >>> API, and then make sure all our UI frontends (gtk, sdl,
> >>> cocoa, etc) do what we require...
> >>
> >> Yes, I am asking Amos to check which of our frontends comply.
> >>
> >> It needs to be checked in the host, because Linux guests emulate
> >> autorepeat anyway.  Or you can test with FreeDOS.
> > 
> > Please correct me if something is wrong, thanks.
> > 
> > When we use VNC/SPICE/SDL, vm Window will captured the key events,
> > then qemu process the events and transfer to guest through emulated PS2
> > device.
> > 
> > When we hold the key in keyboard of host, real keyboard or host OS will
> > do auto-repeat. vm Window will transfer repeated events to guest.
> > In this case, it seems the auto-repeat of emulated PS2 device doesn't
> > needed.
> 
> If you can make emulated autorepeat work also with VNC/SDL/SPICE, it
> would be much better, because then the guest can choose to enable or
> disable the autorepeat as desired.
> 
> That's why I mentioned testing with FreeDOS, which does no emulation.
> You can find DOS programs to change the typematic rate.

Yes, if we don't process events from host, the rate set in guest
doesn't work for SDL/VNC/SPICE/..

I have fixed it by ignoring continual/repated(same keycode) press
events. It works now :)

I just tested by Linux guest (set rate by 'kbdrate -s ..'),
will test with FreeDOS.
 
                Amos.

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Amos Kong]

On Tue, May 21, 2013 at 05:04:30PM +0800, Amos Kong wrote:
> On Tue, May 21, 2013 at 10:38:00AM +0200, Paolo Bonzini wrote:


> > > Please correct me if something is wrong, thanks.
> > > 
> > > When we use VNC/SPICE/SDL, vm Window will captured the key events,
> > > then qemu process the events and transfer to guest through emulated PS2
> > > device.
> > > 
> > > When we hold the key in keyboard of host, real keyboard or host OS will
> > > do auto-repeat. vm Window will transfer repeated events to guest.
> > > In this case, it seems the auto-repeat of emulated PS2 device doesn't
> > > needed.
> > 
> > If you can make emulated autorepeat work also with VNC/SDL/SPICE, it
> > would be much better, because then the guest can choose to enable or
> > disable the autorepeat as desired.
> > 
> > That's why I mentioned testing with FreeDOS, which does no emulation.
> > You can find DOS programs to change the typematic rate.
> 
> Yes, if we don't process events from host, the rate set in guest
> doesn't work for SDL/VNC/SPICE/..
> 
> I have fixed it by ignoring continual/repated(same keycode) press
> events. It works now :)


When I test with linux guest, set rate by 'kbdrate -r ..',
emulated PS2 device can get a keyboard_write (cmd: 0xf3), rate will
be set, auto-repeat rate can be controlled.

I also tested by Win7, set rate by 'mode con rate=2',emulated PS2
device can get a keyboard_write (cmd: 0xf3), rate will
be set, auto-repeat rate can be controlled.

Is it enough to prove the auto-repeat implemented in ps2 is ok?
 
> I just tested by Linux guest (set rate by 'kbdrate -s ..'),
> will test with FreeDOS.

In FreeDOS, I set rate by 'mode con rate=2', got a success prompt.
but emulated PS2 device can't get a keyboard_write (cmd: 0xf3) in init
stage & when I execute mode command. Auto-repeat always use default
rate in qemu-ps2 code.

FreeDOS bug?

Will study it tomorrow.

-- 
			Amos.

Re: [Qemu-devel] [PATCH] ps2: add support of auto-repeat

[Paolo Bonzini]

Il 21/05/2013 11:51, Amos Kong ha scritto:
> On Tue, May 21, 2013 at 05:04:30PM +0800, Amos Kong wrote:
>> On Tue, May 21, 2013 at 10:38:00AM +0200, Paolo Bonzini wrote:
>>>> Please correct me if something is wrong, thanks.
>>>>
>>>> When we use VNC/SPICE/SDL, vm Window will captured the key events,
>>>> then qemu process the events and transfer to guest through emulated PS2
>>>> device.
>>>>
>>>> When we hold the key in keyboard of host, real keyboard or host OS will
>>>> do auto-repeat. vm Window will transfer repeated events to guest.
>>>> In this case, it seems the auto-repeat of emulated PS2 device doesn't
>>>> needed.
>>>
>>> If you can make emulated autorepeat work also with VNC/SDL/SPICE, it
>>> would be much better, because then the guest can choose to enable or
>>> disable the autorepeat as desired.
>>>
>>> That's why I mentioned testing with FreeDOS, which does no emulation.
>>> You can find DOS programs to change the typematic rate.
>>
>> Yes, if we don't process events from host, the rate set in guest
>> doesn't work for SDL/VNC/SPICE/..
>>
>> I have fixed it by ignoring continual/repated(same keycode) press
>> events. It works now :)
> 
> When I test with linux guest, set rate by 'kbdrate -r ..',
> emulated PS2 device can get a keyboard_write (cmd: 0xf3), rate will
> be set, auto-repeat rate can be controlled.
> 
> I also tested by Win7, set rate by 'mode con rate=2',emulated PS2
> device can get a keyboard_write (cmd: 0xf3), rate will
> be set, auto-repeat rate can be controlled.
> 
> Is it enough to prove the auto-repeat implemented in ps2 is ok?
>  
>> I just tested by Linux guest (set rate by 'kbdrate -s ..'),
>> will test with FreeDOS.
> 
> In FreeDOS, I set rate by 'mode con rate=2', got a success prompt.
> but emulated PS2 device can't get a keyboard_write (cmd: 0xf3) in init
> stage & when I execute mode command. Auto-repeat always use default
> rate in qemu-ps2 code.
> 
> FreeDOS bug?

Probably.  Testing Windows is enough, I didn't think of it.

Which backends did you test among SDL/VNC/SPICE/GTK+?

Paolo