[Qemu-devel] Re: [PATCH] add event queueing to USB HID

[Paolo Bonzini]

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