On Wed, 14 Aug 2013, Clemens Ladisch wrote:
In other words, there should be enough URBs so that an entire ALSA buffer can be queued at any time, subject only to the limit on the maximum number of URBs and packets.
The URB queue adds latency, so it should never be made too big, even for big ALSA buffers. Once we have reached a queue length that is practically guaranteed to be safe from underruns, more URBs do not make sense. (The current limit of 24 ms is somewhat arbitrary.)
It doesn't make sense to allocate just enough URBs to cover a single period.
Indeed. I've used two periods in my recent patch, but I don't really know how I would justify this choice in the commit message. ;-)
What doesn't make sense either is the current algorithm that computes a specific value for the total number of packets (total_packs) and then takes great care to allocate the URBs so that this number is reached, even if this means that the number of packets per URBs varies.
And while we're at it: the default number of packets per URB was chosen before the driver had the ability to estimate the delay from the current frame number; it should now be quite safe to increase it.
Okay, so here's my next attempt. It's based on a simple idea: All the difficulty arises from the fact that we don't know beforehand how many URBs will constitute an ALSA period since for playback endpoints, the URB sizes can vary. The problem can be solved simply by setting the number of URBs per period to a fixed value, and truncating URBs as needed to make it work.
Briefly, the patch fixes the max number of packets per URB, based on the minimum packet size. The only requirement is that no URB should exceed 8 ms (MAX_PACKS) or the length of a period. Given this, the number of URBs per period is fixed, and the number of packets in each URB is adjusted during playback so that all the URBs end up having roughly the same number of frames. As a result, we don't need to submit variable numbers of URBs in the completion handler after all.
The total number of URBs is limited to 12 (MAX_URBS) and 16 ms worth (MAX_QUEUE), and it is also restricted so that the total amount of data in the queue does not exceed the size of an ALSA buffer. If the user sets the buffer size too small, he gets what he deserves.
The parameter calculation now ends up being the same for both recording and playback endpoints. This may seem odd, but it follows from the reasoning above.
Although I believe the logic is now sound, the patch itself is incomplete. I don't know the best way of passing the frames/period and periods/buffer values, so they are stubbed as 0 with a FIXME comment. Maybe you can tell me how to do this properly.
The patch eliminates the use of ep->chip->nrpacks and therefore the nrpacks module parameter. Does that matter? I left the parameter in place, but now it doesn't do anything.
I'm not sure about the interaction with ep->curpacksize. I left it alone, since it looks like it won't affect playback endpoints.
Also, the relations between frames_per_period, period_bytes, and frame_bits are a little confusing. It's not clear to me that they will always behave as one would expect, i.e., that period_bytes will always be equal to (frame_bits >> 3) * frames_per_period.
Overall, the patch removes more lines of code than it adds. I think it looks pretty good, as far as it goes. What do you think?
Alan Stern
Index: usb-3.11/sound/usb/card.h =================================================================== --- usb-3.11.orig/sound/usb/card.h +++ usb-3.11/sound/usb/card.h @@ -2,11 +2,11 @@ #define __USBAUDIO_CARD_H
#define MAX_NR_RATES 1024 -#define MAX_PACKS 20 +#define MAX_PACKS 8 /* per URB */ #define MAX_PACKS_HS (MAX_PACKS * 8) /* in high speed mode */ -#define MAX_URBS 8 +#define MAX_URBS 12 #define SYNC_URBS 4 /* always four urbs for sync */ -#define MAX_QUEUE 24 /* try not to exceed this queue length, in ms */ +#define MAX_QUEUE 16 /* try not to exceed this queue length, in ms */
struct audioformat { struct list_head list; @@ -87,6 +87,7 @@ struct snd_usb_endpoint { unsigned int phase; /* phase accumulator */ unsigned int maxpacksize; /* max packet size in bytes */ unsigned int maxframesize; /* max packet size in frames */ + unsigned int max_urb_frames; /* max URB size in frames */ unsigned int curpacksize; /* current packet size in bytes (for capture) */ unsigned int curframesize; /* current packet size in frames (for capture) */ unsigned int syncmaxsize; /* sync endpoint packet size */ @@ -125,6 +126,7 @@ struct snd_usb_substream {
unsigned int hwptr_done; /* processed byte position in the buffer */ unsigned int transfer_done; /* processed frames since last period update */ + unsigned int frame_limit; /* limits number of packets in URB */
/* data and sync endpoints for this stream */ unsigned int ep_num; /* the endpoint number */ Index: usb-3.11/sound/usb/pcm.c =================================================================== --- usb-3.11.orig/sound/usb/pcm.c +++ usb-3.11/sound/usb/pcm.c @@ -1330,6 +1330,7 @@ static void prepare_playback_urb(struct frames = 0; urb->number_of_packets = 0; spin_lock_irqsave(&subs->lock, flags); + subs->frame_limit += ep->max_urb_frames; for (i = 0; i < ctx->packets; i++) { if (ctx->packet_size[i]) counts = ctx->packet_size[i]; @@ -1344,6 +1345,7 @@ static void prepare_playback_urb(struct subs->transfer_done += counts; if (subs->transfer_done >= runtime->period_size) { subs->transfer_done -= runtime->period_size; + subs->frame_limit = 0; period_elapsed = 1; if (subs->fmt_type == UAC_FORMAT_TYPE_II) { if (subs->transfer_done > 0) { @@ -1366,8 +1368,10 @@ static void prepare_playback_urb(struct break; } } - if (period_elapsed && - !snd_usb_endpoint_implicit_feedback_sink(subs->data_endpoint)) /* finish at the period boundary */ + /* finish at the period boundary or after enough frames */ + if ((period_elapsed || + subs->transfer_done >= subs->frame_limit) && + !snd_usb_endpoint_implicit_feedback_sink(ep)) break; } bytes = frames * ep->stride; Index: usb-3.11/sound/usb/endpoint.c =================================================================== --- usb-3.11.orig/sound/usb/endpoint.c +++ usb-3.11/sound/usb/endpoint.c @@ -571,11 +571,14 @@ static int data_ep_set_params(struct snd snd_pcm_format_t pcm_format, unsigned int channels, unsigned int period_bytes, + unsigned int frames_per_period, + unsigned int periods_per_buffer, struct audioformat *fmt, struct snd_usb_endpoint *sync_ep) { - unsigned int maxsize, i, urb_packs, total_packs, packs_per_ms; - int is_playback = usb_pipeout(ep->pipe); + unsigned int maxsize, minsize, packs_per_ms, max_packs_per_urb; + unsigned int max_packs_per_period, urbs_per_period, urb_packs; + unsigned int max_urbs, i; int frame_bits = snd_pcm_format_physical_width(pcm_format) * channels;
if (pcm_format == SNDRV_PCM_FORMAT_DSD_U16_LE && fmt->dsd_dop) { @@ -608,67 +611,44 @@ 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 - packs_per_ms = 1; - - 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); + max_packs_per_urb = MAX_PACKS_HS >> ep->datainterval; } else { - urb_packs = 1; + packs_per_ms = 1; + max_packs_per_urb = MAX_PACKS; } - - urb_packs *= packs_per_ms; - if (sync_ep && !snd_usb_endpoint_implicit_feedback_sink(ep)) - urb_packs = min(urb_packs, 1U << sync_ep->syncinterval); + max_packs_per_urb = min(max_packs_per_urb, + 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; - } + /* 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); + + /* how many packets will contain an entire ALSA period? */ + max_packs_per_period = DIV_ROUND_UP(period_bytes, minsize); + + /* how many URBs contain a period, and how many packets in each? */ + urbs_per_period = DIV_ROUND_UP(max_packs_per_period, max_packs_per_urb); + urb_packs = DIV_ROUND_UP(max_packs_per_period, urbs_per_period); + ep->max_urb_frames = DIV_ROUND_UP(frames_per_period, + urbs_per_period); + + /* try to use enough URBs to contain an entire ALSA buffer */ + max_urbs = min((unsigned) MAX_URBS, + MAX_QUEUE * packs_per_ms / urb_packs); + ep->nurbs = min(max_urbs, urbs_per_period * periods_per_buffer);
/* allocate and initialize data urbs */ for (i = 0; i < ep->nurbs; i++) { struct snd_urb_ctx *u = &ep->urb[i]; u->index = i; u->ep = ep; - u->packets = (i + 1) * total_packs / ep->nurbs - - i * total_packs / ep->nurbs; + u->packets = urb_packs; u->buffer_size = maxsize * u->packets;
if (fmt->fmt_type == UAC_FORMAT_TYPE_II) @@ -790,7 +770,8 @@ int snd_usb_endpoint_set_params(struct s switch (ep->type) { case SND_USB_ENDPOINT_TYPE_DATA: err = data_ep_set_params(ep, pcm_format, channels, - period_bytes, fmt, sync_ep); + period_bytes, 0, 0, fmt, sync_ep); + /* FIXME: buffer size isn't 0! */ break; case SND_USB_ENDPOINT_TYPE_SYNC: err = sync_ep_set_params(ep, fmt);