Re: [Xen-devel] [PATCH 1/2 v2] xenfb: Use qemu_input_handler_* calls directly

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 3 Jul 2017, Owen Smith wrote:
> The xenvkbd input device uses functions from input-legacy.c
> Use the appropriate qemu_input_handler_* functions instead
> of calling functions in input-legacy.c that in turn call
> the correct functions.
> The bulk of this patch removes the extra layer of calls
> by moving the required structure members into the XenInput
> struct.
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>  hw/display/xenfb.c | 121 
> +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 113 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index e76c0d8..88815df 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -27,6 +27,7 @@
>  #include "qemu/osdep.h"
>  
>  #include "hw/hw.h"
> +#include "ui/input.h"
>  #include "ui/console.h"
>  #include "hw/xen/xen_backend.h"
>  
> @@ -54,7 +55,14 @@ struct XenInput {
>      int abs_pointer_wanted; /* Whether guest supports absolute pointer */
>      int button_state;       /* Last seen pointer button state */
>      int extended;
> -    QEMUPutMouseEntry *qmouse;
> +    /* kbd */
> +    QemuInputHandler hkbd;
> +    QemuInputHandlerState *qkbd;
> +    /* mouse */
> +    QemuInputHandler hmouse;
> +    QemuInputHandlerState *qmouse;
> +    int axis[INPUT_AXIS__MAX];
> +    int buttons;
>  };
>  
>  #define UP_QUEUE 8
> @@ -293,6 +301,21 @@ static void xenfb_key_event(void *opaque, int scancode)
>      xenfb_send_key(xenfb, down, scancode2linux[scancode]);
>  }
>  
> +static void xenfb_legacy_key_event(DeviceState *dev, QemuConsole *src,
> +                                   InputEvent *evt)
> +{
> +    struct XenInput *in = (struct XenInput *)dev;
> +    int scancodes[3], i, count;
> +    InputKeyEvent *key = evt->u.key.data;
> +
> +    count = qemu_input_key_value_to_scancode(key->key,
> +                                             key->down,
> +                                             scancodes);
> +    for (i = 0; i < count; ++i) {
> +        xenfb_key_event(in, scancodes[i]);
> +    }
> +}
>  /*
>   * Send a mouse event from the client to the guest OS
>   *
> @@ -333,6 +356,70 @@ static void xenfb_mouse_event(void *opaque,
>      xenfb->button_state = button_state;
>  }
>  
> +static void xenfb_legacy_mouse_event(DeviceState *dev, QemuConsole *src,
> +                                     InputEvent *evt)
> +{
> +    static const int bmap[INPUT_BUTTON__MAX] = {
> +        [INPUT_BUTTON_LEFT]   = MOUSE_EVENT_LBUTTON,
> +        [INPUT_BUTTON_MIDDLE] = MOUSE_EVENT_MBUTTON,
> +        [INPUT_BUTTON_RIGHT]  = MOUSE_EVENT_RBUTTON,
> +    };
> +    struct XenInput *in = (struct XenInput *)dev;
> +    InputBtnEvent *btn;
> +    InputMoveEvent *move;
> +
> +    switch (evt->type) {
> +    case INPUT_EVENT_KIND_BTN:
> +        btn = evt->u.btn.data;
> +        if (btn->down) {
> +            in->buttons |= bmap[btn->button];
> +        } else {
> +            in->buttons &= ~bmap[btn->button];
> +        }
> +        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_UP) {
> +            xenfb_mouse_event(in,
> +                              in->axis[INPUT_AXIS_X],
> +                              in->axis[INPUT_AXIS_Y],
> +                              -1,
> +                              in->buttons);
> +        }
> +        if (btn->down && btn->button == INPUT_BUTTON_WHEEL_DOWN) {
> +            xenfb_mouse_event(in,
> +                              in->axis[INPUT_AXIS_X],
> +                              in->axis[INPUT_AXIS_Y],
> +                              1,
> +                              in->buttons);
> +        }

Why are we sending the WHEEL events from here rather than from
xenfb_legacy_mouse_sync?

Can't we add WHEEL_UP/DOWN to bmap? Unless it is due to a quirk
somewhere, I would store the wheel events in in->buttons or a new field,
then I would send the event to the other end from
xenfb_legacy_mouse_sync. You might have to reset the wheel event in
xenfb_legacy_mouse_sync, because, differently from the buttons, I don't
think we are going to get a corresponding "up" event.


> +        break;
> +    case INPUT_EVENT_KIND_ABS:
> +        move = evt->u.abs.data;
> +        in->axis[move->axis] = move->value;
> +        break;
> +    case INPUT_EVENT_KIND_REL:
> +        move = evt->u.rel.data;
> +        in->axis[move->axis] += move->value;
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static void xenfb_legacy_mouse_sync(DeviceState *dev)
> +{
> +    struct XenInput *in = (struct XenInput *)dev;
> +
> +    xenfb_mouse_event(in,
> +                      in->axis[INPUT_AXIS_X],
> +                      in->axis[INPUT_AXIS_Y],
> +                      0,
> +                      in->buttons);
> +
> +    if (!in->abs_pointer_wanted) {
> +        in->axis[INPUT_AXIS_X] = 0;
> +        in->axis[INPUT_AXIS_Y] = 0;
> +    }

I think we should take the opportunity to rework and simplify
xenfb_mouse_event: we shouldn't keep track of the button state twice, in
in->buttons and in->button_state. I would get rid of the logic in
xenfb_mouse_event that attempts to figure out if a button was already
down before and just send the event.


> +}
> +
>  static int input_init(struct XenDevice *xendev)
>  {
>      xenstore_write_be_int(xendev, "feature-abs-pointer", 1);
> @@ -353,7 +440,6 @@ static int input_initialise(struct XenDevice *xendev)
>      if (rc != 0)
>       return rc;
>  
> -    qemu_add_kbd_event_handler(xenfb_key_event, in);
>      return 0;
>  }
>  
> @@ -366,24 +452,43 @@ static void input_connected(struct XenDevice *xendev)
>          in->abs_pointer_wanted = 0;
>      }
>  
> +    if (in->qkbd) {
> +        qemu_input_handler_unregister(in->qkbd);
> +    }
>      if (in->qmouse) {
> -        qemu_remove_mouse_event_handler(in->qmouse);
> +        qemu_input_handler_unregister(in->qmouse);
>      }

Is there a reason for these checks? I realize we already had one here,
but shouldn't input_disconnect already take care of removing the
handlers?


>      trace_xenfb_input_connected(xendev, in->abs_pointer_wanted);
> -    in->qmouse = qemu_add_mouse_event_handler(xenfb_mouse_event, in,
> -                                           in->abs_pointer_wanted,
> -                                           "Xen PVFB Mouse");
> +
> +    in->hkbd.name = "legacy-kbd";
> +    in->hkbd.mask = INPUT_EVENT_MASK_KEY;
> +    in->hkbd.event = xenfb_legacy_key_event;
> +    in->qkbd = qemu_input_handler_register((DeviceState *)in,
> +                                           &in->hkbd);
> +    qemu_input_handler_activate(in->qkbd);
> +
> +    in->hmouse.name = "Xen PVFB Mouse";
> +    in->hmouse.mask = INPUT_EVENT_MASK_BTN |
> +        (in->abs_pointer_wanted ? INPUT_EVENT_MASK_ABS : 
> INPUT_EVENT_MASK_REL);
> +    in->hmouse.event = xenfb_legacy_mouse_event;
> +    in->hmouse.sync = xenfb_legacy_mouse_sync;
> +    in->qmouse = qemu_input_handler_register((DeviceState *)in,
> +                                             &in->hmouse);
> +    qemu_input_handler_activate(in->qmouse);
>  }
>  
>  static void input_disconnect(struct XenDevice *xendev)
>  {
>      struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
>  
> +    if (in->qkbd) {
> +        qemu_input_handler_unregister(in->qkbd);
> +        in->qkbd = NULL;
> +    }
>      if (in->qmouse) {
> -     qemu_remove_mouse_event_handler(in->qmouse);
> +        qemu_input_handler_unregister(in->qmouse);
>       in->qmouse = NULL;

Please align this correctly since you are at it


>      }
> -    qemu_add_kbd_event_handler(NULL, NULL);
>      common_unbind(&in->c);
>  }
>  
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel