All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/5] ps2: Initial horizontal scroll support
@ 2021-12-22  1:06 Dmitry Petrov
  2021-12-22  1:06 ` [PATCH v2 2/5] ui/cocoa: pass horizontal scroll information to the device code Dmitry Petrov
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Dmitry Petrov @ 2021-12-22  1:06 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dmitry Petrov

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

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 <dpetroff@gmail.com>
---
 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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/5] ui/cocoa: pass horizontal scroll information to the device code
  2021-12-22  1:06 [PATCH v2 1/5] ps2: Initial horizontal scroll support Dmitry Petrov
@ 2021-12-22  1:06 ` Dmitry Petrov
  2021-12-22  1:06 ` [PATCH v2 3/5] ui/gtk: " Dmitry Petrov
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Dmitry Petrov @ 2021-12-22  1:06 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dmitry Petrov

Signed-off-by: Dmitry Petrov <dpetroff@gmail.com>
---
 ui/cocoa.m | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 68a6302184..22a1f6776e 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 deltaX/deltaY also report scroll wheel events we prevent mouse
              * movement code from executing.
              */
             mouse_event = false;
-- 
2.32.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 3/5] ui/gtk: pass horizontal scroll information to the device code
  2021-12-22  1:06 [PATCH v2 1/5] ps2: Initial horizontal scroll support Dmitry Petrov
  2021-12-22  1:06 ` [PATCH v2 2/5] ui/cocoa: pass horizontal scroll information to the device code Dmitry Petrov
@ 2021-12-22  1:06 ` Dmitry Petrov
  2021-12-22  1:06 ` [PATCH v2 4/5] ui/sdl2: " Dmitry Petrov
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Dmitry Petrov @ 2021-12-22  1:06 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dmitry Petrov

Signed-off-by: Dmitry Petrov <dpetroff@gmail.com>
---
 ui/gtk.c | 54 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 12 deletions(-)

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;
 }
 
-- 
2.32.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 4/5] ui/sdl2: pass horizontal scroll information to the device code
  2021-12-22  1:06 [PATCH v2 1/5] ps2: Initial horizontal scroll support Dmitry Petrov
  2021-12-22  1:06 ` [PATCH v2 2/5] ui/cocoa: pass horizontal scroll information to the device code Dmitry Petrov
  2021-12-22  1:06 ` [PATCH v2 3/5] ui/gtk: " Dmitry Petrov
@ 2021-12-22  1:06 ` Dmitry Petrov
  2021-12-22  1:06 ` [PATCH v2 5/5] ui/input-legacy: pass horizontal scroll information Dmitry Petrov
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Dmitry Petrov @ 2021-12-22  1:06 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dmitry Petrov

Signed-off-by: Dmitry Petrov <dpetroff@gmail.com>
---
 ui/sdl2.c | 5 +++++
 1 file changed, 5 insertions(+)

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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 5/5] ui/input-legacy: pass horizontal scroll information
  2021-12-22  1:06 [PATCH v2 1/5] ps2: Initial horizontal scroll support Dmitry Petrov
                   ` (2 preceding siblings ...)
  2021-12-22  1:06 ` [PATCH v2 4/5] ui/sdl2: " Dmitry Petrov
@ 2021-12-22  1:06 ` Dmitry Petrov
  2021-12-22 13:10   ` Marc-André Lureau
  2021-12-22 13:59 ` [PATCH v2 1/5] ps2: Initial horizontal scroll support Marc-André Lureau
  2022-01-04 13:30 ` Daniel P. Berrangé
  5 siblings, 1 reply; 11+ messages in thread
From: Dmitry Petrov @ 2021-12-22  1:06 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dmitry Petrov

This code seems to be used by vmport hack, passing these values allows
to implement horizontal scroll support even when using vmport.
In case it's not supported horizontal scroll will act as a vertical one.

Signed-off-by: Dmitry Petrov <dpetroff@gmail.com>
---
 ui/input-legacy.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 5/5] ui/input-legacy: pass horizontal scroll information
  2021-12-22  1:06 ` [PATCH v2 5/5] ui/input-legacy: pass horizontal scroll information Dmitry Petrov
@ 2021-12-22 13:10   ` Marc-André Lureau
  0 siblings, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2021-12-22 13:10 UTC (permalink / raw)
  To: Dmitry Petrov; +Cc: QEMU

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

Hi

On Wed, Dec 22, 2021 at 5:31 AM Dmitry Petrov <dpetroff@gmail.com> wrote:

> This code seems to be used by vmport hack, passing these values allows
> to implement horizontal scroll support even when using vmport.
> In case it's not supported horizontal scroll will act as a vertical one.
>
> Signed-off-by: Dmitry Petrov <dpetroff@gmail.com>
>

Hmm, apparently Linux kernel handles PSMOUSE_VMMOUSE as a generic ps/2
mouse without the extensions.

And there is no horizontal scrolling in vmmouse protocol I can find.

---
>  ui/input-legacy.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> 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);
> +        }
>

So I think mapping to vertical makes sense, although I am not sure it's
necessary.

         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);
>
>
Please remove that logging


>      s->qemu_put_mouse_event = func;
>      s->qemu_put_mouse_event_opaque = opaque;
> --
> 2.32.0
>
>
>

