On Wed, Aug 27, 2014 at 10:39 PM, Daniel Mack <zonque@xxxxxxxxx> wrote:
> The UAC2 function driver currently responds to all packets at all times
> with wMaxPacketSize packets. That results in way too fast audio
> playback as the function driver (which is in fact supposed to define
> the audio stream pace) delivers as fast as it can.
>
> Fix this by sizing each packet correctly with the following steps:
>
> a) Set the packet's size by dividing the nominal data rate by the
> playback endpoint's interval.
>
> b) If there is a residual value from the calculation in a), add
> it to a accumulator to keep track of it across packets.
>
> c) If the accumulator has gathered at least the number of bytes
> that are needed for one sample frame, increase the packet size.
>
> This way, the packet size calculation will get rid of any kind of
> imprecision that would otherwise occur with a simple division over
> time.
>
> Some of the variables that are needed while processing each packet
> are pre-computed for performance reasons.
>
> Signed-off-by: Daniel Mack <zonque@xxxxxxxxx>
> ---
> drivers/usb/gadget/function/f_uac2.c | 69 +++++++++++++++++++++++++++++++++---
> 1 file changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> index 246a778..a5a27a5 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -92,6 +92,15 @@ struct snd_uac2_chip {
>
> struct snd_card *card;
> struct snd_pcm *pcm;
> +
> + /* timekeeping for the playback endpoint */
> + unsigned int p_interval;
> + unsigned int p_residue;
> +
> + /* pre-calculated values for playback iso completion */
> + unsigned int p_pktsize;
> + unsigned int p_pktsize_residue;
> + unsigned int p_framesize;
> };
>
I admire Alan's simple algo. I couldn't have matched that.
However I am not sure how worth is the implementation if it requires 3
more members to avoid calculations simple enough to cause no
noticeable overhead. Once every millisecond is perfectly bearable IMO.
5 members in uac2 structure for only playback, look ugly.
regards,
-jassi
>
> #define BUFF_SIZE_MAX (PAGE_SIZE * 16)
> @@ -191,8 +200,29 @@ agdev_iso_complete(struct usb_ep *ep, struct usb_request *req)
>
> spin_lock_irqsave(&prm->lock, flags);
>
> - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + /*
> + * For each IN packet, take the quotient of the current data
> + * rate and the endpoint's interval as the base packet size.
> + * If there is a residue from this division, add it to the
> + * residue accumulator.
> + */
> + req->length = uac2->p_pktsize;
> + uac2->p_residue += uac2->p_pktsize_residue;
> +
> + /*
> + * Whenever there are more bytes in the accumulator than we
> + * need to add one more sample frame, increase this packet's
> + * size and decrease the accumulator.
> + */
> + if (uac2->p_residue / uac2->p_interval >= uac2->p_framesize) {
> + req->length += uac2->p_framesize;
> + uac2->p_residue -= uac2->p_framesize *
> + uac2->p_interval;
> + }
> +
> req->actual = req->length;
> + }
>
> pending = prm->hw_ptr % prm->period_size;
> pending += req->actual;
> @@ -346,6 +376,7 @@ static int uac2_pcm_open(struct snd_pcm_substream *substream)
> c_srate = opts->c_srate;
> p_chmask = opts->p_chmask;
> c_chmask = opts->c_chmask;
> + uac2->p_residue = 0;
>
> runtime->hw = uac2_pcm_hardware;
>
> @@ -1077,7 +1108,7 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
> struct usb_request *req;
> struct usb_ep *ep;
> struct uac2_rtd_params *prm;
> - int i;
> + int req_len, i;
>
> /* No i/f has more than 2 alt settings */
> if (alt > 1) {
> @@ -1099,11 +1130,41 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
> prm = &uac2->c_prm;
> config_ep_by_speed(gadget, fn, ep);
> agdev->as_out_alt = alt;
> + req_len = prm->max_psize;
> } else if (intf == agdev->as_in_intf) {
> + struct f_uac2_opts *opts = agdev_to_uac2_opts(agdev);
> + unsigned int factor, rate;
> + struct usb_endpoint_descriptor *ep_desc;
> +
> ep = agdev->in_ep;
> prm = &uac2->p_prm;
> config_ep_by_speed(gadget, fn, ep);
> agdev->as_in_alt = alt;
> +
> + /* pre-calculate the playback endpoint's interval */
> + if (gadget->speed == USB_SPEED_FULL) {
> + ep_desc = &fs_epin_desc;
> + factor = 1000;
> + } else {
> + ep_desc = &hs_epin_desc;
> + factor = 125;
> + }
> +
> + /* pre-compute some values for iso_complete() */
> + uac2->p_framesize = opts->p_ssize *
> + num_channels(opts->p_chmask);
> + rate = opts->p_srate * uac2->p_framesize;
> + uac2->p_interval = (1 << (ep_desc->bInterval - 1)) * factor;
> + uac2->p_pktsize = min_t(unsigned int, rate / uac2->p_interval,
> + prm->max_psize);
> +
> + if (uac2->p_pktsize < prm->max_psize)
> + uac2->p_pktsize_residue = rate % uac2->p_interval;
> + else
> + uac2->p_pktsize_residue = 0;
> +
> + req_len = uac2->p_pktsize;
> + uac2->p_residue = 0;
> } else {
> dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
> return -EINVAL;
> @@ -1128,9 +1189,9 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>
> req->zero = 0;
> req->context = &prm->ureq[i];
> - req->length = prm->max_psize;
> + req->length = req_len;
> req->complete = agdev_iso_complete;
> - req->buf = prm->rbuf + i * req->length;
> + req->buf = prm->rbuf + i * prm->max_psize;
> }
>
> if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
> --
> 2.1.0
>
--
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
