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

["Liao, Bard" <bard.liao@intel.com>]

> -----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_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.

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);
> >   	return ret;
> >   }
> >
> >
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel