Re: [PATCH] ps2: Initial horizontal scroll support

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 13778 bytes --]

Hi

On Tue, Dec 21, 2021 at 4:10 AM Dmitry Petrov <dpetroff@gmail.com> wrote:

> This patch introduces horizontal scroll support for the ps/2 mouse.
> It includes changes in the ps/2 device driver as well as support
> for three display options - cocoa, gtk and sdl, tested and working
> on all of them against guest ubuntu system.
>
> 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


You should split the patch for the different subsystems/ui etc

Looks good to me, although I didn't test it yet. Some comments below


>
> Signed-off-by: Dmitry Petrov <dpetroff@gmail.com>
> ---
>  hw/input/ps2.c    | 54 ++++++++++++++++++++++++++++++++++++++++-------
>  qapi/ui.json      |  2 +-
>  ui/cocoa.m        | 18 ++++++++++------
>  ui/gtk.c          | 54 ++++++++++++++++++++++++++++++++++++-----------
>  ui/input-legacy.c | 16 ++++++++++++++
>  ui/sdl2.c         |  5 +++++
>  6 files changed, 122 insertions(+), 27 deletions(-)
>
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 9376a8f4ce..9e42284cd9 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,38 @@ 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.
> +         */
>

Reading the kernel code helped me guess what is going on, but it would be
nice to have more doc or link to specifications instead.


