-----Original Message----- From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] Sent: Friday, January 17, 2020 7:11 PM To: Bard liao yung-chuan.liao@linux.intel.com; broonie@kernel.org; tiwai@suse.de Cc: alsa-devel@alsa-project.org; liam.r.girdwood@linux.intel.com; kuninori.morimoto.gx@renesas.com; Liao, Bard bard.liao@intel.com Subject: 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?
Thanks Pierre for the review. My point is that we don't test snd_soc_dai_stream_valid in the existing code since we assume the cpu dai will support the set of all codec dais. For example, the cpu dai will support both playback and capture if a dai link with one cpu and two codecs dai which support playback and capture separated. However, we may have two cpus dai which support different directions in multi cpu dai link case.
I do see I miss a snd_soc_dai_stream_valid() test above.
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?
Or adding multi-cpu-dai support first then checking stream? The reason is we don't need to test snd_soc_dai_stream_valid if we don't support multi-cpu-dai
@@ -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 *dai;int i;
if (stream == SNDRV_PCM_STREAM_PLAYBACK)cpu_stream = &cpu_dai_drv->playback;elsecpu_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 moredetails
*/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;
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.
I will try to split the patch.
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?
channels_min is to check if the stream is valid. It is replaced by snd_soc_dai_stream_valid().
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);