[Qemu-devel] [PATCH] uhci: improved TD matching, working ISOC transfers

[Qemu-devel] [PATCH] uhci: improved TD matching, working ISOC transfers

[Max Krasnyansky]

While trying to make VX-3000 camera work on XP under KVM I realized that
we do not necessarily have to find original TD address. All we care about
is the token which identifies the transfer rather well (direction, endpoint,
size, etc).
This is especially important for the isochronous transfers because otherwise
they are being canceled left and right and we do not make much progress.

With this patch all devices that used bulk transfers that I've tried so
far continue to work just as well. And now my USB web cammera (isoc transfers)
is working well tool. It's not as smooth as native Windows but it's pretty
darn smooth.

The cool thing is that new USB code (both usb-uhci and usb-linux) is totaly
generic and does not need any special logic for ISOC.

Signed-off-by: Max Krasnyansky <maxk@kernel.org>
---
 hw/usb-uhci.c |   40 ++++++++++++++++++++++++++++------------
 1 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 1b15074..0714520 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -265,25 +265,41 @@ static void uhci_async_cancel_all(UHCIState *s)
 static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t addr, uint32_t token)
 {
     UHCIAsync *async = s->async_pending;
+    UHCIAsync *match = NULL;
+    int count = 0;
+
+    /* 
+     * We're looking for the best match here. ie both td addr and token.
+     * Otherwise we return last good match. ie just token.
+     * It's ok to match just token because it identifies the transaction
+     * rather well, token includes: device addr, endpoint, size, etc.
+     *
+     * Also since we queue async transactions in reverse order by returning 
+     * last good match we restores the order.
+     *
+     * It's expected that we wont have a ton of outstanding transactions.
+     * If we ever do we'd want to optimize this algorithm. 
+     */
 
     while (async) {
-        if (async->td == addr) {
-            if (async->token == token)
-                return async;
-
-            /*
-             * TD was reused for a different transfer.
-             * Invalidate the original one asap.
-             */
-            if (async->valid > 0) {
-                async->valid = 0;
-                dprintf("husb: bad reuse. td 0x%x\n", async->td);
+        if (async->token == token) {
+            /* Good match */
+            match = async;
+
+            if (async->td == addr) {
+                /* Best match */
+                break;
             }
         }
 
         async = async->next;
+        count++;
     }
-    return NULL;
+
+    if (count > 64)
+	fprintf(stderr, "uhci: warning lots of async transactions\n");
+
+    return match;
 }
 
 static void uhci_attach(USBPort *port1, USBDevice *dev);
-- 
1.5.5.1

Re: [Qemu-devel] [PATCH] uhci: improved TD matching, working ISOC transfers

[Aurelien Jarno]

On Fri, Aug 22, 2008 at 05:54:53AM +0000, Max Krasnyansky wrote:
> While trying to make VX-3000 camera work on XP under KVM I realized that
> we do not necessarily have to find original TD address. All we care about
> is the token which identifies the transfer rather well (direction, endpoint,
> size, etc).
> This is especially important for the isochronous transfers because otherwise
> they are being canceled left and right and we do not make much progress.
> 
> With this patch all devices that used bulk transfers that I've tried so
> far continue to work just as well. And now my USB web cammera (isoc transfers)
> is working well tool. It's not as smooth as native Windows but it's pretty
> darn smooth.
> 
> The cool thing is that new USB code (both usb-uhci and usb-linux) is totaly
> generic and does not need any special logic for ISOC.
> 
> Signed-off-by: Max Krasnyansky <maxk@kernel.org>

Thanks, applied.

> ---
>  hw/usb-uhci.c |   40 ++++++++++++++++++++++++++++------------
>  1 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
> index 1b15074..0714520 100644
> --- a/hw/usb-uhci.c
> +++ b/hw/usb-uhci.c
> @@ -265,25 +265,41 @@ static void uhci_async_cancel_all(UHCIState *s)
>  static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t addr, uint32_t token)
>  {
>      UHCIAsync *async = s->async_pending;
> +    UHCIAsync *match = NULL;
> +    int count = 0;
> +
> +    /* 
> +     * We're looking for the best match here. ie both td addr and token.
> +     * Otherwise we return last good match. ie just token.
> +     * It's ok to match just token because it identifies the transaction
> +     * rather well, token includes: device addr, endpoint, size, etc.
> +     *
> +     * Also since we queue async transactions in reverse order by returning 
> +     * last good match we restores the order.
> +     *
> +     * It's expected that we wont have a ton of outstanding transactions.
> +     * If we ever do we'd want to optimize this algorithm. 
> +     */
>  
>      while (async) {
> -        if (async->td == addr) {
> -            if (async->token == token)
> -                return async;
> -
> -            /*
> -             * TD was reused for a different transfer.
> -             * Invalidate the original one asap.
> -             */
> -            if (async->valid > 0) {
> -                async->valid = 0;
> -                dprintf("husb: bad reuse. td 0x%x\n", async->td);
> +        if (async->token == token) {
> +            /* Good match */
> +            match = async;
> +
> +            if (async->td == addr) {
> +                /* Best match */
> +                break;
>              }
>          }
>  
>          async = async->next;
> +        count++;
>      }
> -    return NULL;
> +
> +    if (count > 64)
> +	fprintf(stderr, "uhci: warning lots of async transactions\n");
> +
> +    return match;
>  }
>  
>  static void uhci_attach(USBPort *port1, USBDevice *dev);
> -- 
> 1.5.5.1
> 
> 
> 
> 

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net