-- 
Marc-André Lureau

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/5] ps2: Initial horizontal scroll support
  2021-12-22  1:06 [PATCH v2 1/5] ps2: Initial horizontal scroll support Dmitry Petrov
                   ` (3 preceding siblings ...)
  2021-12-22  1:06 ` [PATCH v2 5/5] ui/input-legacy: pass horizontal scroll information Dmitry Petrov
@ 2021-12-22 13:59 ` Marc-André Lureau
  2021-12-22 15:10   ` Dmitry Petrov
  2022-01-04 13:30 ` Daniel P. Berrangé
  5 siblings, 1 reply; 11+ messages in thread
From: Marc-André Lureau @ 2021-12-22 13:59 UTC (permalink / raw)
  To: Dmitry Petrov; +Cc: QEMU

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

Hi

On Wed, Dec 22, 2021 at 5:27 AM Dmitry Petrov <dpetroff@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 <dpetroff@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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/5] ps2: Initial horizontal scroll support
  2021-12-22 13:59 ` [PATCH v2 1/5] ps2: Initial horizontal scroll support Marc-André Lureau
@ 2021-12-22 15:10   ` Dmitry Petrov
  2021-12-22 19:50     ` Marc-André Lureau
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Petrov @ 2021-12-22 15:10 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: QEMU

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

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.lureau@gmail.com>
wrote:

> Hi
>
> On Wed, Dec 22, 2021 at 5:27 AM Dmitry Petrov <dpetroff@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 <dpetroff@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
>

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/5] ps2: Initial horizontal scroll support
  2021-12-22 15:10   ` Dmitry Petrov
@ 2021-12-22 19:50     ` Marc-André Lureau
  0 siblings, 0 replies; 11+ messages in thread
From: Marc-André Lureau @ 2021-12-22 19:50 UTC (permalink / raw)
  To: Dmitry Petrov; +Cc: QEMU

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

Hi Dmitry

On Wed, Dec 22, 2021 at 7:11 PM Dmitry Petrov <dpetroff@gmail.com> wrote:

> 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?
>
>
Yes, that would be nice, so more tools and people can review easily.


> It's my first submission, still trying to get used to the tooling and
> requirements.
>

We have some documentation in
https://www.qemu.org/docs/master/devel/submitting-a-patch.html

(I'd recommend using git-publish, although it's good to understand and know
the underlying commands and workflow before using it).


>

-- 
Marc-André Lureau

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/5] ps2: Initial horizontal scroll support
  2021-12-22  1:06 [PATCH v2 1/5] ps2: Initial horizontal scroll support Dmitry Petrov
                   ` (4 preceding siblings ...)
  2021-12-22 13:59 ` [PATCH v2 1/5] ps2: Initial horizontal scroll support Marc-André Lureau
@ 2022-01-04 13:30 ` Daniel P. Berrangé
  2022-01-08 15:42   ` Dmitry Petrov
  5 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrangé @ 2022-01-04 13:30 UTC (permalink / raw)
  To: Dmitry Petrov; +Cc: qemu-devel

On Wed, Dec 22, 2021 at 02:06:43AM +0100, Dmitry Petrov 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 bugs attached to that ticket don't have any Signed-off-by
from Brad. Fortunately at the time he did re-submit them on
qemu-devel with Signed-off-by present.

Can you include this link to his patches in your commit
message to get the paper trail

  https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg00223.html

> 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 <dpetroff@gmail.com>
> ---
>  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
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/5] ps2: Initial horizontal scroll support
  2022-01-04 13:30 ` Daniel P. Berrangé
@ 2022-01-08 15:42   ` Dmitry Petrov
  0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Petrov @ 2022-01-08 15:42 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: QEMU

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

Hi Daniel,

Thanks for the link! I've sent a v4 patch with a cover letter that includes
it as well
as the latest comments by Marc-André to v2 of the patch.

Kind regards, Dmitry

On Tue, 4 Jan 2022 at 13:30, Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Wed, Dec 22, 2021 at 02:06:43AM +0100, Dmitry Petrov 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 bugs attached to that ticket don't have any Signed-off-by
> from Brad. Fortunately at the time he did re-submit them on
> qemu-devel with Signed-off-by present.
>
> Can you include this link to his patches in your commit
> message to get the paper trail
>
>   https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg00223.html
>
> > 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 <dpetroff@gmail.com>
> > ---
> >  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
> >
> >
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-01-08 15:46 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-22  1:06 [PATCH v2 1/5] ps2: Initial horizontal scroll support Dmitry Petrov
2021-12-22  1:06 ` [PATCH v2 2/5] ui/cocoa: pass horizontal scroll information to the device code Dmitry Petrov
2021-12-22  1:06 ` [PATCH v2 3/5] ui/gtk: " Dmitry Petrov
2021-12-22  1:06 ` [PATCH v2 4/5] ui/sdl2: " Dmitry Petrov
2021-12-22  1:06 ` [PATCH v2 5/5] ui/input-legacy: pass horizontal scroll information Dmitry Petrov
2021-12-22 13:10   ` Marc-André Lureau
2021-12-22 13:59 ` [PATCH v2 1/5] ps2: Initial horizontal scroll support Marc-André Lureau
2021-12-22 15:10   ` Dmitry Petrov
2021-12-22 19:50     ` Marc-André Lureau
2022-01-04 13:30 ` Daniel P. Berrangé
2022-01-08 15:42   ` Dmitry Petrov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.