[PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Peter Maydell]

Convert the musicpal key input device to use
qemu_add_kbd_event_handler().  This lets us simplify it because we no
longer need to track whether we're in the middle of a PS/2 multibyte
key sequence.

In the conversion we move the keyboard handler registration from init
to realize, because devices shouldn't disturb the state of the
simulation by doing things like registering input handlers until
they're realized, so that device objects can be introspected
safely.

The behaviour where key-repeat is permitted for the arrow-keys only
is intentional (added in commit 7c6ce4baedfcd0c), so we retain it,
and add a comment to that effect.

This is a migration compatibility break for musicpal.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/musicpal.c | 131 +++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 70 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 9703bfb97fb..e8c1250ab04 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1043,20 +1043,6 @@ static const TypeInfo musicpal_gpio_info = {
 };
 
 /* Keyboard codes & masks */
-#define KEY_RELEASED            0x80
-#define KEY_CODE                0x7f
-
-#define KEYCODE_TAB             0x0f
-#define KEYCODE_ENTER           0x1c
-#define KEYCODE_F               0x21
-#define KEYCODE_M               0x32
-
-#define KEYCODE_EXTENDED        0xe0
-#define KEYCODE_UP              0x48
-#define KEYCODE_DOWN            0x50
-#define KEYCODE_LEFT            0x4b
-#define KEYCODE_RIGHT           0x4d
-
 #define MP_KEY_WHEEL_VOL       (1 << 0)
 #define MP_KEY_WHEEL_VOL_INV   (1 << 1)
 #define MP_KEY_WHEEL_NAV       (1 << 2)
@@ -1074,67 +1060,66 @@ struct musicpal_key_state {
     SysBusDevice parent_obj;
     /*< public >*/
 
-    uint32_t kbd_extended;
     uint32_t pressed_keys;
     qemu_irq out[8];
 };
 
