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

On Mon, Apr 30, 2018 at 05:22:36PM +0100, Charles Keepax wrote:

...

@@ -361,7 +386,16 @@ 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);
  

Do you want cpu_bits at the end here? You are not going to get the max of the cpu_bits here, you will end up with the max of the CODEC bits and the last CPU bits?

You are right, will fix this :)

...

@@ -427,30 +464,47 @@ 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 && rtd->num_cpu_dai == 1) {
  

• 	chan_min = cpu_stream->channels_min;
  

• 	chan_max = cpu_stream->channels_max;
  

• }
  

This still doesn't quite look like, I am not seeing how the multiple CPU DAI and single CPU DAI cases differ with respect to whether we should ignore the CODEC channel settings?

Ok

...

• hw->channels_min = max(hw->channels_min, chan_min);
  

• hw->channels_min = max(hw->channels_min,
  

• 			cpu_stream->channels_min);
  

• hw->channels_max = min(hw->channels_max,
  

• 			cpu_stream->channels_max);
  

• hw->channels_max = min(hw->channels_max,
  

• 			cpu_stream->channels_max);
  

• if (hw->formats)
  

• 	hw->formats &= formats & cpu_stream->formats;
  

Minor nit.

This feels a little funny now, since we are anding each CPU formats with the CODEC formats, and then anding them into the result.

Feels like we should just and in the CODEC format once.

Yes, makes sense. Thanks :)

...

• else
  

• 	hw->formats = formats & cpu_stream->formats;
  

...

• hw->rates = snd_pcm_rate_mask_intersect(rates,
  

• 				cpu_stream->rates);
  

I think this is broken since the rates will end up and the intersection of the last CPU DAI rates and the CODEC rates, not an intersection of all the CPU rates.

Ok, will fix this :) I have had a tough time in handling this function :(

...

@@ -602,7 +668,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) }

pr_debug("ASoC: %s <-> %s info:\n",

• 	codec_dai_name, cpu_dai->name);
  

• 	codec_dai_name, cpu_dai_name);
  

  pr_debug("ASoC: rate mask 0x%x\n", runtime->hw.rates); pr_debug("ASoC: min ch %d max ch %d\n", runtime->hw.channels_min, runtime->hw.channels_max);

@@ -649,9 +715,13 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) platform->driver->ops->close(substream);

platform_err:

• if (cpu_dai->driver->ops->shutdown)

• cpu_dai->driver->ops->shutdown(substream, cpu_dai);
  

-out:

• j = rtd->num_cpu_dai;
• while (--j >= 0) {

• cpu_dai = rtd->cpu_dais[j];
  

• if (cpu_dai->driver->ops->shutdown)
  

• 	cpu_dai->driver->ops->shutdown(substream, cpu_dai);
  

• }

This will call shutdown for DAIs that never had startup called on them, probably better to avoid that.

Ok, will fix this.

...

@@ -1070,12 +1168,18 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, platform->driver->ops->hw_free(substream);

platform_err:

• if (cpu_dai->driver->ops->hw_free)

• cpu_dai->driver->ops->hw_free(substream, cpu_dai);
  

• j = rtd->num_cpu_dai;

interface_err: i = rtd->num_codecs;

• while (--j >= 0) {

• cpu_dai = rtd->cpu_dais[j];
  

• if (cpu_dai->driver->ops && cpu_dai->driver->ops->hw_free)
  

• 	cpu_dai->driver->ops->hw_free(substream, cpu_dai);
  

• }

Minor nit might be nicer to add this before the i = since that kinda relates to the code underneath.

Sure, that aids readability. I will check if we can do that at other places as well :)

Thanks for the review!

--Shreyas --