Re: [PATCH v3] escc: convert Sun mouse to use QemuInputHandler

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 4/9/24 12:23, Mark Cave-Ayland wrote:
> Update the Sun mouse implementation to use QemuInputHandler instead of the
> legacy qemu_add_mouse_event_handler() function.
> 
> Note that this conversion adds extra sunmouse_* members to ESCCChannelState
> but they are not added to the migration stream (similar to the Sun keyboard
> members). If this were desired in future, the Sun devices should be split
> into separate devices and added to the migration stream there instead.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2518
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

On v3 there is also an implicit:
Tested-by: Carl Hauser <chauser@pullman.com>

> ---
>   hw/char/escc.c         | 88 +++++++++++++++++++++++++++++++-----------
>   include/hw/char/escc.h |  3 ++
>   2 files changed, 69 insertions(+), 22 deletions(-)
> 
> v3:
> - Subtract the deltas in sunmouse_sync() instead of resetting them to zero
>    which provides better tracking if the mouse movement exceeds the 8-bit
>    delta limit of the MSC protocol
> 
> - Add R-B tag from Richard
> 
> v2:
> - Only allow left, middle and right button events (use bit 7 which is always
>    set in the first byte to indicate a valid event)
> 
> - Remove zero entries from the bmap table as static entries should be
>    zero anyway
> 
> 
> diff --git a/hw/char/escc.c b/hw/char/escc.c
> index d450d70eda..245a7b19d3 100644
> --- a/hw/char/escc.c
> +++ b/hw/char/escc.c
> @@ -287,6 +287,7 @@ static void escc_reset_chn(ESCCChannelState *s)
>       s->rxint = s->txint = 0;
>       s->rxint_under_svc = s->txint_under_svc = 0;
>       s->e0_mode = s->led_mode = s->caps_lock_mode = s->num_lock_mode = 0;
> +    s->sunmouse_dx = s->sunmouse_dy = s->sunmouse_buttons = 0;
>       clear_queue(s);
>   }
>   
> @@ -952,53 +953,96 @@ static void handle_kbd_command(ESCCChannelState *s, int val)
>       }
>   }
>   
> -static void sunmouse_event(void *opaque,
> -                               int dx, int dy, int dz, int buttons_state)
> +static void sunmouse_handle_event(DeviceState *dev, QemuConsole *src,
> +                                  InputEvent *evt)
>   {
> -    ESCCChannelState *s = opaque;
> -    int ch;
> +    ESCCChannelState *s = (ESCCChannelState *)dev;
> +    InputMoveEvent *move;
> +    InputBtnEvent *btn;
> +    static const int bmap[INPUT_BUTTON__MAX] = {
> +        [INPUT_BUTTON_LEFT]   = 0x4,
> +        [INPUT_BUTTON_MIDDLE] = 0x2,
> +        [INPUT_BUTTON_RIGHT]  = 0x1,
> +    };
> +
> +    switch (evt->type) {
> +    case INPUT_EVENT_KIND_REL:
> +        move = evt->u.rel.data;
> +        if (move->axis == INPUT_AXIS_X) {
> +            s->sunmouse_dx += move->value;
> +        } else if (move->axis == INPUT_AXIS_Y) {
> +            s->sunmouse_dy -= move->value;
> +        }
> +        break;
>   
> -    trace_escc_sunmouse_event(dx, dy, buttons_state);
> -    ch = 0x80 | 0x7; /* protocol start byte, no buttons pressed */
> +    case INPUT_EVENT_KIND_BTN:
> +        btn = evt->u.btn.data;
> +        if (bmap[btn->button]) {
> +            if (btn->down) {
> +                s->sunmouse_buttons |= bmap[btn->button];
> +            } else {
> +                s->sunmouse_buttons &= ~bmap[btn->button];
> +            }
> +            /* Indicate we have a supported button event */
> +            s->sunmouse_buttons |= 0x80;
> +        }
> +        break;
>   
> -    if (buttons_state & MOUSE_EVENT_LBUTTON) {
> -        ch ^= 0x4;
> -    }
> -    if (buttons_state & MOUSE_EVENT_MBUTTON) {
> -        ch ^= 0x2;
> +    default:
> +        /* keep gcc happy */
> +        break;
>       }
> -    if (buttons_state & MOUSE_EVENT_RBUTTON) {
> -        ch ^= 0x1;
> +}
> +
> +static void sunmouse_sync(DeviceState *dev)
> +{
> +    ESCCChannelState *s = (ESCCChannelState *)dev;
> +    int ch;
> +
> +    if (s->sunmouse_dx == 0 && s->sunmouse_dy == 0 &&
> +        (s->sunmouse_buttons & 0x80) == 0) {
> +            /* Nothing to do after button event filter */
> +            return;
>       }
>   
> +    /* Clear our button event flag */
> +    s->sunmouse_buttons &= ~0x80;
> +    trace_escc_sunmouse_event(s->sunmouse_dx, s->sunmouse_dy,
> +                              s->sunmouse_buttons);
> +    ch = 0x80 | 0x7; /* protocol start byte, no buttons pressed */
> +    ch ^= s->sunmouse_buttons;
>       put_queue(s, ch);
>   
> -    ch = dx;
> -
> +    ch = s->sunmouse_dx;
>       if (ch > 127) {
>           ch = 127;
>       } else if (ch < -127) {
>           ch = -127;
>       }
> -
>       put_queue(s, ch & 0xff);
> +    s->sunmouse_dx -= ch;
>   
> -    ch = -dy;
> -
> +    ch = s->sunmouse_dy;
>       if (ch > 127) {
>           ch = 127;
>       } else if (ch < -127) {
>           ch = -127;
>       }
> -
>       put_queue(s, ch & 0xff);
> +    s->sunmouse_dy -= ch;
>   
>       /* MSC protocol specifies two extra motion bytes */
> -
>       put_queue(s, 0);
>       put_queue(s, 0);
>   }
>   
> +static const QemuInputHandler sunmouse_handler = {
> +    .name  = "QEMU Sun Mouse",
> +    .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
> +    .event = sunmouse_handle_event,
> +    .sync  = sunmouse_sync,
> +};
> +
>   static void escc_init1(Object *obj)
>   {
>       ESCCState *s = ESCC(obj);
> @@ -1036,8 +1080,8 @@ static void escc_realize(DeviceState *dev, Error **errp)
>       }
>   
>       if (s->chn[0].type == escc_mouse) {
> -        qemu_add_mouse_event_handler(sunmouse_event, &s->chn[0], 0,
> -                                     "QEMU Sun Mouse");
> +        s->chn[0].hs = qemu_input_handler_register((DeviceState *)(&s->chn[0]),
> +                                                   &sunmouse_handler);
>       }
>       if (s->chn[1].type == escc_kbd) {
>           s->chn[1].hs = qemu_input_handler_register((DeviceState *)(&s->chn[1]),
> diff --git a/include/hw/char/escc.h b/include/hw/char/escc.h
> index 5669a5b811..8c4c6a7730 100644
> --- a/include/hw/char/escc.h
> +++ b/include/hw/char/escc.h
> @@ -46,6 +46,9 @@ typedef struct ESCCChannelState {
>       uint8_t rx, tx;
>       QemuInputHandlerState *hs;
>       char *sunkbd_layout;
> +    int sunmouse_dx;
> +    int sunmouse_dy;
> +    int sunmouse_buttons;
>   } ESCCChannelState;
>   
>   struct ESCCState {