Re: [PATCH] ehci-hcd: complete iso urbs ASAP for number_of_packets != (n * 8) also

On Saturday 10 November 2007, Karsten Wiese wrote:
> Here goes take 3: 

Can you give this updated version a try?  There were some glitches
in the hardware schedule updating vs (s)ITD recycling, plus you only
relocated part of the ITD completion path.  I also updated the patch
description.

This version is just compile-tested.

- Dave

======================
From: Karsten Wiese <[email protected]>

ehci-hcd: complete ISO URBs sooner

This patch lets some ISO URBs complete sooner, by scanning ITDs and sITDs up
to the current microframe and then reporting completions immediately.

Previously some completions for high speed transfers wouldn't be reported
right away, because the code waited until a complete frame was scanned and
such a scan might not happen immediately.  Soundcard drivers have used a
partial work around for this, transferring ISO urbs using entire frames.

ISO transfers now activate/deactivate the periodic schedule based on when
an DMA descriptor is added to the schedule, rather than slightly later when
the URB is fully scheduled.  The scanning logic is simplified a bit, making
the three results distinct:  a descriptor may still need processing; or it
may have completed, either with or without an URB completion callback.

---
 drivers/usb/host/ehci-sched.c |  181 ++++++++++++++++++++++--------------------
 drivers/usb/host/ehci.h       |    3 
 2 files changed, 96 insertions(+), 88 deletions(-)

--- g26.orig/drivers/usb/host/ehci-sched.c	2007-10-12 16:27:05.000000000 -0700
+++ g26/drivers/usb/host/ehci-sched.c	2007-11-11 23:56:43.000000000 -0800
@@ -1486,6 +1486,8 @@ itd_link (struct ehci_hcd *ehci, unsigne
 	itd->frame = frame;
 	wmb ();
 	ehci->periodic[frame] = cpu_to_hc32(ehci, itd->itd_dma | Q_TYPE_ITD);
+	if (unlikely(!ehci->periodic_sched++))
+		enable_periodic(ehci);
 }
 
 /* fit urb's itds into the selected schedule slot; activate as needed */
@@ -1558,30 +1560,33 @@ itd_link_urb (
 	urb->hcpriv = NULL;
 
 	timer_action (ehci, TIMER_IO_WATCHDOG);
-	if (unlikely (!ehci->periodic_sched++))
-		return enable_periodic (ehci);
 	return 0;
 }
 
+/* mask for return values used by itd_scan() and sitd_scan() */
+#define XITD_UNLINK	(1 << 0)	/* needs unlink from hw schedule */
+#define URB_COMPLETE	(1 << 1)	/* schedule may have new entries */
+
 #define	ISO_ERRS (EHCI_ISOC_BUF_ERR | EHCI_ISOC_BABBLE | EHCI_ISOC_XACTERR)
 
 static unsigned
