Re: [PATCH v3 2/3] ASoC: Add Multi CPU DAI support for PCM ops

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Thu, Apr 19, 2018 at 03:19:18PM +0530, Vinod Koul wrote:
> From: Shreyas NC <shreyas.nc@intel.com>
> 
> This adds support for Multi CPU DAIs in PCM ops and stream
> handling functions.
> 
> Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
> @@ -347,10 +372,10 @@ static void soc_pcm_set_msb(struct snd_pcm_substream *substream, int bits)
>  static void soc_pcm_apply_msb(struct snd_pcm_substream *substream)
>  {
>  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	struct snd_soc_dai *cpu_dai;
>  	struct snd_soc_dai *codec_dai;
>  	int i;
> -	unsigned int bits = 0, cpu_bits;
> +	unsigned int bits = 0, cpu_bits = 0;
>  
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>  		for (i = 0; i < rtd->num_codecs; i++) {
> @@ -361,7 +386,16 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream)
>  			}
>  			bits = max(codec_dai->driver->playback.sig_bits, bits);
>  		}
> -		cpu_bits = cpu_dai->driver->playback.sig_bits;
> +
> +		for (i = 0; i < rtd->num_cpu_dai; i++) {
> +			cpu_dai = rtd->cpu_dais[i];
> +			if (cpu_dai->driver->playback.sig_bits == 0) {
> +				cpu_bits = 0;
> +				break;
> +			}
> +
> +			cpu_bits = max(cpu_dai->driver->playback.sig_bits, bits);

Do you want cpu_bits at the end here? You are not going to get
the max of the cpu_bits here, you will end up with the max of the
CODEC bits and the last CPU bits?

> @@ -427,30 +464,47 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
>  		rates = snd_pcm_rate_mask_intersect(codec_stream->rates, rates);
>  	}
>  
> -	/*
> -	 * chan min/max cannot be enforced if there are multiple CODEC DAIs
> -	 * connected to a single CPU DAI, use CPU DAI's directly and let
> -	 * channel allocation be fixed up later
> -	 */
> -	if (rtd->num_codecs > 1) {
> -		chan_min = cpu_stream->channels_min;
> -		chan_max = cpu_stream->channels_max;
> -	}
> +	for (i = 0; i < rtd->num_cpu_dai; i++) {
> +		cpu_dai_drv = rtd->cpu_dais[i]->driver;
>  
> -	hw->channels_min = max(chan_min, cpu_stream->channels_min);
> -	hw->channels_max = min(chan_max, cpu_stream->channels_max);
> -	if (hw->formats)
> -		hw->formats &= formats & cpu_stream->formats;
> -	else
> -		hw->formats = formats & cpu_stream->formats;
> -	hw->rates = snd_pcm_rate_mask_intersect(rates, cpu_stream->rates);
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			cpu_stream = &cpu_dai_drv->playback;
> +		else
> +			cpu_stream = &cpu_dai_drv->capture;
>  
> -	snd_pcm_limit_hw_rates(runtime);
> +		/*
> +		 * chan min/max cannot be enforced if there are multiple
> +		 * CODEC DAIs connected to a single CPU DAI, use CPU DAI's
> +		 * directly and let channel allocation be fixed up later
> +		 */
> +		if (rtd->num_codecs > 1 && rtd->num_cpu_dai == 1) {
> +			chan_min = cpu_stream->channels_min;
> +			chan_max = cpu_stream->channels_max;
> +		}

This still doesn't quite look like, I am not seeing how the
multiple CPU DAI and single CPU DAI cases differ with respect to
whether we should ignore the CODEC channel settings?

> +
> +		hw->channels_min = max(hw->channels_min, chan_min);
> +		hw->channels_min = max(hw->channels_min,
> +					cpu_stream->channels_min);
> +		hw->channels_max = min(hw->channels_max,
> +					cpu_stream->channels_max);
> +		hw->channels_max = min(hw->channels_max,
> +					cpu_stream->channels_max);
> +
> +		if (hw->formats)
> +			hw->formats &= formats & cpu_stream->formats;

Minor nit.

This feels a little funny now, since we are anding each CPU
formats with the CODEC formats, and then anding them into the
result.

Feels like we should just and in the CODEC format once.

> +		else
> +			hw->formats = formats & cpu_stream->formats;

> +
> +		hw->rates = snd_pcm_rate_mask_intersect(rates,
> +						cpu_stream->rates);

I think this is broken since the rates will end up and the
intersection of the last CPU DAI rates and the CODEC rates, not
an intersection of all the CPU rates.

> @@ -602,7 +668,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
>  	}
>  
>  	pr_debug("ASoC: %s <-> %s info:\n",
> -			codec_dai_name, cpu_dai->name);
> +			codec_dai_name, cpu_dai_name);
>  	pr_debug("ASoC: rate mask 0x%x\n", runtime->hw.rates);
>  	pr_debug("ASoC: min ch %d max ch %d\n", runtime->hw.channels_min,
>  		 runtime->hw.channels_max);
> @@ -649,9 +715,13 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
>  		platform->driver->ops->close(substream);
>  
>  platform_err:
> -	if (cpu_dai->driver->ops->shutdown)
> -		cpu_dai->driver->ops->shutdown(substream, cpu_dai);
> -out:
> +	j = rtd->num_cpu_dai;
> +	while (--j >= 0) {
> +		cpu_dai = rtd->cpu_dais[j];
> +		if (cpu_dai->driver->ops->shutdown)
> +			cpu_dai->driver->ops->shutdown(substream, cpu_dai);
> +	}
> +

This will call shutdown for DAIs that never had startup called on
them, probably better to avoid that.

> @@ -1070,12 +1168,18 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
>  		platform->driver->ops->hw_free(substream);
>  
>  platform_err:
> -	if (cpu_dai->driver->ops->hw_free)
> -		cpu_dai->driver->ops->hw_free(substream, cpu_dai);
> +	j = rtd->num_cpu_dai;
>  
>  interface_err:
>  	i = rtd->num_codecs;
>  
> +	while (--j >= 0) {
> +		cpu_dai = rtd->cpu_dais[j];
> +
> +		if (cpu_dai->driver->ops && cpu_dai->driver->ops->hw_free)
> +			cpu_dai->driver->ops->hw_free(substream, cpu_dai);
> +	}
> +

Minor nit might be nicer to add this before the i = since that
kinda relates to the code underneath.

>  codec_err:
>  	while (--i >= 0) {
>  		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];

Thanks,
Charles