| Message ID | 20240902213816.89071-1-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | escc: convert Sun mouse to use QemuInputHandler | expand |
This still, but less frequently, shows the behavior of having the cursor leap downwards occasionally. I may not be able to work on debugging it until next week, but I'll try to see if I can figure it out sooner. The hypothesis with the old code was that it was sending floods of mouse messages and the Sun driver was dropping a byte at some point. So that's the first thing I'll look into with this new code, but it could be something different. This is definitely better than the old code -- just not sending anything in response to mouse wheel movement helps a lot. On Mon, Sep 2, 2024 at 2:38 PM Mark Cave-Ayland < mark.cave-ayland@ilande.co.uk> 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 > --- > hw/char/escc.c | 79 ++++++++++++++++++++++++++++++------------ > include/hw/char/escc.h | 3 ++ > roms/seabios | 2 +- > 3 files changed, 60 insertions(+), 24 deletions(-) > > diff --git a/hw/char/escc.c b/hw/char/escc.c > index d450d70eda..6d4e3e3350 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,85 @@ 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, > + [INPUT_BUTTON_SIDE] = 0x0, > + [INPUT_BUTTON_EXTRA] = 0x0, > + }; > + > + 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 (btn->down) { > + s->sunmouse_buttons |= bmap[btn->button]; > + } else { > + s->sunmouse_buttons &= ~bmap[btn->button]; > + } > + break; > > - if (buttons_state & MOUSE_EVENT_LBUTTON) { > - ch ^= 0x4; > - } > - if (buttons_state & MOUSE_EVENT_MBUTTON) { > - ch ^= 0x2; > - } > - if (buttons_state & MOUSE_EVENT_RBUTTON) { > - ch ^= 0x1; > + default: > + /* keep gcc happy */ > + break; > } > +} > > - put_queue(s, ch); > +static void sunmouse_sync(DeviceState *dev) > +{ > + ESCCChannelState *s = (ESCCChannelState *)dev; > + int ch; > > - ch = dx; > + trace_escc_sunmouse_event(s->sunmouse_dx, s->sunmouse_dy, 0); > + ch = 0x80 | 0x7; /* protocol start byte, no buttons pressed */ > + ch ^= s->sunmouse_buttons; > + put_queue(s, ch); > > + ch = s->sunmouse_dx; > if (ch > 127) { > ch = 127; > } else if (ch < -127) { > ch = -127; > } > - > put_queue(s, ch & 0xff); > + s->sunmouse_dx = 0; > > - 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 = 0; > > /* 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 +1069,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 { > diff --git a/roms/seabios b/roms/seabios > index a6ed6b701f..7d0c606870 160000 > --- a/roms/seabios > +++ b/roms/seabios > @@ -1 +1 @@ > -Subproject commit a6ed6b701f0a57db0569ab98b0661c12a6ec3ff8 > +Subproject commit 7d0c6068703eae9f2498be0c900ab95b25b4f07a > -- > 2.39.2 > > >
Well, I was wrong -- it is sending a duplicate mouse packets when the mouse wheel is rotated. The packets correctly represent the mouse buttons state. I just now discovered that one of my Logitech mice sends continuous mouse events when the wheel is rotated half a notch and held there. Another Logitech mouse doesn't do that but does send multiple (6-10) events per notch. A Microsoft mouse sends 2 events per notch. I don't know where these should be suppressed. Mouse wheel rotation of a host mouse shouldn't send anything to the emulated Sun mouse. I suspect that the unwanted host events are propagating down to escc via calls to sunmouse_sync. So is sunmouse_sync where they should be filtered out? Probably, because the calling code is not specific to sunmouse and for other mice those calls are needed. -- Carl On Mon, Sep 2, 2024 at 4:16 PM Carl Hauser <chauser@pullman.com> wrote: > This still, but less frequently, shows the behavior of having the cursor > leap downwards occasionally. I may not be able to work on debugging it > until next week, but I'll try to see if I can figure it out sooner. The > hypothesis with the old code was that it was sending floods of mouse > messages and the Sun driver was dropping a byte at some point. So that's > the first thing I'll look into with this new code, but it could be > something different. This is definitely better than the old code -- just > not sending anything in response to mouse wheel movement helps a lot. > > On Mon, Sep 2, 2024 at 2:38 PM Mark Cave-Ayland < > mark.cave-ayland@ilande.co.uk> 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 >> --- >> hw/char/escc.c | 79 ++++++++++++++++++++++++++++++------------ >> include/hw/char/escc.h | 3 ++ >> roms/seabios | 2 +- >> 3 files changed, 60 insertions(+), 24 deletions(-) >> >> diff --git a/hw/char/escc.c b/hw/char/escc.c >> index d450d70eda..6d4e3e3350 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,85 @@ 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, >> + [INPUT_BUTTON_SIDE] = 0x0, >> + [INPUT_BUTTON_EXTRA] = 0x0, >> + }; >> + >> + 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 (btn->down) { >> + s->sunmouse_buttons |= bmap[btn->button]; >> + } else { >> + s->sunmouse_buttons &= ~bmap[btn->button]; >> + } >> + break; >> >> - if (buttons_state & MOUSE_EVENT_LBUTTON) { >> - ch ^= 0x4; >> - } >> - if (buttons_state & MOUSE_EVENT_MBUTTON) { >> - ch ^= 0x2; >> - } >> - if (buttons_state & MOUSE_EVENT_RBUTTON) { >> - ch ^= 0x1; >> + default: >> + /* keep gcc happy */ >> + break; >> } >> +} >> >> - put_queue(s, ch); >> +static void sunmouse_sync(DeviceState *dev) >> +{ >> + ESCCChannelState *s = (ESCCChannelState *)dev; >> + int ch; >> >> - ch = dx; >> + trace_escc_sunmouse_event(s->sunmouse_dx, s->sunmouse_dy, 0); >> + ch = 0x80 | 0x7; /* protocol start byte, no buttons pressed */ >> + ch ^= s->sunmouse_buttons; >> + put_queue(s, ch); >> >> + ch = s->sunmouse_dx; >> if (ch > 127) { >> ch = 127; >> } else if (ch < -127) { >> ch = -127; >> } >> - >> put_queue(s, ch & 0xff); >> + s->sunmouse_dx = 0; >> >> - 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 = 0; >> >> /* 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 +1069,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 { >> diff --git a/roms/seabios b/roms/seabios >> index a6ed6b701f..7d0c606870 160000 >> --- a/roms/seabios >> +++ b/roms/seabios >> @@ -1 +1 @@ >> -Subproject commit a6ed6b701f0a57db0569ab98b0661c12a6ec3ff8 >> +Subproject commit 7d0c6068703eae9f2498be0c900ab95b25b4f07a >> -- >> 2.39.2 >> >> >>
On 03/09/2024 02:33, Carl Hauser via wrote: > Well, I was wrong -- it is sending a duplicate mouse packets when the mouse wheel is > rotated. The packets correctly represent the mouse buttons state. I just now > discovered that one of my Logitech mice sends continuous mouse events when the wheel > is rotated half a notch and held there. Another Logitech mouse doesn't do that but > does send multiple (6-10) events per notch. A Microsoft mouse sends 2 events per notch. > > I don't know where these should be suppressed. Mouse wheel rotation of a host mouse > shouldn't send anything to the emulated Sun mouse. I suspect that the unwanted host > events are propagating down to escc via calls to sunmouse_sync. So is sunmouse_sync > where they should be filtered out? Probably, because the calling code is not specific > to sunmouse and for other mice those calls are needed. Ah so it's from the events generated by the mouse wheel? I think we can safely assume that Sun never produced any wheeled serial mice, in which case it should be possible to filter those events out before they get to the guest. I should have a bit of time later today to make the relevant changes and send out a v2 patch. ATB, Mark.
diff --git a/hw/char/escc.c b/hw/char/escc.c index d450d70eda..6d4e3e3350 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,85 @@ 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, + [INPUT_BUTTON_SIDE] = 0x0, + [INPUT_BUTTON_EXTRA] = 0x0, + }; + + 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 (btn->down) { + s->sunmouse_buttons |= bmap[btn->button]; + } else { + s->sunmouse_buttons &= ~bmap[btn->button]; + } + break; - if (buttons_state & MOUSE_EVENT_LBUTTON) { - ch ^= 0x4; - } - if (buttons_state & MOUSE_EVENT_MBUTTON) { - ch ^= 0x2; - } - if (buttons_state & MOUSE_EVENT_RBUTTON) { - ch ^= 0x1; + default: + /* keep gcc happy */ + break; } +} - put_queue(s, ch); +static void sunmouse_sync(DeviceState *dev) +{ + ESCCChannelState *s = (ESCCChannelState *)dev; + int ch; - ch = dx; + trace_escc_sunmouse_event(s->sunmouse_dx, s->sunmouse_dy, 0); + ch = 0x80 | 0x7; /* protocol start byte, no buttons pressed */ + ch ^= s->sunmouse_buttons; + put_queue(s, ch); + ch = s->sunmouse_dx; if (ch > 127) { ch = 127; } else if (ch < -127) { ch = -127; } - put_queue(s, ch & 0xff); + s->sunmouse_dx = 0; - 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 = 0; /* 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 +1069,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 { diff --git a/roms/seabios b/roms/seabios index a6ed6b701f..7d0c606870 160000 --- a/roms/seabios +++ b/roms/seabios @@ -1 +1 @@ -Subproject commit a6ed6b701f0a57db0569ab98b0661c12a6ec3ff8 +Subproject commit 7d0c6068703eae9f2498be0c900ab95b25b4f07a
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 --- hw/char/escc.c | 79 ++++++++++++++++++++++++++++++------------ include/hw/char/escc.h | 3 ++ roms/seabios | 2 +- 3 files changed, 60 insertions(+), 24 deletions(-)