-itd_complete (
-	struct ehci_hcd	*ehci,
-	struct ehci_itd	*itd
-) {
+itd_scan(struct ehci_hcd *ehci, struct ehci_itd *itd, unsigned uframe_after)
+{
 	struct urb				*urb = itd->urb;
 	struct usb_iso_packet_descriptor	*desc;
 	u32					t;
 	unsigned				uframe;
 	int					urb_index = -1;
 	struct ehci_iso_stream			*stream = itd->stream;
-	struct usb_device			*dev;
+	unsigned				complete = 0;
 
 	/* for each uframe with a packet */
-	for (uframe = 0; uframe < 8; uframe++) {
+	for (uframe = itd->uframe_scanned; uframe < uframe_after; uframe++) {
+		struct usb_device *dev;
+
 		if (likely (itd->index[uframe] == -1))
 			continue;
+
 		urb_index = itd->index[uframe];
 		desc = &urb->iso_frame_desc [urb_index];
 
@@ -1607,46 +1612,66 @@ itd_complete (
 		} else if (likely ((t & EHCI_ISOC_ACTIVE) == 0)) {
 			desc->status = 0;
 			desc->actual_length = EHCI_ITD_LENGTH (t);
+		} else {
+			/* SHOULD NOT HAPPEN!! */
+			WARN_ON(1);
 		}
-	}
 
-	usb_put_urb (urb);
-	itd->urb = NULL;
-	itd->stream = NULL;
-	list_move (&itd->itd_list, &stream->free_list);
-	iso_stream_put (ehci, stream);
-
-	/* handle completion now? */
-	if (likely ((urb_index + 1) != urb->number_of_packets))
-		return 0;
+		/* handle completion now? */
+		if (urb_index + 1 != urb->number_of_packets)
+			continue;
 
-	/* ASSERT: it's really the last itd for this urb
-	list_for_each_entry (itd, &stream->td_list, itd_list)
-		BUG_ON (itd->urb == urb);
-	 */
+#if 1
+		/* ASSERT: it's really the last itd for this urb */
+		{
+			struct ehci_itd	*tmp;
 
-	/* give urb back to the driver ... can be out-of-order */
-	dev = urb->dev;
-	ehci_urb_done(ehci, urb, 0);
-	urb = NULL;
+			list_for_each_entry(tmp, &stream->td_list, itd_list)
+				BUG_ON(tmp != itd && tmp->urb == urb);
+		}
+#endif
 
-	/* defer stopping schedule; completion can submit */
-	ehci->periodic_sched--;
-	if (unlikely (!ehci->periodic_sched))
-		(void) disable_periodic (ehci);
-	ehci_to_hcd(ehci)->self.bandwidth_isoc_reqs--;
+		/* give urb back to the driver ... can be out-of-order */
+		dev = urb->dev;
+		ehci_urb_done(ehci, urb, 0);
+		urb = NULL;
+		complete = URB_COMPLETE;
+
+		/* deferred schedule disable; completion often submits */
+		ehci->periodic_sched--;
+		if (unlikely(!ehci->periodic_sched))
+			disable_periodic(ehci);
+		ehci_to_hcd(ehci)->self.bandwidth_isoc_reqs--;
+
+		if (unlikely(list_empty(&stream->td_list))) {
+			ehci_to_hcd(ehci)->self.bandwidth_allocated
+					-= stream->bandwidth;
+			ehci_vdbg(ehci,
+				"deschedule devp %s ep%d%s-iso\n",
+				dev->devpath,
+				stream->bEndpointAddress & 0x0f,
+				(stream->bEndpointAddress & USB_DIR_IN)
+					?  "in" : "out");
+		}
+		iso_stream_put(ehci, stream);
+		goto recycle;
+	}
+	itd->uframe_scanned = uframe_after;
 
-	if (unlikely (list_empty (&stream->td_list))) {
-		ehci_to_hcd(ehci)->self.bandwidth_allocated
-				-= stream->bandwidth;
-		ehci_vdbg (ehci,
-			"deschedule devp %s ep%d%s-iso\n",
-			dev->devpath, stream->bEndpointAddress & 0x0f,
-			(stream->bEndpointAddress & USB_DIR_IN) ? "in" : "out");
+	/* If this ITD still has active transfers, leave it scheduled */
+	while (uframe < 8) {
+		if (itd->index[uframe++] != -1)
+			return complete;
 	}
-	iso_stream_put (ehci, stream);
 
-	return 1;
+recycle:
+	usb_put_urb(urb);
+	itd->urb = NULL;
+	itd->stream = NULL;
+	list_move(&itd->itd_list, &stream->free_list);
+	iso_stream_put(ehci, stream);
+
+	return complete | XITD_UNLINK;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1878,6 +1903,8 @@ sitd_link (struct ehci_hcd *ehci, unsign
 	sitd->frame = frame;
 	wmb ();
 	ehci->periodic[frame] = cpu_to_hc32(ehci, sitd->sitd_dma | Q_TYPE_SITD);
+	if (unlikely(!ehci->periodic_sched++))
+		return enable_periodic(ehci);
 }
 
 /* fit urb's sitds into the selected schedule slot; activate as needed */
@@ -1940,8 +1967,6 @@ sitd_link_urb (
 	urb->hcpriv = NULL;
 
 	timer_action (ehci, TIMER_IO_WATCHDOG);
-	if (!ehci->periodic_sched++)
-		return enable_periodic (ehci);
 	return 0;
 }
 
@@ -1951,10 +1976,8 @@ sitd_link_urb (
 				| SITD_STS_XACT | SITD_STS_MMF)
 
 static unsigned
-sitd_complete (
-	struct ehci_hcd		*ehci,
-	struct ehci_sitd	*sitd
-) {
+sitd_scan(struct ehci_hcd *ehci, struct ehci_sitd *sitd, int live)
+{
 	struct urb				*urb = sitd->urb;
 	struct usb_iso_packet_descriptor	*desc;
 	u32					t;
@@ -1962,9 +1985,12 @@ sitd_complete (
 	struct ehci_iso_stream			*stream = sitd->stream;
 	struct usb_device			*dev;
 
+	t = hc32_to_cpup(ehci, &sitd->hw_results);
+	if ((t & SITD_STS_ACTIVE) && live)
+		return 0;
+
 	urb_index = sitd->index;
 	desc = &urb->iso_frame_desc [urb_index];
-	t = hc32_to_cpup(ehci, &sitd->hw_results);
 
 	/* report transfer status */
 	if (t & SITD_ERRS) {
@@ -1991,7 +2017,7 @@ sitd_complete (
 
 	/* handle completion now? */
 	if ((urb_index + 1) != urb->number_of_packets)
-		return 0;
+		return XITD_UNLINK;
 
 	/* ASSERT: it's really the last sitd for this urb
 	list_for_each_entry (sitd, &stream->td_list, sitd_list)
@@ -2019,7 +2045,7 @@ sitd_complete (
 	}
 	iso_stream_put (ehci, stream);
 
-	return 1;
+	return XITD_UNLINK | URB_COMPLETE;
 }
 
 
@@ -2092,11 +2118,9 @@ sitd_submit (struct ehci_hcd *ehci, stru
 }
 
 static inline unsigned
-sitd_complete (
-	struct ehci_hcd		*ehci,
-	struct ehci_sitd	*sitd
-) {
-	ehci_err (ehci, "sitd_complete %p?\n", sitd);
+sitd_scan(struct ehci_hcd *ehci, struct ehci_sitd *sitd, int live)
+{
+	ehci_err(ehci, "%s %p?\n", __FUNCTION__, sitd);
 	return 0;
 }
 
@@ -2148,7 +2172,6 @@ restart:
 		modified = 0;
 
 		while (q.ptr != NULL) {
-			unsigned		uf;
 			union ehci_shadow	temp;
 			int			live;
 
@@ -2175,47 +2198,33 @@ restart:
 				q = q.fstn->fstn_next;
 				break;
 			case Q_TYPE_ITD:
-				/* skip itds for later in the frame */
 				rmb ();
-				for (uf = live ? uframes : 8; uf < 8; uf++) {
-					if (0 == (q.itd->hw_transaction [uf]
-							& ITD_ACTIVE(ehci)))
-						continue;
+				type = Q_NEXT_TYPE(ehci, q.itd->hw_next);
+				modified = itd_scan(ehci, q.itd,
+						live ? uframes : 8);
+				if (modified & XITD_UNLINK) {
+					modified &= ~XITD_UNLINK;
+					*q_p = q.itd->itd_next;
+					*hw_p = q.itd->hw_next;
+					wmb();
+				} else {
 					q_p = &q.itd->itd_next;
 					hw_p = &q.itd->hw_next;
-					type = Q_NEXT_TYPE(ehci,
-							q.itd->hw_next);
-					q = *q_p;
-					break;
 				}
-				if (uf != 8)
-					break;
-
-				/* this one's ready ... HC won't cache the
-				 * pointer for much longer, if at all.
-				 */
-				*q_p = q.itd->itd_next;
-				*hw_p = q.itd->hw_next;
-				type = Q_NEXT_TYPE(ehci, q.itd->hw_next);
-				wmb();
-				modified = itd_complete (ehci, q.itd);
 				q = *q_p;
 				break;
 			case Q_TYPE_SITD:
-				if ((q.sitd->hw_results & SITD_ACTIVE(ehci))
-						&& live) {
+				type = Q_NEXT_TYPE(ehci, q.sitd->hw_next);
+				modified = sitd_scan(ehci, q.sitd, live);
+				if (modified & XITD_UNLINK) {
+					modified &= ~XITD_UNLINK;
+					*q_p = q.sitd->sitd_next;
+					*hw_p = q.sitd->hw_next;
+					wmb();
+				} else {
 					q_p = &q.sitd->sitd_next;
 					hw_p = &q.sitd->hw_next;
-					type = Q_NEXT_TYPE(ehci,
-							q.sitd->hw_next);
-					q = *q_p;
-					break;
 				}
-				*q_p = q.sitd->sitd_next;
-				*hw_p = q.sitd->hw_next;
-				type = Q_NEXT_TYPE(ehci, q.sitd->hw_next);
-				wmb();
-				modified = sitd_complete (ehci, q.sitd);
 				q = *q_p;
 				break;
 			default:
--- g26.orig/drivers/usb/host/ehci.h	2007-11-11 15:10:19.000000000 -0800
+++ g26/drivers/usb/host/ehci.h	2007-11-11 19:34:24.000000000 -0800
@@ -589,6 +589,7 @@ struct ehci_itd {
 	struct urb		*urb;
 	struct ehci_iso_stream	*stream;	/* endpoint's queue */
 	struct list_head	itd_list;	/* list of stream's itds */
+	unsigned		uframe_scanned;
 
 	/* any/all hw_transactions here may be used by that urb */
 	unsigned		frame;		/* where scheduled */
@@ -623,8 +624,6 @@ struct ehci_sitd {
 #define	SITD_STS_MMF	(1 << 2)	/* incomplete split transaction */
 #define	SITD_STS_STS	(1 << 1)	/* split transaction state */
 
-#define SITD_ACTIVE(ehci)	cpu_to_hc32(ehci, SITD_STS_ACTIVE)
-
 	__hc32			hw_buf [2];		/* EHCI table 3-12 */
 	__hc32			hw_backpointer;		/* EHCI table 3-13 */
 	__hc32			hw_buf_hi [2];		/* Appendix B */


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel