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

Am Montag, 12. November 2007 schrieb David Brownell:
> 
> 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.

Wrongly gives back empty in urb uframes to the soundcarddriver.
My patch changes ehci->periodic_sched urb wise for SITD transactions
and ITD wise for high_speed iso.
ITD wise that makes the "tree-ish hardware-caching" possible.
Is it glitchy for SITDs because its wrong somehow or because its
inconvinient to differ from ITD mode?

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

Here you add to ehci->periodic_sched per ITD.
>  
> +/* 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);

here ehci->periodic_sched-- is done per urb. That wasn't intended I guess?

> +		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;
>  }

ehci->periodic_sched++ per SITD.
Really needed or just nicer to mimic TDI mode?
>  
> @@ -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();

Put wmb() here to keep hardware happy, don't need it in the else branch?

I've got two more patches here to reduce struct ehci_itd size by removing
index[] and usecs[]. They size ehci_itd down to 160bytes.
Thats 2 32byte alignment steps here on x86_64.
Should I post "next take" with wmb() tweeks and ehci_idt slimming?
Add per SITD ehci->periodic_sched++?

      Karsten

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