Re: [PATCH] [RFC] alsaloop: add feedback frequency control support for UAC2 gadgets

[Jerome Brunet <jbrunet@baylibre.com>]

On Thu 12 Nov 2020 at 23:34, Ruslan Bilovol <ruslan.bilovol@gmail.com> wrote:

> Add support of special "PCM Feedback Frequency Hz"
> UAC2 Gadget mixer control that is designed to notify
> host about real sampling frequency of the gadget so
> it can adjust number of samples that hosts sends to
> the gadget.
>
> This is useful if both host and gadget has its own
> internal freerunning clock, so host can adjust
> number of samples sent, preventing overrun/underrun
> conditions.
>
> This patch reuses logic of the "PCM Rate Shift 100000"
> control used in case of in-kernel ALSA loopback
> driver. The only difference is alsaloop reports not
> rate shift but frequency in Hz
>
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> ---
>  alsaloop/alsaloop.h |  1 +
>  alsaloop/pcmjob.c   | 35 +++++++++++++++++++++++++----------
>  2 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/alsaloop/alsaloop.h b/alsaloop/alsaloop.h
> index c4aa618..9a50a42 100644
> --- a/alsaloop/alsaloop.h
> +++ b/alsaloop/alsaloop.h
> @@ -122,6 +122,7 @@ struct loopback_handle {
>  	unsigned int ctl_pollfd_count;
>  	snd_ctl_elem_value_t *ctl_notify;
>  	snd_ctl_elem_value_t *ctl_rate_shift;
> +	snd_ctl_elem_value_t *ctl_fback_freq;
>  	snd_ctl_elem_value_t *ctl_active;
>  	snd_ctl_elem_value_t *ctl_format;
>  	snd_ctl_elem_value_t *ctl_rate;
> diff --git a/alsaloop/pcmjob.c b/alsaloop/pcmjob.c
> index 6a9aff4..b3802a8 100644
> --- a/alsaloop/pcmjob.c
> +++ b/alsaloop/pcmjob.c
> @@ -1058,15 +1058,22 @@ static int set_notify(struct loopback_handle *lhandle, int enable)
>  
>  static int set_rate_shift(struct loopback_handle *lhandle, double pitch)
>  {
> -	int err;
> +	int err = 0;
>  
> -	if (lhandle->ctl_rate_shift == NULL)
> -		return 0;
> -	snd_ctl_elem_value_set_integer(lhandle->ctl_rate_shift, 0, pitch * 100000);
> -	err = snd_ctl_elem_write(lhandle->ctl, lhandle->ctl_rate_shift);
> -	if (err < 0) {
> -		logit(LOG_CRIT, "Cannot set PCM Rate Shift element for %s: %s\n", lhandle->id, snd_strerror(err));
> -		return err;
> +	if (lhandle->ctl_rate_shift) {
> +		snd_ctl_elem_value_set_integer(lhandle->ctl_rate_shift, 0, pitch * 100000);
> +		err = snd_ctl_elem_write(lhandle->ctl, lhandle->ctl_rate_shift);
> +		if (err < 0) {
> +			logit(LOG_CRIT, "Cannot set PCM Rate Shift element for %s: %s\n", lhandle->id, snd_strerror(err));
> +			return err;
> +		}
> +	} else if (lhandle->ctl_fback_freq) {
> +		snd_ctl_elem_value_set_integer(lhandle->ctl_fback_freq, 0, lhandle->rate * (2.0 - pitch));
> +		err = snd_ctl_elem_write(lhandle->ctl, lhandle->ctl_fback_freq);
> +		if (err < 0) {
> +			logit(LOG_CRIT, "Cannot set PCM Feedback Frequency element for %s: %s\n", lhandle->id, snd_strerror(err));
> +			return err;
> +		}

Hi Ruslan,

I wonder why bother adding a control of another type for the audio
gadget ? Why not give the gadget a "Rate Shift" control, instead "Feedback
Frequency" and let the driver deal with shift as necessary ?

It would be easier for the applications to re-use the same logic.

>  	}
>  	return 0;
>  }
> @@ -1195,6 +1202,7 @@ static int openctl(struct loopback_handle *lhandle, int device, int subdevice)
>  	int err;
>  
>  	lhandle->ctl_rate_shift = NULL;
> +	lhandle->ctl_fback_freq = NULL;
>  	if (lhandle->loopback->play == lhandle) {
>  		if (lhandle->loopback->controls)
>  			goto __events;
> @@ -1204,6 +1212,8 @@ static int openctl(struct loopback_handle *lhandle, int device, int subdevice)
>  			&lhandle->ctl_notify);
>  	openctl_elem(lhandle, device, subdevice, "PCM Rate Shift 100000",
>  			&lhandle->ctl_rate_shift);
> +	openctl_elem(lhandle, device, subdevice, "PCM Feedback Frequency Hz",
> +			&lhandle->ctl_fback_freq);
>  	set_rate_shift(lhandle, 1);
>  	openctl_elem(lhandle, device, subdevice, "PCM Slave Active",
>  			&lhandle->ctl_active);
> @@ -1289,6 +1299,9 @@ static int closeit(struct loopback_handle *lhandle)
>  	if (lhandle->ctl_rate_shift)
>  		snd_ctl_elem_value_free(lhandle->ctl_rate_shift);
>  	lhandle->ctl_rate_shift = NULL;
> +	if (lhandle->ctl_fback_freq)
> +		snd_ctl_elem_value_free(lhandle->ctl_fback_freq);
> +	lhandle->ctl_fback_freq = NULL;
>  	if (lhandle->ctl)
>  		err = snd_ctl_close(lhandle->ctl);
>  	lhandle->ctl = NULL;
> @@ -1334,9 +1347,11 @@ int pcmjob_init(struct loopback *loop)
>  	snprintf(id, sizeof(id), "%s/%s", loop->play->id, loop->capt->id);
>  	id[sizeof(id)-1] = '\0';
>  	loop->id = strdup(id);
> -	if (loop->sync == SYNC_TYPE_AUTO && loop->capt->ctl_rate_shift)
> +	if (loop->sync == SYNC_TYPE_AUTO && (loop->capt->ctl_rate_shift ||
> +			loop->capt->ctl_fback_freq))
>  		loop->sync = SYNC_TYPE_CAPTRATESHIFT;
> -	if (loop->sync == SYNC_TYPE_AUTO && loop->play->ctl_rate_shift)
> +	if (loop->sync == SYNC_TYPE_AUTO && (loop->play->ctl_rate_shift ||
> +			loop->play->ctl_fback_freq))
>  		loop->sync = SYNC_TYPE_PLAYRATESHIFT;
>  #ifdef USE_SAMPLERATE
>  	if (loop->sync == SYNC_TYPE_AUTO && loop->src_enable)