At Thu, 25 Jul 2013 10:50:49 -0400 (EDT), Alan Stern wrote:
On Thu, 25 Jul 2013, Takashi Iwai wrote:
Besides, what's the reason for allocating 10 URBs if you're not going to submit more than 2 of them at a time?
I don't know how you deduced 10 urbs in your example,
I made it up. :-)
but in general, ep->nurbs is calculated so that the driver can hold at least two ep->periods (i.e. double buffer). The USB audio driver has essentially two buffers: an internal hardware buffer via URBs and an intermediate buffer via vmalloc. The latter is exposed to user-space and its content is copied to the URBs appropriately via complete callbacks.
Due to this design, we just need two periods for URB buffers, ideally, no matter how many periods are used in the latter buffer specified by user. That's why no buffer_size is needed in ep->nurbs estimation. The actual calculation is, however, a bit more complicated to achieve enough fine-grained updates but yet not too big buffer size. I guess this can be simplified / improved.
Ah, that makes sense. Thanks for the explanation.
The existing code has a problem: Under some conditions the URB queue will be too short. EHCI requires at least 10 microframes on the queue at all times (even when an URB is completing and is therefore no longer on the queue) to avoid underruns. Well, 9 microframes is probably good enough, but I'll use 10 to be safe. A typical arrangement of 2 URBs each with 8 packets each (at 1 packet/uframe) violates this constraint, and I have seen underruns in practice.
The patch below fixes this problem and drastically simplifies the calculation. How does it look?
Looks good through a quick glance. But I'd rather like to let review Clemens and Daniel as I already forgot the old voodoo logic of the current driver code :)
Thanks!
Takashi
Alan Stern
Index: usb-3.10/sound/usb/endpoint.c
--- usb-3.10.orig/sound/usb/endpoint.c +++ usb-3.10/sound/usb/endpoint.c @@ -575,6 +575,7 @@ static int data_ep_set_params(struct snd struct snd_usb_endpoint *sync_ep) { unsigned int maxsize, i, urb_packs, total_packs, packs_per_ms;
- unsigned int min_queued_packs, max_packs; int is_playback = usb_pipeout(ep->pipe); int frame_bits = snd_pcm_format_physical_width(pcm_format) * channels;
@@ -608,11 +609,21 @@ static int data_ep_set_params(struct snd else ep->curpacksize = maxsize;
- if (snd_usb_get_speed(ep->chip->dev) != USB_SPEED_FULL)
- if (snd_usb_get_speed(ep->chip->dev) != USB_SPEED_FULL) { packs_per_ms = 8 >> ep->datainterval;
- else
/* high speed needs 10 USB uframes on the queue at all times */min_queued_packs = DIV_ROUND_UP(10, 8 / packs_per_ms);max_packs = MAX_PACKS_HS;} else { packs_per_ms = 1;
/* full speed needs one USB frame on the queue at all times */min_queued_packs = 1;max_packs = MAX_PACKS;}
max_packs = min(max_packs, MAX_QUEUE * packs_per_ms);
if (is_playback && !snd_usb_endpoint_implicit_feedback_sink(ep)) { urb_packs = max(ep->chip->nrpacks, 1); urb_packs = min(urb_packs, (unsigned int) MAX_PACKS);
@@ -625,42 +636,18 @@ static int data_ep_set_params(struct snd if (sync_ep && !snd_usb_endpoint_implicit_feedback_sink(ep)) urb_packs = min(urb_packs, 1U << sync_ep->syncinterval);
- /* decide how many packets to be used */
- if (is_playback && !snd_usb_endpoint_implicit_feedback_sink(ep)) {
unsigned int minsize, maxpacks;/* determine how small a packet can be */minsize = (ep->freqn >> (16 - ep->datainterval))* (frame_bits >> 3);/* with sync from device, assume it can be 12% lower */if (sync_ep)minsize -= minsize >> 3;minsize = max(minsize, 1u);total_packs = (period_bytes + minsize - 1) / minsize;/* we need at least two URBs for queueing */if (total_packs < 2) {total_packs = 2;} else {/* and we don't want too long a queue either */maxpacks = max(MAX_QUEUE * packs_per_ms, urb_packs * 2);total_packs = min(total_packs, maxpacks);}- } else {
while (urb_packs > 1 && urb_packs * maxsize >= period_bytes)urb_packs >>= 1;total_packs = MAX_URBS * urb_packs;- }
- ep->nurbs = (total_packs + urb_packs - 1) / urb_packs;
- if (ep->nurbs > MAX_URBS) {
/* too much... */ep->nurbs = MAX_URBS;total_packs = MAX_URBS * urb_packs;- } else if (ep->nurbs < 2) {
/* too little - we need at least two packets* to ensure contiguous playback/capture*/ep->nurbs = 2;
/* each URB must fit into one period */
urb_packs = min(urb_packs, period_bytes / maxsize);
urb_packs = max(1u, urb_packs);
/* at least one more URB than the minimum queue size */
ep->nurbs = 1 + DIV_ROUND_UP(min_queued_packs, urb_packs);
if (ep->nurbs * urb_packs > max_packs) {
/* too much -- URBs have too many packets */urb_packs = max_packs / ep->nurbs;}
total_packs = ep->nurbs * urb_packs;
/* allocate and initialize data urbs */ for (i = 0; i < ep->nurbs; i++) {