Hi Marc-André, Thank you for the comments!
The reason of adding the support there was because I've been working on horizontal scroll support for another os (serenity os - https://github.com/SerenityOS/serenity/pull/11246) and wanted to make sure that any changes I made worked fine and ps/2 mouse code was the most straightforward way I could find especially because I could test it both against linux and serenity os code. Did I get it right that the only changes needed for these patches are: - Drop unnecessary log line - Submit again as v3 with a cover letter? It's my first submission, still trying to get used to the tooling and requirements. Thanks! Dmitry Petrov On Wed, 22 Dec 2021 at 14:59, Marc-André Lureau <marcandre.lur...@gmail.com> wrote: > Hi > > On Wed, Dec 22, 2021 at 5:27 AM Dmitry Petrov <dpetr...@gmail.com> wrote: > >> v2: >> - Patch is split into a sequence >> - value is clamped to 31 for horizontal scroll >> >> This patch introduces horizontal scroll support for the ps/2 >> mouse. >> >> The patch is based on the previous work >> by Brad Jorsch done in 2010 >> but never merge, see >> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=579968 >> >> > The patch series doesn't form a thread. You should also add a cover > letter. This way, tools like patchew/patchwork can handle it correctly. > > >> This change adds support for horizontal scroll to ps/2 mouse device >> code. The code is implemented to match the logic of linux kernel >> which is used as a reference. >> >> Signed-off-by: Dmitry Petrov <dpetr...@gmail.com> >> > > I just realized that ps2.c isn't reused by vmmouse. > > What's really the point in adding horizontal scroll to ps2? > > A VM without absolute pointer is not really acceptable or working these > days. Especially on Wayland with the QEMU GTK display. (fwiw, spice-gtk and > rdw do implement pointer contraints & relative extensions so it's kinda > working ok). > > Have you looked at adding virtio-input-hid support instead? That should be > more straightforward. > > > --- >> hw/input/ps2.c | 57 +++++++++++++++++++++++++++++++++++++++++++------- >> qapi/ui.json | 2 +- >> 2 files changed, 50 insertions(+), 9 deletions(-) >> >> diff --git a/hw/input/ps2.c b/hw/input/ps2.c >> index 9376a8f4ce..6236711e1b 100644 >> --- a/hw/input/ps2.c >> +++ b/hw/input/ps2.c >> @@ -123,6 +123,7 @@ typedef struct { >> int mouse_dx; /* current values, needed for 'poll' mode */ >> int mouse_dy; >> int mouse_dz; >> + int mouse_dw; >> uint8_t mouse_buttons; >> } PS2MouseState; >> >> @@ -715,7 +716,7 @@ static int ps2_mouse_send_packet(PS2MouseState *s) >> /* IMPS/2 and IMEX send 4 bytes, PS2 sends 3 bytes */ >> const int needed = s->mouse_type ? 4 : 3; >> unsigned int b; >> - int dx1, dy1, dz1; >> + int dx1, dy1, dz1, dw1; >> >> if (PS2_QUEUE_SIZE - s->common.queue.count < needed) { >> return 0; >> @@ -724,6 +725,7 @@ static int ps2_mouse_send_packet(PS2MouseState *s) >> dx1 = s->mouse_dx; >> dy1 = s->mouse_dy; >> dz1 = s->mouse_dz; >> + dw1 = s->mouse_dw; >> /* XXX: increase range to 8 bits ? */ >> if (dx1 > 127) >> dx1 = 127; >> @@ -740,6 +742,9 @@ static int ps2_mouse_send_packet(PS2MouseState *s) >> /* extra byte for IMPS/2 or IMEX */ >> switch(s->mouse_type) { >> default: >> + /* Just ignore the wheels if not supported */ >> + s->mouse_dz = 0; >> + s->mouse_dw = 0; >> break; >> case 3: >> if (dz1 > 127) >> @@ -747,13 +752,41 @@ static int ps2_mouse_send_packet(PS2MouseState *s) >> else if (dz1 < -127) >> dz1 = -127; >> ps2_queue_noirq(&s->common, dz1 & 0xff); >> + s->mouse_dz -= dz1; >> + s->mouse_dw = 0; >> break; >> case 4: >> - if (dz1 > 7) >> - dz1 = 7; >> - else if (dz1 < -7) >> - dz1 = -7; >> - b = (dz1 & 0x0f) | ((s->mouse_buttons & 0x18) << 1); >> + /* >> + * This matches what the Linux kernel expects for exps/2 in >> + * drivers/input/mouse/psmouse-base.c. Note, if you happen to >> + * press/release the 4th or 5th buttons at the same moment as a >> + * horizontal wheel scroll, those button presses will get lost. >> I'm not >> + * sure what to do about that, since by this point we don't know >> + * whether those buttons actually changed state. >> + */ >> + if (dw1 != 0) { >> + if (dw1 > 31) { >> + dw1 = 31; >> + } else if (dw1 < -31) { >> + dw1 = -31; >> + } >> + >> + /* >> + * linux kernel expects first 6 bits to represent the value >> + * for horizontal scroll >> + */ >> + b = (dw1 & 0x3f) | 0x40; >> + s->mouse_dw -= dw1; >> + } else { >> + if (dz1 > 7) { >> + dz1 = 7; >> + } else if (dz1 < -7) { >> + dz1 = -7; >> + } >> + >> + b = (dz1 & 0x0f) | ((s->mouse_buttons & 0x18) << 1); >> + s->mouse_dz -= dz1; >> + } >> ps2_queue_noirq(&s->common, b); >> break; >> } >> @@ -764,7 +797,6 @@ static int ps2_mouse_send_packet(PS2MouseState *s) >> /* update deltas */ >> s->mouse_dx -= dx1; >> s->mouse_dy -= dy1; >> - s->mouse_dz -= dz1; >> >> return 1; >> } >> @@ -806,6 +838,12 @@ static void ps2_mouse_event(DeviceState *dev, >> QemuConsole *src, >> } else if (btn->button == INPUT_BUTTON_WHEEL_DOWN) { >> s->mouse_dz++; >> } >> + >> + if (btn->button == INPUT_BUTTON_WHEEL_RIGHT) { >> + s->mouse_dw--; >> + } else if (btn->button == INPUT_BUTTON_WHEEL_LEFT) { >> + s->mouse_dw++; >> + } >> } else { >> s->mouse_buttons &= ~bmap[btn->button]; >> } >> @@ -833,8 +871,10 @@ static void ps2_mouse_sync(DeviceState *dev) >> /* if not remote, send event. Multiple events are sent if >> too big deltas */ >> while (ps2_mouse_send_packet(s)) { >> - if (s->mouse_dx == 0 && s->mouse_dy == 0 && s->mouse_dz == 0) >> + if (s->mouse_dx == 0 && s->mouse_dy == 0 >> + && s->mouse_dz == 0 && s->mouse_dw == 0) { >> break; >> + } >> } >> } >> } >> @@ -1036,6 +1076,7 @@ static void ps2_mouse_reset(void *opaque) >> s->mouse_dx = 0; >> s->mouse_dy = 0; >> s->mouse_dz = 0; >> + s->mouse_dw = 0; >> s->mouse_buttons = 0; >> } >> >> diff --git a/qapi/ui.json b/qapi/ui.json >> index d7567ac866..9dac1bf548 100644 >> --- a/qapi/ui.json >> +++ b/qapi/ui.json >> @@ -905,7 +905,7 @@ >> ## >> { 'enum' : 'InputButton', >> 'data' : [ 'left', 'middle', 'right', 'wheel-up', 'wheel-down', >> 'side', >> - 'extra' ] } >> + 'extra', 'wheel-left', 'wheel-right' ] } >> >> ## >> # @InputAxis: >> -- >> 2.32.0 >> >> >> > No objection otherwise, patch looks fine (I couldn't really test it on > wayland..). > > -- > Marc-André Lureau >
