On Thu, Feb 17, 2011 at 04:53:41PM +0100, Takashi Iwai wrote:
> 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?
Maybe. I've got a couple patches in my queue that I need to send to
Greg that involve fixes in the ring TRB counting calculation. Can you
reset against my for-usb-linus-pending branch in the xhci.git tree on
kernel.org and see if those patches fix your issue?
> BTW, during checking this problem, I made up a patch like below
> (for understanding the pending trbs).
Yes, I've been meaning to rip out that code and replace it with
something simpler like your patch.
> Doesn't the handling of stalled trbs change the number of queued
> items, or the recalculation of num_enqueued field below is needed?
The problem with stalls is that the xHCI driver issues a command to move
the dequeue pointer past the stalled transfer, which only completes
later. At the point the command completes, we really have no idea how
far the dequeue pointer moved, so the dequeue pointer doesn't get
updated. I suppose we could look at the command on the command ring and
see where we moved the dequeue pointer.
I think the dequeue pointer will get moved on the next completed
transfer. But if there's several stalled transfers, and no transfer
gets through, then we can't enqueue any more transfers, and the ring is
basically useless. I'll look and see if I can come up with a fix.
But why is the driver crashing if we tell it we can't enqueue any more
transfers? It should be able to deal with that case.
Sarah Sharp
> ---
> 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
