Re: [RFC 3/3] EHCI: handle late isochronous submissions
On Thu, 29 Aug 2013, Ming Lei wrote:
On Wed, 28 Aug 2013 12:23:31 -0400 (EDT)
Alan Stern st...@rowland.harvard.edu wrote:
You must never alter ehci-last_iso_frame like this. It violates the
driver's invariants for time to run backward. After all, there may
already be other TDs scheduled for the frames you are about to scan
again; they mustn't get scanned until the frame pointer has wrapped
around.
This last problem in particular means your proposal can't be accepted.
On Thu, Aug 29, 2013 at 8:43 AM, Ming Lei tom.leim...@gmail.com wrote:
No, no other TDs scheduled for the frames since the queue is empty
when iso_stream_rebase is running.
The above isn't true, but no rescan on other TDs introduced:
- suppose URB0 is scheduled via stream0 when underrun without ISO_ASAP
- iso_stream_rebase() find that stream0-next_uframe is behind
ehci-last_iso_frame but URB0 isn't completed before
ehci-last_iso_frame, then adjust it as stream0-next_uframe 3,
and record its old value as ehci-last_iso_frame'.
- when next ehci irq comes, scan_isoc() scans from stream0-next_uframe
to 'now', and the extra scan introduced by iso_stream_rebase() is from
stream0-next_uframe to ehci-last_iso_frame' - 1, during this period,
only TDs belonged to undrun URBs without ISO_ASAP will scanned, and no
other TDs scheduled in these frames at all.(ehci-last_iso_frame doesn't
affect schedule, only decides start point of the next scan)
ehci-last_iso_frame does indeed affect the schedule:
base = ehci-last_iso_frame 3;
next = (next - base) (mod - 1);
start = (stream-next_uframe - base) (mod - 1);
/* Is the schedule already full? */
if (unlikely(start period)) {
ehci_dbg(ehci, iso sched full %p (%u-%u %u mod
%u)\n,
urb, stream-next_uframe, base,
period, mod);
status = -ENOSPC;
goto fail;
}
Note that start depends on base, which is derived from
ehci-last_iso_frame. If you decrease ehci-last_iso_frame, earlier
calculations using the larger value will now be incorrect.
So no sort of run 'backward' things, and URBs are always completed in
time order, aren't they?
No. Here's an example. I will exaggerate some of the values to help
make the point clear.
An SMI blocks interrupts for 100 ms. When the SMI ends, we
give back all the isochronous URBs on endpoints A and B. The
frame counter is now equal to 600, but the URBs we gave back
were scheduled to end in frame 520. The endpoint queues are
empty.
The completion handler for ep A submits an URB lasting for 1000
frames (11 packets with interval = 100); it is scheduled to
start in frame 620 and its last packet is scheduled for frame
596 after wrapping around.
The completion hanlder for ep B then runs, and it submits an
URB lasting 95 frames; it is scheduled to start in frame 521
and its last packet is scheduled for frame 615. Since this is
larger than the current frame counter, you rebase
ehci-last_iso_frame back to 520 or earlier.
Now the next time an interrupt occurs, the driver will scan
the last TD for ep A in frame 596. But that TD shouldn't be
scanned until 1020 ms have elapsed.
Like I said, this is exaggerated and probably will never happen. But
it is legal according to the API and therefore we have to allow it.
Besides, at this point it's not that much extra work for you to add the
sched-first_packet field. With that in place, you don't need to
rebase last_iso_frame, which removes the problem. It also allows the
driver to be improved by not scheduling any TDs that lie between
last_iso_frame and the current frame counter.
Of course, once you do this, your patch will start to look an awful lot
like mine.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 3/3] EHCI: handle late isochronous submissions
On Mon, 26 Aug 2013 14:50:53 -0400 (EDT)
Alan Stern st...@rowland.harvard.edu wrote:
This patch does the real work. It fixes up ehci-hcd so that an URB
submitted by a completion handler will keep the isochronous stream
alive, even if the handler was delayed by running in a tasklet and the
queue has emptied out.
As I mentioned earlier, this is not a simple change.
Alan Stern
drivers/usb/host/ehci-sched.c | 164
+-
drivers/usb/host/ehci.h |1
2 files changed, 101 insertions(+), 64 deletions(-)
Below is the approach I proposed(mentioned in another thread), which should be
simper than this one, any comments?
drivers/usb/host/ehci-sched.c | 53 ++---
drivers/usb/host/ehci.h |1 +
2 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 83be03f..80ef95d 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -1370,6 +1370,33 @@ sitd_slot_ok (
return 1;
}
+/* rebase in case of underun without ISO_ASAP together */
+static int iso_stream_rebase(struct ehci_hcd *ehci, struct urb *urb,
+ struct ehci_iso_stream *stream, u32 span, u32 period)
+{
+ u32 i, end;
+ u32 mod = ehci-periodic_size;
+
+ if (ehci-last_base == -1)
+ return 0;
+
+ end = ((stream-next_uframe + span - period) 3) (mod - 1);
+ for (i = ehci-last_base; i != ehci-last_iso_frame;
+ i = (i + 1) (mod - 1)) {
+ /* don't schedule URB which is behind base totally */
+ if (end == i) {
+ for (i = 0; i urb-number_of_packets; i++) {
+ urb-iso_frame_desc[i].length = 0;
+ urb-iso_frame_desc[i].status = 0;
+ }
+ return 1;
+ }
+ if (((stream-next_uframe 3) (mod - 1)) == i)
+ ehci-last_iso_frame = i;
+ }
+ return 0;
+}
+
/*
* This scheduler plans almost as far into the future as it has actual
* periodic schedule slots. (Affected by TUNE_FLS, which defaults to
@@ -1409,7 +1436,9 @@ iso_stream_schedule (
* (irq delays etc). If there are, the behavior depends on
* whether URB_ISO_ASAP is set.
*/
- if (likely (!list_empty (stream-td_list))) {
+ if (likely (!list_empty (stream-td_list) ||
+ hcd_complete_in_progress(
+ ehci_to_hcd(ehci), urb))) {
/* Take the isochronous scheduling threshold into account */
if (ehci-i_thresh)
@@ -1417,6 +1446,14 @@ iso_stream_schedule (
else
next = (now + 2 + 7) ~0x07; /* full frame cache */
+ if (list_empty(stream-td_list)
+ !(urb-transfer_flags URB_ISO_ASAP))
+ if (iso_stream_rebase(ehci, urb, stream, span,
+ period)) {
+ status = 1;
+ goto fail;
+ }
+
/*
* Use ehci-last_iso_frame as the base. There can't be any
* TDs scheduled for earlier than that.
@@ -1517,6 +1554,7 @@ iso_stream_schedule (
/* Make sure scan_isoc() sees these */
if (ehci-isoc_count == 0)
ehci-last_iso_frame = now 3;
+ ehci-last_base = -1;
return 0;
fail:
@@ -1838,7 +1876,10 @@ static int itd_submit (struct ehci_hcd *ehci, struct urb
*urb,
status = iso_stream_schedule(ehci, urb, stream);
if (likely (status == 0))
itd_link_urb (ehci, urb, ehci-periodic_size 3, stream);
- else
+ else if (status 0) {
+ ehci_urb_done(ehci, urb, 0);
+ status = 0;
+ } else
usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
done_not_linked:
spin_unlock_irqrestore (ehci-lock, flags);
@@ -2224,7 +2265,10 @@ static int sitd_submit (struct ehci_hcd *ehci, struct
urb *urb,
status = iso_stream_schedule(ehci, urb, stream);
if (status == 0)
sitd_link_urb (ehci, urb, ehci-periodic_size 3, stream);
- else
+ else if (status 0) {
+ ehci_urb_done(ehci, urb, 0);
+ status = 0;
+ } else
usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
done_not_linked:
spin_unlock_irqrestore (ehci-lock, flags);
@@ -2255,6 +2299,9 @@ static void scan_isoc(struct ehci_hcd *ehci)
}
ehci-now_frame = now_frame;
+ if (ehci-last_base == -1)
+ ehci-last_base = ehci-last_iso_frame;
+
frame = ehci-last_iso_frame;
for (;;) {
union ehci_shadow q,
Re: [RFC 3/3] EHCI: handle late isochronous submissions
On Wed, 28 Aug 2013, Ming Lei wrote:
Below is the approach I proposed(mentioned in another thread), which should be
simper than this one, any comments?
drivers/usb/host/ehci-sched.c | 53
++---
drivers/usb/host/ehci.h |1 +
2 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 83be03f..80ef95d 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -1370,6 +1370,33 @@ sitd_slot_ok (
return 1;
}
+/* rebase in case of underun without ISO_ASAP together */
+static int iso_stream_rebase(struct ehci_hcd *ehci, struct urb *urb,
+ struct ehci_iso_stream *stream, u32 span, u32 period)
+{
+ u32 i, end;
+ u32 mod = ehci-periodic_size;
+
+ if (ehci-last_base == -1)
+ return 0;
+
+ end = ((stream-next_uframe + span - period) 3) (mod - 1);
+ for (i = ehci-last_base; i != ehci-last_iso_frame;
+ i = (i + 1) (mod - 1)) {
+ /* don't schedule URB which is behind base totally */
+ if (end == i) {
+ for (i = 0; i urb-number_of_packets; i++) {
+ urb-iso_frame_desc[i].length = 0;
+ urb-iso_frame_desc[i].status = 0;
+ }
+ return 1;
+ }
+ if (((stream-next_uframe 3) (mod - 1)) == i)
+ ehci-last_iso_frame = i;
+ }
Why do you use a loop here? This looks like a straightforward thing to
test: Starting from last_base, which comes first: next_uframe or
last_iso_frame?
It's not a very good idea to change iso_frame_desc[i].length. HCDs
aren't suppose to touch that field. Also, why set the frame
descriptor status to 0? It should remain equal to -EXDEV, because the
frame never gets sent over the bus.
You must never alter ehci-last_iso_frame like this. It violates the
driver's invariants for time to run backward. After all, there may
already be other TDs scheduled for the frames you are about to scan
again; they mustn't get scanned until the frame pointer has wrapped
around.
This last problem in particular means your proposal can't be accepted.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 3/3] EHCI: handle late isochronous submissions
On Thu, Aug 29, 2013 at 12:23 AM, Alan Stern st...@rowland.harvard.edu wrote:
On Wed, 28 Aug 2013, Ming Lei wrote:
Below is the approach I proposed(mentioned in another thread), which should
be
simper than this one, any comments?
drivers/usb/host/ehci-sched.c | 53
++---
drivers/usb/host/ehci.h |1 +
2 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 83be03f..80ef95d 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -1370,6 +1370,33 @@ sitd_slot_ok (
return 1;
}
+/* rebase in case of underun without ISO_ASAP together */
+static int iso_stream_rebase(struct ehci_hcd *ehci, struct urb *urb,
+ struct ehci_iso_stream *stream, u32 span, u32 period)
+{
+ u32 i, end;
+ u32 mod = ehci-periodic_size;
+
+ if (ehci-last_base == -1)
+ return 0;
+
+ end = ((stream-next_uframe + span - period) 3) (mod - 1);
+ for (i = ehci-last_base; i != ehci-last_iso_frame;
+ i = (i + 1) (mod - 1)) {
+ /* don't schedule URB which is behind base totally */
+ if (end == i) {
+ for (i = 0; i urb-number_of_packets; i++) {
+ urb-iso_frame_desc[i].length = 0;
+ urb-iso_frame_desc[i].status = 0;
+ }
+ return 1;
+ }
+ if (((stream-next_uframe 3) (mod - 1)) == i)
+ ehci-last_iso_frame = i;
+ }
Why do you use a loop here? This looks like a straightforward thing to
test: Starting from last_base, which comes first: next_uframe or
last_iso_frame?
OK, that is simper.
It's not a very good idea to change iso_frame_desc[i].length. HCDs
aren't suppose to touch that field. Also, why set the frame
descriptor status to 0? It should remain equal to -EXDEV, because the
frame never gets sent over the bus.
Right.
You must never alter ehci-last_iso_frame like this. It violates the
driver's invariants for time to run backward. After all, there may
already be other TDs scheduled for the frames you are about to scan
No, no other TDs scheduled for the frames since the queue is empty
when iso_stream_rebase is running.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 3/3] EHCI: handle late isochronous submissions
This patch does the real work. It fixes up ehci-hcd so that an URB
submitted by a completion handler will keep the isochronous stream
alive, even if the handler was delayed by running in a tasklet and the
queue has emptied out.
As I mentioned earlier, this is not a simple change.
Alan Stern
drivers/usb/host/ehci-sched.c | 164 +-
drivers/usb/host/ehci.h |1
2 files changed, 101 insertions(+), 64 deletions(-)
Index: usb-3.11/drivers/usb/host/ehci.h
===
--- usb-3.11.orig/drivers/usb/host/ehci.h
+++ usb-3.11/drivers/usb/host/ehci.h
@@ -433,6 +433,7 @@ struct ehci_iso_packet {
struct ehci_iso_sched {
struct list_headtd_list;
unsignedspan;
+ unsignedfirst_packet;
struct ehci_iso_packet packet [0];
};
Index: usb-3.11/drivers/usb/host/ehci-sched.c
===
--- usb-3.11.orig/drivers/usb/host/ehci-sched.c
+++ usb-3.11/drivers/usb/host/ehci-sched.c
@@ -1379,8 +1379,6 @@ sitd_slot_ok (
* given EHCI_TUNE_FLS and the slop). Or, write a smarter scheduler!
*/
-#define SCHEDULING_DELAY 40 /* microframes */
-
static int
iso_stream_schedule (
struct ehci_hcd *ehci,
@@ -1388,10 +1386,12 @@ iso_stream_schedule (
struct ehci_iso_stream *stream
)
{
- u32 now, base, next, start, period, span;
- int status;
+ u32 now, base, next, start, period, span, now2;
+ u32 wrap = 0, skip = 0;
+ int status = 0;
unsignedmod = ehci-periodic_size 3;
struct ehci_iso_sched *sched = urb-hcpriv;
+ boolempty = list_empty(stream-td_list);
period = urb-interval;
span = sched-span;
@@ -1402,6 +1402,19 @@ iso_stream_schedule (
now = ehci_read_frame_index(ehci) (mod - 1);
+ /* Take the isochronous scheduling threshold into account */
+ if (ehci-i_thresh)
+ next = now + ehci-i_thresh;/* uframe cache */
+ else
+ next = (now + 2 + 7) ~0x07; /* full frame cache */
+
+ /*
+* Use ehci-last_iso_frame as the base. There can't be any
+* TDs scheduled for earlier than that.
+*/
+ base = ehci-last_iso_frame 3;
+ next = (next - base) (mod - 1);
+
/*
* Need to schedule; when's the next (u)frame we could start?
* This is bigger than ehci-i_thresh allows; scheduling itself
@@ -1409,19 +1422,17 @@ iso_stream_schedule (
* can also help high bandwidth if the dma and irq loads don't
* jump until after the queue is primed.
*/
- if (unlikely(list_empty(stream-td_list))) {
- int done = 0;
-
- base = now ~0x07;
- start = base + SCHEDULING_DELAY;
+ if (unlikely(empty !urb-ep-giveback_in_progress)) {
+ int done = 0;
+ u32 last;
/* find a uframe slot with enough bandwidth.
* Early uframes are more precious because full-speed
* iso IN transfers can't use late uframes,
* and therefore they should be allocated last.
*/
- next = start;
- start += period;
+ last = ((++ehci-random_frame) 3) (period - 1);
+ start = last + period;
do {
start--;
/* check schedule: enough space? */
@@ -1436,7 +1447,7 @@ iso_stream_schedule (
start, sched, period))
done = 1;
}
- } while (start next !done);
+ } while (start last !done);
/* no room in the schedule */
if (!done) {
@@ -1444,6 +1455,9 @@ iso_stream_schedule (
status = -ENOSPC;
goto fail;
}
+
+ start -= base;
+ goto do_ASAP;
}
/*
@@ -1452,72 +1466,85 @@ iso_stream_schedule (
* (irq delays etc). If there are, the behavior depends on
* whether URB_ISO_ASAP is set.
*/
- else {
+ start = (stream-next_uframe - base) (mod - 1);
+ now2 = (now - base) (mod - 1);
- /* Take the isochronous scheduling threshold into account */
- if (ehci-i_thresh)
- next = now + ehci-i_thresh;/* uframe cache */
- else
- next = (now + 2 + 7) ~0x07; /* full frame cache */
+ /* Is the schedule already full? */
+ if (unlikely(!empty start period)) {
+
