Re: [PATCH v2 1/5] ps2: Initial horizontal scroll support

[Dmitry Petrov <dpetroff@gmail.com>]

[-- 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 --]