[alsa-devel] Fwd: [PATCH] ehci-hcd: complete iso urbs ASAP for number_of_packets != (n * 8) also
Hi
I just sent attached patch to usb-devel. It makes special workaround for iso usb communication using ehci-hcd highspeed needless. Comments, Bug/success reports welcome!
kind regards Karsten
Upstream (as of 2.6.23) ehci-hcd only completes iso urbs, if the last frame (= 8 uframes) they overlapped with has elapsed. That can be as late as when the following urb emits its interrupt on completion. soundcard drivers tend to work around by only transfering iso urbs with number_of_packets = (n * 8). Patch lets iso urbs complete asap by scanning itd always up to the elapsed uframe. An itd's last scanned uframe is stored in the new struct ehci_itd member uframe_scanned. Itds stay kept in the hcd's schedule until the complete frame they cover has elapsed, like without patch. To make this possible, ehci->periodic_sched is changed based on the number of active itds instead of the number of active iso urbs.
Signed-off-by: Karsten Wiese fzu@wemgehoertderstaat.de --- drivers/usb/host/ehci-sched.c | 108 ++++++++++++++++++++--------------------- drivers/usb/host/ehci.h | 1 + 2 files changed, 54 insertions(+), 55 deletions(-)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c index e682f23..c456b16 100644 --- a/drivers/usb/host/ehci-sched.c +++ b/drivers/usb/host/ehci-sched.c @@ -1481,6 +1481,8 @@ itd_link (struct ehci_hcd *ehci, unsigned frame, struct ehci_itd *itd) 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 */ @@ -1553,30 +1555,30 @@ itd_link_urb ( urb->hcpriv = NULL;
timer_action (ehci, TIMER_IO_WATCHDOG); - if (unlikely (!ehci->periodic_sched++)) - return enable_periodic (ehci); return 0; }
#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 -) { +static bool +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;
/* for each uframe with a packet */ - for (uframe = 0; uframe < 8; uframe++) { + for (uframe = itd->uframe_scanned; uframe < uframe_after; uframe++) { if (likely (itd->index[uframe] == -1)) continue; + urb_index = itd->index[uframe]; desc = &urb->iso_frame_desc [urb_index];
@@ -1603,45 +1605,53 @@ itd_complete ( desc->status = 0; desc->actual_length = EHCI_ITD_LENGTH (t); } + + /* handle completion now? */ + if (urb_index + 1 == urb->number_of_packets) { + struct usb_device *dev; + + usb_put_urb (urb); + /* ASSERT: it's really the last itd for this urb + struct ehci_itd *stritd; + list_for_each_entry (stritd, &stream->td_list, itd_list) + BUG_ON (stritd != itd && stritd->urb == urb); + */ + itd->urb = NULL; + + /* give urb back to the driver ... can be out-of-order */ + dev = urb->dev; + ehci_urb_done (ehci, urb); + urb = NULL; + + 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); + } }
- usb_put_urb (urb); - itd->urb = NULL; + itd->uframe_scanned = uframe_after; + if (uframe_after < 8) + return false; + itd->stream = NULL; + itd->uframe_scanned = 0; 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; - - /* 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); - */ - - /* give urb back to the driver ... can be out-of-order */ - dev = urb->dev; - ehci_urb_done (ehci, urb); - urb = NULL; - /* 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--;
- 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); - - return 1; + return true; }
/*-------------------------------------------------------------------------*/ @@ -2156,30 +2166,18 @@ 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; + modified = itd_scan(ehci, q.itd, uf = live ? uframes : 8); + if (!modified) { 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; + } else { + *q_p = q.itd->itd_next; + *hw_p = q.itd->hw_next; } - 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: diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h index 951d69f..df35964 100644 --- a/drivers/usb/host/ehci.h +++ b/drivers/usb/host/ehci.h @@ -581,6 +581,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 */
Hi,
attached patches implement a simple driver for the tascam us-122l. Everything except midi out works i think ;-)
For USB 2.0 you also have to aply http://marc.info/?l=linux-usb-devel&m=119444869917392&w=2 named "ehci-hcd: complete iso urbs ASAP ..."
Will post a jack driver to use it soon. so far you can #define DEBUG_USB_STREAM in usb_stream.c to playback what is captured.
no alsa pcm interface there yet. only a hwdep to talk to the "usb stream" thing.
A userspace plugin that works like the jack driver would be fine too. Is that easy to do? I mean that userspace driver should export all those standard alsa pcm interfaces then.
Comments bug/success reports, patches welcome.
Karsten
Am Mittwoch, 7. November 2007 schrieb Karsten Wiese:
Hi,
attached patches implement a simple driver for the tascam us-122l. Everything except midi out works i think ;-)
For USB 2.0 you also have to aply http://marc.info/?l=linux-usb-devel&m=119444869917392&w=2 named "ehci-hcd: complete iso urbs ASAP ..."
Will post a jack driver to use it soon.
Attached. It has to be cleaned up alot, but it works. Patch taken against jackd 0.103.
Karsten Wiese wrote:
attached patches implement a simple driver for the tascam us-122l.
QUIRK_AUDIO_EDIROL_UA700_UA25, QUIRK_AUDIO_EDIROL_UA1000, QUIRK_AUDIO_EDIROL_UA101,
- QUIRK_MIDI_US122L, QUIRK_TYPE_COUNT
};
Why not put it together with the other QUIRK_MIDI_ symbols?
break;
- case QUIRK_MIDI_US122L:
case QUIRK_MIDI_FIXED_ENDPOINT: memcpy(&endpoints[0], quirk->data, sizeof(struct snd_usb_midi_endpoint_info));umidi->usb_protocol_ops = &snd_usbmidi_122l_ops;
Aaargh! If you really want to do it this way, at least add a "/* fall through */" so that it's obvious that this is not a mistake.
Regards, Clemens
Am Donnerstag, 8. November 2007 schrieb Clemens Ladisch:
Why not put it together with the other QUIRK_MIDI_ symbols?
break;
- case QUIRK_MIDI_US122L:
case QUIRK_MIDI_FIXED_ENDPOINT: memcpy(&endpoints[0], quirk->data, sizeof(struct snd_usb_midi_endpoint_info));umidi->usb_protocol_ops = &snd_usbmidi_122l_ops;
Aaargh! If you really want to do it this way, at least add a "/* fall through */" so that it's obvious that this is not a mistake.
Done here, will post again.
I'd like you to try the "ehci-hcd: complete iso urbs ASAP for number_of_packets != (n * 8) also" http://marc.info/?l=linux-usb-devel&m=119444869917392&w=2 with snd-usb-audio.
snd-usb-audio uses number_of_packets equalling multiples of 8 currently when transfering over USB 2.0, no? If so, with above patch that "multiples of 8"-thing could go away and snd-usb-audio could achieve up to 8 times less userspace-activation-jitter in low latency setups like 128 frames/period, 2 periods.
thanks, Karsten
Karsten Wiese wrote:
snd-usb-audio uses number_of_packets equalling multiples of 8 currently when transfering over USB 2.0, no? If so, with above patch that "multiples of 8"-thing could go away and snd-usb-audio could achieve up to 8 times less userspace-activation-jitter in low latency setups like 128 frames/period, 2 periods.
I'm using the 8-microframes alignment because ehci-hcd configures the controller to interrupt at most every 1 ms. It has a module option to change this, but as long as snd-usb-audio cannot detect a lower setting, it would not make sense to allow shorter periods.
Regards, Clemens
Am Donnerstag, 8. November 2007 schrieb Clemens Ladisch:
I'm using the 8-microframes alignment because ehci-hcd configures the controller to interrupt at most every 1 ms. It has a module option to change this, but as long as snd-usb-audio cannot detect a lower setting, it would not make sense to allow shorter periods.
For an 1ms interrupt alignment ehci-hcd must be loaded with non-default parameters. In ehci-hcd.c as of 2.6.23:
/* Initial IRQ latency: faster than hw default */ static int log2_irq_thresh = 0; // 0 to 6 module_param (log2_irq_thresh, int, S_IRUGO); MODULE_PARM_DESC (log2_irq_thresh, "log2 IRQ latency, 1-64 microframes");
That makes a default IRQ latency of 1/8ms. This is also what I see here: The us122l driver works with 64frames/period 2 periods at 96kHz sample rate. Thats about 1400 interupts/s. Wouldn't be possible with 1ms alignment.
kind regards, Karsten
participants (2)
-
Clemens Ladisch
-
Karsten Wiese