Message ID | 1368678651-3561-1-git-send-email-akong@redhat.com |
---|---|
State | New |
Headers | show |
在 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; > } >
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; > } >
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
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
在 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'
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
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; > } >
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.
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)); > >+ } > > }
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)); >>> + } >>> }
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; > } > >
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
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
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
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
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).
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.
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. >
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.
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.
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
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; }
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(+)