Xen devel - Jun 2013 - [PATCH RFC 0/8] monitor: Fix mouse_button and improve mouse_move commands

Hi everyone,

Here''s a new stab at fixing mouse_button HMP command with absolute
coordinates,
as seen with the following devices: usb-tablet, usb-wacom-tablet, ads7846 and
depending on settings vmmouse and xenfb, as well as pre-qdev tsc2005, tsc2102,
tsc2301 touchscreens.

openQA [3] is a framework for recording input sequences and expected graphical
guest output. An absolute coordinate system is handy there. :)
When sending mouse_button command though, QEMU generates the event at
coordinates (0,0), i.e. top left corner for absolute coordinates, so that a
sequence of
  (qemu) mouse_move 42 42
  (qemu) mouse_button 1
may lead the guest to recognize a mouse drag from (42,42) to (0,0) rather
than a click at (42,42).

Since there can be more than one mouse (and there is for -device usb-tablet
without -no-defaults) and since there are multiple ways to interact with the
mouse beyond the monitor, using a single global state in the monitor is ugly.
We therefore need to push the event handling to ui/input.c, where the
mouse_handlers list (QEMUPutMouseEntry) is accessible.

My first attempt was to add coordinates state to QEMUPutMouseEntry, so that
we can safely switch via mouse_set between absolute and non-absolute mice.
The downside of that approach was that state at that level is not migratable,
i.e. we would still warp to (0,0) on mouse_button after migration, and it is
not being reset either.

My solution is to obtain position and buttons state from where the VMState is:
I clean up the mouse event handler registration code a bit and extend it:
* one mandatory callback to obtain mouse button state and
* one optional callback to obtain absolute position state from backends.

Since I didn''t know most mice implementations, these could use some
review.
* For the msmouse chardev backend I needed to persist buttons state alongside
the CharDriverState (no device) because it just wrote it to serial.
Should we turn this backend into a device to reset this state?
* It seemed that escc is not fully migration-ready (e.g., SerIOQueue and
keyboard state missing) so I did not bother to add VMSD fields or a subsection
for the fields added. Similarly it just wrote it to said SerIOQueue and
get_queue() would''ve destructively read from it.
* xenfb stored buttons state but not coordinates, so added them to XenInput
state.
No VMStateDescription at all here.
* hid and vmmouse read the position from some queue (vmmouse also button state),
not sure how safe that is and whether we may need to add it to state directly?

Ludwig, can you test these with the openQA qemu package please? Thanks!

Regards,
Andreas

From Brad Hards'' mouse_button patch:
* Instead of adding more global monitor-only state, delegate to ui/input.c
  (suggested by Gerd [1] and Paolo [2]).
  But instead of keeping state just in ui/input.c:QEMUPutMouseEntry,
  delegate it to the individual mouse event handlers.
* Prepend some code cleanups.
* Introduce MouseOps for callbacks (inspired by MemoryRegionOps).
* Implement MouseOps::get_position() for all absolute mice.
* Implement MouseOps::get_buttons_state() for all mice.
* Improve mouse_move while at it (suggested by Gerd to Brad [1]).

[1] http://lists.gnu.org/archive/html/qemu-devel/2011-04/msg02408.html
[2] https://bugs.launchpad.net/qemu/+bug/752476
[3] http://openqa.opensuse.org/

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Luiz Capitulino <lcapitulino@redhat.com>

Cc: Bernhard M. Wiedemann <bwiedemann@suse.de>
Cc: Ludwig Nussel <lnussel@suse.de>
Cc: Stephan Kulow <coolo@suse.de>
Cc: Brad Hards <bradh@frogmouth.net>

Cc: Blue Swirl <blauwirbel@gmail.com> (escc)
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> (escc)
Cc: Alexander Graf <agraf@suse.de> (adb)
Cc: qemu-ppc <qemu-ppc@nongnu.org> (adb)
Cc: Andrzej Zaborowski <balrogg@gmail.com> (tsc2005, tsc210x, ads7846)
Cc: Peter Maydell <peter.maydell@linaro.org> (tsc2005, tsc210x, ads7846)
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> (xenfb)
Cc: xen-devel <xen-devel@lists.xensource.com> (xenfb)

Andreas Färber (8):
  ui/input: Clean up QEMUPutMouseEntry struct
  ui/input: Simplify kbd_mouse_event()
  ui/input: Use bool for qemu_add_mouse_event_handler()
  ui/input: Introduce MouseOps for qemu_add_mouse_event_handler()
  ui/input: Introduce MouseOps::get_buttons_state()
  monitor: Eliminate global mouse buttons state for mouse_move
  ui/input: Introduce MouseOps::get_position()
  monitor: Fix mouse_button command for absolute coordinates

 backends/msmouse.c   | 29 ++++++++++++---
 hw/char/escc.c       | 17 ++++++++-
 hw/display/ads7846.c | 25 +++++++++++--
 hw/display/xenfb.c   | 49 +++++++++++++++++++++++---
 hw/input/adb.c       | 13 ++++++-
 hw/input/hid.c       | 51 ++++++++++++++++++++++++---
 hw/input/ps2.c       | 14 +++++++-
 hw/input/tsc2005.c   | 25 +++++++++++--
 hw/input/tsc210x.c   | 29 ++++++++++++---
 hw/input/vmmouse.c   | 33 ++++++++++++++++--
 hw/usb/dev-wacom.c   | 34 +++++++++++++++---
 include/ui/console.h | 27 +++++++++++---
 monitor.c            | 10 +++---
 ui/input.c           | 99 ++++++++++++++++++++++++++++++++++------------------
 14 files changed, 380 insertions(+), 75 deletions(-)

-- 
1.8.1.4

Hi,
> My solution is to obtain position and buttons state from where the VMState
is:
> I clean up the mouse event handler registration code a bit and extend it:
> * one mandatory callback to obtain mouse button state and
> * one optional callback to obtain absolute position state from backends.
Hmm, querying state from the mouse drivers just to be able to pass it
back in is a bit backwards.

I''d rather go fix the event reporting interface.  Instead of having
*one* callback with *all* parameters I think we should have a callback
per parameter, or maybe better a parameter type enum.  i.e. instead of:

  mouse_event(x, y, z, button_state)

We''ll have

  mouse_event(type, value)

Then we''ll report moves this way:

  mouse_event(REL_X, 23);     // or ABS_X for the tablet.
  mouse_event(REL_Y, 42);
  mouse_event(SYNC, 0);       // <<= means "done, flush events to
guest"

And klicks this way:

  mouse_event(DOWN, BTN_LEFT);
  mouse_event(SYNC);

  mouse_event(UP, BTN_LEFT);
  mouse_event(SYNC);

The linux input layer works in a simliar way.

cheers,
  Gerd