-static void musicpal_key_event(void *opaque, int keycode)
+static void musicpal_key_event(DeviceState *dev, QemuConsole *src,
+                               InputEvent *evt)
 {
-    musicpal_key_state *s = opaque;
+    musicpal_key_state *s = MUSICPAL_KEY(dev);
+    InputKeyEvent *key = evt->u.key.data;
+    int qcode = qemu_input_key_value_to_qcode(key->key);
     uint32_t event = 0;
     int i;
 
-    if (keycode == KEYCODE_EXTENDED) {
-        s->kbd_extended = 1;
-        return;
+    switch (qcode) {
+    case Q_KEY_CODE_UP:
+        event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
+        break;
+
+    case Q_KEY_CODE_DOWN:
+        event = MP_KEY_WHEEL_NAV;
+        break;
+
+    case Q_KEY_CODE_LEFT:
+        event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
+        break;
+
+    case Q_KEY_CODE_RIGHT:
+        event = MP_KEY_WHEEL_VOL;
+        break;
+
+    case Q_KEY_CODE_F:
+        event = MP_KEY_BTN_FAVORITS;
+        break;
+
+    case Q_KEY_CODE_TAB:
+        event = MP_KEY_BTN_VOLUME;
+        break;
+
+    case Q_KEY_CODE_RET:
+        event = MP_KEY_BTN_NAVIGATION;
+        break;
+
+    case Q_KEY_CODE_M:
+        event = MP_KEY_BTN_MENU;
+        break;
     }
 
-    if (s->kbd_extended) {
-        switch (keycode & KEY_CODE) {
-        case KEYCODE_UP:
-            event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
-            break;
-
-        case KEYCODE_DOWN:
-            event = MP_KEY_WHEEL_NAV;
-            break;
-
-        case KEYCODE_LEFT:
-            event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
-            break;
-
-        case KEYCODE_RIGHT:
-            event = MP_KEY_WHEEL_VOL;
-            break;
-        }
-    } else {
-        switch (keycode & KEY_CODE) {
-        case KEYCODE_F:
-            event = MP_KEY_BTN_FAVORITS;
-            break;
-
-        case KEYCODE_TAB:
-            event = MP_KEY_BTN_VOLUME;
-            break;
-
-        case KEYCODE_ENTER:
-            event = MP_KEY_BTN_NAVIGATION;
-            break;
-
-        case KEYCODE_M:
-            event = MP_KEY_BTN_MENU;
-            break;
-        }
-        /* Do not repeat already pressed buttons */
-        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
+    /*
+     * We allow repeated wheel-events when the arrow keys are held down,
+     * but do not repeat already-pressed buttons for the other key inputs.
+     */
+    if (!(event & (MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_VOL))) {
+        if (key->down && (s->pressed_keys & event)) {
             event = 0;
         }
     }
 
     if (event) {
         /* Raise GPIO pin first if repeating a key */
-        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
+        if (key->down && (s->pressed_keys & event)) {
             for (i = 0; i <= 7; i++) {
                 if (event & (1 << i)) {
                     qemu_set_irq(s->out[i], 1);
@@ -1143,17 +1128,15 @@ static void musicpal_key_event(void *opaque, int keycode)
         }
         for (i = 0; i <= 7; i++) {
             if (event & (1 << i)) {
-                qemu_set_irq(s->out[i], !!(keycode & KEY_RELEASED));
+                qemu_set_irq(s->out[i], !key->down);
             }
         }
-        if (keycode & KEY_RELEASED) {
-            s->pressed_keys &= ~event;
-        } else {
+        if (key->down) {
             s->pressed_keys |= event;
+        } else {
+            s->pressed_keys &= ~event;
         }
     }
-
-    s->kbd_extended = 0;
 }
 
 static void musicpal_key_init(Object *obj)
@@ -1162,20 +1145,27 @@ static void musicpal_key_init(Object *obj)
     DeviceState *dev = DEVICE(sbd);
     musicpal_key_state *s = MUSICPAL_KEY(dev);
 
-    s->kbd_extended = 0;
     s->pressed_keys = 0;
 
     qdev_init_gpio_out(dev, s->out, ARRAY_SIZE(s->out));
+}
 
-    qemu_add_kbd_event_handler(musicpal_key_event, s);
+static const QemuInputHandler musicpal_key_handler = {
+    .name = "musicpal_key",
+    .mask = INPUT_EVENT_MASK_KEY,
+    .event = musicpal_key_event,
+};
+
+static void musicpal_key_realize(DeviceState *dev, Error **errp)
+{
+    qemu_input_handler_register(dev, &musicpal_key_handler);
 }
 
 static const VMStateDescription musicpal_key_vmsd = {
     .name = "musicpal_key",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(kbd_extended, musicpal_key_state),
         VMSTATE_UINT32(pressed_keys, musicpal_key_state),
         VMSTATE_END_OF_LIST()
     }
@@ -1186,6 +1176,7 @@ static void musicpal_key_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &musicpal_key_vmsd;
+    dc->realize = musicpal_key_realize;
 }
 
 static const TypeInfo musicpal_key_info = {
-- 
2.34.1

Re: [PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Peter Maydell]

Ping for code review, since we're approaching the end of the
8.2 freeze ?

thanks
-- PMM

On Fri, 3 Nov 2023 at 18:27, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Convert the musicpal key input device to use
> qemu_add_kbd_event_handler().  This lets us simplify it because we no
> longer need to track whether we're in the middle of a PS/2 multibyte
> key sequence.
>
> In the conversion we move the keyboard handler registration from init
> to realize, because devices shouldn't disturb the state of the
> simulation by doing things like registering input handlers until
> they're realized, so that device objects can be introspected
> safely.
>
> The behaviour where key-repeat is permitted for the arrow-keys only
> is intentional (added in commit 7c6ce4baedfcd0c), so we retain it,
> and add a comment to that effect.
>
> This is a migration compatibility break for musicpal.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/musicpal.c | 131 +++++++++++++++++++++-------------------------
>  1 file changed, 61 insertions(+), 70 deletions(-)
>
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 9703bfb97fb..e8c1250ab04 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -1043,20 +1043,6 @@ static const TypeInfo musicpal_gpio_info = {
>  };
>
>  /* Keyboard codes & masks */
> -#define KEY_RELEASED            0x80
> -#define KEY_CODE                0x7f
> -
> -#define KEYCODE_TAB             0x0f
> -#define KEYCODE_ENTER           0x1c
> -#define KEYCODE_F               0x21
> -#define KEYCODE_M               0x32
> -
> -#define KEYCODE_EXTENDED        0xe0
> -#define KEYCODE_UP              0x48
> -#define KEYCODE_DOWN            0x50
> -#define KEYCODE_LEFT            0x4b
> -#define KEYCODE_RIGHT           0x4d
> -
>  #define MP_KEY_WHEEL_VOL       (1 << 0)
>  #define MP_KEY_WHEEL_VOL_INV   (1 << 1)
>  #define MP_KEY_WHEEL_NAV       (1 << 2)
> @@ -1074,67 +1060,66 @@ struct musicpal_key_state {
>      SysBusDevice parent_obj;
>      /*< public >*/
>
> -    uint32_t kbd_extended;
>      uint32_t pressed_keys;
>      qemu_irq out[8];
>  };
>
> -static void musicpal_key_event(void *opaque, int keycode)
> +static void musicpal_key_event(DeviceState *dev, QemuConsole *src,
> +                               InputEvent *evt)
>  {
> -    musicpal_key_state *s = opaque;
> +    musicpal_key_state *s = MUSICPAL_KEY(dev);
> +    InputKeyEvent *key = evt->u.key.data;
> +    int qcode = qemu_input_key_value_to_qcode(key->key);
>      uint32_t event = 0;
>      int i;
>
> -    if (keycode == KEYCODE_EXTENDED) {
> -        s->kbd_extended = 1;
> -        return;
> +    switch (qcode) {
> +    case Q_KEY_CODE_UP:
> +        event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
> +        break;
> +
> +    case Q_KEY_CODE_DOWN:
> +        event = MP_KEY_WHEEL_NAV;
> +        break;
> +
> +    case Q_KEY_CODE_LEFT:
> +        event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
> +        break;
> +
> +    case Q_KEY_CODE_RIGHT:
> +        event = MP_KEY_WHEEL_VOL;
> +        break;
> +
> +    case Q_KEY_CODE_F:
> +        event = MP_KEY_BTN_FAVORITS;
> +        break;
> +
> +    case Q_KEY_CODE_TAB:
> +        event = MP_KEY_BTN_VOLUME;
> +        break;
> +
> +    case Q_KEY_CODE_RET:
> +        event = MP_KEY_BTN_NAVIGATION;
> +        break;
> +
> +    case Q_KEY_CODE_M:
> +        event = MP_KEY_BTN_MENU;
> +        break;
>      }
>
> -    if (s->kbd_extended) {
> -        switch (keycode & KEY_CODE) {
> -        case KEYCODE_UP:
> -            event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
> -            break;
> -
> -        case KEYCODE_DOWN:
> -            event = MP_KEY_WHEEL_NAV;
> -            break;
> -
> -        case KEYCODE_LEFT:
> -            event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
> -            break;
> -
> -        case KEYCODE_RIGHT:
> -            event = MP_KEY_WHEEL_VOL;
> -            break;
> -        }
> -    } else {
> -        switch (keycode & KEY_CODE) {
> -        case KEYCODE_F:
> -            event = MP_KEY_BTN_FAVORITS;
> -            break;
> -
> -        case KEYCODE_TAB:
> -            event = MP_KEY_BTN_VOLUME;
> -            break;
> -
> -        case KEYCODE_ENTER:
> -            event = MP_KEY_BTN_NAVIGATION;
> -            break;
> -
> -        case KEYCODE_M:
> -            event = MP_KEY_BTN_MENU;
> -            break;
> -        }
> -        /* Do not repeat already pressed buttons */
> -        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
> +    /*
> +     * We allow repeated wheel-events when the arrow keys are held down,
> +     * but do not repeat already-pressed buttons for the other key inputs.
> +     */
> +    if (!(event & (MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_VOL))) {
> +        if (key->down && (s->pressed_keys & event)) {
>              event = 0;
>          }
>      }
>
>      if (event) {
>          /* Raise GPIO pin first if repeating a key */
> -        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
> +        if (key->down && (s->pressed_keys & event)) {
>              for (i = 0; i <= 7; i++) {
>                  if (event & (1 << i)) {
>                      qemu_set_irq(s->out[i], 1);
> @@ -1143,17 +1128,15 @@ static void musicpal_key_event(void *opaque, int keycode)
>          }
>          for (i = 0; i <= 7; i++) {
>              if (event & (1 << i)) {
> -                qemu_set_irq(s->out[i], !!(keycode & KEY_RELEASED));
> +                qemu_set_irq(s->out[i], !key->down);
>              }
>          }
> -        if (keycode & KEY_RELEASED) {
> -            s->pressed_keys &= ~event;
> -        } else {
> +        if (key->down) {
>              s->pressed_keys |= event;
> +        } else {
> +            s->pressed_keys &= ~event;
>          }
>      }
> -
> -    s->kbd_extended = 0;
>  }
>
>  static void musicpal_key_init(Object *obj)
> @@ -1162,20 +1145,27 @@ static void musicpal_key_init(Object *obj)
>      DeviceState *dev = DEVICE(sbd);
>      musicpal_key_state *s = MUSICPAL_KEY(dev);
>
> -    s->kbd_extended = 0;
>      s->pressed_keys = 0;
>
>      qdev_init_gpio_out(dev, s->out, ARRAY_SIZE(s->out));
> +}
>
> -    qemu_add_kbd_event_handler(musicpal_key_event, s);
> +static const QemuInputHandler musicpal_key_handler = {
> +    .name = "musicpal_key",
> +    .mask = INPUT_EVENT_MASK_KEY,
> +    .event = musicpal_key_event,
> +};
> +
> +static void musicpal_key_realize(DeviceState *dev, Error **errp)
> +{
> +    qemu_input_handler_register(dev, &musicpal_key_handler);
>  }
>
>  static const VMStateDescription musicpal_key_vmsd = {
>      .name = "musicpal_key",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(kbd_extended, musicpal_key_state),
>          VMSTATE_UINT32(pressed_keys, musicpal_key_state),
>          VMSTATE_END_OF_LIST()
>      }
> @@ -1186,6 +1176,7 @@ static void musicpal_key_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->vmsd = &musicpal_key_vmsd;
> +    dc->realize = musicpal_key_realize;
>  }
>
>  static const TypeInfo musicpal_key_info = {
> --
> 2.34.1
>

Re: [PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Peter Maydell]

Ping^2 for code review?

thanks
-- PMM

On Fri, 15 Dec 2023 at 14:07, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Ping for code review, since we're approaching the end of the
> 8.2 freeze ?
>
> thanks
> -- PMM
>
> On Fri, 3 Nov 2023 at 18:27, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > Convert the musicpal key input device to use
> > qemu_add_kbd_event_handler().  This lets us simplify it because we no
> > longer need to track whether we're in the middle of a PS/2 multibyte
> > key sequence.
> >
> > In the conversion we move the keyboard handler registration from init
> > to realize, because devices shouldn't disturb the state of the
> > simulation by doing things like registering input handlers until
> > they're realized, so that device objects can be introspected
> > safely.
> >
> > The behaviour where key-repeat is permitted for the arrow-keys only
> > is intentional (added in commit 7c6ce4baedfcd0c), so we retain it,
> > and add a comment to that effect.
> >
> > This is a migration compatibility break for musicpal.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/arm/musicpal.c | 131 +++++++++++++++++++++-------------------------
> >  1 file changed, 61 insertions(+), 70 deletions(-)
> >
> > diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> > index 9703bfb97fb..e8c1250ab04 100644
> > --- a/hw/arm/musicpal.c
> > +++ b/hw/arm/musicpal.c
> > @@ -1043,20 +1043,6 @@ static const TypeInfo musicpal_gpio_info = {
> >  };
> >
> >  /* Keyboard codes & masks */
> > -#define KEY_RELEASED            0x80
> > -#define KEY_CODE                0x7f
> > -
> > -#define KEYCODE_TAB             0x0f
> > -#define KEYCODE_ENTER           0x1c
> > -#define KEYCODE_F               0x21
> > -#define KEYCODE_M               0x32
> > -
> > -#define KEYCODE_EXTENDED        0xe0
> > -#define KEYCODE_UP              0x48
> > -#define KEYCODE_DOWN            0x50
> > -#define KEYCODE_LEFT            0x4b
> > -#define KEYCODE_RIGHT           0x4d
> > -
> >  #define MP_KEY_WHEEL_VOL       (1 << 0)
> >  #define MP_KEY_WHEEL_VOL_INV   (1 << 1)
> >  #define MP_KEY_WHEEL_NAV       (1 << 2)
> > @@ -1074,67 +1060,66 @@ struct musicpal_key_state {
> >      SysBusDevice parent_obj;
> >      /*< public >*/
> >
> > -    uint32_t kbd_extended;
> >      uint32_t pressed_keys;
> >      qemu_irq out[8];
> >  };
> >
> > -static void musicpal_key_event(void *opaque, int keycode)
> > +static void musicpal_key_event(DeviceState *dev, QemuConsole *src,
> > +                               InputEvent *evt)
> >  {
> > -    musicpal_key_state *s = opaque;
> > +    musicpal_key_state *s = MUSICPAL_KEY(dev);
> > +    InputKeyEvent *key = evt->u.key.data;
> > +    int qcode = qemu_input_key_value_to_qcode(key->key);
> >      uint32_t event = 0;
> >      int i;
> >
> > -    if (keycode == KEYCODE_EXTENDED) {
> > -        s->kbd_extended = 1;
> > -        return;
> > +    switch (qcode) {
> > +    case Q_KEY_CODE_UP:
> > +        event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
> > +        break;
> > +
> > +    case Q_KEY_CODE_DOWN:
> > +        event = MP_KEY_WHEEL_NAV;
> > +        break;
> > +
> > +    case Q_KEY_CODE_LEFT:
> > +        event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
> > +        break;
> > +
> > +    case Q_KEY_CODE_RIGHT:
> > +        event = MP_KEY_WHEEL_VOL;
> > +        break;
> > +
> > +    case Q_KEY_CODE_F:
> > +        event = MP_KEY_BTN_FAVORITS;
> > +        break;
> > +
> > +    case Q_KEY_CODE_TAB:
> > +        event = MP_KEY_BTN_VOLUME;
> > +        break;
> > +
> > +    case Q_KEY_CODE_RET:
> > +        event = MP_KEY_BTN_NAVIGATION;
> > +        break;
> > +
> > +    case Q_KEY_CODE_M:
> > +        event = MP_KEY_BTN_MENU;
> > +        break;
> >      }
> >
> > -    if (s->kbd_extended) {
> > -        switch (keycode & KEY_CODE) {
> > -        case KEYCODE_UP:
> > -            event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
> > -            break;
> > -
> > -        case KEYCODE_DOWN:
> > -            event = MP_KEY_WHEEL_NAV;
> > -            break;
> > -
> > -        case KEYCODE_LEFT:
> > -            event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
> > -            break;
> > -
> > -        case KEYCODE_RIGHT:
> > -            event = MP_KEY_WHEEL_VOL;
> > -            break;
> > -        }
> > -    } else {
> > -        switch (keycode & KEY_CODE) {
> > -        case KEYCODE_F:
> > -            event = MP_KEY_BTN_FAVORITS;
> > -            break;
> > -
> > -        case KEYCODE_TAB:
> > -            event = MP_KEY_BTN_VOLUME;
> > -            break;
> > -
> > -        case KEYCODE_ENTER:
> > -            event = MP_KEY_BTN_NAVIGATION;
> > -            break;
> > -
> > -        case KEYCODE_M:
> > -            event = MP_KEY_BTN_MENU;
> > -            break;
> > -        }
> > -        /* Do not repeat already pressed buttons */
> > -        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
> > +    /*
> > +     * We allow repeated wheel-events when the arrow keys are held down,
> > +     * but do not repeat already-pressed buttons for the other key inputs.
> > +     */
> > +    if (!(event & (MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_VOL))) {
> > +        if (key->down && (s->pressed_keys & event)) {
> >              event = 0;
> >          }
> >      }
> >
> >      if (event) {
> >          /* Raise GPIO pin first if repeating a key */
> > -        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
> > +        if (key->down && (s->pressed_keys & event)) {
> >              for (i = 0; i <= 7; i++) {
> >                  if (event & (1 << i)) {
> >                      qemu_set_irq(s->out[i], 1);
> > @@ -1143,17 +1128,15 @@ static void musicpal_key_event(void *opaque, int keycode)
> >          }
> >          for (i = 0; i <= 7; i++) {
> >              if (event & (1 << i)) {
> > -                qemu_set_irq(s->out[i], !!(keycode & KEY_RELEASED));
> > +                qemu_set_irq(s->out[i], !key->down);
> >              }
> >          }
> > -        if (keycode & KEY_RELEASED) {
> > -            s->pressed_keys &= ~event;
> > -        } else {
> > +        if (key->down) {
> >              s->pressed_keys |= event;
> > +        } else {
> > +            s->pressed_keys &= ~event;
> >          }
> >      }
> > -
> > -    s->kbd_extended = 0;
> >  }
> >
> >  static void musicpal_key_init(Object *obj)
> > @@ -1162,20 +1145,27 @@ static void musicpal_key_init(Object *obj)
> >      DeviceState *dev = DEVICE(sbd);
> >      musicpal_key_state *s = MUSICPAL_KEY(dev);
> >
> > -    s->kbd_extended = 0;
> >      s->pressed_keys = 0;
> >
> >      qdev_init_gpio_out(dev, s->out, ARRAY_SIZE(s->out));
> > +}
> >
> > -    qemu_add_kbd_event_handler(musicpal_key_event, s);
> > +static const QemuInputHandler musicpal_key_handler = {
> > +    .name = "musicpal_key",
> > +    .mask = INPUT_EVENT_MASK_KEY,
> > +    .event = musicpal_key_event,
> > +};
> > +
> > +static void musicpal_key_realize(DeviceState *dev, Error **errp)
> > +{
> > +    qemu_input_handler_register(dev, &musicpal_key_handler);
> >  }
> >
> >  static const VMStateDescription musicpal_key_vmsd = {
> >      .name = "musicpal_key",
> > -    .version_id = 1,
> > -    .minimum_version_id = 1,
> > +    .version_id = 2,
> > +    .minimum_version_id = 2,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_UINT32(kbd_extended, musicpal_key_state),
> >          VMSTATE_UINT32(pressed_keys, musicpal_key_state),
> >          VMSTATE_END_OF_LIST()
> >      }
> > @@ -1186,6 +1176,7 @@ static void musicpal_key_class_init(ObjectClass *klass, void *data)
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >
> >      dc->vmsd = &musicpal_key_vmsd;
> > +    dc->realize = musicpal_key_realize;
> >  }
> >
> >  static const TypeInfo musicpal_key_info = {
> > --
> > 2.34.1
> >

Re: [PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Philippe Mathieu-Daudé]

Cc'ing Gerd & Marc-André for UI.

On 3/11/23 19:27, Peter Maydell wrote:
> Convert the musicpal key input device to use
> qemu_add_kbd_event_handler().  This lets us simplify it because we no
> longer need to track whether we're in the middle of a PS/2 multibyte
> key sequence.
> 
> In the conversion we move the keyboard handler registration from init
> to realize, because devices shouldn't disturb the state of the
> simulation by doing things like registering input handlers until
> they're realized, so that device objects can be introspected
> safely.
> 
> The behaviour where key-repeat is permitted for the arrow-keys only
> is intentional (added in commit 7c6ce4baedfcd0c), so we retain it,
> and add a comment to that effect.
> 
> This is a migration compatibility break for musicpal.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/arm/musicpal.c | 131 +++++++++++++++++++++-------------------------
>   1 file changed, 61 insertions(+), 70 deletions(-)
> 
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 9703bfb97fb..e8c1250ab04 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -1043,20 +1043,6 @@ static const TypeInfo musicpal_gpio_info = {
>   };
>   
>   /* Keyboard codes & masks */
> -#define KEY_RELEASED            0x80
> -#define KEY_CODE                0x7f
> -
> -#define KEYCODE_TAB             0x0f
> -#define KEYCODE_ENTER           0x1c
> -#define KEYCODE_F               0x21
> -#define KEYCODE_M               0x32
> -
> -#define KEYCODE_EXTENDED        0xe0
> -#define KEYCODE_UP              0x48
> -#define KEYCODE_DOWN            0x50
> -#define KEYCODE_LEFT            0x4b
> -#define KEYCODE_RIGHT           0x4d
> -
>   #define MP_KEY_WHEEL_VOL       (1 << 0)
>   #define MP_KEY_WHEEL_VOL_INV   (1 << 1)
>   #define MP_KEY_WHEEL_NAV       (1 << 2)
> @@ -1074,67 +1060,66 @@ struct musicpal_key_state {
>       SysBusDevice parent_obj;
>       /*< public >*/
>   
> -    uint32_t kbd_extended;
>       uint32_t pressed_keys;
>       qemu_irq out[8];
>   };
>   
> -static void musicpal_key_event(void *opaque, int keycode)
> +static void musicpal_key_event(DeviceState *dev, QemuConsole *src,
> +                               InputEvent *evt)
>   {
> -    musicpal_key_state *s = opaque;
> +    musicpal_key_state *s = MUSICPAL_KEY(dev);
> +    InputKeyEvent *key = evt->u.key.data;
> +    int qcode = qemu_input_key_value_to_qcode(key->key);
>       uint32_t event = 0;
>       int i;
>   
> -    if (keycode == KEYCODE_EXTENDED) {
> -        s->kbd_extended = 1;
> -        return;
> +    switch (qcode) {
> +    case Q_KEY_CODE_UP:
> +        event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
> +        break;
> +
> +    case Q_KEY_CODE_DOWN:
> +        event = MP_KEY_WHEEL_NAV;
> +        break;
> +
> +    case Q_KEY_CODE_LEFT:
> +        event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
> +        break;
> +
> +    case Q_KEY_CODE_RIGHT:
> +        event = MP_KEY_WHEEL_VOL;
> +        break;
> +
> +    case Q_KEY_CODE_F:
> +        event = MP_KEY_BTN_FAVORITS;
> +        break;
> +
> +    case Q_KEY_CODE_TAB:
> +        event = MP_KEY_BTN_VOLUME;
> +        break;
> +
> +    case Q_KEY_CODE_RET:
> +        event = MP_KEY_BTN_NAVIGATION;
> +        break;
> +
> +    case Q_KEY_CODE_M:
> +        event = MP_KEY_BTN_MENU;
> +        break;
>       }
>   
> -    if (s->kbd_extended) {
> -        switch (keycode & KEY_CODE) {
> -        case KEYCODE_UP:
> -            event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
> -            break;
> -
> -        case KEYCODE_DOWN:
> -            event = MP_KEY_WHEEL_NAV;
> -            break;
> -
> -        case KEYCODE_LEFT:
> -            event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
> -            break;
> -
> -        case KEYCODE_RIGHT:
> -            event = MP_KEY_WHEEL_VOL;
> -            break;
> -        }
> -    } else {
> -        switch (keycode & KEY_CODE) {
> -        case KEYCODE_F:
> -            event = MP_KEY_BTN_FAVORITS;
> -            break;
> -
> -        case KEYCODE_TAB:
> -            event = MP_KEY_BTN_VOLUME;
> -            break;
> -
> -        case KEYCODE_ENTER:
> -            event = MP_KEY_BTN_NAVIGATION;
> -            break;
> -
> -        case KEYCODE_M:
> -            event = MP_KEY_BTN_MENU;
> -            break;
> -        }
> -        /* Do not repeat already pressed buttons */
> -        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
> +    /*
> +     * We allow repeated wheel-events when the arrow keys are held down,
> +     * but do not repeat already-pressed buttons for the other key inputs.
> +     */
> +    if (!(event & (MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_VOL))) {
> +        if (key->down && (s->pressed_keys & event)) {
>               event = 0;
>           }
>       }
>   
>       if (event) {
>           /* Raise GPIO pin first if repeating a key */
> -        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
> +        if (key->down && (s->pressed_keys & event)) {
>               for (i = 0; i <= 7; i++) {
>                   if (event & (1 << i)) {
>                       qemu_set_irq(s->out[i], 1);
> @@ -1143,17 +1128,15 @@ static void musicpal_key_event(void *opaque, int keycode)
>           }
>           for (i = 0; i <= 7; i++) {
>               if (event & (1 << i)) {
> -                qemu_set_irq(s->out[i], !!(keycode & KEY_RELEASED));
> +                qemu_set_irq(s->out[i], !key->down);
>               }
>           }
> -        if (keycode & KEY_RELEASED) {
> -            s->pressed_keys &= ~event;
> -        } else {
> +        if (key->down) {
>               s->pressed_keys |= event;
> +        } else {
> +            s->pressed_keys &= ~event;
>           }
>       }
> -
> -    s->kbd_extended = 0;
>   }
>   
>   static void musicpal_key_init(Object *obj)
> @@ -1162,20 +1145,27 @@ static void musicpal_key_init(Object *obj)
>       DeviceState *dev = DEVICE(sbd);
>       musicpal_key_state *s = MUSICPAL_KEY(dev);
>   
> -    s->kbd_extended = 0;
>       s->pressed_keys = 0;
>   
>       qdev_init_gpio_out(dev, s->out, ARRAY_SIZE(s->out));
> +}
>   
> -    qemu_add_kbd_event_handler(musicpal_key_event, s);
> +static const QemuInputHandler musicpal_key_handler = {
> +    .name = "musicpal_key",
> +    .mask = INPUT_EVENT_MASK_KEY,
> +    .event = musicpal_key_event,
> +};
> +
> +static void musicpal_key_realize(DeviceState *dev, Error **errp)
> +{
> +    qemu_input_handler_register(dev, &musicpal_key_handler);
>   }
>   
>   static const VMStateDescription musicpal_key_vmsd = {
>       .name = "musicpal_key",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>       .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(kbd_extended, musicpal_key_state),
>           VMSTATE_UINT32(pressed_keys, musicpal_key_state),
>           VMSTATE_END_OF_LIST()
>       }
> @@ -1186,6 +1176,7 @@ static void musicpal_key_class_init(ObjectClass *klass, void *data)
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       dc->vmsd = &musicpal_key_vmsd;
> +    dc->realize = musicpal_key_realize;
>   }
>   
>   static const TypeInfo musicpal_key_info = {

Re: [PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Convert the musicpal key input device to use
> qemu_add_kbd_event_handler().  This lets us simplify it because we no
> longer need to track whether we're in the middle of a PS/2 multibyte
> key sequence.
>
> In the conversion we move the keyboard handler registration from init
> to realize, because devices shouldn't disturb the state of the
> simulation by doing things like registering input handlers until
> they're realized, so that device objects can be introspected
> safely.
>
> The behaviour where key-repeat is permitted for the arrow-keys only
> is intentional (added in commit 7c6ce4baedfcd0c), so we retain it,
> and add a comment to that effect.

Well the key input all works as intended and looks good to me. I'm a
little disappointed I couldn't get audio working on the musicpal machine
but that is not a problem for this patch.

Tested-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Jan Kiszka]

On 19.01.24 12:24, Alex Bennée wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Convert the musicpal key input device to use
>> qemu_add_kbd_event_handler().  This lets us simplify it because we no
>> longer need to track whether we're in the middle of a PS/2 multibyte
>> key sequence.
>>
>> In the conversion we move the keyboard handler registration from init
>> to realize, because devices shouldn't disturb the state of the
>> simulation by doing things like registering input handlers until
>> they're realized, so that device objects can be introspected
>> safely.
>>
>> The behaviour where key-repeat is permitted for the arrow-keys only
>> is intentional (added in commit 7c6ce4baedfcd0c), so we retain it,
>> and add a comment to that effect.
>
> Well the key input all works as intended and looks good to me. I'm a
> little disappointed I couldn't get audio working on the musicpal machine
> but that is not a problem for this patch.
>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>

Looks good to me as well, all keys still work fine.

No idea what's the issue with sound, though. I think I haven't run the
whole stuff in a decade or so, had to search for all the pieces first of
all again. The webradio service original behind this stopped their
operations, at least for this device, but manually entered favorits
still work on the real device - I still have one, though that is
starting to get some issues as well.

Thanks,
Jan

Re: [PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Alex Bennée]

Jan Kiszka <jan.kiszka@web.de> writes:

> On 19.01.24 12:24, Alex Bennée wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> Convert the musicpal key input device to use
>>> qemu_add_kbd_event_handler().  This lets us simplify it because we no
>>> longer need to track whether we're in the middle of a PS/2 multibyte
>>> key sequence.
<snip>
>>
>> Well the key input all works as intended and looks good to me. I'm a
>> little disappointed I couldn't get audio working on the musicpal machine
>> but that is not a problem for this patch.
>>
>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>
>
> Looks good to me as well, all keys still work fine.
>
> No idea what's the issue with sound, though. I think I haven't run the
> whole stuff in a decade or so, had to search for all the pieces first of
> all again. The webradio service original behind this stopped their
> operations, at least for this device, but manually entered favorits
> still work on the real device - I still have one, though that is
> starting to get some issues as well.

I navigated through the favourites and after pressing some keys it seems
to indicate there was a stream of some sort (or at least a bitrate was
reported ;-).

The main issue I was having with sound was with pipewire - this would
eventually generate a lot of warning messages because input devices are
created but I guess the model wasn't draining the input buffers so
eventually we get:

  qemu: 0x7f1490259500: overrun write:5859188 filled:5842804 + size:940 > max:4194304
  qemu: 0x7f14902680a0: overrun write:5860128 filled:5843744 + size:940 > max:4194304
  qemu: 0x7f1490259500: overrun write:5861068 filled:5844684 + size:940 > max:4194304
  qemu: 0x7f14902680a0: overrun write:5862008 filled:5845624 + size:940 > max:4194304

Is your image just a hacked up version of the original firmware or
something we have source for? I guess there was never a rockbox port for
the device?

>
> Thanks,
> Jan

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Jan Kiszka]

On 22.01.24 10:50, Alex Bennée wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
>
>> On 19.01.24 12:24, Alex Bennée wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> Convert the musicpal key input device to use
>>>> qemu_add_kbd_event_handler().  This lets us simplify it because we no
>>>> longer need to track whether we're in the middle of a PS/2 multibyte
>>>> key sequence.
> <snip>
>>>
>>> Well the key input all works as intended and looks good to me. I'm a
>>> little disappointed I couldn't get audio working on the musicpal machine
>>> but that is not a problem for this patch.
>>>
>>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>
>>
>> Looks good to me as well, all keys still work fine.
>>
>> No idea what's the issue with sound, though. I think I haven't run the
>> whole stuff in a decade or so, had to search for all the pieces first of
>> all again. The webradio service original behind this stopped their
>> operations, at least for this device, but manually entered favorits
>> still work on the real device - I still have one, though that is
>> starting to get some issues as well.
>
> I navigated through the favourites and after pressing some keys it seems
> to indicate there was a stream of some sort (or at least a bitrate was
> reported ;-).
>
> The main issue I was having with sound was with pipewire - this would
> eventually generate a lot of warning messages because input devices are
> created but I guess the model wasn't draining the input buffers so
> eventually we get:
>
>   qemu: 0x7f1490259500: overrun write:5859188 filled:5842804 + size:940 > max:4194304
>   qemu: 0x7f14902680a0: overrun write:5860128 filled:5843744 + size:940 > max:4194304
>   qemu: 0x7f1490259500: overrun write:5861068 filled:5844684 + size:940 > max:4194304
>   qemu: 0x7f14902680a0: overrun write:5862008 filled:5845624 + size:940 > max:4194304
>

I'm getting these here:

pulseaudio: set_source_output_volume() failed
pulseaudio: Reason: Invalid argument
...

> Is your image just a hacked up version of the original firmware or
> something we have source for? I guess there was never a rockbox port for
> the device?
>

It's an original firmware, nothing hacked. I do have some sources here,
but just partial ones: U-Boot, kernel, not the complete userland and
even not all kernel drivers IIRC.

Jan

Re: [PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Alex Bennée]

Jan Kiszka <jan.kiszka@web.de> writes:

> On 22.01.24 10:50, Alex Bennée wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>>
>>> On 19.01.24 12:24, Alex Bennée wrote:
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>
>>>>> Convert the musicpal key input device to use
>>>>> qemu_add_kbd_event_handler().  This lets us simplify it because we no
>>>>> longer need to track whether we're in the middle of a PS/2 multibyte
>>>>> key sequence.
>> <snip>
>>>>
>>>> Well the key input all works as intended and looks good to me. I'm a
>>>> little disappointed I couldn't get audio working on the musicpal machine
>>>> but that is not a problem for this patch.
>>>>
>>>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>>
>>>
>>> Looks good to me as well, all keys still work fine.
>>>
>>> No idea what's the issue with sound, though. I think I haven't run the
>>> whole stuff in a decade or so, had to search for all the pieces first of
>>> all again. The webradio service original behind this stopped their
>>> operations, at least for this device, but manually entered favorits
>>> still work on the real device - I still have one, though that is
>>> starting to get some issues as well.
>>
>> I navigated through the favourites and after pressing some keys it seems
>> to indicate there was a stream of some sort (or at least a bitrate was
>> reported ;-).
>>
>> The main issue I was having with sound was with pipewire - this would
>> eventually generate a lot of warning messages because input devices are
>> created but I guess the model wasn't draining the input buffers so
>> eventually we get:
>>
>>   qemu: 0x7f1490259500: overrun write:5859188 filled:5842804 + size:940 > max:4194304
>>   qemu: 0x7f14902680a0: overrun write:5860128 filled:5843744 + size:940 > max:4194304
>>   qemu: 0x7f1490259500: overrun write:5861068 filled:5844684 + size:940 > max:4194304
>>   qemu: 0x7f14902680a0: overrun write:5862008 filled:5845624 + size:940 > max:4194304
>>
>
> I'm getting these here:
>
> pulseaudio: set_source_output_volume() failed
> pulseaudio: Reason: Invalid argument
> ...

Yeah I get that with -M musicpal,audiodev=system ... -audiodev pa,id=system

>
>> Is your image just a hacked up version of the original firmware or
>> something we have source for? I guess there was never a rockbox port for
>> the device?
>>
>
> It's an original firmware, nothing hacked. I do have some sources here,
> but just partial ones: U-Boot, kernel, not the complete userland and
> even not all kernel drivers IIRC.

With -nic user,hostfwd=tcp::8888-:80 I'm able to attempt to connect to
the webpage bit the documented admin:admin doesn't work. I can see
/etc/passwd has something set for admin.

I did find:

  https://musicpal.mcproductions.nl/

which has a bunch of firmwares but the mpimage.bin is raw data (not a
jffs2 partition) so I suspect that is something that gets unpacked
during the flashing:

  ➜  file Firmware\ 1.68/mp*
  Firmware 1.68/mp2image.bin: data
  Firmware 1.68/mpimage.bin:  data
  🕙17:17:29 alex@draig:tests/testcases/FreecomMusicPalFirmware  
  ➜  file ../musicpal/musicpal.image 
  ../musicpal/musicpal.image: Linux jffs2 filesystem data little endian

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro