Re: [RFC] How to handle delays in isochronous transfers?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 11 Sep 2012, Clemens Ladisch wrote:

> > The test patch should be ready for posting soon.  Would you like to try
> > it out?
> 
> Yes (if I find time).

Here it is.  This is actually five separate patches combined into one.  
It implements the new documented behavior in ehci-hcd and adds 
debugging code for performing tests.  It also changes the snd-usb-audio 
driver by removing the URB_ISO_ASAP flag from the data URBs.

Here's how the testing code should be used.  The patch creates a file
named "test" under /sys/kernel/debug/usb/ehci/X/ where X is the PCI
device name of the particular controller (for example, 0000:00:1d.7).  
You write two integer values to the test file.  The first tells
ehci-hcd how many isochronous completion events to ignore and the
second tells ehci-hcd how many microseconds to delay before then
scanning for isochronous completions.

For example, with my USB audio device, the snd-usb-audio driver uses a 
queue of 2 iso data URBs each containing 3 packets (usually).  
Therefore I test by playing an audio file and doing:

	echo 2 1000 >test

while the audio is running.  The driver ignores the completion events
for the two URBs in the queue when they occur, and then handles them
1000 us after the second completion.  As a result, the next URB
submitted by the sound driver is one frame too late.  The submission
succeeds, but I can see the delay in a usbmon trace as well as the fact
that when the URB completes, its first packet gets a status of -EXDEV
instead of 0.

If instead I do "echo 2 3000 >test" then the next URB is submitted 3
frames too late.  Since the URB contains only 3 packets, all of its
slots have already expired.  The submission fails and the audio stops
playing, although the ogg123 program continues running and the synch
URBs continue to be submitted.

If the URB_ISO_ASAP flag is retained in the sound driver then the
delayed URB submission does not fail.  Instead there is a brief gap and
the audio continues, now out of synchronization.  In theory this gap
could be made as large as you want, although in practice the udelay()
routine has an upper limit on the size of the argument it accepts.

Alan Stern



Index: usb-3.6/drivers/usb/host/ehci.h
===================================================================
--- usb-3.6.orig/drivers/usb/host/ehci.h
+++ usb-3.6/drivers/usb/host/ehci.h
@@ -112,6 +112,7 @@ struct ehci_hcd {			/* one per controlle
 	__u32			hcs_params;	/* cached register copy */
 	spinlock_t		lock;
 	enum ehci_rh_state	rh_state;
+	int			test1, test2;
 
 	/* general schedule support */
 	bool			scanning:1;
@@ -143,7 +144,7 @@ struct ehci_hcd {			/* one per controlle
 	struct ehci_qh		*intr_unlink_last;
 	unsigned		intr_unlink_cycle;
 	unsigned		now_frame;	/* frame from HC hardware */
-	unsigned		next_frame;	/* scan periodic, start here */
+	unsigned		last_iso_frame;	/* last frame scanned for iso */
 	unsigned		intr_count;	/* intr activity count */
 	unsigned		isoc_count;	/* isoc activity count */
 	unsigned		periodic_count;	/* periodic activity count */
@@ -193,7 +194,6 @@ struct ehci_hcd {			/* one per controlle
 	unsigned		has_amcc_usb23:1;
 	unsigned		need_io_watchdog:1;
 	unsigned		amd_pll_fix:1;
-	unsigned		fs_i_thresh:1;	/* Intel iso scheduling */
 	unsigned		use_dummy_qh:1;	/* AMD Frame List table quirk*/
 	unsigned		has_synopsys_hc_bug:1; /* Synopsys HC */
 	unsigned		frame_index_bug:1; /* MosChip (AKA NetMos) */
Index: usb-3.6/drivers/usb/host/ehci-sched.c
===================================================================
--- usb-3.6.orig/drivers/usb/host/ehci-sched.c
+++ usb-3.6/drivers/usb/host/ehci-sched.c
@@ -1361,7 +1361,7 @@ sitd_slot_ok (
  * given EHCI_TUNE_FLS and the slop).  Or, write a smarter scheduler!
  */
 
-#define SCHEDULE_SLOP	80	/* microframes */
+#define SCHEDULING_DELAY	40	/* microframes */
 
 static int
 iso_stream_schedule (
@@ -1370,7 +1370,7 @@ iso_stream_schedule (
 	struct ehci_iso_stream	*stream
 )
 {
-	u32			now, next, start, period, span;
+	u32			now, base, next, start, period, span;
 	int			status;
 	unsigned		mod = ehci->periodic_size << 3;
 	struct ehci_iso_sched	*sched = urb->hcpriv;
@@ -1382,62 +1382,75 @@ iso_stream_schedule (
 		span <<= 3;
 	}
 
-	if (span > mod - SCHEDULE_SLOP) {
-		ehci_dbg (ehci, "iso request %p too long\n", urb);
-		status = -EFBIG;
-		goto fail;
-	}
-
 	now = ehci_read_frame_index(ehci) & (mod - 1);
 
 	/* Typical case: reuse current schedule, stream is still active.
 	 * Hopefully there are no gaps from the host falling behind
-	 * (irq delays etc), but if there are we'll take the next
-	 * slot in the schedule, implicitly assuming URB_ISO_ASAP.
+	 * (irq delays etc).  If there are, the behavior depends on
+	 * whether URB_ISO_ASAP is set.
 	 */
 	if (likely (!list_empty (&stream->td_list))) {
-		u32	excess;
 
-		/* For high speed devices, allow scheduling within the
-		 * isochronous scheduling threshold.  For full speed devices
-		 * and Intel PCI-based controllers, don't (work around for
-		 * Intel ICH9 bug).
-		 */
-		if (!stream->highspeed && ehci->fs_i_thresh)
-			next = now + ehci->i_thresh;
+		/* Take the isochronous scheduling threshold into account */
+		if (ehci->i_thresh)
+			next = now + ehci->i_thresh;	/* uframe cache */
 		else
-			next = now;
+			next = (now + 2 + 7) & ~0x07;	/* full frame cache */
 
-		/* Fell behind (by up to twice the slop amount)?
-		 * We decide based on the time of the last currently-scheduled
-		 * slot, not the time of the next available slot.
+		/*
+		 * Use ehci->last_iso_frame as the base.  There can't be any
+		 * TDs scheduled for earlier than that.
 		 */
-		excess = (stream->next_uframe - period - next) & (mod - 1);
-		if (excess >= mod - 2 * SCHEDULE_SLOP)
-			start = next + excess - mod + period *
-					DIV_ROUND_UP(mod - excess, period);
-		else
-			start = next + excess + period;
-		if (start - now >= mod) {
-			ehci_dbg(ehci, "request %p would overflow (%d+%d >= %d)\n",
-					urb, start - now - period, period,
-					mod);
-			status = -EFBIG;
+		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;
 		}
+
+		/* Behind the scheduling threshold? */
+		if (unlikely(start < next)) {
+
+			/* USB_ISO_ASAP: Use the first available slot */
+			if (urb->transfer_flags & URB_ISO_ASAP) {
+				start += period * DIV_ROUND_UP(next - start,
+						period);
+			}
+
+			/*
+			 * Not ASAP: Use the next slot in the stream.  If
+			 * the entire URB falls before the threshold, fail.
+			 */
+			else if (start + span - period < next) {
+				ehci_dbg(ehci, "iso urb late %p (%u+%u < %u)\n",
+						urb, start + base,
+						span - period, next + base);
+				status = -EXDEV;
+				goto fail;
+			}
+		}
+
+		start += base;
 	}
 
 	/* need to schedule; when's the next (u)frame we could start?
 	 * this is bigger than ehci->i_thresh allows; scheduling itself
-	 * isn't free, the slop should handle reasonably slow cpus.  it
+	 * isn't free, the delay should handle reasonably slow cpus.  it
 	 * can also help high bandwidth if the dma and irq loads don't
 	 * jump until after the queue is primed.
 	 */
 	else {
 		int done = 0;
-		start = SCHEDULE_SLOP + (now & ~0x07);
 
-		/* NOTE:  assumes URB_ISO_ASAP, to limit complexity/bugs */
+		base = now & ~0x07;
+		start = base + SCHEDULING_DELAY;
 
 		/* find a uframe slot with enough bandwidth.
 		 * Early uframes are more precious because full-speed
@@ -1464,19 +1477,16 @@ iso_stream_schedule (
 
 		/* no room in the schedule */
 		if (!done) {
-			ehci_dbg(ehci, "iso resched full %p (now %d max %d)\n",
-				urb, now, now + mod);
+			ehci_dbg(ehci, "iso sched full %p", urb);
 			status = -ENOSPC;
 			goto fail;
 		}
 	}
 
 	/* Tried to schedule too far into the future? */
-	if (unlikely(start - now + span - period
-				>= mod - 2 * SCHEDULE_SLOP)) {
-		ehci_dbg(ehci, "request %p would overflow (%d+%d >= %d)\n",
-				urb, start - now, span - period,
-				mod - 2 * SCHEDULE_SLOP);
+	if (unlikely(start - base + span - period >= mod)) {
+		ehci_dbg(ehci, "request %p would overflow (%u+%u >= %u)\n",
+				urb, start - base, span - period, mod);
 		status = -EFBIG;
 		goto fail;
 	}
@@ -1490,7 +1500,7 @@ iso_stream_schedule (
 
 	/* Make sure scan_isoc() sees these */
 	if (ehci->isoc_count == 0)
-		ehci->next_frame = now >> 3;
+		ehci->last_iso_frame = now >> 3;
 	return 0;
 
  fail:
@@ -1708,7 +1718,7 @@ static bool itd_complete(struct ehci_hcd
 			urb->actual_length += desc->actual_length;
 		} else {
 			/* URB was too late */
-			desc->status = -EXDEV;
+			urb->error_count++;
 		}
 	}
 
@@ -2081,7 +2091,7 @@ static bool sitd_complete(struct ehci_hc
 	t = hc32_to_cpup(ehci, &sitd->hw_results);
 
 	/* report transfer status */
-	if (t & SITD_ERRS) {
+	if (unlikely(t & SITD_ERRS)) {
 		urb->error_count++;
 		if (t & SITD_STS_DBE)
 			desc->status = usb_pipein (urb->pipe)
@@ -2091,6 +2101,9 @@ static bool sitd_complete(struct ehci_hc
 			desc->status = -EOVERFLOW;
 		else /* XACT, MMF, etc */
 			desc->status = -EPROTO;
+	} else if (unlikely(t & SITD_STS_ACTIVE)) {
+		/* URB was too late */
+		urb->error_count++;
 	} else {
 		desc->status = 0;
 		desc->actual_length = desc->length - SITD_LENGTH(t);
@@ -2210,6 +2223,18 @@ static void scan_isoc(struct ehci_hcd *e
 	unsigned	fmask = ehci->periodic_size - 1;
 	bool		modified, live;
 
+	if (ehci->test1 > 0) {
+		--ehci->test1;
+		ehci_dbg(ehci, "counter %d\n", ehci->test1);
+		if (ehci->test1 > 0)
+			return;
+		if (ehci->test2 > 0)
+			udelay(ehci->test2);
+		ehci_dbg(ehci, "after delay\n");
+	}
+
+ 	/* complete the unlinking of some qh [4.15.2.3] */
+
 	/*
 	 * When running, scan from last scan point up to "now"
 	 * else clean up by scanning everything that's left.
@@ -2220,16 +2245,16 @@ static void scan_isoc(struct ehci_hcd *e
 		now_frame = (uf >> 3) & fmask;
 		live = true;
 	} else  {
-		now_frame = (ehci->next_frame - 1) & fmask;
+		now_frame = (ehci->last_iso_frame - 1) & fmask;
 		live = false;
 	}
 	ehci->now_frame = now_frame;
 
-	frame = ehci->next_frame;
 	for (;;) {
 		union ehci_shadow	q, *q_p;
 		__hc32			type, *hw_p;
 
+		frame = ehci->last_iso_frame;
 restart:
 		/* scan each element in frame's queue for completions */
 		q_p = &ehci->pshadow [frame];
@@ -2334,7 +2359,6 @@ restart:
 		/* Stop when we have reached the current frame */
 		if (frame == now_frame)
 			break;
-		frame = (frame + 1) & fmask;
+		ehci->last_iso_frame = (frame + 1) & fmask;
 	}
-	ehci->next_frame = now_frame;
 }
Index: usb-3.6/drivers/usb/host/ehci-pci.c
===================================================================
--- usb-3.6.orig/drivers/usb/host/ehci-pci.c
+++ usb-3.6/drivers/usb/host/ehci-pci.c
@@ -103,7 +103,6 @@ static int ehci_pci_setup(struct usb_hcd
 		}
 		break;
 	case PCI_VENDOR_ID_INTEL:
-		ehci->fs_i_thresh = 1;
 		if (pdev->device == PCI_DEVICE_ID_INTEL_CE4100_USB)
 			hcd->has_tt = 1;
 		break;
Index: usb-3.6/drivers/usb/host/ehci-hcd.c
===================================================================
--- usb-3.6.orig/drivers/usb/host/ehci-hcd.c
+++ usb-3.6/drivers/usb/host/ehci-hcd.c
@@ -503,7 +503,7 @@ static int ehci_init(struct usb_hcd *hcd
 
 	/* controllers may cache some of the periodic schedule ... */
 	if (HCC_ISOC_CACHE(hcc_params))		// full frame cache
-		ehci->i_thresh = 2 + 8;
+		ehci->i_thresh = 0;
 	else					// N microframes cached
 		ehci->i_thresh = 2 + HCC_ISOC_THRES(hcc_params);
 
Index: usb-3.6/drivers/usb/host/ehci-dbg.c
===================================================================
--- usb-3.6.orig/drivers/usb/host/ehci-dbg.c
+++ usb-3.6/drivers/usb/host/ehci-dbg.c
@@ -1053,6 +1053,42 @@ static ssize_t debug_lpm_write(struct fi
 	return count;
 }
 
+static ssize_t debug_test_write(struct file *file, const char __user *user_buf,
+		size_t count, loff_t *ppos)
+{
+	struct ehci_hcd		*ehci = file->private_data;
+	char			buf[50];
+	size_t			len;
+	int			m, n;
+	struct timeval		tval;
+	unsigned		tstamp;
+
+	len = min(count, sizeof(buf) - 1);
+	if (copy_from_user(buf, user_buf, len))
+		return -EFAULT;
+	buf[len] = 0;
+
+	if (sscanf(buf, "%d %d", &m, &n) != 2)
+		return -EINVAL;
+
+	spin_lock_irq(&ehci->lock);
+	ehci->test1 = m;
+	ehci->test2 = n;
+	do_gettimeofday(&tval);
+	spin_unlock_irq(&ehci->lock);
+
+	tstamp = tval.tv_sec & 0xFFF;
+	tstamp = tstamp * 1000000 + tval.tv_usec;
+	ehci_dbg(ehci, "Test write at %u\n", tstamp);
+	return count;
+}
+
+static const struct file_operations debug_test_fops = {
+	.owner		= THIS_MODULE,
+	.open		= simple_open,
+	.write		= debug_test_write,
+};
+
 static inline void create_debug_files (struct ehci_hcd *ehci)
 {
 	struct usb_bus *bus = &ehci_to_hcd(ehci)->self;
@@ -1077,6 +1113,9 @@ static inline void create_debug_files (s
 						    &debug_lpm_fops))
 		goto file_error;
 
+	if (!debugfs_create_file("test", 0200, ehci->debug_dir, ehci,
+			&debug_test_fops))
+		goto file_error;
 	return;
 
 file_error:
Index: usb-3.6/sound/usb/endpoint.c
===================================================================
--- usb-3.6.orig/sound/usb/endpoint.c
+++ usb-3.6/sound/usb/endpoint.c
@@ -669,7 +669,7 @@ static int data_ep_set_params(struct snd
 		if (!u->urb->transfer_buffer)
 			goto out_of_memory;
 		u->urb->pipe = ep->pipe;
-		u->urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
+		u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
 		u->urb->interval = 1 << ep->datainterval;
 		u->urb->context = u;
 		u->urb->complete = snd_complete_urb;

--
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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux