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

On 06/20/2018 05:54 AM, Shreyas NC wrote:

Add support in PCM operations to invoke multiple cpu dais as we do for multiple codec dais. Also the symmetry calculations are updated to reflect multiple cpu dais.

This doesn't apply on Mark's tree? Looks like you need to rebase on top of 244e293690a6 ("ASoC: pcm: Tidy up open/hw_params handling") contributed by Charles on June 19.

Reviewed-by: Charles Keepax ckeepax@opensource.cirrus.com Signed-off-by: Vinod Koul vkoul@kernel.org Signed-off-by: Shreyas NC shreyas.nc@intel.com

---

sound/soc/soc-pcm.c | 491 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 332 insertions(+), 159 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 5e7ae47..cdcbc4f 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -64,23 +64,27 @@ static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream) */ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) {

• struct snd_soc_dai *cpu_dai = rtd->cpu_dai; int i;

  lockdep_assert_held(&rtd->pcm_mutex);

  if (stream == SNDRV_PCM_STREAM_PLAYBACK) {

• cpu_dai->playback_active++;
  

• for (i = 0; i < rtd->num_cpu_dai; i++)
  

• 	rtd->cpu_dais[i]->playback_active++;
  

  for (i = 0; i < rtd->num_codecs; i++) rtd->codec_dais[i]->playback_active++; } else {

• cpu_dai->capture_active++;
  

• for (i = 0; i < rtd->num_cpu_dai; i++)
  

• 	rtd->cpu_dais[i]->capture_active++;
  

  for (i = 0; i < rtd->num_codecs; i++) rtd->codec_dais[i]->capture_active++; }

• cpu_dai->active++;
• cpu_dai->component->active++;

• for (i = 0; i < rtd->num_cpu_dai; i++) {

• rtd->cpu_dais[i]->component->active++;
  

• rtd->cpu_dais[i]->active++;
  

• }

This is becoming complicated, how many times do we need to ref-count?

• for (i = 0; i < rtd->num_codecs; i++) { rtd->codec_dais[i]->active++; rtd->codec_dais[i]->component->active++;

@@ -99,23 +103,27 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) */ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream) {

• struct snd_soc_dai *cpu_dai = rtd->cpu_dai; int i;

  lockdep_assert_held(&rtd->pcm_mutex);

  if (stream == SNDRV_PCM_STREAM_PLAYBACK) {

• cpu_dai->playback_active--;
  

• for (i = 0; i < rtd->num_cpu_dai; i++)
  

• 	rtd->cpu_dais[i]->playback_active--;
  

  for (i = 0; i < rtd->num_codecs; i++) rtd->codec_dais[i]->playback_active--; } else {

• cpu_dai->capture_active--;
  

• for (i = 0; i < rtd->num_cpu_dai; i++)
  

• 	rtd->cpu_dais[i]->capture_active--;
  

  for (i = 0; i < rtd->num_codecs; i++) rtd->codec_dais[i]->capture_active--; }

• cpu_dai->active--;
• cpu_dai->component->active--;

• for (i = 0; i < rtd->num_cpu_dai; i++) {

• rtd->cpu_dais[i]->component->active--;
  

• rtd->cpu_dais[i]->active--;
  

• }
• for (i = 0; i < rtd->num_codecs; i++) { rtd->codec_dais[i]->component->active--; rtd->codec_dais[i]->active--;

@@ -258,7 +266,6 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_soc_pcm_runtime *rtd = substream->private_data;

• struct snd_soc_dai *cpu_dai = rtd->cpu_dai; unsigned int rate, channels, sample_bits, symmetry, i;

  rate = params_rate(params);

@@ -266,41 +273,54 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream, sample_bits = snd_pcm_format_physical_width(params_format(params));

/* reject unmatched parameters when applying symmetry */

• symmetry = cpu_dai->driver->symmetric_rates ||

• rtd->dai_link->symmetric_rates;
  

This was a logical OR, but...

• symmetry = rtd->dai_link->symmetric_rates;
• for (i = 0; i < rtd->num_cpu_dai; i++)

• symmetry |= rtd->cpu_dais[i]->driver->symmetric_rates;
  

this is a bitwise OR. is this ok?

for (i = 0; i < rtd->num_codecs; i++) symmetry |= rtd->codec_dais[i]->driver->symmetric_rates;

• if (symmetry && cpu_dai->rate && cpu_dai->rate != rate) {

• dev_err(rtd->dev, "ASoC: unmatched rate symmetry: %d - %d\n",
  

• 		cpu_dai->rate, rate);
  

• return -EINVAL;
  

• }

• for (i = 0; i < rtd->num_cpu_dai; i++)

• if (symmetry && rtd->cpu_dais[i]->rate &&
  

you could move the if (symmetry) out of the for loop to simplify

• 			rtd->cpu_dais[i]->rate != rate) {
  

• 	dev_err(rtd->dev, "ASoC: unmatched rate symmetry: %d - %d\n",
  

• 			rtd->cpu_dais[i]->rate, rate);
  

• 	return -EINVAL;
  

• }
  
  

• symmetry = cpu_dai->driver->symmetric_channels ||

• rtd->dai_link->symmetric_channels;
  

• symmetry = rtd->dai_link->symmetric_channels;

• for (i = 0; i < rtd->num_cpu_dai; i++)

• symmetry |= rtd->cpu_dais[i]->driver->symmetric_channels;
  
  

  for (i = 0; i < rtd->num_codecs; i++) symmetry |= rtd->codec_dais[i]->driver->symmetric_channels;

• if (symmetry && cpu_dai->channels && cpu_dai->channels != channels) {

• dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n",
  

• 		cpu_dai->channels, channels);
  

• return -EINVAL;
  

• }

• for (i = 0; i < rtd->num_cpu_dai; i++)

• if (symmetry && rtd->cpu_dais[i]->channels &&
  

here as well

• 		rtd->cpu_dais[i]->channels != channels) {
  

• 	dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n",
  

• 			rtd->cpu_dais[i]->channels, channels);
  

• 	return -EINVAL;
  

• }
  
  

• symmetry = cpu_dai->driver->symmetric_samplebits ||

• rtd->dai_link->symmetric_samplebits;
  

• symmetry = rtd->dai_link->symmetric_samplebits;

• for (i = 0; i < rtd->num_cpu_dai; i++)

• symmetry |= rtd->cpu_dais[i]->driver->symmetric_samplebits;
  
  

  for (i = 0; i < rtd->num_codecs; i++) symmetry |= rtd->codec_dais[i]->driver->symmetric_samplebits;

• if (symmetry && cpu_dai->sample_bits && cpu_dai->sample_bits != sample_bits) {

• dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n",
  

• 		cpu_dai->sample_bits, sample_bits);
  

• return -EINVAL;
  

• }

• for (i = 0; i < rtd->num_cpu_dai; i++)

• if (symmetry && rtd->cpu_dais[i]->sample_bits &&
  

• 		rtd->cpu_dais[i]->sample_bits != sample_bits) {
  

• 	dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n",
  

• 				rtd->cpu_dais[i]->sample_bits,
  

• 						sample_bits);
  

• 	return -EINVAL;
  

• }
  
  

  return 0; }

@@ -308,13 +328,18 @@ 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;

• symmetry = cpu_driver->symmetric_rates || link->symmetric_rates ||

• cpu_driver->symmetric_channels || link->symmetric_channels ||
  

• cpu_driver->symmetric_samplebits || link->symmetric_samplebits;
  

• symmetry = link->symmetric_rates || link->symmetric_channels ||

• 	link->symmetric_samplebits;
  

• /* Apply symmetery for multiple cpu dais */

You haven't fixed this comment, is this patch the correct one?

@@ -422,30 +461,55 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) rates = snd_pcm_rate_mask_intersect(codec_stream->rates, rates); }

• for (i = 0; i < rtd->num_cpu_dai; i++) {

• cpu_dai_drv = rtd->cpu_dais[i]->driver;
  

• if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
  

• 	cpu_stream = &cpu_dai_drv->playback;
  

• else
  

• 	cpu_stream = &cpu_dai_drv->capture;
  

• cpu_chan_min = max(cpu_chan_min,
  

• 			cpu_stream->channels_min);
  

• cpu_chan_max = min(cpu_chan_max,
  

• 			cpu_stream->channels_max);
  

• if (hw->formats)
  

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

• else
  

• 	hw->formats = cpu_stream->formats;
  

Can you double-check the 'else' case? This doesn't seem right, you will always use the format used for the last cpu_dai. If the formats are identical for all cpu_dais, then this can be moved outside of the loop.

@@ -963,11 +1070,14 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, if (ret < 0) goto component_err;

• /* store the parameters for each DAIs */
• cpu_dai->rate = params_rate(params);
• cpu_dai->channels = params_channels(params);
• cpu_dai->sample_bits =

• snd_pcm_format_physical_width(params_format(params));
  

• for (i = 0; i < rtd->num_cpu_dai; i++) {

• /* store the parameters for each DAIs */
  

• cpu_dai = rtd->cpu_dais[i];
  

• cpu_dai->rate = params_rate(params);
  

• cpu_dai->channels = params_channels(params);
  

• cpu_dai->sample_bits =
  

• 	snd_pcm_format_physical_width(params_format(params));
  

• }

I think I've asked this before but can't recall the answer: does this mean we assume the same number of channels for each codec_dai[j] and cpu_dai[i]?

• for (i = 0; i < rtd->num_cpu_dai; i++) {

• cpu_dai = rtd->cpu_dais[i];
  

• if (cpu_dai->driver->ops->trigger) {
  

• 	ret = cpu_dai->driver->ops->trigger(substream,
  

• 						cmd, cpu_dai);
  

• 	if (ret < 0)
  

• 		return ret;
  

• }
  

Maybe add a comment that in the multi_cpu case the triggers are supposed to be aligned, i.e. the first trigger starts the others - that would be consistent with the other comments on delay below.

@@ -1778,11 +1941,13 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream, be_substream->runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;

/* Symmetry only applies if we've got an active stream. */

• if (rtd->cpu_dai->active) {
  

• 	err = soc_pcm_apply_symmetry(fe_substream,
  

• 				     rtd->cpu_dai);
  

• 	if (err < 0)
  

• 		return err;
  

• for (i = 0; i < rtd->num_cpu_dai; i++) {
  

• 	if (rtd->cpu_dais[i]->active) {
  

• 		err = soc_pcm_apply_symmetry(be_substream,
  

• 					rtd->cpu_dais[i]);
  

• 		if (err < 0)
  

• 			return err;
  

• 	}
  

  }

Can you recheck this block? In the original code the symmetry is with the fe_substream, it's now with a be_substream. Looks to me like a major typo having significant impact of the result...

for (i = 0; i < rtd->num_codecs; i++) {

@@ -2884,13 +3049,13 @@ static int soc_rtdcom_mmap(struct snd_pcm_substream *substream, int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) { struct snd_soc_dai *codec_dai;

• struct snd_soc_dai *cpu_dai = rtd->cpu_dai;

• struct snd_soc_dai *cpu_dai; struct snd_soc_component *component; struct snd_soc_rtdcom_list *rtdcom; struct snd_pcm *pcm; char new_name[64]; int ret = 0, playback = 0, capture = 0;

• int i;

• int i, cpu_capture = 0, cpu_playback = 0;

  if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) { playback = rtd->dai_link->dpcm_playback;

@@ -2904,8 +3069,16 @@ 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];
  

• 	if (cpu_dai->driver->playback.channels_min)
  

• 		cpu_playback = 1;
  

• 	if (cpu_dai->driver->capture.channels_min)
  

• 		cpu_capture = 1;
  

• }
  

• playback = playback && cpu_playback;
  

• capture = capture && cpu_capture;
  

  }

  if (rtd->dai_link->playback_only) {

@@ -3026,7 +3199,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_cpu_dai > 1) ? "multicpu" : rtd->cpu_dai->name);
  

  return ret; }

Took me a couple of hours to reach the end of this patch ... I had to use a visual diff to figure it out, the diff format is just too hard to look at.