Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Pavel Hofman <pavel.hofman@ivitera.com>]

Dne 24. 04. 20 v 4:24 Alexander Tsoy napsal(a):
> For computation of the the next frame size current value of fs/fps and
> accumulated fractional parts of fs/fps are used, where values are stored
> in Q16.16 format. This is quite natural for computing frame size for
> asynchronous endpoints driven by explicit feedback, since in this case
> fs/fps is a value provided by the feedback endpoint and it's already in
> the Q format. If an error is accumulated over time, the device can
> adjust fs/fps value to prevent buffer overruns/underruns.
> 
> But for synchronous endpoints the accuracy provided by these computations
> is not enough. Due to accumulated error the driver periodically produces
> frames with incorrect size (+/- 1 audio sample).
> 
> This patch fixes this issue by implementing a different algorithm for
> frame size computation. It is based on accumulating of the remainders
> from division fs/fps and it doesn't accumulate errors over time. This
> new method is enabled for synchronous and adaptive playback endpoints.
> 
> Signed-off-by: Alexander Tsoy <alexander@tsoy.me>
> ---
>  sound/usb/card.h     |  4 ++++
>  sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>  sound/usb/endpoint.h |  1 +
>  sound/usb/pcm.c      |  2 ++
>  4 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/usb/card.h b/sound/usb/card.h
> index 395403a2d33f..820e564656ed 100644
> --- a/sound/usb/card.h
> +++ b/sound/usb/card.h
> @@ -84,6 +84,10 @@ struct snd_usb_endpoint {
>  	dma_addr_t sync_dma;		/* DMA address of syncbuf */
>  
>  	unsigned int pipe;		/* the data i/o pipe */
> +	unsigned int framesize[2];	/* small/large frame sizes in samples */
> +	unsigned int sample_rem;	/* remainder from division fs/fps */
> +	unsigned int sample_accum;	/* sample accumulator */
> +	unsigned int fps;		/* frames per second */
>  	unsigned int freqn;		/* nominal sampling rate in fs/fps in Q16.16 format */
>  	unsigned int freqm;		/* momentary sampling rate in fs/fps in Q16.16 format */
>  	int	   freqshift;		/* how much to shift the feedback value to get Q16.16 */
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index 4a9a2f6ef5a4..d8dc7cb56d43 100644
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -124,12 +124,12 @@ int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep)
>  
>  /*
>   * For streaming based on information derived from sync endpoints,
> - * prepare_outbound_urb_sizes() will call next_packet_size() to
> + * prepare_outbound_urb_sizes() will call slave_next_packet_size() to
>   * determine the number of samples to be sent in the next packet.

Please should not this read

"For streaming based on information derived from async endpoints,"

or

"For streaming based on information derived from sync-master endpoints,"?

Because the next method says:

For adaptive and synchronous endpoints, prepare_outbound_urb_sizes()...

Thanks for the great patch,

Pavel.