Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Anthony Liguori <aliguori@us.ibm.com>]

Amos Kong <akong@redhat.com> writes:

> On Fri, May 31, 2013 at 08:31:17PM +0800, Amos Kong wrote:
>> On Thu, May 30, 2013 at 11:48:46AM -0500, Anthony Liguori wrote:
>> > Amos Kong <akong@redhat.com> writes:
>> > 
>> > > 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. The repeated events
>> > > from host are ignored and re-implements ps2's auto-repeat.
>> > >
>> > > Guest ps2 driver sets autorepeat to fastest possible in reset,
>> > > period: 250ms, delay: 33ms
>> > >
>> > > Tested by 'sendkey' monitor command.
>> > > Tested by Linux & Windows guests with SDL, VNC, SPICE, GTK+
>> > >
>> > > referenced: http://www.computer-engineering.org/ps2keyboard/
>> > >
>> > > Signed-off-by: Amos Kong <akong@redhat.com>
>> > > ---
>> > >  hw/input/ps2.c | 41 +++++++++++++++++++++++++++++++++++++++++
>> > >  1 file changed, 41 insertions(+)
>> > >
>> > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c
>> > > index 3412079..8adbb4a 100644
>> > > --- a/hw/input/ps2.c
>> > > +++ b/hw/input/ps2.c
>> > > @@ -94,6 +94,10 @@ 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 */
>> > > +    QEMUTimer *repeat_timer;
>> > 
>> > This state needs to be migrated, no?  I suspect it can/should be done
>> > via a subsection too.
>> 
>> It sounds only reasonable for 'sendkey' command. We want to repeat one
>> key for 100 times, the key should be continaully repeated in the dest
>> vm until it reaches to 100 times.
>> 
>> For implement this, we should also migrate key_timer in ui/input.c,
>> then it will send a release event to ps2 queue when the key_timer
>> is expired. The bottom patch migrates repeat_timer & repeat_key,
>> where should we save key_timer for migration?
>> 
>> ----
>> 
>> For the VNC/SPICE/SDL/GTK+ windows, qemu gets repeated press events
>> from host. After vm migrates to dest host, if we don't touch the
>> keyboard, there will be no repeat / release event comes from host,
>> 
>> 1) continue to repeat? but qemu no long gets press event from host
>> 2) stop to repeat? but qemu has not got the release event, auto-repeat
>> should continue in real keyboard in this condition.
>> 
>> I prefect 2), because we need to emulate a release event in the
>> prepare stage of migrate.
>> 
>> For implement 2), we should not load/restart the repeat_timer if
>> the migrated key_timer is already expired.
>> 
>> ---
>> 
>> Or just migrate the repeat_rate/repeat_period. We don't have
>> real request of auto-repeat in libvirt.
>> 
>> This might be the best solution as Paolo said ;)
>> http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg02207.html
>  
> ping Anthony, Paolo
>
> I don't think we need to migrate the repeat timer/state.
> only need to migrate related setup (rate/delay) as in last patch.

I'd feel more comfortable migrating all of the state.

I suspect you're neglecting to consider seamless migration mode with
Spice.  In that case, there would be a release event I suspect (perhaps
Gerd can chime in).

Regards,

Anthony Liguori

> If you agree with this, I can merge those to patches together for
> avoiding to break the bisect.
>  
> Amos.
>
>> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
>> index cdb18e6..fdb9912 100644
>> --- a/hw/input/ps2.c
>> +++ b/hw/input/ps2.c
>> @@ -615,7 +615,17 @@ static bool ps2_keyboard_repeatstate_needed(void
>> *opaque)
>>  {
>>      PS2KbdState *s = opaque;
>>  
>> -    return s->repeat_period || s->repeat_delay;
>> +    return s->repeat_period || s->repeat_delay || s->repeat_key ||
>> s->repeat_timer;
>> +}
>> +
>> +static int ps2_kbd_repeatstate_load(QEMUFile *f, void *opaque, int
>> version_id)
>> +{
>> +    PS2KbdState *s = opaque;
>> +    qemu_get_timer(f, s->repeat_timer);
>> +    qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) +
>> +                   muldiv64(get_ticks_per_sec(), s->repeat_period,
>> 1000));
>> +
>> +    return 0;
>>  }
>>  
>>  static bool ps2_keyboard_ledstate_needed(void *opaque)
>> @@ -638,9 +648,12 @@ static const VMStateDescription
>> vmstate_ps2_keyboard_repeatstate = {
>>      .version_id = 3,
>>      .minimum_version_id = 2,
>>      .minimum_version_id_old = 2,
>> +    .load_state_old = ps2_kbd_repeatstate_load,
>>      .fields      = (VMStateField[]) {
>>          VMSTATE_INT32(repeat_period, PS2KbdState),
>>          VMSTATE_INT32(repeat_delay, PS2KbdState),
>> +        VMSTATE_INT32(repeat_key, PS2KbdState),
>> +        VMSTATE_TIMER(repeat_timer, PS2KbdState),
>>          VMSTATE_END_OF_LIST()
>>      }
>>  };
>>