On Mon, 5 Apr 2010, Nate Case wrote:
> Nice! Leaking the sitds eliminated the problem for both my custom board
> setup with an ISP1568A controller, and also the VT6212 controller setup.
> Sounds like your original idea of delaying their re-use is what needs to
> be implemented.
Okay. Below is a patch that should do what we need.
> If you would like hardware to replicate this problem, let me know and I
> could probably get some shipped out to you.
>
> On the usbaudio side, I wonder if something still needs to be done about
> the sync URBs for this case. If I re-enable them (SYNC_URBS = 2) after
> making the change to leak sitds, my audio loopback tests fail. My
> playback/record timing seemed to be screwed up and I eventually get XRUN
> on ALSA writes. It could be a problem with my own code since
> aplay/arecord seem to work properly.
That appears to be a separate issue. Clemens will be better able to
help you with it, assuming it's not an error in your program.
But we need to discuss the question of the number of sync URBs. The
bug fix from before will cause usbaudio to fail on some systems, unless
the number is reduced to 2. And endpoints with higher periods will
cause even more trouble.
Clemens, the problem is that ehci-hcd uses an isochronous scheduling
horizon of only 256 ms, if the hardware permits. Allowing for
interrupt delays and other latency reduces the horizon to 236 ms. For
each endpoint, the next unallocated frame must fall within the horizon.
As a result, if an endpoint has a period of 64 frames, you won't always
be able to have three URBs in flight. For example, suppose URB U is
scheduled for frame N and the completion routine is therefore called
during frame N+1. Assuming URBs U+1 and U+2 have already been queued,
consider what happens when the completion routine tries to submit URB
U+3: U+1 is scheduled to start in frame N+64 and U+2 is scheduled to
start in N+128, so U+3 will be scheduled to start in N+192, and the
first unallocated frame would then be N+256. But that's 255 frames
past the current time, so it's beyond the horizon and the submission
will fail.
Similarly, if an isochronous endpoint has a period of 128 frames then
it won't even be possible always to have one URB in flight -- which
would of course be a disaster.
The only reason things worked in the past was because of a bug in
ehci-hcd. The question is, now that the bug has been identified, what
should we do about these long-period isochronous endpoints?
If you know that it won't be necessary to deal with periods larger than
64, then reducing the number of URBs to two will work. Alternatively,
ehci-hcd could avoid using such a short horizon, although that would
mean doing more work when scheduling interrupt URBs.
What do you think?
Alan Stern
Index: usb-2.6/drivers/usb/host/ehci.h
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci.h
+++ usb-2.6/drivers/usb/host/ehci.h
@@ -87,8 +87,9 @@ struct ehci_hcd { /* one per controlle
int next_uframe; /* scan periodic, start here */
unsigned periodic_sched; /* periodic activity count */
- /* list of itds completed while clock_frame was still active */
+ /* list of itds & sitds completed while clock_frame was still active */
struct list_head cached_itd_list;
+ struct list_head cached_sitd_list;
unsigned clock_frame;
/* per root hub port */
@@ -195,7 +196,7 @@ timer_action_done (struct ehci_hcd *ehci
clear_bit (action, &ehci->actions);
}
-static void free_cached_itd_list(struct ehci_hcd *ehci);
+static void free_cached_lists(struct ehci_hcd *ehci);
/*-------------------------------------------------------------------------*/
Index: usb-2.6/drivers/usb/host/ehci-mem.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-mem.c
+++ usb-2.6/drivers/usb/host/ehci-mem.c
@@ -136,7 +136,7 @@ static inline void qh_put (struct ehci_q
static void ehci_mem_cleanup (struct ehci_hcd *ehci)
{
- free_cached_itd_list(ehci);
+ free_cached_lists(ehci);
if (ehci->async)
qh_put (ehci->async);
ehci->async = NULL;
Index: usb-2.6/drivers/usb/host/ehci-sched.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-sched.c
+++ usb-2.6/drivers/usb/host/ehci-sched.c
@@ -510,7 +510,7 @@ static int disable_periodic (struct ehci
ehci_writel(ehci, cmd, &ehci->regs->command);
/* posted write ... */
- free_cached_itd_list(ehci);
+ free_cached_lists(ehci);
ehci->next_uframe = -1;
return 0;
@@ -2128,13 +2128,27 @@ sitd_complete (
(stream->bEndpointAddress & USB_DIR_IN) ? "in" : "out");
}
iso_stream_put (ehci, stream);
- /* OK to recycle this SITD now that its completion callback ran. */
+
done:
sitd->urb = NULL;
- sitd->stream = NULL;
- list_move(&sitd->sitd_list, &stream->free_list);
- iso_stream_put(ehci, stream);
-
+ if (ehci->clock_frame != sitd->frame) {
+ /* OK to recycle this SITD now. */
+ sitd->stream = NULL;
+ list_move(&sitd->sitd_list, &stream->free_list);
+ iso_stream_put(ehci, stream);
+ } else {
+ /* HW might remember this SITD, so we can't recycle it yet.
+ * Move it to a safe place until a new frame starts.
+ */
+ list_move(&sitd->sitd_list, &ehci->cached_sitd_list);
+ if (stream->refcount == 2) {
+ /* If iso_stream_put() were called here, stream
+ * would be freed. Instead, just prevent reuse.
+ */
+ stream->ep->hcpriv = NULL;
+ stream->ep = NULL;
+ }
+ }
return retval;
}
@@ -2200,9 +2214,10 @@ done:
/*-------------------------------------------------------------------------*/
-static void free_cached_itd_list(struct ehci_hcd *ehci)
+static void free_cached_lists(struct ehci_hcd *ehci)
{
struct ehci_itd *itd, *n;
+ struct ehci_sitd *sitd, *sn;
list_for_each_entry_safe(itd, n, &ehci->cached_itd_list, itd_list) {
struct ehci_iso_stream *stream = itd->stream;
@@ -2210,6 +2225,13 @@ static void free_cached_itd_list(struct
list_move(&itd->itd_list, &stream->free_list);
iso_stream_put(ehci, stream);
}
+
+ list_for_each_entry_safe(sitd, sn, &ehci->cached_sitd_list, sitd_list) {
+ struct ehci_iso_stream *stream = sitd->stream;
+ sitd->stream = NULL;
+ list_move(&sitd->sitd_list, &stream->free_list);
+ iso_stream_put(ehci, stream);
+ }
}
/*-------------------------------------------------------------------------*/
@@ -2236,7 +2258,7 @@ scan_periodic (struct ehci_hcd *ehci)
clock_frame = -1;
}
if (ehci->clock_frame != clock_frame) {
- free_cached_itd_list(ehci);
+ free_cached_lists(ehci);
ehci->clock_frame = clock_frame;
}
clock %= mod;
@@ -2403,7 +2425,7 @@ restart:
clock = now;
clock_frame = clock >> 3;
if (ehci->clock_frame != clock_frame) {
- free_cached_itd_list(ehci);
+ free_cached_lists(ehci);
ehci->clock_frame = clock_frame;
}
} else {
--
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
