Hi Marc-André,

Hi Lucas

On Thu, Jun 8, 2023 at 9:49 PM Lucas Chollet <lucas.chol...@free.fr> wrote:

    No functional changes intended.

    Signed-off-by: Lucas Chollet <lucas.chol...@free.fr>
    ---
     hw/i386/vmmouse.c | 95
    +++++++++++++++++++++++++++++++++++------------
     1 file changed, 71 insertions(+), 24 deletions(-)


The patch diff isn't great, and there is a risk of regressions. I wonder if input-legacy is really meant to be deprecated (after 10y it is still around).

I had the same conclusion but as I'm not familiar with QEMU development, I thought that you guys would quickly tell me if it's not worth.


Otherwise, the patch looks ok to me, except some question below:


    diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
    index a56c185f15..bdddbb64ac 100644
    --- a/hw/i386/vmmouse.c
    +++ b/hw/i386/vmmouse.c
    @@ -24,7 +24,7 @@

     #include "qemu/osdep.h"
     #include "qapi/error.h"
    -#include "ui/console.h"
    +#include "ui/input.h"
     #include "hw/i386/vmport.h"
     #include "hw/input/i8042.h"
     #include "hw/qdev-properties.h"
    @@ -61,7 +61,10 @@ struct VMMouseState {
         uint16_t nb_queue;
         uint16_t status;
         uint8_t absolute;
    -    QEMUPutMouseEntry *entry;
    +    int32_t last_x;
    +    int32_t last_y;
    +    int32_t last_buttons;
    +    QemuInputHandlerState *entry;
         ISAKBDState *i8042;
     };

    @@ -91,33 +94,72 @@ static uint32_t
    vmmouse_get_status(VMMouseState *s)
         return (s->status << 16) | s->nb_queue;
     }

    -static void vmmouse_mouse_event(void *opaque, int x, int y, int
    dz, int buttons_state)
    +static void vmmouse_mouse_event(DeviceState *dev, QemuConsole *src,
    +                                InputEvent *evt)
     {
    -    VMMouseState *s = opaque;
    -    int buttons = 0;
    +    static const int bmap[INPUT_BUTTON__MAX] = {
    +        [INPUT_BUTTON_LEFT]   = 0x20,
    +        [INPUT_BUTTON_MIDDLE] = 0x08,
    +        [INPUT_BUTTON_RIGHT]  = 0x10,
    +    };
    +
    +    VMMouseState *s = VMMOUSE(dev);
    +    InputMoveEvent *move;
    +    InputBtnEvent *btn;
    +
    +    int32_t dz = 0;

         if (s->nb_queue > (VMMOUSE_QUEUE_SIZE - 4))
             return;

    -    DPRINTF("vmmouse_mouse_event(%d, %d, %d, %d)\n",
    -            x, y, dz, buttons_state);
    +    switch (evt->type) {
    +    case INPUT_EVENT_KIND_REL:
    +        move = evt->u.rel.data;
    +        if (move->axis == INPUT_AXIS_X) {
    +            s->last_x += move->value;
    +        } else if (move->axis == INPUT_AXIS_Y) {
    +            s->last_y -= move->value;
    +        }
    +        break;

    -    if ((buttons_state & MOUSE_EVENT_LBUTTON))
    -        buttons |= 0x20;
    -    if ((buttons_state & MOUSE_EVENT_RBUTTON))
    -        buttons |= 0x10;
    -    if ((buttons_state & MOUSE_EVENT_MBUTTON))
    -        buttons |= 0x08;
    +    case INPUT_EVENT_KIND_ABS:
    +        move = evt->u.rel.data;
    +        if (move->axis == INPUT_AXIS_X) {
    +            s->last_x = move->value;
    +        } else if (move->axis == INPUT_AXIS_Y) {
    +            s->last_y = move->value;
    +        }
    +        break;

    -    if (s->absolute) {
    -        x <<= 1;
    -        y <<= 1;
    +    case INPUT_EVENT_KIND_BTN:
    +        btn = evt->u.btn.data;
    +        if (btn->down) {
    +            s->last_buttons |= bmap[btn->button];
    +            if (btn->button == INPUT_BUTTON_WHEEL_UP) {
    +                dz--;
    +            } else if (btn->button == INPUT_BUTTON_WHEEL_DOWN) {
    +                dz++;
    +            }
    +
    +        } else {
    +          s->last_buttons &= ~bmap[btn->button];
    +        }
    +        break;
    +
    +    default:
    +        /* keep gcc happy */
    +        break;
         }

    -    s->queue[s->nb_queue++] = buttons;
    -    s->queue[s->nb_queue++] = x;
    -    s->queue[s->nb_queue++] = y;
    +    s->queue[s->nb_queue++] = s->last_buttons;
    +    s->queue[s->nb_queue++] = s->absolute ? s->last_x << 1 :
    s->last_x;
    +    s->queue[s->nb_queue++] = s->absolute ? s->last_y << 1 :
    s->last_y;
         s->queue[s->nb_queue++] = dz;
    +}
    +
    +static void vmmouse_mouse_sync(DeviceState *dev)
    +{
    +    VMMouseState *s = VMMOUSE(dev);

         /* need to still generate PS2 events to notify driver to
            read from queue */
    @@ -127,11 +169,18 @@ static void vmmouse_mouse_event(void
    *opaque, int x, int y, int dz, int buttons_
     static void vmmouse_remove_handler(VMMouseState *s)
     {
         if (s->entry) {
    -        qemu_remove_mouse_event_handler(s->entry);
    +        qemu_input_handler_unregister(s->entry);
             s->entry = NULL;
         }
     }

    +static QemuInputHandler vm_mouse_handler = {
    +    .name  = "vmmouse",
    +    .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_ABS,
    +    .event = vmmouse_mouse_event,
    +    .sync  = vmmouse_mouse_sync,
    +};
    +
     static void vmmouse_update_handler(VMMouseState *s, int absolute)
     {
         if (s->status != 0) {
    @@ -142,10 +191,8 @@ static void
    vmmouse_update_handler(VMMouseState *s, int absolute)
             vmmouse_remove_handler(s);
         }
         if (s->entry == NULL) {
    -        s->entry = qemu_add_mouse_event_handler(vmmouse_mouse_event,
    -                                                s, s->absolute,
    -                                                "vmmouse");
    -        qemu_activate_mouse_event_handler(s->entry);
    +        s->entry = qemu_input_handler_register(DEVICE(s),
    &vm_mouse_handler);


You don't take "absolute" state into account anymore, so vm_mouse_handler.mask is always with MASK_ABS. I think this may be problematic as mouse_mode won't change properly from abs/rel.

Ohh you're right, I wonder what's the best solution here. We can either have both `QemuInputHandler vm_mouse_handler_rel` and `QemuInputHandler vm_mouse_handler_abs` or make `vm_mouse_handler` accept both type of events.

Still, I'm a bit surprised because I tested to remove the call to put the mouse in absolute mode in the guest kernel and it works.


    +        qemu_input_handler_activate(s->entry);
         }
     }

-- 2.39.2




--
Marc-André Lureau

Reply via email to