[RFC] ASoC: Add Multi CPU DAI support by bardliao · Pull Request #1644 · thesofproject/linux

bardliao · 2019-12-20T06:47:16Z

This is a rebase version of https://www.alsa-project.org/pipermail/alsa-devel/2018-June/137190.html

The original commit message is:

As discussed in [1], ASoC core supports multi codec DAIs
on a DAI link. However it does not do so for CPU DAIs.

So, add support for multi CPU DAIs on a DAI Link by adding
multi CPU DAI in Card instantiation, suspend and resume
functions, PCM ops, stream handling functions and DAPM.

[1]: https://www.spinics.net/lists/alsa-devel/msg71369.html

Thanks @ranj063 for the comments. I addressed some, but I need more
time or help from others for the other comments.

ranj063 · 2019-12-20T07:21:16Z

 		int i;

-		pinctrl_pm_select_sleep_state(cpu_dai->dev);
+		for_each_rtd_codec_dai(rtd, i, cpu_dai) {


@bardliao typo here. For_each_cpu_dai?

Thank will update it.

ranj063 · 2019-12-20T07:22:24Z

-
-		if (cpu_dai->active)
-			pinctrl_pm_select_default_state(cpu_dai->dev);
+		for_each_rtd_codec_dai(rtd, i, cpu_dai) {


Looks like typo here as well? For each cpu dai?

Indeed. Thanks.

ranj063 · 2019-12-20T07:23:05Z


+	/*
+	 * for rtd->cpu_dais
+	 */


This comment is not very helpful at all

This is to be consistent with the existing code. ie

/* * for rtd->codec_dais */

above.

ranj063 · 2019-12-20T07:27:15Z

-		/* prune the BE if it's no longer in our active list */
-		if (widget && widget_in_list(list, widget))
+			/*
+			 * The BE is pruned only if none of the the cpu_dai


Duplicate "the" in comment

Thank will update it.

plbossart

Looks good @bardliao, see comments below

plbossart · 2019-12-20T22:48:30Z


+	struct snd_soc_dai **cpu_dais;
+	unsigned int num_cpu_dai;
+


I wonder why we need to keep 'codec_dai' now that we have 'codec_dais" and 'num_codecs'?

Also why do we need 'cpu_dai' as well as 'cpus_dais' and 'num_cpu_dai'

Looks like room for additional cleanups

I wonder why we need to keep 'codec_dai' now that we have 'codec_dais" and 'num_codecs'?

Also why do we need 'cpu_dai' as well as 'cpus_dais' and 'num_cpu_dai'

Looks like room for additional cleanups

Indeed, we can send another patch to remove cpu_dai and codec_dai

plbossart · 2019-12-20T22:52:00Z

-	if (cpu_dai->component->driver->non_legacy_dai_naming) {
-		unsigned int inv_dai_fmt;
+	for_each_rtd_cpu_dai(rtd, i, cpu_dai) {
+		if (cpu_dai->component->driver->non_legacy_dai_naming) {


what is the 'legacy_dai_naming' and can it be removed now?

what is the 'legacy_dai_naming' and can it be removed now?

@plbossart No, I don't think it can be removed now. Codec will be registered to cpu dai in CODEC <-> CODEC link, and we can use the non_legacy_dai_naming flag to know that is codec.

plbossart · 2019-12-20T22:55:32Z

+	for_each_rtd_cpu_dai(rtd, i, cpu_dai) {
+		ret = snd_soc_dai_hw_params(cpu_dai, substream, params);
+		if (ret < 0)
+			goto interface_err;


you need to add the error handling here, the interface_err only deals with the codec case

you need to add the error handling here, the interface_err only deals with the codec case

Yes, thanks @plbossart

@plbossart Updated

plbossart · 2019-12-20T22:56:29Z

+			if (widget && widget_in_list(list, widget))
+				do_prune = 0;
+		}
+		if (!do_prune)


this will conflict with @lyakh 's work...

this will conflict with @lyakh 's work...

Sorry, @plbossart @lyakh Could you describe it in more detail? Which PR will be affected?

@bardliao it's #1501 but I think it should be ok for you to proceed with your PR for now. I'm not sure what @plbossart's plan is, but as far as I understand, even after #1501 is fully reviewed and approved here, we'll first need to also get it reviewed on ALSA and virtualisation lists, so, it will take a while.

plbossart · 2019-12-20T22:58:28Z

 	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);


this looks really ugly, what cant' we use rtd->cpu_dais[0].name and completely remove the use of rtd->cpu_dai?

plbossart · 2019-12-20T23:01:24Z

+	for_each_rtd_codec_dai(rtd, i, codec_dai) {
+		for_each_rtd_cpu_dai(rtd, j, cpu_dai) {
+			dapm_add_valid_dai_widget(card, rtd,
+						  codec_dai, cpu_dai);


this seems odd. You will not have a connection between each CPU dai and each codec_dai

e,g. for Soundwire, you will connect ALH1.3 to RT1308.1.aif1 and ALH2.3 to RT1308.2.aif1

There will be no routing/link between e.g. ALH1.3 and RT1308.2.aif1

this seems odd. You will not have a connection between each CPU dai and each codec_dai

e,g. for Soundwire, you will connect ALH1.3 to RT1308.1.aif1 and ALH2.3 to RT1308.2.aif1

There will be no routing/link between e.g. ALH1.3 and RT1308.2.aif1

Yes, it is what we are missing now. Thanks @plbossart

plbossart · 2019-12-20T23:02:38Z

@morimoto if you have spare cycles your feedback on this PR would be more than appreciated.

bardliao · 2020-01-06T06:08:04Z

@plbossart I compared and merged @morimoto 's version. Could you review it? I have two question.

Do we need to support multi-cpu for FE? What is the scenario of multi-cpu on codec to codec dai link?

lyakh · 2020-01-06T08:12:57Z

+		cpu_rate_max = min_not_zero(cpu_rate_max, cpu_stream->rate_max);
+		formats &= cpu_stream->formats;
+		cpu_rates = snd_pcm_rate_mask_intersect(cpu_stream->rates,
+							rates);


Good catch. Thanks @lyakh

lyakh · 2020-01-06T09:00:24Z

+			if (widget && widget_in_list(list, widget))
+				do_prune = 0;
+		}
+		if (!do_prune)


@bardliao it's #1501 but I think it should be ok for you to proceed with your PR for now. I'm not sure what @plbossart's plan is, but as far as I understand, even after #1501 is fully reviewed and approved here, we'll first need to also get it reviewed on ALSA and virtualisation lists, so, it will take a while.

lyakh · 2020-01-06T09:04:47Z

+				dapm_add_valid_dai_widget(card, rtd,
+							  codec_dai,
+							  rtd->cpu_dais[i]);
+			}


superfluous parentheses

Updated. Thanks.

lyakh · 2020-01-06T09:05:06Z

+		for_each_rtd_codec_dai(rtd, i, codec_dai) {
+			dapm_add_valid_dai_widget(card, rtd,
+						  codec_dai, rtd->cpu_dais[0]);
 		}


Updated. Thanks.

lyakh · 2020-01-06T09:07:59Z


+	if (rtd->num_cpus > 1)
+		dev_warn(rtd->dev,
+			 "%s doesn't support Multi CPU yet\n", __func__);


What would actually happen in such a case? Would it still work at least with just one CPU codec? Shouldn't we return an error in this and similar cases instrad?

What would actually happen in such a case? Would it still work at least with just one CPU codec? Shouldn't we return an error in this and similar cases instrad?

To be honest, I don't know how to test it. I should still work with one CPU. I don't want to return an error for now is because I want to know what will happen if these functions are invoked with multi cpus.

plbossart · 2020-01-15T01:08:29Z

@bardliao I've lost track of the directions here, Mark Brown seemed to think this was a good patchset, are you going to update the PR, send the changes directly to alsa-devel? Let me know, thanks!

bardliao · 2020-01-15T01:13:51Z

@bardliao I've lost track of the directions here, Mark Brown seemed to think this was a good patchset, are you going to update the PR, send the changes directly to alsa-devel? Let me know, thanks!

@plbossart Yes, I will double check the comments and send a new version to alsa-devel after addressing all comments.

dbaluta · 2020-01-17T08:06:15Z

+				link->name);
+			return -EINVAL;
+		}



Something is off here. Either the error message is wrong or the if condition is wrong. If none of cpu->name / cpu->of_node are set we won't hit the error message.

This is not a blocker for this PR as the issue was pre-existent. Will send an email to alsa-devel.

ASoC core supports multiple codec DAIs but supports only a CPU DAI. To support multiple cpu DAIs, add cpu_dai and num_cpu_dai in snd_soc_dai_link and snd_soc_pcm_runtime structures similar to support for codec_dai. This is intended as a preparatory patch to eventually support the unification of the Codec and CPU DAI. Inline with multiple codec DAI approach, add support to allocate, init, bind and probe multiple cpu_dai on init if driver specifies that. Also add support to loop over multiple cpu_dai during suspend and resume. This is intended as a preparatory patch to eventually unify the CPU and Codec DAI into DAI components. Signed-off-by: Shreyas NC <shreyas.nc@intel.com> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

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. Signed-off-by: Vinod Koul <vkoul@kernel.org> Signed-off-by: Shreyas NC <shreyas.nc@intel.com> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Adding a helper to connect widget for a specific cpu and codec dai The helper will help dapm_connect_dai_link_widgets() to reduce indents. Signed-off-by: Vinod Koul <vkoul@kernel.org> Signed-off-by: Shreyas NC <shreyas.nc@intel.com> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

DAPM handles DAIs during soc_dapm_stream_event() and during addition and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and dapm_connect_dai_link_widgets(). Extend these functions to handle multiple cpu dai. Signed-off-by: Vinod Koul <vkoul@kernel.org> Signed-off-by: Shreyas NC <shreyas.nc@intel.com> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Multi cpu is not supported by all functions yet. Add an error message and return. Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>

Now multi-cpu-dai is supported. We have the same resaon as multi-codec-dai to skip CPUs which don't support the current stream. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart · 2020-02-20T18:35:57Z

replaced by PR #1802

bardliao requested review from RanderWang, keyonjie, kv2019i, lgirdwood, morimoto, plbossart and ranj063 December 20, 2019 06:47

ranj063 reviewed Dec 20, 2019

View reviewed changes

bardliao force-pushed the for-multi-cpu branch from 8f63a25 to bd28394 Compare December 20, 2019 08:09

plbossart requested changes Dec 20, 2019

View reviewed changes

bardliao force-pushed the for-multi-cpu branch 5 times, most recently from fa4266b to 30cc579 Compare December 26, 2019 05:53

bardliao force-pushed the for-multi-cpu branch 2 times, most recently from 8d64b21 to 4a4b971 Compare January 2, 2020 09:07

bardliao force-pushed the for-multi-cpu branch from 4a4b971 to fdce38f Compare January 6, 2020 06:51

mengdonglin mentioned this pull request Jan 6, 2020

[Bug][SDW][CML_2 RTC1308] Paplay via speaker only has Left Channel Output. #1564

Closed

lyakh requested changes Jan 6, 2020

View reviewed changes

bardliao force-pushed the for-multi-cpu branch from fdce38f to 0e7bc90 Compare January 7, 2020 02:46

bardliao force-pushed the for-multi-cpu branch from 0e7bc90 to e695059 Compare January 15, 2020 03:33

bardliao force-pushed the for-multi-cpu branch 2 times, most recently from ce89c61 to 279476a Compare January 17, 2020 07:54

dbaluta reviewed Jan 17, 2020

View reviewed changes

bardliao force-pushed the for-multi-cpu branch from 279476a to 39b2ecc Compare January 17, 2020 08:08

bardliao force-pushed the for-multi-cpu branch from 39b2ecc to a2aee90 Compare January 20, 2020 14:15

shreyasnc1 and others added 5 commits January 20, 2020 22:59

ASoC: return error if the function is not support multi cpu yet.

c58d23e

Multi cpu is not supported by all functions yet. Add an error message and return. Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>

ASoC: pcm: check if cpu DAI support the given stream

918814f

Now multi-cpu-dai is supported. We have the same resaon as multi-codec-dai to skip CPUs which don't support the current stream. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the for-multi-cpu branch from df0bcf1 to 918814f Compare January 20, 2020 15:07

plbossart closed this Feb 20, 2020

Conversation

bardliao commented Dec 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Dec 20, 2019

Uh oh!

bardliao commented Jan 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Jan 15, 2020

Uh oh!

bardliao commented Jan 15, 2020

Uh oh!

dbaluta Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Feb 20, 2020

Uh oh!

dbaluta Jan 17, 2020 •

edited

Loading