[alsa-devel] [PATCH RFC v3 2/4] ASoC: Add multiple CPU DAI support for PCM ops

[Pierre-Louis Bossart]

I noticed a couple of code changes, we should probably do a code 
refactor first and add those changes, then add the multi-cpu support.

> @@ -892,10 +979,17 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
>   component_err:
>   	soc_pcm_components_hw_free(substream, component);
>   
> -	snd_soc_dai_hw_free(cpu_dai, substream);
> -	cpu_dai->rate = 0;
> +	i = rtd->num_cpus;
>   
>   interface_err:
> +	for_each_rtd_cpu_dai_rollback(rtd, i, cpu_dai) {
> +		if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream))
> +			continue;

maybe this check should be added to the existing code before adding 
multi-cpu support? it looks copy/pasted from the codec case, but is it a 
miss in the existing code?

> +
> +		snd_soc_dai_hw_free(cpu_dai, substream);
> +		cpu_dai->rate = 0;
> +	}
> +

[...]

> @@ -965,7 +1062,12 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
>   		snd_soc_dai_hw_free(codec_dai, substream);
>   	}
>   
> -	snd_soc_dai_hw_free(cpu_dai, substream);
> +	for_each_rtd_cpu_dai(rtd, i, cpu_dai) {
> +		if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream))
> +			continue;
> +
> +		snd_soc_dai_hw_free(cpu_dai, substream);
> +	}

same here, the hw_free should be first made conditional on the stream 
being valid, before introducing multi-cpu-dai support?


> @@ -1672,18 +1804,32 @@ static void dpcm_runtime_merge_chan(struct snd_pcm_substream *substream,
>   
>   	for_each_dpcm_be(fe, stream, dpcm) {
>   		struct snd_soc_pcm_runtime *be = dpcm->be;
> -		struct snd_soc_dai_driver *cpu_dai_drv =  be->cpu_dai->driver;
> +		struct snd_soc_dai_driver *cpu_dai_drv;
>   		struct snd_soc_dai_driver *codec_dai_drv;
>   		struct snd_soc_pcm_stream *codec_stream;
>   		struct snd_soc_pcm_stream *cpu_stream;
> +		struct snd_soc_dai *dai;
> +		int i;
>   
> -		if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> -			cpu_stream = &cpu_dai_drv->playback;
> -		else
> -			cpu_stream = &cpu_dai_drv->capture;
> +		for_each_rtd_cpu_dai(be, i, dai) {
> +			/*
> +			 * Skip CPUs which don't support the current stream
> +			 * type. See soc_pcm_init_runtime_hw() for more details
> +			 */
> +			if (!snd_soc_dai_stream_valid(dai, stream))
> +				continue;

and here as well, this is a new test that didn't exist before?


> @@ -2847,23 +3012,33 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>   		playback = rtd->dai_link->dpcm_playback;
>   		capture = rtd->dai_link->dpcm_capture;
>   	} else {
> +		int stream_playback;
> +		int stream_capture;
>   		/* Adapt stream for codec2codec links */
> -		struct snd_soc_pcm_stream *cpu_capture = rtd->dai_link->params ?
> -			&cpu_dai->driver->playback : &cpu_dai->driver->capture;
> -		struct snd_soc_pcm_stream *cpu_playback = rtd->dai_link->params ?
> -			&cpu_dai->driver->capture : &cpu_dai->driver->playback;
> +		if (rtd->dai_link->params) {
> +			stream_playback = SNDRV_PCM_STREAM_CAPTURE;
> +			stream_capture  = SNDRV_PCM_STREAM_PLAYBACK;
> +		} else {
> +			stream_playback = SNDRV_PCM_STREAM_PLAYBACK;
> +			stream_capture  = SNDRV_PCM_STREAM_CAPTURE;
> +		}
>   
> -		for_each_rtd_codec_dai(rtd, i, codec_dai) {
> -			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
> -			    snd_soc_dai_stream_valid(cpu_dai,   SNDRV_PCM_STREAM_PLAYBACK))

the logic for this entire block isn't easy to follow, maybe we should 
first move the cpu case out of the codec_dai loop and refactor the code 
before adding the multi-cpu case.

> -				playback = 1;
> -			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
> -			    snd_soc_dai_stream_valid(cpu_dai,   SNDRV_PCM_STREAM_CAPTURE))
> -				capture = 1;
> +		playback = 1;
> +		capture = 1;
> +
> +		for_each_rtd_cpu_dai(rtd, i, cpu_dai) {
> +			if (!snd_soc_dai_stream_valid(cpu_dai, stream_playback))
> +				playback = 0;
> +			if (!snd_soc_dai_stream_valid(cpu_dai, stream_capture))
> +				capture = 0;
>   		}
>   
> -		capture = capture && cpu_capture->channels_min;
> -		playback = playback && cpu_playback->channels_min;

channels_min is no longer used so it's somewhat confusing if the new 
code is iso-functionality?
I'd prefer a code refactor that we can double check, then add the 
cpu_dai loop.

> +		for_each_rtd_codec_dai(rtd, i, codec_dai) {
> +			if (!snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK))
> +				playback = 0;
> +			if (!snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE))
> +				capture = 0;
> +		}
>   	}
>   
>   	if (rtd->dai_link->playback_only) {
> @@ -2977,7 +3152,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>   out:
>   	dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
>   		 (rtd->num_codecs > 1) ? "multicodec" : rtd->codec_dai->name,
> -		 cpu_dai->name);
> +		 (rtd->num_cpus > 1) ? "multicpu" : rtd->cpu_dai->name);
>   	return ret;
>   }
>   
>