At Thu, 1 Aug 2013 13:37:45 -0400 (EDT),
Alan Stern wrote:
>
> On Mon, 29 Jul 2013, Clemens Ladisch wrote:
>
> > Alan Stern wrote:
> > > Clemens remarked some time ago that keeping the queue full would be
> > > trivial, if only he knew how full it needed to be. The answer to that
> > > is given above. I have been trying to make the appropriate changes,
> > > but I'm not finding it "trivial".
> >
> > What I meant was that it would be trivial to change the lower bound of
> > the period size (from which many queueing parameters are derived).
>
> Here's what I've got. In turns out the predicting the optimum number
> of URBs needed is extremely difficult. I decided it would be better to
> make an overestimate and then to submit URBs as needed, rather than
> keeping all of them active all the time.
>
> I ended up with this patch (untested). It is certainly incomplete
> because it doesn't address endpoints with implicit sync. It also
> suffers from a race between snd_submit_urbs() and deactivate_urbs():
> an URB may be resubmitted after it has been deactivated.
What's the reason to clear ep->active_mask at the beginning of
snd_complete_urb()?
> (In all
> fairness, I think this race probably exists in the current code too --
> there are no spinlocks for synchronization.)
The calls of usb_submit_urb() and usb_unlink_urb() might race indeed.
The current code somehow assumed that the USB accepts such concurrent
calls. deactivate_urbs() just kills the pending urbs and it doesn't
change ep->active_mask bit by itself, and the synchronization is done
in wait_clear_urbs(). So, if the concurrent calls of usb_submit_urb()
and usb_unlink_urbs() were allowed, it should work as is (in the worst
case, the complete will be called once again, but then it goes to
exit_clear).
thanks,
Takashi
> The patch also fixes a couple of unrelated items: MAX_PACKS vs.
> MAX_PACKS_HS, and the maxsize calculation should realize that a packet
> can't contain a partial frame.
>
> What do you think of this approach?
>
> Alan Stern
>
>
>
> sound/usb/card.h | 7 +
> sound/usb/endpoint.c | 185 +++++++++++++++++++++++++++++----------------------
> sound/usb/pcm.c | 3
> 3 files changed, 114 insertions(+), 81 deletions(-)
>
> Index: usb-3.11/sound/usb/card.h
> ===================================================================
> --- usb-3.11.orig/sound/usb/card.h
> +++ usb-3.11/sound/usb/card.h
> @@ -4,7 +4,7 @@
> #define MAX_NR_RATES 1024
> #define MAX_PACKS 20
> #define MAX_PACKS_HS (MAX_PACKS * 8) /* in high speed mode */
> -#define MAX_URBS 8
> +#define MAX_URBS 11
> #define SYNC_URBS 4 /* always four urbs for sync */
> #define MAX_QUEUE 24 /* try not to exceed this queue length, in ms */
>
> @@ -43,6 +43,7 @@ struct snd_urb_ctx {
> struct snd_usb_endpoint *ep;
> int index; /* index for urb array */
> int packets; /* number of packets per urb */
> + int data_packets; /* current number of data packets */
> int packet_size[MAX_PACKS_HS]; /* size of packets for next submission */
> struct list_head ready_list;
> };
> @@ -99,6 +100,10 @@ struct snd_usb_endpoint {
> int skip_packets; /* quirks for devices to ignore the first n packets
> in a stream */
>
> + unsigned int min_queued_packs; /* USB hardware queue requirement */
> + unsigned int queued_packs; /* number of packets currently queued */
> + unsigned int queued_urbs; /* number of URBs currently queued */
> + int next_urb; /* next to submit */
> spinlock_t lock;
> struct list_head list;
> };
> Index: usb-3.11/sound/usb/pcm.c
> ===================================================================
> --- usb-3.11.orig/sound/usb/pcm.c
> +++ usb-3.11/sound/usb/pcm.c
> @@ -1328,7 +1328,7 @@ static void prepare_playback_urb(struct
> stride = runtime->frame_bits >> 3;
>
> frames = 0;
> - urb->number_of_packets = 0;
> + ctx->data_packets = urb->number_of_packets = 0;
> spin_lock_irqsave(&subs->lock, flags);
> for (i = 0; i < ctx->packets; i++) {
> if (ctx->packet_size[i])
> @@ -1341,6 +1341,7 @@ static void prepare_playback_urb(struct
> urb->iso_frame_desc[i].length = counts * ep->stride;
> frames += counts;
> urb->number_of_packets++;
> + ctx->data_packets++;
> subs->transfer_done += counts;
> if (subs->transfer_done >= runtime->period_size) {
> subs->transfer_done -= runtime->period_size;
> Index: usb-3.11/sound/usb/endpoint.c
> ===================================================================
> --- usb-3.11.orig/sound/usb/endpoint.c
> +++ usb-3.11/sound/usb/endpoint.c
> @@ -217,6 +217,7 @@ static void prepare_outbound_urb(struct
> }
>
> urb->number_of_packets = ctx->packets;
> + ctx->data_packets = ctx->packets;
> urb->transfer_buffer_length = offs * ep->stride;
> memset(urb->transfer_buffer, ep->silence_value,
> offs * ep->stride);
> @@ -273,6 +274,7 @@ static inline void prepare_inbound_urb(s
>
> urb->transfer_buffer_length = offs;
> urb->number_of_packets = urb_ctx->packets;
> + urb_ctx->data_packets = urb_ctx->packets;
> break;
>
> case SND_USB_ENDPOINT_TYPE_SYNC:
> @@ -341,6 +343,47 @@ static void queue_pending_output_urbs(st
> }
> }
>
> +/**
> + * snd_submit_urbs: Submit data URBs for endpoints without implicit feedback
> + *
> + * @ep: The endpoint to use
> + * @ctx: Context for the first URB on the queue (or to be queued)
> + *
> + * Submit enough URBs so that when the next one completes, the hardware queue
> + * will still contain sufficiently many packets.
> + *
> + * Note: If the hardware queue is empty (and @ctx refers to the next URB to be
> + * submitted), then ctx->data_packets must be equal to 0.
> + */
> +static int snd_submit_urbs(struct snd_usb_endpoint *ep, struct snd_urb_ctx *ctx)
> +{
> + int err = 0;
> +
> + while (ep->queued_packs - ctx->data_packets < ep->min_queued_packs &&
> + ep->queued_urbs < ep->nurbs) {
> + struct snd_urb_ctx *u = &ep->urb[ep->next_urb];
> +
> + if (usb_pipeout(ep->pipe))
> + prepare_outbound_urb(ep, u);
> + else
> + prepare_inbound_urb(ep, u);
> +
> + err = usb_submit_urb(u->urb, GFP_ATOMIC);
> + if (err) {
> + snd_printk(KERN_ERR "cannot submit urb (err = %d: %s)\n",
> + err, usb_error_string(err));
> + break;
> + }
> + set_bit(ep->next_urb, &ep->active_mask);
> + ep->queued_packs += u->data_packets;
> + ++ep->queued_urbs;
> +
> + if (++ep->next_urb >= ep->nurbs)
> + ep->next_urb = 0;
> + }
> + return err;
> +}
> +
> /*
> * complete callback for urbs
> */
> @@ -348,20 +391,23 @@ static void snd_complete_urb(struct urb
> {
> struct snd_urb_ctx *ctx = urb->context;
> struct snd_usb_endpoint *ep = ctx->ep;
> - int err;
> +
> + clear_bit(ctx->index, &ep->active_mask);
> + ep->queued_packs -= ctx->data_packets;
> + --ep->queued_urbs;
>
> if (unlikely(urb->status == -ENOENT || /* unlinked */
> urb->status == -ENODEV || /* device removed */
> urb->status == -ECONNRESET || /* unlinked */
> urb->status == -ESHUTDOWN || /* device disabled */
> ep->chip->shutdown)) /* device disconnected */
> - goto exit_clear;
> + return;
>
> if (usb_pipeout(ep->pipe)) {
> retire_outbound_urb(ep, ctx);
> /* can be stopped during retire callback */
> if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags)))
> - goto exit_clear;
> + return;
>
> if (snd_usb_endpoint_implicit_feedback_sink(ep)) {
> unsigned long flags;
> @@ -371,28 +417,24 @@ static void snd_complete_urb(struct urb
> spin_unlock_irqrestore(&ep->lock, flags);
> queue_pending_output_urbs(ep);
>
> - goto exit_clear;
> + return;
> }
>
> - prepare_outbound_urb(ep, ctx);
> } else {
> retire_inbound_urb(ep, ctx);
> /* can be stopped during retire callback */
> if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags)))
> - goto exit_clear;
> -
> - prepare_inbound_urb(ep, ctx);
> + return;
> }
>
> - err = usb_submit_urb(urb, GFP_ATOMIC);
> - if (err == 0)
> - return;
> + ctx->data_packets = 0;
> + ++ctx;
> + if (ctx >= ep->urb + ep->nurbs)
> + ctx = &ep->urb[0];
>
> - snd_printk(KERN_ERR "cannot submit urb (err = %d)\n", err);
> + if (snd_submit_urbs(ep, ctx) == 0)
> + return;
> //snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
> -
> -exit_clear:
> - clear_bit(ctx->index, &ep->active_mask);
> }
>
> /**
> @@ -574,7 +616,7 @@ static int data_ep_set_params(struct snd
> struct audioformat *fmt,
> struct snd_usb_endpoint *sync_ep)
> {
> - unsigned int maxsize, i, urb_packs, total_packs, packs_per_ms;
> + unsigned int maxsize, i, urb_packs, max_packs, packs_per_ms;
> int is_playback = usb_pipeout(ep->pipe);
> int frame_bits = snd_pcm_format_physical_width(pcm_format) * channels;
>
> @@ -593,8 +635,8 @@ static int data_ep_set_params(struct snd
>
> /* assume max. frequency is 25% higher than nominal */
> ep->freqmax = ep->freqn + (ep->freqn >> 2);
> - maxsize = ((ep->freqmax + 0xffff) * (frame_bits >> 3))
> - >> (16 - ep->datainterval);
> + maxsize = ((ep->freqmax + 0xffff) >> (16 - ep->datainterval))
> + * (frame_bits >> 3);
> /* but wMaxPacketSize might reduce this */
> if (ep->maxpacksize && ep->maxpacksize < maxsize) {
> /* whatever fits into a max. size packet */
> @@ -608,11 +650,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 */
> + ep->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 */
> + ep->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,41 +677,32 @@ 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;
> - }
> + /* no point having an URB much longer than one period */
> + urb_packs = min(urb_packs, 1 + period_bytes / maxsize);
>
> - 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;
> + /*
> + * We can't tell in advance how many URBs are needed to fill the
> + * USB hardware queue, because the number of packets per URB is
> + * variable (it depends on the period size and the device's clock
> + * frequency). Instead, get a worst-case overestimate by assuming
> + * the number of packets alternates between 1 and urb_packs.
> + *
> + * The total number of URBs needed is one higher than this, because
> + * the hardware queue must remain full even while one URB is
> + * completing (and therefore not on the queue).
> + *
> + * Recording endpoints with no sync always use the same number of
> + * packets per URB, so we can get a better estimate for them.
> + */
> + if (is_playback || sync_ep)
> + ep->nurbs = 1 + 2 * DIV_ROUND_UP(ep->min_queued_packs,
> + urb_packs + 1);
> + else
> + ep->nurbs = 1 + DIV_ROUND_UP(ep->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;
> }
>
> /* allocate and initialize data urbs */
> @@ -667,8 +710,8 @@ static int data_ep_set_params(struct snd
> 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->data_packets = 0;
> u->buffer_size = maxsize * u->packets;
>
> if (fmt->fmt_type == UAC_FORMAT_TYPE_II)
> @@ -861,30 +904,14 @@ int snd_usb_endpoint_start(struct snd_us
> return 0;
> }
>
> - for (i = 0; i < ep->nurbs; i++) {
> - struct urb *urb = ep->urb[i].urb;
> -
> - if (snd_BUG_ON(!urb))
> - goto __error;
> -
> - if (usb_pipeout(ep->pipe)) {
> - prepare_outbound_urb(ep, urb->context);
> - } else {
> - prepare_inbound_urb(ep, urb->context);
> - }
> -
> - err = usb_submit_urb(urb, GFP_ATOMIC);
> - if (err < 0) {
> - snd_printk(KERN_ERR "cannot submit urb %d, error %d: %s\n",
> - i, err, usb_error_string(err));
> - goto __error;
> - }
> - set_bit(i, &ep->active_mask);
> - }
> -
> - return 0;
> + ep->queued_packs = 0;
> + ep->queued_urbs = 0;
> + ep->next_urb = 0;
> + ep->urb[0].data_packets = 0;
> + err = snd_submit_urbs(ep, &ep->urb[0]);
> + if (!err)
> + return 0;
>
> -__error:
> clear_bit(EP_FLAG_RUNNING, &ep->flags);
> ep->use_count--;
> deactivate_urbs(ep, false);
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
