Message ID | cb338cdc-d09d-4513-ba16-5ff3f792bbfe@pullman.com (mailing list archive) |
---|---|
State | New, archived |
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.
More on whether the sunmouse_prev_state needs to be migrated. I think that if it were NOT included in the migration state the following would hold, assuming that it would be initialized to 0 in the "to" environment: 1. If prev_state is 0 in the "from" environment the prev_state in the "to" environment would be correct -- and this would be the case almost all the time. 2. If it is non-0 in the "from" environment (so the guest OS believes there is a mouse button pressed) and the first mouse event in the "to" environment has motion, the event will be sent correctly and the prev_state is then correct; likely almost all of the rest of the time. 3. If it is non-0 in the "from" environment (so the guest OS believes there is a mouse button pressed) and the first mouse event in the "to" environment has no motion and non-zero button state the event will be sent and there are two cases: 3a. if the button state is NOT the same as the prev state in the "from" environment the event will have been sent correctly. *3b. if the button state IS the same as prev_state in the "from" environment (so the guest OS believes there is a mouse button pressed) the event will have been sent incorrectly and is a duplicate. The prev_state in the "to" environment will now be correct and no further duplicates will be sent. *4. If it is non-0 in the "from" environment (so the guest OS believes there is a mouse button pressed) and the first mouse event(s) in the "to" environment has no motion and zero button state, the event will not be sent. It is not apparent that the code that carries mouse events from the host to the escc driver level would ever produce such an event as the first one or ones, but if it did the state would persist until there was mouse motion or a button press event. The consequence would be that the guest would continue to understand that a mouse button was pressed when it was not. The situation gets corrected on any mouse movement or button press. So the starred cases, 3b and 4, are the only incorrect behaviors. Both are likely extremely rare and hard to make happen. Case 3b results in one spurious duplicate event sent to the guest. I have only seen Solaris misbehave in the face of a flood of duplicate events. My testing with linux didn't uncover any misbehavior even when flooded with duplicates. NetBSD 10 has other mouse misbehaviors that make it hard to tell whether or not duplicates matter there. In case 4, the guest would continue to believe a mouse button was depressed when it wasn't but this would be cured by any button press or movement. It appears that this is all quite similar to the way caps_lock_mode and num_lock_mode are handled in escc -- they are both part of the ESCCChannelState but not part of the migration state and I would expect would exhibit similar need to see transitions in some cases before getting the state fully correct after a migration. On Wed, Aug 21, 2024 at 7:18 AM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > 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 {