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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel