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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 04/09/2024 11:53, Philippe Mathieu-Daudé wrote:

> 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>

That's true, although I'm hesitant to add such a tag without a nod from the tester. 
Carl, are you happy for me to add your Tested-by tag upon merge?

>> ---
>>   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 {


ATB,

Mark.