From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=32983 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1Pb7EJ-00085G-Eu
	for qemu-devel@nongnu.org; Fri, 07 Jan 2011 02:59:24 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1Pb7EI-0005F8-Da
	for qemu-devel@nongnu.org; Fri, 07 Jan 2011 02:59:23 -0500
Received: from mx1.redhat.com ([209.132.183.28]:1026)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1Pb7EI-0005F4-5C
	for qemu-devel@nongnu.org; Fri, 07 Jan 2011 02:59:22 -0500
Message-ID: <4D26C7C8.1020203@redhat.com>
Date: Fri, 07 Jan 2011 08:59:04 +0100
From: Gerd Hoffmann <kraxel@redhat.com>
MIME-Version: 1.0
References: <1293116270-7069-1-git-send-email-pbonzini@redhat.com>
In-Reply-To: <1293116270-7069-1-git-send-email-pbonzini@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: [Qemu-devel] Re: [PATCH] add event queueing to USB HID
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>, qemu-devel@nongnu.org, Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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