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

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; }