[alsa-devel] Buffer size for ALSA USB PCM audio

Takashi Iwai tiwai at suse.de
Mon Aug 12 15:22:06 CEST 2013


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


More information about the Alsa-devel mailing list