[alsa-devel] [PATCH 2/3] ASoC: Add Multi CPU DAI support for PCM ops

[Shreyas NC]

On Fri, Mar 09, 2018 at 09:23:58PM +0530, Charles Keepax wrote:
> On Tue, Mar 06, 2018 at 04:30:29PM +0530, Shreyas NC wrote:
> > This adds support for Multi CPU DAIs in PCM ops and stream
> > handling functions.
> > 
> > Signed-off-by: Shreyas NC <shreyas.nc at intel.com>
> > ---
> > @@ -313,13 +333,17 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
> >  static bool soc_pcm_has_symmetry(struct snd_pcm_substream *substream)
> >  {
> >  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > -	struct snd_soc_dai_driver *cpu_driver = rtd->cpu_dai->driver;
> >  	struct snd_soc_dai_link *link = rtd->dai_link;
> > -	unsigned int symmetry, i;
> > +	unsigned int symmetry = 0, i;
> >  
> > -	symmetry = cpu_driver->symmetric_rates || link->symmetric_rates ||
> > -		cpu_driver->symmetric_channels || link->symmetric_channels ||
> > -		cpu_driver->symmetric_samplebits || link->symmetric_samplebits;
> > +	/* Apply symmetery for multiple cpu dais */
> > +	for (i = 0; i < rtd->num_cpu_dai; i++)
> > +		symmetry = rtd->cpu_dais[i]->driver->symmetric_rates ||
> > +						link->symmetric_rates ||
> > +			rtd->cpu_dais[i]->driver->symmetric_channels ||
> > +						link->symmetric_channels ||
> > +			rtd->cpu_dais[i]->driver->symmetric_samplebits ||
> > +						link->symmetric_samplebits;
> 
> No need to bring the link-> stuff into the loop, it won't change
> on each iteration. Would also make the code look neater to leave
> it before the loop.
> 

Sure, makes sense.

> >  
> >  	for (i = 0; i < rtd->num_codecs; i++)
> >  		symmetry = symmetry ||
> > @@ -347,10 +371,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 +385,17 @@ 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);
> 
> Can help but feel this would look nicer if you only wrapped the
> second argument down a line. Although tbf its only 1 character so
> you could probably just run over the line length as well.
> 

Ok

> > @@ -427,30 +464,41 @@ 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) {
> > +			chan_min = cpu_stream->channels_min;
> > +			chan_max = cpu_stream->channels_max;
> > +		}
> > +
> > +		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;
> >  
> 
> I don't think actually ends up with the correct values in
> hw->channels_min/max. Nothing compares one iteration of the
> loop to the previous one so you don't end up with the min/max for
> all the CPU DAIs you just end up with the values from the last
> CPU DAI.
> 

Yes, you are right. Thanks for pointing that out :)

> >  /*
> > @@ -465,12 +513,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
> >  	struct snd_soc_platform *platform = rtd->platform;
> >  	struct snd_soc_component *component;
> >  	struct snd_soc_rtdcom_list *rtdcom;
> > -	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> > +	struct snd_soc_dai *cpu_dai;
> >  	struct snd_soc_dai *codec_dai;
> >  	const char *codec_dai_name = "multicodec";
> > -	int i, ret = 0, __ret;
> > +	const char *cpu_dai_name = "multicpu";
> > +	int i, ret = 0, __ret, j;
> > +
> > +	for (i = 0; i < rtd->num_cpu_dai; i++)
> > +		pinctrl_pm_select_default_state(rtd->cpu_dais[i]->dev);
> >  
> > -	pinctrl_pm_select_default_state(cpu_dai->dev);
> >  	for (i = 0; i < rtd->num_codecs; i++)
> >  		pinctrl_pm_select_default_state(rtd->codec_dais[i]->dev);
> >  
> > @@ -483,12 +534,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
> >  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> >  
> >  	/* startup the audio subsystem */
> > -	if (cpu_dai->driver->ops->startup) {
> > -		ret = cpu_dai->driver->ops->startup(substream, cpu_dai);
> > -		if (ret < 0) {
> > -			dev_err(cpu_dai->dev, "ASoC: can't open interface"
> > -				" %s: %d\n", cpu_dai->name, ret);
> > -			goto out;
> > +	for (i = 0; i < rtd->num_cpu_dai; i++) {
> > +		cpu_dai = rtd->cpu_dais[i];
> > +		if (cpu_dai->driver->ops->startup) {
> > +			ret = cpu_dai->driver->ops->startup(substream, cpu_dai);
> > +			if (ret < 0) {
> > +				dev_err(cpu_dai->dev, "ASoC: can't open interface %s: %d\n",
> > +							cpu_dai->name, ret);
> > +				goto out;
> 
> I don't believe this jumps to the right place anymore since you
> need to shutdown any CPU DAIs you have already started up.
> 

Ok, I will re-visit this and correct it in v2.

> > @@ -1276,8 +1388,12 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
> >  		break;
> >  	}
> >  
> > -	if (cpu_dai->driver->ops->delay)
> > -		delay += cpu_dai->driver->ops->delay(substream, cpu_dai);
> > +	for (i = 0; i < rtd->num_cpu_dai; i++) {
> > +		cpu_dai = rtd->cpu_dais[i];
> > +		if (cpu_dai->driver->ops->delay)
> > +			delay += cpu_dai->driver->ops->delay(substream,
> > +								cpu_dai);
> > +	}
> 
> I am not clear we should be adding the delays here can't they all
> run in parallel? In which case shouldn't we be taking the max?
> 

Yes, you are right that we shouldn't be adding the delays and max() would be
a right check.

> > @@ -2998,8 +3139,13 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> >  				capture = 1;
> >  		}
> >  
> > -		capture = capture && cpu_dai->driver->capture.channels_min;
> > -		playback = playback && cpu_dai->driver->playback.channels_min;
> > +		for (i = 0; i < rtd->num_cpu_dai; i++) {
> > +			cpu_dai = rtd->cpu_dais[i];
> > +			capture = capture &&
> > +					cpu_dai->driver->capture.channels_min;
> > +			playback = playback &&
> > +					cpu_dai->driver->playback.channels_min;
> > +		}
> 
> This doesn't look right either since you will end up with the
> values for the last CPU DAI surely you want some sort of combined
> value.
> 

Yeah, in hindsight it does not make sense. So, I think it would be better to
keep the logic same as above (multiple codec DAI)

> I am also a little nervous about the mapping between widgets and
> DAIs in dpcm_prune_paths. But I don't think I really understand
> that bit of the code well enough to provide good comments.
> Hopefully someone with more understanding than me can have a look
> :-)
> 

Even I am not too sure about this and I followed what is done for codec DAI
which as you suspect may not be the right thing.
Let us wait for others to review :)

--Shreyas
--