* [Qemu-devel] [PATCH] add event queueing to USB HID
@ 2010-12-23 14:57 Paolo Bonzini
2011-01-05 17:06 ` [Qemu-devel] " Stefano Stabellini
2011-01-07 7:59 ` Gerd Hoffmann
0 siblings, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2010-12-23 14:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Ian Jackson, Gerd Hoffman, Stefano Stabellini
The polling nature of the USB HID device makes it very hard to double
click or drag while on a high-latency VNC connection. This patch,
based on work done in the Xen qemu-dm tree by Ian Jackson, fixes this
bug by adding an event queue to the device. The event queue associates
each movement with the correct button state (and each button state change
with the correct location); it also remembers all button presses and
releases as well.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffman <kraxel@redhat.com>
---
The same thing could be done for USB Wacom, but this patch
doesn't do it.
hw/usb-hid.c | 234 ++++++++++++++++++++++++++++++++++-----------------------
1 files changed, 139 insertions(+), 95 deletions(-)
diff --git a/hw/usb-hid.c b/hw/usb-hid.c
index 882d933..63b6217 100644
--- a/hw/usb-hid.c
+++ b/hw/usb-hid.c
@@ -44,9 +44,18 @@
#define USB_TABLET 2
#define USB_KEYBOARD 3
+typedef struct USBPointerEvent {
+ int xdx, ydy; /* relative iff it's a mouse, otherwise absolute */
+ int dz, buttons_state;
+} USBPointerEvent;
+
+#define QUEUE_LENGTH 16 /* should be enough for a triple-click */
+#define QUEUE_MASK (QUEUE_LENGTH-1u)
+#define QUEUE_INCR(v) ((v)++, (v) &= QUEUE_MASK)
+
typedef struct USBMouseState {
- int dx, dy, dz, buttons_state;
- int x, y;
+ USBPointerEvent queue[QUEUE_LENGTH];
+ unsigned head, tail; /* indices into circular queue */
int mouse_grabbed;
QEMUPutMouseEntry *eh_entry;
} USBMouseState;
@@ -68,7 +77,7 @@ typedef struct USBHIDState {
int protocol;
uint8_t idle;
int64_t next_idle_clock;
- int changed;
+ int have_data, changed;
void *datain_opaque;
void (*datain)(void *);
} USBHIDState;
@@ -408,37 +417,66 @@ static const uint8_t usb_hid_usage_keys[0x100] = {
static void usb_hid_changed(USBHIDState *hs)
{
+ hs->have_data = 1;
hs->changed = 1;
if (hs->datain)
hs->datain(hs->datain_opaque);
}
-static void usb_mouse_event(void *opaque,
- int dx1, int dy1, int dz1, int buttons_state)
-{
- USBHIDState *hs = opaque;
- USBMouseState *s = &hs->ptr;
-
- s->dx += dx1;
- s->dy += dy1;
- s->dz += dz1;
- s->buttons_state = buttons_state;
+static void usb_pointer_event_clear(USBPointerEvent *e, int buttons) {
+ e->xdx = e->ydy = e->dz = 0;
+ e->buttons_state = buttons;
+}
- usb_hid_changed(hs);
+static void usb_pointer_event_combine(USBPointerEvent *e, int xyrel,
+ int x1, int y1, int z1) {
+ if (xyrel) {
+ e->xdx += x1;
+ e->ydy += y1;
+ } else {
+ e->xdx = x1;
+ e->ydy = y1;
+ }
+ e->dz += z1;
}
-static void usb_tablet_event(void *opaque,
- int x, int y, int dz, int buttons_state)
+static void usb_pointer_event(void *opaque,
+ int x1, int y1, int z1, int buttons_state)
{
USBHIDState *hs = opaque;
USBMouseState *s = &hs->ptr;
-
- s->x = x;
- s->y = y;
- s->dz += dz;
- s->buttons_state = buttons_state;
-
+ unsigned use_slot = (s->tail - 1) & QUEUE_MASK;
+ unsigned previous_slot = (use_slot - 1) & QUEUE_MASK;
+
+ /* We combine events where feasible to keep the queue small.
+ We shouldn't combine anything with the first event with
+ a particular button state, as that would change the
+ location of the button state change. */
+ if (!hs->have_data) {
+ use_slot = s->tail;
+ QUEUE_INCR(s->tail);
+ usb_pointer_event_clear(&s->queue[use_slot], buttons_state);
+ } else if (use_slot == s->head ||
+ s->queue[use_slot].buttons_state != buttons_state ||
+ s->queue[previous_slot].buttons_state != buttons_state) {
+ /* can't or shouldn't combine this event with previous one */
+ use_slot = s->tail;
+ QUEUE_INCR(s->tail);
+ if (use_slot == s->head) {
+ /* queue full, discard something while preserving motion. */
+ QUEUE_INCR(s->head);
+ usb_pointer_event_combine(&s->queue[s->head],
+ hs->kind == USB_MOUSE,
+ s->queue[use_slot].xdx,
+ s->queue[use_slot].ydy,
+ s->queue[use_slot].dz);
+ }
+ usb_pointer_event_clear(&s->queue[use_slot], buttons_state);
+ }
+ usb_pointer_event_combine(&s->queue[use_slot],
+ hs->kind == USB_MOUSE,
+ x1, y1, z1);
usb_hid_changed(hs);
}
@@ -506,83 +544,91 @@ static inline int int_clamp(int val, int vmin, int vmax)
return val;
}
-static int usb_mouse_poll(USBHIDState *hs, uint8_t *buf, int len)
+static int usb_pointer_poll(USBHIDState *hs, uint8_t *buf, int len)
{
int dx, dy, dz, b, l;
+ int index;
USBMouseState *s = &hs->ptr;
+ USBPointerEvent *e;
if (!s->mouse_grabbed) {
qemu_activate_mouse_event_handler(s->eh_entry);
- s->mouse_grabbed = 1;
+ s->mouse_grabbed = 1;
}
- dx = int_clamp(s->dx, -127, 127);
- dy = int_clamp(s->dy, -127, 127);
- dz = int_clamp(s->dz, -127, 127);
+ /* When the buffer is empty, return the last event. Relative
+ movements will all be zero. */
+ index = (hs->have_data ? s->head : s->head - 1);
+ e = &s->queue[index & QUEUE_MASK];
- s->dx -= dx;
- s->dy -= dy;
- s->dz -= dz;
-
- /* Appears we have to invert the wheel direction */
- dz = 0 - dz;
+ if (hs->kind == USB_MOUSE) {
+ dx = int_clamp(e->xdx, -127, 127);
+ dy = int_clamp(e->ydy, -127, 127);
+ e->xdx -= dx;
+ e->ydy -= dy;
+ } else {
+ dx = e->xdx;
+ dy = e->ydy;
+ }
+ dz = int_clamp(e->dz, -127, 127);
+ e->dz -= dz;
b = 0;
- if (s->buttons_state & MOUSE_EVENT_LBUTTON)
+ if (e->buttons_state & MOUSE_EVENT_LBUTTON)
b |= 0x01;
- if (s->buttons_state & MOUSE_EVENT_RBUTTON)
+ if (e->buttons_state & MOUSE_EVENT_RBUTTON)
b |= 0x02;
- if (s->buttons_state & MOUSE_EVENT_MBUTTON)
+ if (e->buttons_state & MOUSE_EVENT_MBUTTON)
b |= 0x04;
- l = 0;
- if (len > l)
- buf[l ++] = b;
- if (len > l)
- buf[l ++] = dx;
- if (len > l)
- buf[l ++] = dy;
- if (len > l)
- buf[l ++] = dz;
- return l;
-}
-
-static int usb_tablet_poll(USBHIDState *hs, uint8_t *buf, int len)
-{
- int dz, b, l;
- USBMouseState *s = &hs->ptr;
-
- if (!s->mouse_grabbed) {
- qemu_activate_mouse_event_handler(s->eh_entry);
- s->mouse_grabbed = 1;
+ if (hs->have_data &&
+ !e->dz &&
+ (hs->kind == USB_TABLET || (!e->xdx && !e->ydy))) {
+ /* that deals with this event */
+ QUEUE_INCR(s->head);
+ hs->have_data = (s->head != s->tail);
}
- dz = int_clamp(s->dz, -127, 127);
- s->dz -= dz;
-
/* Appears we have to invert the wheel direction */
dz = 0 - dz;
- b = 0;
- if (s->buttons_state & MOUSE_EVENT_LBUTTON)
- b |= 0x01;
- if (s->buttons_state & MOUSE_EVENT_RBUTTON)
- b |= 0x02;
- if (s->buttons_state & MOUSE_EVENT_MBUTTON)
- b |= 0x04;
+ l = 0;
+ switch (hs->kind) {
+ case USB_MOUSE:
+ if (len > l)
+ buf[l++] = b;
+ if (len > l)
+ buf[l++] = dx;
+ if (len > l)
+ buf[l++] = dy;
+ if (len > l)
+ buf[l++] = dz;
+ break;
- buf[0] = b;
- buf[1] = s->x & 0xff;
- buf[2] = s->x >> 8;
- buf[3] = s->y & 0xff;
- buf[4] = s->y >> 8;
- buf[5] = dz;
- l = 6;
+ case USB_TABLET:
+ if (len > l)
+ buf[l++] = b;
+ if (len > l)
+ buf[l++] = dx & 0xff;
+ if (len > l)
+ buf[l++] = dx >> 8;
+ if (len > l)
+ buf[l++] = dy & 0xff;
+ if (len > l)
+ buf[l++] = dy >> 8;
+ if (len > l)
+ buf[l++] = dz;
+ break;
+
+ default:
+ abort();
+ }
return l;
}
-static int usb_keyboard_poll(USBKeyboardState *s, uint8_t *buf, int len)
+static int usb_keyboard_poll(USBHIDState *hs, uint8_t *buf, int len)
{
+ USBKeyboardState *s = &hs->kbd;
if (len < 2)
return 0;
@@ -593,6 +639,7 @@ static int usb_keyboard_poll(USBKeyboardState *s, uint8_t *buf, int len)
else
memcpy(buf + 2, s->key, MIN(8, len) - 2);
+ hs->have_data = 0;
return MIN(8, len);
}
@@ -621,12 +668,10 @@ static void usb_mouse_handle_reset(USBDevice *dev)
{
USBHIDState *s = (USBHIDState *)dev;
- s->ptr.dx = 0;
- s->ptr.dy = 0;
- s->ptr.dz = 0;
- s->ptr.x = 0;
- s->ptr.y = 0;
- s->ptr.buttons_state = 0;
+ memset (s->ptr.queue, 0, sizeof (s->ptr.queue));
+ s->ptr.head = 0;
+ s->ptr.tail = 0;
+ s->have_data = 0;
s->protocol = 1;
}
@@ -777,12 +822,10 @@ static int usb_hid_handle_control(USBDevice *dev, int request, int value,
}
break;
case GET_REPORT:
- if (s->kind == USB_MOUSE)
- ret = usb_mouse_poll(s, data, length);
- else if (s->kind == USB_TABLET)
- ret = usb_tablet_poll(s, data, length);
+ if (s->kind == USB_MOUSE || s->kind == USB_TABLET)
+ ret = usb_pointer_poll(s, data, length);
else if (s->kind == USB_KEYBOARD)
- ret = usb_keyboard_poll(&s->kbd, data, length);
+ ret = usb_keyboard_poll(s, data, length);
break;
case SET_REPORT:
if (s->kind == USB_KEYBOARD)
@@ -831,13 +874,13 @@ static int usb_hid_handle_data(USBDevice *dev, USBPacket *p)
if (!s->changed && (!s->idle || s->next_idle_clock - curtime > 0))
return USB_RET_NAK;
usb_hid_set_next_idle(s, curtime);
- s->changed = 0;
- if (s->kind == USB_MOUSE)
- ret = usb_mouse_poll(s, p->data, p->len);
- else if (s->kind == USB_TABLET)
- ret = usb_tablet_poll(s, p->data, p->len);
- else if (s->kind == USB_KEYBOARD)
- ret = usb_keyboard_poll(&s->kbd, p->data, p->len);
+ if (s->kind == USB_MOUSE || s->kind == USB_TABLET) {
+ ret = usb_pointer_poll(s, p->data, p->len);
+ }
+ else if (s->kind == USB_KEYBOARD) {
+ ret = usb_keyboard_poll(s, p->data, p->len);
+ }
+ s->changed = s->have_data;
} else {
goto fail;
}
@@ -871,14 +914,15 @@ static int usb_hid_initfn(USBDevice *dev, int kind)
s->kind = kind;
if (s->kind == USB_MOUSE) {
- s->ptr.eh_entry = qemu_add_mouse_event_handler(usb_mouse_event, s,
+ s->ptr.eh_entry = qemu_add_mouse_event_handler(usb_pointer_event, s,
0, "QEMU USB Mouse");
} else if (s->kind == USB_TABLET) {
- s->ptr.eh_entry = qemu_add_mouse_event_handler(usb_tablet_event, s,
+ s->ptr.eh_entry = qemu_add_mouse_event_handler(usb_pointer_event, s,
1, "QEMU USB Tablet");
}
-
+
/* Force poll routine to be run and grab input the first time. */
+ s->have_data = 1;
s->changed = 1;
return 0;
}
--
1.7.3.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] Re: [PATCH] add event queueing to USB HID
2010-12-23 14:57 [Qemu-devel] [PATCH] add event queueing to USB HID Paolo Bonzini
@ 2011-01-05 17:06 ` Stefano Stabellini
2011-01-07 7:59 ` Gerd Hoffmann
1 sibling, 0 replies; 6+ messages in thread
From: Stefano Stabellini @ 2011-01-05 17:06 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Gerd Hoffman, Ian Jackson, qemu-devel, Stefano Stabellini
On Thu, 23 Dec 2010, Paolo Bonzini wrote:
> The polling nature of the USB HID device makes it very hard to double
> click or drag while on a high-latency VNC connection. This patch,
> based on work done in the Xen qemu-dm tree by Ian Jackson, fixes this
> bug by adding an event queue to the device. The event queue associates
> each movement with the correct button state (and each button state change
> with the correct location); it also remembers all button presses and
> releases as well.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Gerd Hoffman <kraxel@redhat.com>
Thanks for sending this upstream!
I meant to do it for quite some time now...
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] Re: [PATCH] add event queueing to USB HID
2010-12-23 14:57 [Qemu-devel] [PATCH] add event queueing to USB HID Paolo Bonzini
2011-01-05 17:06 ` [Qemu-devel] " Stefano Stabellini
@ 2011-01-07 7:59 ` Gerd Hoffmann
2011-01-07 12:20 ` Ian Jackson
2011-01-10 9:05 ` Paolo Bonzini
1 sibling, 2 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2011-01-07 7:59 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Ian Jackson, qemu-devel, Stefano Stabellini
On 12/23/10 15:57, Paolo Bonzini wrote:
> The polling nature of the USB HID device makes it very hard to double
> click or drag while on a high-latency VNC connection. This patch,
> based on work done in the Xen qemu-dm tree by Ian Jackson, fixes this
> bug by adding an event queue to the device. The event queue associates
> each movement with the correct button state (and each button state change
> with the correct location); it also remembers all button presses and
> releases as well.
> @@ -68,7 +77,7 @@ typedef struct USBHIDState {
> int protocol;
> uint8_t idle;
> int64_t next_idle_clock;
> - int changed;
> + int have_data, changed;
What is the difference between have_data and changed?
Do you need both? And can't you just compare head and tail of the ring
instead?
I think it makes sense to do the same for the keyboard, which might even
simplify the code overall as both mouse and keyboard will work in a
simliar way then.
> +static void usb_pointer_event_clear(USBPointerEvent *e, int buttons) {
> + e->xdx = e->ydy = e->dz = 0;
> + e->buttons_state = buttons;
> +}
Code style.
>
> - usb_hid_changed(hs);
> +static void usb_pointer_event_combine(USBPointerEvent *e, int xyrel,
> + int x1, int y1, int z1) {
> + if (xyrel) {
Here too.
> + /* When the buffer is empty, return the last event.
Why can this happen in the first place? Shouldn't the device NAK polls
when it has no events to deliver?
cheers,
Gerd
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] Re: [PATCH] add event queueing to USB HID
2011-01-07 7:59 ` Gerd Hoffmann
@ 2011-01-07 12:20 ` Ian Jackson
2011-01-07 12:26 ` Ian Jackson
2011-01-10 9:05 ` Paolo Bonzini
1 sibling, 1 reply; 6+ messages in thread
From: Ian Jackson @ 2011-01-07 12:20 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Paolo Bonzini, qemu-devel, Stefano Stabellini
Gerd Hoffmann writes ("Re: [PATCH] add event queueing to USB HID"):
> On 12/23/10 15:57, Paolo Bonzini wrote:
> > @@ -68,7 +77,7 @@ typedef struct USBHIDState {
> > int protocol;
> > uint8_t idle;
> > int64_t next_idle_clock;
> > - int changed;
> > + int have_data, changed;
>
> What is the difference between have_data and changed?
> Do you need both? And can't you just compare head and tail of the ring
> instead?
I think you are right. In the version of this code in the qemu-xen
tree, I moved "changed" from USBHIDState to USBKeyboardState. The
USBPointerState struct doesn't contain either "changed" or
"have_data".
> I think it makes sense to do the same for the keyboard, which might even
> simplify the code overall as both mouse and keyboard will work in a
> simliar way then.
That would be a reasonable approach although I didn't do it in my
changes in qemu-xen. The queue isn't directly reuseable between the
two kinds of device because the queue entries don't have the same
type, so the amount of code sharing is still going to be limited
(barring heavy use of macros, which I didn't think would be popular).
In my version there is no usb_hid_changed function any more.
> > + /* When the buffer is empty, return the last event.
>
> Why can this happen in the first place? Shouldn't the device NAK polls
> when it has no events to deliver?
The actual USB HID protocol is based around the idea of a "report"
which is available all of the time. IIRC the device is only supposed
to start NAKing polls when "nothing has changed" if the host has done
a SET IDLE 0 to turn off idle event reporting.
Paulo: For reference, here is a diff of the relevant functionality.
It's against qemu 0.10.0 (ish) but it may be a better starting point
than what you used.
Ian.
diff --git a/hw/usb-hid.c b/hw/usb-hid.c
index c850a91..4d889d6 100644
--- a/hw/usb-hid.c
+++ b/hw/usb-hid.c
@@ -43,30 +43,41 @@
#define USB_TABLET 2
#define USB_KEYBOARD 3
-typedef struct USBMouseState {
- int dx, dy, dz, buttons_state;
- int x, y;
- int mouse_grabbed;
+typedef struct USBPointerEvent {
+ int xdx, ydy; /* relative iff it's a mouse, otherwise absolute */
+ int dz, buttons_state;
+} USBPointerEvent;
+
+#define QUEUELENSHIFT 4 /* 16 events should be enough for a triple-click */
+#define QUEUELEN (1u<<QUEUELENSHIFT)
+#define QUEUEINDEXMASK (QUEUELEN-1u)
+#define QUEUE_INCR(v) ((v)++, (v) &= QUEUEINDEXMASK)
+
+typedef struct USBPointerState {
+ USBPointerEvent queue[QUEUELEN];
+ unsigned head, tail; /* indices into circular queue */
+
+ int mouse_grabbed, xyrel;
QEMUPutMouseEntry *eh_entry;
-} USBMouseState;
+} USBPointerState;
typedef struct USBKeyboardState {
uint16_t modifiers;
uint8_t leds;
uint8_t key[16];
int keys;
+ int changed;
} USBKeyboardState;
typedef struct USBHIDState {
USBDevice dev;
union {
- USBMouseState ptr;
+ USBPointerState ptr;
USBKeyboardState kbd;
};
int kind;
int protocol;
uint8_t idle;
- int changed;
void *datain_opaque;
void (*datain)(void *);
} USBHIDState;
@@ -404,40 +415,64 @@ static const uint8_t usb_hid_usage_keys[0x100] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
};
-static void usb_hid_changed(USBHIDState *hs)
+static void usb_notify_datain_cb(USBHIDState *hs)
{
- hs->changed = 1;
-
if (hs->datain)
hs->datain(hs->datain_opaque);
}
-static void usb_mouse_event(void *opaque,
- int dx1, int dy1, int dz1, int buttons_state)
-{
- USBHIDState *hs = opaque;
- USBMouseState *s = &hs->ptr;
-
- s->dx += dx1;
- s->dy += dy1;
- s->dz += dz1;
- s->buttons_state = buttons_state;
-
- usb_hid_changed(hs);
+static void usb_pointer_event_clear(USBPointerEvent *e, int buttons) {
+ e->xdx = e->ydy = e->dz = 0;
+ e->buttons_state = buttons;
}
-static void usb_tablet_event(void *opaque,
- int x, int y, int dz, int buttons_state)
-{
- USBHIDState *hs = opaque;
- USBMouseState *s = &hs->ptr;
+static void usb_pointer_event_combine(USBPointerEvent *e, int xyrel,
+ int x1, int y1, int z1) {
+ if (xyrel) {
+ e->xdx += x1;
+ e->ydy += y1;
+ } else {
+ e->xdx = x1;
+ e->ydy = y1;
+ }
+ e->dz += z1;
+}
- s->x = x;
- s->y = y;
- s->dz += dz;
- s->buttons_state = buttons_state;
+static void usb_pointer_event(void *hs_v, int x1, int y1, int z1,
+ int buttons_state) {
+ /* We combine events where feasible to keep the queue small.
+ * We shouldn't combine anything with the first event with
+ * a particular button state, as that would change the
+ * location of the button state change. */
+ USBHIDState *hs= hs_v;
+ USBPointerState *s = &hs->ptr;
+ unsigned use_slot= (s->tail-1) & QUEUEINDEXMASK;
+ unsigned previous_slot= (use_slot-1) & QUEUEINDEXMASK;
+
+ if (s->tail == s->head) {
+ use_slot= s->tail;
+ QUEUE_INCR(s->tail);
+ usb_pointer_event_clear(&s->queue[use_slot], buttons_state);
+ } else if (use_slot == s->head ||
+ s->queue[use_slot].buttons_state != buttons_state ||
+ s->queue[previous_slot].buttons_state != buttons_state) {
+ /* can't or shouldn't combine this event with previous one */
+ use_slot= s->tail;
+ QUEUE_INCR(s->tail);
+ if (use_slot == s->head) {
+ /* queue full, oh well, discard something */
+ s->head++; s->head &= QUEUEINDEXMASK;
+ /* but we preserve the relative motions */
+ usb_pointer_event_combine(&s->queue[s->head], s->xyrel,
+ s->queue[use_slot].xdx,
+ s->queue[use_slot].ydy,
+ s->queue[use_slot].dz);
+ }
+ usb_pointer_event_clear(&s->queue[use_slot], buttons_state);
+ }
+ usb_pointer_event_combine(&s->queue[use_slot],s->xyrel, x1,y1,z1);
- usb_hid_changed(hs);
+ usb_notify_datain_cb(hs);
}
static void usb_keyboard_event(void *opaque, int keycode)
@@ -451,6 +486,9 @@ static void usb_keyboard_event(void *opaque, int keycode)
hid_code = usb_hid_usage_keys[key | ((s->modifiers >> 1) & (1 << 7))];
s->modifiers &= ~(1 << 8);
+ s->changed = 1;
+ usb_notify_datain_cb(hs);
+
switch (hid_code) {
case 0x00:
return;
@@ -475,7 +513,6 @@ static void usb_keyboard_event(void *opaque, int keycode)
if (s->key[i] == hid_code) {
s->key[i] = s->key[-- s->keys];
s->key[s->keys] = 0x00;
- usb_hid_changed(hs);
break;
}
if (i < 0)
@@ -490,8 +527,6 @@ static void usb_keyboard_event(void *opaque, int keycode)
} else
return;
}
-
- usb_hid_changed(hs);
}
static inline int int_clamp(int val, int vmin, int vmax)
@@ -504,85 +539,100 @@ static inline int int_clamp(int val, int vmin, int vmax)
return val;
}
-static int usb_mouse_poll(USBHIDState *hs, uint8_t *buf, int len)
+static int usb_suppress_report(USBHIDState *hs, int unchanged) {
+ /* TODO: Implement finite idle delays. */
+ return !hs->idle && unchanged; /* SET_IDLE 0 means do not report */
+}
+
+static int usb_pointer_poll(USBHIDState *hs, uint8_t *buf, int len)
{
int dx, dy, dz, b, l;
- USBMouseState *s = &hs->ptr;
+ USBPointerState *s = &hs->ptr;
+ USBPointerEvent *e;
if (!s->mouse_grabbed) {
- s->eh_entry = qemu_add_mouse_event_handler(usb_mouse_event, hs,
- 0, "QEMU USB Mouse");
+ s->eh_entry = qemu_add_mouse_event_handler(usb_pointer_event, hs,
+ !s->xyrel, "QEMU USB Pointer");
s->mouse_grabbed = 1;
}
- dx = int_clamp(s->dx, -127, 127);
- dy = int_clamp(s->dy, -127, 127);
- dz = int_clamp(s->dz, -127, 127);
-
- s->dx -= dx;
- s->dy -= dy;
- s->dz -= dz;
-
- /* Appears we have to invert the wheel direction */
- dz = 0 - dz;
+ if (usb_suppress_report(hs, s->head == s->tail))
+ return USB_RET_NAK;
- b = 0;
- if (s->buttons_state & MOUSE_EVENT_LBUTTON)
- b |= 0x01;
- if (s->buttons_state & MOUSE_EVENT_RBUTTON)
- b |= 0x02;
- if (s->buttons_state & MOUSE_EVENT_MBUTTON)
- b |= 0x04;
+ if (s->head == s->tail)
+ /* use the last report */
+ s->head = (s->head - 1) & QUEUEINDEXMASK;
- l = 0;
- if (len > l)
- buf[l ++] = b;
- if (len > l)
- buf[l ++] = dx;
- if (len > l)
- buf[l ++] = dy;
- if (len > l)
- buf[l ++] = dz;
- return l;
-}
+ e = &s->queue[s->head];
-static int usb_tablet_poll(USBHIDState *hs, uint8_t *buf, int len)
-{
- int dz, b, l;
- USBMouseState *s = &hs->ptr;
+ dz = int_clamp(e->dz, -128, 127);
- if (!s->mouse_grabbed) {
- s->eh_entry = qemu_add_mouse_event_handler(usb_tablet_event, hs,
- 1, "QEMU USB Tablet");
- s->mouse_grabbed = 1;
+ e->dz -= dz;
+ if (s->xyrel) {
+ dx = int_clamp(e->xdx, -128, 127);
+ dy = int_clamp(e->ydy, -128, 127);
+ e->xdx -= dx;
+ e->ydy -= dy;
+ } else {
+ dx = e->xdx;
+ dy = e->ydy;
}
-
- dz = int_clamp(s->dz, -127, 127);
- s->dz -= dz;
-
/* Appears we have to invert the wheel direction */
dz = 0 - dz;
+
+ if (!(e->dz ||
+ (s->xyrel && (e->xdx || e->ydy)))) {
+ /* that deals with this event */
+ QUEUE_INCR(s->head);
+ }
+
b = 0;
- if (s->buttons_state & MOUSE_EVENT_LBUTTON)
+ if (e->buttons_state & MOUSE_EVENT_LBUTTON)
b |= 0x01;
- if (s->buttons_state & MOUSE_EVENT_RBUTTON)
+ if (e->buttons_state & MOUSE_EVENT_RBUTTON)
b |= 0x02;
- if (s->buttons_state & MOUSE_EVENT_MBUTTON)
+ if (e->buttons_state & MOUSE_EVENT_MBUTTON)
b |= 0x04;
- buf[0] = b;
- buf[1] = s->x & 0xff;
- buf[2] = s->x >> 8;
- buf[3] = s->y & 0xff;
- buf[4] = s->y >> 8;
- buf[5] = dz;
- l = 6;
+ switch (hs->kind) {
+ case USB_MOUSE:
+ l = 0;
+ if (len > l)
+ buf[l ++] = b;
+ if (len > l)
+ buf[l ++] = dx;
+ if (len > l)
+ buf[l ++] = dy;
+ if (len > l)
+ buf[l ++] = dz;
+ break;
+
+ case USB_TABLET:
+ buf[0] = b;
+ buf[1] = dx & 0xff;
+ buf[2] = dx >> 8;
+ buf[3] = dy & 0xff;
+ buf[4] = dy >> 8;
+ buf[5] = dz;
+ l = 6;
+ break;
+
+ default:
+ abort();
+ }
return l;
}
-static int usb_keyboard_poll(USBKeyboardState *s, uint8_t *buf, int len)
+static int usb_keyboard_poll(USBHIDState *hs, uint8_t *buf, int len)
{
+ USBKeyboardState *s= &hs->kbd;
+
+ if (usb_suppress_report(hs, !s->changed))
+ return USB_RET_NAK;
+
+ s->changed= 0;
+
if (len < 2)
return 0;
@@ -609,16 +659,11 @@ static int usb_keyboard_write(USBKeyboardState *s, uint8_t *buf, int len)
return 0;
}
-static void usb_mouse_handle_reset(USBDevice *dev)
+static void usb_pointer_handle_reset(USBDevice *dev)
{
USBHIDState *s = (USBHIDState *)dev;
- s->ptr.dx = 0;
- s->ptr.dy = 0;
- s->ptr.dz = 0;
- s->ptr.x = 0;
- s->ptr.y = 0;
- s->ptr.buttons_state = 0;
+ s->ptr.head= s->ptr.tail = 0;
s->protocol = 1;
}
@@ -764,12 +809,10 @@ static int usb_hid_handle_control(USBDevice *dev, int request, int value,
}
break;
case GET_REPORT:
- if (s->kind == USB_MOUSE)
- ret = usb_mouse_poll(s, data, length);
- else if (s->kind == USB_TABLET)
- ret = usb_tablet_poll(s, data, length);
+ if (s->kind == USB_MOUSE || s->kind == USB_TABLET)
+ ret = usb_pointer_poll(s, data, length);
else if (s->kind == USB_KEYBOARD)
- ret = usb_keyboard_poll(&s->kbd, data, length);
+ ret = usb_keyboard_poll(s, data, length);
break;
case SET_REPORT:
if (s->kind == USB_KEYBOARD)
@@ -813,16 +856,10 @@ static int usb_hid_handle_data(USBDevice *dev, USBPacket *p)
switch(p->pid) {
case USB_TOKEN_IN:
if (p->devep == 1) {
- /* TODO: Implement finite idle delays. */
- if (!(s->changed || s->idle))
- return USB_RET_NAK;
- s->changed = 0;
- if (s->kind == USB_MOUSE)
- ret = usb_mouse_poll(s, p->data, p->len);
- else if (s->kind == USB_TABLET)
- ret = usb_tablet_poll(s, p->data, p->len);
+ if (s->kind == USB_MOUSE || s->kind == USB_TABLET)
+ ret = usb_pointer_poll(s, p->data, p->len);
else if (s->kind == USB_KEYBOARD)
- ret = usb_keyboard_poll(&s->kbd, p->data, p->len);
+ ret = usb_keyboard_poll(s, p->data, p->len);
} else {
goto fail;
}
@@ -846,7 +883,7 @@ static void usb_hid_handle_destroy(USBDevice *dev)
qemu_free(s);
}
-USBDevice *usb_tablet_init(void)
+static USBDevice *usb_pointer_init(int kind, int xyrel, const char *devname)
{
USBHIDState *s;
@@ -854,38 +891,31 @@ USBDevice *usb_tablet_init(void)
s->dev.speed = USB_SPEED_FULL;
s->dev.handle_packet = usb_generic_handle_packet;
- s->dev.handle_reset = usb_mouse_handle_reset;
+ s->dev.handle_reset = usb_pointer_handle_reset;
s->dev.handle_control = usb_hid_handle_control;
s->dev.handle_data = usb_hid_handle_data;
s->dev.handle_destroy = usb_hid_handle_destroy;
- s->kind = USB_TABLET;
- /* Force poll routine to be run and grab input the first time. */
- s->changed = 1;
- pstrcpy(s->dev.devname, sizeof(s->dev.devname), "QEMU USB Tablet");
+ s->kind = kind;
+ s->ptr.xyrel = xyrel;
+ usb_pointer_handle_reset((USBDevice*)s);
+ /* Force poll routine to be run and grab input the first time. */
+ usb_pointer_event_clear(&s->ptr.queue[0], 0);
+ s->ptr.tail = 1;
+
+ pstrcpy(s->dev.devname, sizeof(s->dev.devname), devname);
return (USBDevice *)s;
}
-USBDevice *usb_mouse_init(void)
+USBDevice *usb_tablet_init(void)
{
- USBHIDState *s;
-
- s = qemu_mallocz(sizeof(USBHIDState));
- s->dev.speed = USB_SPEED_FULL;
- s->dev.handle_packet = usb_generic_handle_packet;
-
- s->dev.handle_reset = usb_mouse_handle_reset;
- s->dev.handle_control = usb_hid_handle_control;
- s->dev.handle_data = usb_hid_handle_data;
- s->dev.handle_destroy = usb_hid_handle_destroy;
- s->kind = USB_MOUSE;
- /* Force poll routine to be run and grab input the first time. */
- s->changed = 1;
-
- pstrcpy(s->dev.devname, sizeof(s->dev.devname), "QEMU USB Mouse");
+ return usb_pointer_init(USB_TABLET, 0, "QEMU USB Tablet");
+}
- return (USBDevice *)s;
+USBDevice *usb_mouse_init(void)
+{
+ return usb_pointer_init(USB_MOUSE, 1, "QEMU USB Mouse");
}
USBDevice *usb_keyboard_init(void)
@@ -901,6 +931,7 @@ USBDevice *usb_keyboard_init(void)
s->dev.handle_data = usb_hid_handle_data;
s->dev.handle_destroy = usb_hid_handle_destroy;
s->kind = USB_KEYBOARD;
+ s->kbd.changed= 0;
pstrcpy(s->dev.devname, sizeof(s->dev.devname), "QEMU USB Keyboard");
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] Re: [PATCH] add event queueing to USB HID
2011-01-07 12:20 ` Ian Jackson
@ 2011-01-07 12:26 ` Ian Jackson
0 siblings, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2011-01-07 12:26 UTC (permalink / raw)
To: Gerd Hoffmann, Paolo Bonzini, qemu-devel, Stefano Stabellini
I wrote:
> Paulo: For reference, here is a diff of the relevant functionality.
> It's against qemu 0.10.0 (ish) but it may be a better starting point
> than what you used.
I just had a go at seeing how that applies to current upstream master
and it doesn't look like it would be hard to fix up.
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] Re: [PATCH] add event queueing to USB HID
2011-01-07 7:59 ` Gerd Hoffmann
2011-01-07 12:20 ` Ian Jackson
@ 2011-01-10 9:05 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2011-01-10 9:05 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Ian Jackson, qemu-devel, Stefano Stabellini
On 01/07/2011 08:59 AM, Gerd Hoffmann wrote:
> On 12/23/10 15:57, Paolo Bonzini wrote:
>> The polling nature of the USB HID device makes it very hard to double
>> click or drag while on a high-latency VNC connection. This patch,
>> based on work done in the Xen qemu-dm tree by Ian Jackson, fixes this
>> bug by adding an event queue to the device. The event queue associates
>> each movement with the correct button state (and each button state change
>> with the correct location); it also remembers all button presses and
>> releases as well.
>
>> @@ -68,7 +77,7 @@ typedef struct USBHIDState {
>> int protocol;
>> uint8_t idle;
>> int64_t next_idle_clock;
>> - int changed;
>> + int have_data, changed;
>
> What is the difference between have_data and changed?
The difference is that after a reset have_data is zero (the queue is
empty) but changed will still be 1 if the poll routine has never run.
This matches the behavior of current QEMU. I also think Windows 2003
didn't boot without it, but I may remember wrong.
> Do you need both? And can't you just compare head and tail of the ring
> instead?
I don't think that would allow me to distinguish an entirely empty queue
and an entirely full queue? I added have_data after reading this this
code from Ian's patch:
+ if (s->tail == s->head) {
+ use_slot= s->tail;
+ QUEUE_INCR(s->tail);
+ usb_pointer_event_clear(&s->queue[use_slot], buttons_state);
+ } else if (use_slot == s->head ||
+ s->queue[use_slot].buttons_state != buttons_state ||
+ s->queue[previous_slot].buttons_state != buttons_state) {
+ /* can't or shouldn't combine this event with previous one */
+ use_slot= s->tail;
+ QUEUE_INCR(s->tail);
+ if (use_slot == s->head) {
+ /* queue full, oh well, discard something */
The first "if" tests the empty-queue case. Then, in the else, the same
test "value of s->tail on entry == value of s->head on entry" is used to
test for a full queue. The invariants on the queue were not documented
in the Xen patch; so, without unit testing and without an easy way to
test the full-queue case I preferred to be safe.
> I think it makes sense to do the same for the keyboard, which might even
> simplify the code overall as both mouse and keyboard will work in a
> simliar way then.
That would be a bit different because the keyboard events cannot be
merged. I thought about it, but it would be pretty much a completely
different patch.
Also, I couldn't even send Ctrl-Alt-Del to an USB keyboard on VNC in my
testing, which decreased substantially the priority of USB keyboard
buffering. It is possible that buffering would fix this, but then it
means that likely nobody is using the USB keyboard. In practice, the
USB tablet is very useful for Windows but the PS/2 keyboard is usually
good enough for everything.
>> + /* When the buffer is empty, return the last event.
>
> Why can this happen in the first place? Shouldn't the device NAK polls
> when it has no events to deliver?
See reply from Ian.
By the way, on further review this code:
> + } else if (use_slot == s->head ||
> + s->queue[use_slot].buttons_state != buttons_state ||
> + s->queue[previous_slot].buttons_state != buttons_state) {
looks like it should be instead
/* Only one event in queue */
use_slot == s->head ||
/* This is a button press or release */
s->queue[use_slot].buttons_state != buttons_state ||
/* use_slot was a button press or release */
s->queue[previous_slot].buttons_state !=
s->queue[use_slot].buttons_state
Any ideas?
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-01-10 9:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-23 14:57 [Qemu-devel] [PATCH] add event queueing to USB HID Paolo Bonzini
2011-01-05 17:06 ` [Qemu-devel] " Stefano Stabellini
2011-01-07 7:59 ` Gerd Hoffmann
2011-01-07 12:20 ` Ian Jackson
2011-01-07 12:26 ` Ian Jackson
2011-01-10 9:05 ` Paolo Bonzini
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.