Sdw/multi cpu dai support #1802

plbossart · 2020-02-18T23:43:50Z

This is an updated version of @bardliao 's v4 series, on top of @morimoto latest cleanup.

Please review the last 6 patches and check if anything is missing

RanderWang

resaon or reason in commit message ? multi-codec DAI ? I grep "git log sound" and found DAI is more popular than dai.

update : I just referred to f7c909a

plbossart · 2020-02-20T01:51:04Z

@bardliao @RanderWang @slawblauciak this is an untested compile-only proof of concept to show how we can deal with multiple links at the kernel level without the need for the 'multi-dai' concept inside the firmware.

@RanderWang I need your help to complete the work and actually add the .ops in your machine driver, as well as create a dailink with 2 cpu dais.

@bardliao please review if I am missing any additional steps, I only took one patch from your test branch.

Thanks!

sound/soc/intel/boards/sdw_rt711_rt1308_rt715.c

RanderWang · 2020-02-20T12:26:14Z

@bardliao @RanderWang @slawblauciak this is an untested compile-only proof of concept to show how we can deal with multiple links at the kernel level without the need for the 'multi-dai' concept inside the firmware.

@RanderWang I need your help to complete the work and actually add the .ops in your machine driver, as well as create a dailink with 2 cpu dais.

@bardliao please review if I am missing any additional steps, I only took one patch from your test branch.

Thanks!

I implemented a prototype, tomorrow I will create a PR

[    3.131981]  sdw_rt711_rt1308_rt715: rt711-aif1 <-> SDW0 Pin3 mapping ok
[    3.131983]  sdw_rt711_rt1308_rt715: ASoC: registered pcm #17 (SDW0-Capture)
[    3.131983]  sdw_rt711_rt1308_rt715: rt711-aif1 <-> SDW0 Pin3 mapping ok
[    3.131988]  sdw_rt711_rt1308_rt715: ASoC: registered pcm #18 (SDW1-Playback)
[    3.131989]  sdw_rt711_rt1308_rt715: multicodec <-> multicpu mapping ok
[    3.131991]  sdw_rt711_rt1308_rt715: ASoC: registered pcm #19 (SDW3-Capture)
[    3.131991]  sdw_rt711_rt1308_rt715: rt715-aif2 <-> SDW3 Pin2 mapping ok