> +        if (dw1 != 0) {
> +            if (dw1 > 15) {
> +                dw1 = 15;
> +            } else if (dw1 < -15) {
> +                dw1 = -15;
> +            }
> +
> +            /* 0x3f was found by trial and error vs ubuntu instance */
> +            b = (dw1 & 0x3f) | 0x40;
>

Ok, clamp at 15 (I think you could go at 31 actually, since 5 bits seem to
be used) and go to the kernel:
case 0x40: /* horizontal scroll on IntelliMouse Explorer 4.0 */

This case doesn't handle buttons simultaneously indeed.

I think 0x3f comes from 5 bits + 1 sign bit.

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

Here clamp at 7, since we should fall in the kernel
case 0x00:

and only 3 bits seem to be used (thus & 0x0f for 3+1 sign).

This case handles buttons simultaneously, but only vertical scroll (unless
a4tech_workaround is set and triggered)





> +        }
>          ps2_queue_noirq(&s->common, b);
>          break;
>      }
> @@ -764,7 +794,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 +835,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 +868,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 +1073,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:
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 68a6302184..c898a8aeaa 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -970,21 +970,27 @@ QemuCocoaView *cocoaView;
>               */
>
>              /*
> -             * When deltaY is zero, it means that this scrolling event was
> -             * either horizontal, or so fine that it only appears in
> -             * scrollingDeltaY. So we drop the event.
> +             * We shouldn't have got a scroll event when deltaY and delta
> Y
> +             * are zero, hence no harm in dropping the event
>               */
> -            if ([event deltaY] != 0) {
> +            if ([event deltaY] != 0 || [event deltaX] != 0) {
>              /* Determine if this is a scroll up or scroll down event */
> -                buttons = ([event deltaY] > 0) ?
> +                if ([event deltaY] != 0) {
> +                  buttons = ([event deltaY] > 0) ?
>                      INPUT_BUTTON_WHEEL_UP : INPUT_BUTTON_WHEEL_DOWN;
> +                } else if ([event deltaX] != 0) {
> +                  buttons = ([event deltaX] > 0) ?
> +                    INPUT_BUTTON_WHEEL_LEFT : INPUT_BUTTON_WHEEL_RIGHT;
> +                }
> +
>                  qemu_input_queue_btn(dcl.con, buttons, true);
>                  qemu_input_event_sync();
>                  qemu_input_queue_btn(dcl.con, buttons, false);
>                  qemu_input_event_sync();
>              }
> +
>              /*
> -             * Since deltaY also reports scroll wheel events we prevent
> mouse
> +             * Since deltaY/deltaY also report scroll wheel events we
> prevent mouse
>               * movement code from executing.
>               */
>              mouse_event = false;
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 428f02f2df..b52eec6fe9 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -963,33 +963,63 @@ static gboolean gd_scroll_event(GtkWidget *widget,
> GdkEventScroll *scroll,
>                                  void *opaque)
>  {
>      VirtualConsole *vc = opaque;
> -    InputButton btn;
> +    InputButton btn_vertical;
> +    InputButton btn_horizontal;
> +    bool has_vertical = false;
> +    bool has_horizontal = false;
>
>      if (scroll->direction == GDK_SCROLL_UP) {
> -        btn = INPUT_BUTTON_WHEEL_UP;
> +        btn_vertical = INPUT_BUTTON_WHEEL_UP;
> +        has_vertical = true;
>      } else if (scroll->direction == GDK_SCROLL_DOWN) {
> -        btn = INPUT_BUTTON_WHEEL_DOWN;
> +        btn_vertical = INPUT_BUTTON_WHEEL_DOWN;
> +        has_vertical = true;
> +    } else if (scroll->direction == GDK_SCROLL_LEFT) {
> +        btn_horizontal = INPUT_BUTTON_WHEEL_LEFT;
> +        has_horizontal = true;
> +    } else if (scroll->direction == GDK_SCROLL_RIGHT) {
> +        btn_horizontal = INPUT_BUTTON_WHEEL_RIGHT;
> +        has_horizontal = true;
>      } else if (scroll->direction == GDK_SCROLL_SMOOTH) {
>          gdouble delta_x, delta_y;
>          if (!gdk_event_get_scroll_deltas((GdkEvent *)scroll,
>                                           &delta_x, &delta_y)) {
>              return TRUE;
>          }
> -        if (delta_y == 0) {
> -            return TRUE;
> -        } else if (delta_y > 0) {
> -            btn = INPUT_BUTTON_WHEEL_DOWN;
> +
> +        if (delta_y > 0) {
> +            btn_vertical = INPUT_BUTTON_WHEEL_DOWN;
> +            has_vertical = true;
> +        } else if (delta_y < 0) {
> +            btn_vertical = INPUT_BUTTON_WHEEL_UP;
> +            has_vertical = true;
> +        } else if (delta_x > 0) {
> +            btn_horizontal = INPUT_BUTTON_WHEEL_RIGHT;
> +            has_horizontal = true;
> +        } else if (delta_x < 0) {
> +            btn_horizontal = INPUT_BUTTON_WHEEL_LEFT;
> +            has_horizontal = true;
>          } else {
> -            btn = INPUT_BUTTON_WHEEL_UP;
> +            return TRUE;
>          }
>      } else {
>          return TRUE;
>      }
>
> -    qemu_input_queue_btn(vc->gfx.dcl.con, btn, true);
> -    qemu_input_event_sync();
> -    qemu_input_queue_btn(vc->gfx.dcl.con, btn, false);
> -    qemu_input_event_sync();
> +    if (has_vertical) {
> +        qemu_input_queue_btn(vc->gfx.dcl.con, btn_vertical, true);
> +        qemu_input_event_sync();
> +        qemu_input_queue_btn(vc->gfx.dcl.con, btn_vertical, false);
> +        qemu_input_event_sync();
> +    }
> +
> +    if (has_horizontal) {
> +        qemu_input_queue_btn(vc->gfx.dcl.con, btn_horizontal, true);
> +        qemu_input_event_sync();
> +        qemu_input_queue_btn(vc->gfx.dcl.con, btn_horizontal, false);
> +        qemu_input_event_sync();
> +    }
> +
>      return TRUE;
>  }
>
> diff --git a/ui/input-legacy.c b/ui/input-legacy.c
> index 9fc78a639b..2c9a215d7f 100644
> --- a/ui/input-legacy.c
> +++ b/ui/input-legacy.c
> @@ -23,6 +23,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "qapi/qapi-commands-ui.h"
>  #include "ui/console.h"
>  #include "keymaps.h"
> @@ -179,6 +180,20 @@ static void legacy_mouse_event(DeviceState *dev,
> QemuConsole *src,
>                                      1,
>                                      s->buttons);
>          }
> +        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_RIGHT) {
> +            s->qemu_put_mouse_event(s->qemu_put_mouse_event_opaque,
> +                                    s->axis[INPUT_AXIS_X],
> +                                    s->axis[INPUT_AXIS_Y],
> +                                    -2,
> +                                    s->buttons);
> +        }
> +        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_LEFT) {
> +            s->qemu_put_mouse_event(s->qemu_put_mouse_event_opaque,
> +                                    s->axis[INPUT_AXIS_X],
> +                                    s->axis[INPUT_AXIS_Y],
> +                                    2,
> +                                    s->buttons);
> +        }
>          break;
>      case INPUT_EVENT_KIND_ABS:
>          move = evt->u.abs.data;
> @@ -216,6 +231,7 @@ QEMUPutMouseEntry
> *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
>      QEMUPutMouseEntry *s;
>
>      s = g_new0(QEMUPutMouseEntry, 1);
> +    qemu_log("qemu_add_mouse_event_handler %s", name);
>
>      s->qemu_put_mouse_event = func;
>      s->qemu_put_mouse_event_opaque = opaque;
> diff --git a/ui/sdl2.c b/ui/sdl2.c
> index 17c0ec30eb..19bbc1fdd4 100644
> --- a/ui/sdl2.c
> +++ b/ui/sdl2.c
> @@ -33,6 +33,7 @@
>  #include "sysemu/runstate-action.h"
>  #include "sysemu/sysemu.h"
>  #include "ui/win32-kbd-hook.h"
> +#include "qemu/log.h"
>
>  static int sdl2_num_outputs;
>  static struct sdl2_console *sdl2_console;
> @@ -535,6 +536,10 @@ static void handle_mousewheel(SDL_Event *ev)
>          btn = INPUT_BUTTON_WHEEL_UP;
>      } else if (wev->y < 0) {
>          btn = INPUT_BUTTON_WHEEL_DOWN;
> +    } else if (wev->x < 0) {
> +        btn = INPUT_BUTTON_WHEEL_RIGHT;
> +    } else if (wev->x > 0) {
> +        btn = INPUT_BUTTON_WHEEL_LEFT;
>      } else {
>          return;
>      }
> --
> 2.32.0
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 17792 bytes --]