[alsa-devel] [FT C400, PATCH RFC, v4 02/10] usb-audio: correct sync ep init

[Takashi Iwai]

At Wed, 28 Nov 2012 23:55:33 +0100,
Eldad Zack wrote:
> 
> Commit 947d299686aa9cc8aecf749d54e8475c6e498956
> "ALSA: snd-usb: properly initialize the sync endpoint" introduced a
> regression for devices that has a sync endpoint with a channel
> number different than the data endpoint channel.
> Due to a different channel and period bytes count, attempting to
> initialize the sync endpoint will fail. With xhci it yields:
> 
> cannot submit urb 0, error -90: internal error
> 
> With this patch, if a sync endpoint has multiple audioformats, an
> audioformat with a matching number of channels is preferred. Otherwise,
> an audioformat will be picked randomly (i.e., first audioformat with a
> non-zero channel count) and the period bytes count is adjusted
> accordingly.
> 
> Cc: Jeffrey Barish <jeff_barish at earthlink.net>
> Cc: Daniel Mack <zonque at gmail.com>
> Signed-off-by: Eldad Zack <eldad at fogrefinery.com>
> ---
>  sound/usb/pcm.c |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 72 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index ff8cbbf..2158943 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -438,6 +438,76 @@ add_sync_ep:
>  }
>  
>  /*
> + * Configure the sync ep using the rate and pcm format of the data ep.
> + */
> +static int configure_sync_endpoint(struct snd_usb_substream *subs)
> +{
> +	int ret;
> +	struct list_head *p;
> +	struct audioformat *sync_fp;
> +	int sync_channels = 0;
> +	int sync_period_bytes = subs->period_bytes;
> +	int direction = subs->direction ^ 1;

Better to rename for avoiding misunderstanding.
"direction" sounds like the direction of this substream.

> +	struct snd_usb_substream *sync_subs =
> +		&subs->stream->substream[direction];
> +
> +	if (sync_subs->ep_num != subs->sync_endpoint->ep_num) {
> +		snd_printk(KERN_ERR "%s: cannot find the sync ep (direction %d, found %x, wanted %x).\n",
> +			__func__, direction,
> +				subs->stream->substream[direction].ep_num,
> +				subs->sync_endpoint->ep_num);
> +		return -EINVAL;
> +	}

This sanity check would suit better where the implicit_fb is checked.
It doesn't have to be performed at each time.

And, the whole restriction you introduced is only for implicit fb
case, no?

> +
> +	/*
> +	 * Try to find a format with a number of channels matching
> +	 * the data substream. If no such format was found, pick one
> +	 * with non zero channel number at random.
> +	 */
> +	list_for_each(p, &sync_subs->fmt_list) {

Cleaner to use list_for_each_entry().
(Yes, there are other codes using list_for_each() there, but remember
 how old the usb-audio driver code is :)

> +		struct audioformat *fp;
> +		fp = list_entry(p, struct audioformat, list);
> +
> +		if (sync_channels == 0) {
> +			sync_fp = fp;
> +			sync_channels = fp->channels;
> +		} else {
> +			if (fp->channels == subs->channels) {
> +				sync_channels = fp->channels;
> +				sync_fp = fp;
> +			}

Can be simplified like

		if (!sync_channels || fp->channels == sub->channels) {
			sync_channels = fp->channels;
			sync_fp = fp;
		}

But, I wonder whether you don't have to check other parameters such as
rate, format, etc?


thanks,

Takashi

> +		}
> +	}
> +
> +	if (sync_channels < 1) {
> +		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
> +			__func__, sync_subs->ep_num);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Recalculated the period bytes if channel number differ between
> +	 * data and sync ep.
> +	 */
> +	if (sync_channels != subs->channels) {
> +		sync_period_bytes = (subs->period_bytes / subs->channels) *
> +			sync_channels;
> +		snd_printdd(KERN_INFO "%s: sync ep period bytes recalc %d -> %d\n",
> +			__func__, subs->period_bytes, sync_period_bytes);
> +	}
> +
> +	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> +					  subs->pcm_format,
> +					  sync_channels,
> +					  sync_period_bytes,
> +					  subs->cur_rate,
> +					  sync_fp,
> +					  NULL);
> +
> +	return ret;
> +}
> +
> +/*
>   * configure endpoint params
>   *
>   * called  during initial setup and upon resume
> @@ -459,13 +529,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
>  		return ret;
>  
>  	if (subs->sync_endpoint)
> -		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> -						  subs->pcm_format,
> -						  subs->channels,
> -						  subs->period_bytes,
> -						  subs->cur_rate,
> -						  subs->cur_audiofmt,
> -						  NULL);
> +		ret = configure_sync_endpoint(subs);
> +
>  	return ret;
>  }
>  
> -- 
> 1.7.8.6
>