| Message ID | cb338cdc-d09d-4513-ba16-5ff3f792bbfe@pullman.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/char: suppress sunmouse events with no changes | expand |
On 8/20/24 09:18, Carl Hauser wrote: > @@ -959,6 +960,15 @@ static void sunmouse_event(void *opaque, > int ch; > > trace_escc_sunmouse_event(dx, dy, buttons_state); > + > + /* Don't send duplicate events without motion */ > + if (dx == 0 && > + dy == 0 && > + (s->sunmouse_prev_state ^ buttons_state) == 0) { Were you intending to mask vs MOUSE_EVENT_*BUTTON? Otherwise this is just plain equality. > diff --git a/include/hw/char/escc.h b/include/hw/char/escc.h > index 5669a5b811..bc5ba4f564 100644 > --- a/include/hw/char/escc.h > +++ b/include/hw/char/escc.h > @@ -46,6 +46,7 @@ typedef struct ESCCChannelState { > uint8_t rx, tx; > QemuInputHandlerState *hs; > char *sunkbd_layout; > + int sunmouse_prev_state; This adds new state that must be migrated. While the patch is relatively simple, I do wonder if this code could be improved by converting away from the legacy mouse interface to qemu_input_handler_register. Especially if that might help avoid needing to add migration state that isn't "really" part of the device. Mark? r~
On 20/08/2024 08:34, Richard Henderson wrote: > On 8/20/24 09:18, Carl Hauser wrote: >> @@ -959,6 +960,15 @@ static void sunmouse_event(void *opaque, >> int ch; >> >> trace_escc_sunmouse_event(dx, dy, buttons_state); >> + >> + /* Don't send duplicate events without motion */ >> + if (dx == 0 && >> + dy == 0 && >> + (s->sunmouse_prev_state ^ buttons_state) == 0) { > > Were you intending to mask vs MOUSE_EVENT_*BUTTON? > Otherwise this is just plain equality. > >> diff --git a/include/hw/char/escc.h b/include/hw/char/escc.h >> index 5669a5b811..bc5ba4f564 100644 >> --- a/include/hw/char/escc.h >> +++ b/include/hw/char/escc.h >> @@ -46,6 +46,7 @@ typedef struct ESCCChannelState { >> uint8_t rx, tx; >> QemuInputHandlerState *hs; >> char *sunkbd_layout; >> + int sunmouse_prev_state; > > This adds new state that must be migrated. > > While the patch is relatively simple, I do wonder if this code could be improved by > converting away from the legacy mouse interface to qemu_input_handler_register. > Especially if that might help avoid needing to add migration state that isn't > "really" part of the device. > > Mark? Ooof I didn't even realise that qemu_add_mouse_event_handler() was legacy - is that documented anywhere at all? At first glance (e.g. https://gitlab.com/qemu-project/qemu/-/blob/master/hw/input/ps2.c?ref_type=heads#L789) it appears that movement and button events are handled separately which I think would solve the problem. I can try and put something together quickly for Carl to test and improve if that helps, although I'm quite tied up with client work and life in general right now(!). ATB, Mark.
diff --git a/hw/char/escc.c b/hw/char/escc.c index d450d70eda..7732141cf5 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_prev_state = 0; clear_queue(s); } @@ -959,6 +960,15 @@ static void sunmouse_event(void *opaque, int ch; trace_escc_sunmouse_event(dx, dy, buttons_state); + + /* Don't send duplicate events without motion */ + if (dx == 0 && + dy == 0 && + (s->sunmouse_prev_state ^ buttons_state) == 0) { + return; + } + s->sunmouse_prev_state = buttons_state; + ch = 0x80 | 0x7; /* protocol start byte, no buttons pressed */ if (buttons_state & MOUSE_EVENT_LBUTTON) { diff --git a/include/hw/char/escc.h b/include/hw/char/escc.h index 5669a5b811..bc5ba4f564 100644 --- a/include/hw/char/escc.h +++ b/include/hw/char/escc.h @@ -46,6 +46,7 @@ typedef struct ESCCChannelState { uint8_t rx, tx; QemuInputHandlerState *hs; char *sunkbd_layout; + int sunmouse_prev_state; } ESCCChannelState; struct ESCCState {