At Thu, 17 Feb 2011 16:43:28 +0100,
Takashi Iwai wrote:
>
> [OK, moved now to linux-usb ML...]
>
> At Fri, 11 Feb 2011 10:44:57 -0800,
> Sarah Sharp wrote:
> >
> > On Fri, Feb 11, 2011 at 11:11:29AM +0100, Takashi Iwai wrote:
> > > Actually the audio on xhci seems broken, so far.
> > > Recently I tested usb audio devices with xhci on new HP laptops, and
> > > got several different errors per kernel versions.
> >
> > I would be interested if you could send those errors to the linux-usb
> > mailing list and Cc me.
> >
> > > I had no time to dig into the issue yet, but if it's related with
> > > bandwidth setup, we'd need definitely to work on it together.
> >
> > The HP laptops have an NEC host controller in them, and I have heard
> > some complaints about their isoc support. They do have a firmware update
> > that improves that support, so you may want to google around for that.
> >
> > The other issue is the xHCI driver design. Currently, with 0.96 xHCI
> > hardware, we get multiple interrupts per isoc URB (as many interrupts as
> > there are isoc buffers). I have a work around in mind, but it involves
> > updating the xHCI ring code, which is always somewhat hairy and may take
> > a month or so. However, for xHCI 1.0 host controllers, the spec
> > architects have made some improvements that will easily allow us to get
> > less interrupts (once per URB, or even less if the driver submits
> > several URBs and marks some of them with the URB_NO_INTERRUPT flag).
> > That only involves setting one bit, so that would be an easy one day
> > fix.
> >
> > Then there's the fact that the xHCI driver's ring code is recursive. :(
> > I wrote it at 3 in the morning a couple days before a demo, and I really
> > need to fix that ASAP. There's also still performance issues, like
> > register reads in the interrupt path that really should be removed.
> >
> > But we should really pull in the linux-usb mailing list into any further
> > discussion of this. :)
>
> Looking down more at the errors, it seems that the controller stalls
> after some point. First, "short transfer on control ep" comes up once
> or twice:
>
> [ 1391.490134] xhci_hcd 0000:27:00.0: WARN: short transfer on control ep
> [ 1391.479142] xhci_hcd 0000:27:00.0: WARN: short transfer on control ep
>
> Then it starts "Stalled endpoint":
>
> [ 1391.498122] xhci_hcd 0000:27:00.0: WARN: Stalled endpoint
> [ 1391.505114] xhci_hcd 0000:27:00.0: WARN: Stalled endpoint
> [ 1391.512114] xhci_hcd 0000:27:00.0: WARN: Stalled endpoint
> ...
>
> At each time a stall happens, 3 trbs are queued up and pending. So,
> after repeating this cycle, it finally reaches to "no room":
>
> [ 1401.120101] xhci_hcd 0000:27:00.0: ERROR no room on ep ring
>
> The behavior looks consistent no matter which kernel version is used
> between 2.6.32 and 2.6.38-rc5. So, the problem is before isoc things,
> but in other points.
>
> FWIW, the ring causing this error is no ctrl but slot 1 ep 0.
>
> I tested with two USB audio 1.x devices, and both show the similar
> behavior.
>
> Can this be a better hint for further looking?
BTW, during checking this problem, I made up a patch like below
(for understanding the pending trbs).
Doesn't the handling of stalled trbs change the number of queued
items, or the recalculation of num_enqueued field below is needed?
thanks,
Takashi
---
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 1d0f45f..809ef4a 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -141,6 +141,7 @@ static void xhci_initialize_ring_info(struct xhci_ring *ring)
/* Not necessary for new rings, but needed for re-initialized rings */
ring->enq_updates = 0;
ring->deq_updates = 0;
+ ring->num_enqueued = 0;
}
/**
@@ -162,6 +163,7 @@ static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
return NULL;
INIT_LIST_HEAD(&ring->td_list);
+ ring->num_segs = num_segs;
if (num_segs == 0)
return ring;
@@ -191,6 +193,10 @@ static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
" segment %p (virtual), 0x%llx (DMA)\n",
prev, (unsigned long long)prev->dma);
}
+ if (link_trbs)
+ ring->num_trbs = ring->num_segs * (TRBS_PER_SEGMENT - 1);
+ else
+ ring->num_trbs = ring->num_segs * TRBS_PER_SEGMENT;
xhci_initialize_ring_info(ring);
return ring;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 3e8211c..4abf5b7 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -149,6 +149,8 @@ static void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring, bool consumer
unsigned long long addr;
ring->deq_updates++;
+ if (ring != xhci->event_ring)
+ ring->num_enqueued--;
/* Update the dequeue pointer further if that was a link TRB or we're at
* the end of an event ring segment (which doesn't have link TRBS)
*/
@@ -199,6 +201,8 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
chain = ring->enqueue->generic.field[3] & TRB_CHAIN;
next = ++(ring->enqueue);
+ if (ring != xhci->event_ring)
+ ring->num_enqueued++;
ring->enq_updates++;
/* Update the dequeue pointer further if that was a link TRB or we're at
@@ -255,54 +259,13 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
/*
* Check to see if there's room to enqueue num_trbs on the ring. See rules
* above.
- * FIXME: this would be simpler and faster if we just kept track of the number
- * of free TRBs in a ring.
*/
static int room_on_ring(struct xhci_hcd *xhci, struct xhci_ring *ring,
unsigned int num_trbs)
{
- int i;
- union xhci_trb *enq = ring->enqueue;
- struct xhci_segment *enq_seg = ring->enq_seg;
- struct xhci_segment *cur_seg;
- unsigned int left_on_ring;
-
- /* If we are currently pointing to a link TRB, advance the
- * enqueue pointer before checking for space */
- while (last_trb(xhci, ring, enq_seg, enq)) {
- enq_seg = enq_seg->next;
- enq = enq_seg->trbs;
- }
-
- /* Check if ring is empty */
- if (enq == ring->dequeue) {
- /* Can't use link trbs */
- left_on_ring = TRBS_PER_SEGMENT - 1;
- for (cur_seg = enq_seg->next; cur_seg != enq_seg;
- cur_seg = cur_seg->next)
- left_on_ring += TRBS_PER_SEGMENT - 1;
-
- /* Always need one TRB free in the ring. */
- left_on_ring -= 1;
- if (num_trbs > left_on_ring) {
- xhci_warn(xhci, "Not enough room on ring; "
- "need %u TRBs, %u TRBs left\n",
- num_trbs, left_on_ring);
- return 0;
- }
- return 1;
- }
- /* Make sure there's an extra empty TRB available */
- for (i = 0; i <= num_trbs; ++i) {
- if (enq == ring->dequeue)
- return 0;
- enq++;
- while (last_trb(xhci, ring, enq_seg, enq)) {
- enq_seg = enq_seg->next;
- enq = enq_seg->trbs;
- }
- }
- return 1;
+ int left = ring->num_trbs - ring->num_enqueued;
+ /* Always need one TRB free in the ring. */
+ return (left >= num_trbs + 1);
}
/* Ring the host controller doorbell after placing a command on the ring */
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7f236fd..f1fbc55 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1094,6 +1094,9 @@ struct xhci_ring {
struct xhci_segment *deq_seg;
unsigned int deq_updates;
struct list_head td_list;
+ unsigned int num_segs;
+ unsigned int num_trbs;
+ unsigned int num_enqueued;
/*
* Write the cycle state into the TRB cycle field to give ownership of
* the TRB to the host controller (if we are the producer), or to check
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