```

The use of parentheses to protect the argument is fine for (i)++ but not for (i--). Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

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> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@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> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@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> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@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> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@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> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Now multi-cpu-dais are supported, we can skip cpi-dais which don't support the current stream, following the example of multi-codec-dais. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Extend code from single cpu-dai to multi-dai Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

These prototypes are no longer used, remove. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

In a multi-cpu DAI context, the stream routines may be called from multiple DAI callbacks. Make sure the stream state only changes for the first call, and don't return error messages if the target state is already reached. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

We only have a set() operation, provide the dual get() operation to retrieve the stream information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

This is needed to retrieve the information when the stream is allocated at the dai_link level. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

To handle streams at the dailink level, expose two helpers that will be called from machine drivers. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

To support streaming across multiple links, the stream allocation/free needs to be at the dailink level, not the dai. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The current memory allocation is somewhat strange: the dma_data is allocated in set_sdw_stream, but released in the intel DAI shutdown. This no longer works with the multi-cpu implementation, since the dma_data is released in the dai shutdown which takes place before the dailink shutdown. Move to a more symmetric allocation where the dma_data is allocated with non-NULL SoundWire stream, and conversely released when a NULL stream is provided - for consistency with the stream startup and shutdown operations. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Now that the DMA data is allocated/freed in set_sdw_stream(), remove free operations. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Use .startup and .shutdown callback to allocate/free stream Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

… in dailink Use .startup and .shutdown callback to allocate/free stream Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-02-20T18:32:29Z

@bardliao @RanderWang This version seems to work on my Olympic device, with only one CPU DAI per DAI link. I think we need to see Rander's prototype to see how we add 2 CPU dais.

I have however a regression on HDMI, somehow I see an IPC error, not sure if this comes from topic/sof-dev or my own changes.

plbossart · 2020-02-20T18:51:22Z

HDMI regression confirmed, bisected to this commit

4097910d5bc262a073026aafc48961acec191914 is the first bad commit
commit 4097910d5bc262a073026aafc48961acec191914
Author: Bard Liao <yung-chuan.liao@linux.intel.com>
Date:   Mon Jan 20 21:29:28 2020 +0800

    ASoC: pcm: check if cpu-dai supports a given stream
    
    Now multi-cpu-dais are supported, we can skip cpi-dais which don't
    support the current stream, following the example of multi-codec-dais.
    
    Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
    Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Not sure if I did a bad merge or if something's missing. Gah.

This reverts commit 4097910. We will remove this revert when we figure out how to setup the channels_min, discussion at thesofproject#1812 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

bardliao · 2020-02-21T05:30:23Z

HDMI regression confirmed, bisected to this commit

4097910d5bc262a073026aafc48961acec191914 is the first bad commit
commit 4097910d5bc262a073026aafc48961acec191914
Author: Bard Liao <yung-chuan.liao@linux.intel.com>
Date:   Mon Jan 20 21:29:28 2020 +0800

    ASoC: pcm: check if cpu-dai supports a given stream
    
    Now multi-cpu-dais are supported, we can skip cpi-dais which don't
    support the current stream, following the example of multi-codec-dais.
    
    Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
    Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Not sure if I did a bad merge or if something's missing. Gah.

The issue is that we didn't set stream capability in hda-dai.c. 4097910 is just to make the issue visible. :)

lyakh · 2020-02-21T09:16:36Z

sound/soc/soc-pcm.c

+				break;
+			}
+			cpu_bits = max(cpu_dai->driver->playback.sig_bits,
+				       cpu_bits);


I don't know the physical meaning of this, but logically this seems a bit strange. Is this really correct? If one of .sig_bits is 0, we set cpu_bits = 0 and stop iterating. Otherwise we set cpu_bits to the maximum of the old cpu_bits value and .sig_bits. Not minimum?

it's the same logic as on line 370..405

lyakh · 2020-02-21T09:20:29Z

sound/soc/soc-pcm.c

+		cpu_chan_min = max(cpu_chan_min, cpu_stream->channels_min);
+		cpu_chan_max = min(cpu_chan_max, cpu_stream->channels_max);
+		cpu_rate_min = max(cpu_rate_min, cpu_stream->rate_min);
+		cpu_rate_max = min_not_zero(cpu_rate_max, cpu_stream->rate_max);


interesting logic...

lyakh · 2020-02-21T09:44:45Z

sound/soc/soc-pcm.c

+			 * widgets are in the active list.
+			 */
+			if (widget && widget_in_list(list, widget))
+				do_prune = 0;


you can break out of the loop here too

same logic as on line 1463

lyakh · 2020-02-21T09:53:21Z

sound/soc/soc-dapm.c

 	struct snd_soc_dapm_widget *playback = NULL, *capture = NULL;
 	struct snd_soc_dapm_widget *codec, *playback_cpu, *capture_cpu;
 	struct snd_pcm_substream *substream;
 	struct snd_pcm_str *streams = rtd->pcm->streams;
-	int i;


oh... those 3 lines of code below:

if (rtd->dai_link->params) { playback_cpu = cpu_dai->capture_widget; capture_cpu = cpu_dai->playback_widget;

is it only me or could they indeed benefit from a little comment? Just one line maybe?..

not our code... This was added for codec2codec links, no real idea what it means.

lyakh · 2020-02-21T10:05:09Z

sound/soc/soc-generic-dmaengine-pcm.c

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


this function returns a pointer. Should it have been ERR_PTR(-EINVAL) or NULL?

yes, fixed in update (reported independently by kbuild)

lyakh · 2020-02-21T10:15:14Z

drivers/soundwire/stream.c

@@ -1859,3 +1862,102 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
 	return ret;
 }
 EXPORT_SYMBOL(sdw_deprepare_stream);
+
+static int set_stream(struct snd_pcm_substream *substream,


sdw_set_stream?

static -> no prefix
And sdw_set_stream is already used for something else

RanderWang · 2020-02-21T11:21:27Z

@plbossart I created a PR at plbossart#15

confusion The patch 'ASoC: Return error if the function does not support multi-cpu' generates a warning sound/soc/soc-generic-dmaengine-pcm.c: In function 'dmaengine_pcm_compat_request_channel': >> sound/soc/soc-generic-dmaengine-pcm.c:203:10: warning: returning 'int' from a function with return type 'struct dma_chan *' makes pointer from integer without a cast [-Wint-conversion] 203 | return -EINVAL; | ^ Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-02-21T23:02:22Z

I am going to merge that series since it's really needed for validation. I expect some updates and fixes though.

plbossart requested review from morimoto, ranj063, bardliao and RanderWang February 18, 2020 23:43

plbossart requested a review from lgirdwood as a code owner February 18, 2020 23:43

plbossart mentioned this pull request Feb 18, 2020

Add multi-cpu dai support #1803

Closed

RanderWang reviewed Feb 19, 2020

View reviewed changes

bardliao mentioned this pull request Feb 19, 2020

[RFC] Topology change for multi dai #1804

Closed

plbossart mentioned this pull request Feb 19, 2020

Merge/sound upstream 20200219 #1805

Merged

plbossart force-pushed the sdw/multi-cpu-dai branch from f7c909a to b455dff Compare February 20, 2020 01:48

plbossart requested review from juimonen and singalsu as code owners February 20, 2020 01:48

plbossart requested a review from RanderWang February 20, 2020 01:51

bardliao reviewed Feb 20, 2020

View reviewed changes

sound/soc/intel/boards/sdw_rt711_rt1308_rt715.c Show resolved Hide resolved

plbossart and others added 14 commits February 20, 2020 11:47

ASoC: fix for_rtd_codec_dai_rollback() macro

9a09de4

The use of parentheses to protect the argument is fine for (i)++ but not for (i--). Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: topology: connect dai widget to all cpu-dais

0d5413a

Extend code from single cpu-dai to multi-dai Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: cadence: remove useless prototypes

c2e38f1

These prototypes are no longer used, remove. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: soc-dai: add get_sdw_stream() callback

b27f0d8

We only have a set() operation, provide the dual get() operation to retrieve the stream information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: intel: implement get_sdw_stream() operations

eeef703

This is needed to retrieve the information when the stream is allocated at the dai_link level. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: stream: add helper to startup/shutdown streams

216ebe4

To handle streams at the dailink level, expose two helpers that will be called from machine drivers. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: intel: remove stream allocation/free

f8914ba

To support streaming across multiple links, the stream allocation/free needs to be at the dailink level, not the dai. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart added 4 commits February 20, 2020 12:04

soundwire: intel: don't free dma_data in DAI shutdown

47b4921

Now that the DMA data is allocated/freed in set_sdw_stream(), remove free operations. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: Intel: sdw_rt700: handle SoundWire stream creation in dailink

4476a47

Use .startup and .shutdown callback to allocate/free stream Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: Intel: sdw_rt711_rt1308_rt715: handle SoundWire stream creation…

fced860

… in dailink Use .startup and .shutdown callback to allocate/free stream Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart force-pushed the sdw/multi-cpu-dai branch from b455dff to fced860 Compare February 20, 2020 18:30

plbossart changed the title ~~[RFC] Sdw/multi cpu dai~~ Sdw/multi cpu dai support Feb 20, 2020

plbossart mentioned this pull request Feb 20, 2020

[RFC] ASoC: Add Multi CPU DAI support #1644

Closed

Revert "ASoC: pcm: check if cpu-dai supports a given stream"

352771c

This reverts commit 4097910. We will remove this revert when we figure out how to setup the channels_min, discussion at thesofproject#1812 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

lyakh suggested changes Feb 21, 2020

View reviewed changes

plbossart mentioned this pull request Feb 21, 2020

ASoC: SOF: hda-dai: add stream capability #1817

Merged

plbossart requested review from dbaluta and kv2019i as code owners February 21, 2020 22:53

plbossart merged commit 1d2ad93 into thesofproject:topic/sof-dev Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sdw/multi cpu dai support #1802

Sdw/multi cpu dai support #1802

plbossart commented Feb 18, 2020 •

edited

Loading

RanderWang left a comment •

edited

Loading

plbossart commented Feb 20, 2020

RanderWang commented Feb 20, 2020

plbossart commented Feb 20, 2020

plbossart commented Feb 20, 2020

bardliao commented Feb 21, 2020

lyakh Feb 21, 2020

plbossart Feb 21, 2020

lyakh Feb 21, 2020

lyakh Feb 21, 2020

plbossart Feb 21, 2020

lyakh Feb 21, 2020

plbossart Feb 21, 2020

lyakh Feb 21, 2020

plbossart Feb 21, 2020

lyakh Feb 21, 2020

plbossart Feb 21, 2020

RanderWang commented Feb 21, 2020

plbossart commented Feb 21, 2020

Sdw/multi cpu dai support #1802

Sdw/multi cpu dai support #1802

Conversation

plbossart commented Feb 18, 2020 • edited Loading

RanderWang left a comment • edited Loading

Choose a reason for hiding this comment

plbossart commented Feb 20, 2020

RanderWang commented Feb 20, 2020

plbossart commented Feb 20, 2020

plbossart commented Feb 20, 2020

bardliao commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented Feb 21, 2020

plbossart commented Feb 21, 2020

plbossart commented Feb 18, 2020 •

edited

Loading

RanderWang left a comment •

edited

Loading