alternate multi-dai and multi-config proposal by plbossart · Pull Request #2363 · thesofproject/sof

plbossart · 2020-02-11T00:03:30Z

PR #2258 has been going in circles for a while, here's a tentative alternate proposal to make progress.

The basic principle is that we cannot mix multi-configurations and multi-dai, it's just unmanageable and needlessly complicated.

we can probably make things work by adding a subtype that relies on all existing definitions, and only add what we need for the multi-dai case: a list of subdai indices and a stream map.

having a list of payloads does enable us to deal with multiple configs, but we still need to figure out how firmware would use a specific configuration at a given time.

@slawblauciak would this work for you? Let's talk tomorrow, shall we?

We don't use this field, move back to reserved. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-02-11T18:01:42Z

@slawblauciak would this work for you now?

I am still trying to think of the multi-config case, wondering if we need to embed the hw_params the configuration maps to, so that firmware can choose one config. e.g. for 2ch 48kHz we use config0 and for 1ch 16kHz we use config1. But I am unclear if the DAIs receive hw_params in the first place.

slawblauciak

Seems fine to me

paulstelian97

While I still don't like the multiple flex arrays in the dai_config structure, I don't think there is a way to avoid them. Otherwise looks good.

The multi-dai configuration is only a bundle of elementary DAIs, combined with a stream map to generate the right data for each elementary DAI. When the multi-dai configuration is used, a DAI subtype such as ALH needs to be used, and the payload for each elementary DAIs is associated with a specific subdai_index. For example, with the MULTI_DAI type, subtype ALH, num_payloads 2, the first alh_params would use a subdai_index 0x102 while the second would use 0x202. The stream map ext_id values would be restricted to those two subdai_index. When the multi-dai configuration is not used, the dai_config can be used to provide multiple configurations to an elementary dai. To simplify data handling all configurations are passed in a single IPC, and the firmware only needs to use the config_index field to use the relevant configuration. In this pathset the dai_data is always accessed with a zero index, but in the future the driver may signal a different configuration is required for a specific use case. Update DAI-specific files as needed to avoid breaking compilation and bisect. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-02-12T18:24:07Z

Feb 12 update: remove RFC status, use a flex array of unions, add support for multi-configurations with new index.

ranj063 · 2020-02-12T18:36:21Z


 	ssp->config = *config;
-	ssp->params = config->ssp;
+	ssp->params = config->dai_data[0].ssp;


@plbossart should we change this all to config->dai_data[config->config_index] instead of 0 to make it ready when the config_index would be something other than 0?

i wanted to do this in follow-up patches when we have support for this at the kernel level. For now it's simpler.
And I am not sure that we want to have multiple configurations for all DAI types anyways, so it's rather a case-by-case change, not a blanket one.

look good in that case @plbossart . we've still got some build failures on a couple of platforms

dbaluta · 2020-02-12T19:33:28Z

@@ -688,7 +688,7 @@ static int dai_config(struct comp_dev *dev, struct sof_ipc_dai_config *config)
 	switch (config->type) {


@plbossart the commit message looks very good. I think would be a nice addition to explain the current supported scenario and WHY do you need multi-dai/multi-config. The description must be made in natural language so that can be understood even by the users of SOF.

So, as far as I understand now once a topology is loaded we can support just 1 DAI with 1 configuration. For example, we can support playback over SSP for PCM streams with rate 48KHz, channels 2, format S16_LE.

Is that right?

Now, if we want to play a file with 4 channels for example this is not really possible due to limited topology?

Going forward: With this change would be somehow possible to specify a topology that supports multiple DAIs like for example on i.MX8 -> ESAI and SAI, and also support multiple configurations (like for example we can support playing files at 44.1Khz / 48Khz) dynamically without the need to reload the topology?

Is my understanding correct? If so, this is a great feature!

The multi-dai is really an 'aggregation' feature, so that a stream can be rendered on two separate interfaces. For SoundWire, we have e.g. the left amplifier on one link and the right amplifier on the second. The multi-dai support provides you with the ability to build a 'compound' dai from existing components. From the kernel side you only see one DAI, but the firmware will program and synchronize the individual components. It's an alternative to the 'multi-cpu dai' approach at the kernel level, which could have been a solution as well but might be more complicated to handle at the firmware level.

The multi-configuration is unrelated, it's rather to support different modes of operation. We need this for Bluetooth support, where we can either use 48kHz 2ch 24 bits or 16kHz mono 16 bits. There is no dynamic switch between configs, and it may not be usable by e.g. PulseAudio, but a dedicated Bluetooth app or stack would know what to do.

I'll update the commit messages, for now I am in 'don't break anything' validation mode.

@plbossart I see. I'm very interested about multi-configuration stuff. What do you understand by dynamic switching between configs?

Ideally, I'm wishing this scenario to work:

Run aplay -Dhw:0,0 -f S32_LE -c 2 -r 48000 sample.wav

At some point this will call hw_params() in Linux kernel which in turn will set the correct configuration on FW.

Then:

Run aplay -Dhw:0,0 -f S32_LE -c 4 -r 44100 sample.wav

Similarly, this will call hw_params() in Linux kernel which in turn will set the correct configuration on FW.

AFAIK this use case is not really supported because the topology on FW is created at the same time when the sound card is created on Linux kernel.

that's exactly what @ranj063 and I were talking about earlier, we'd need the kernel to send the config IPC at hw_params time for all DAIs (currently it's only done for HDaudio and ALH). the kernel would select which config_index is desired and the firrmware woud apply configuration1 for 48 kHZ and configuration2 for 44.1kHz.

The point about switching is that you have to close the PCM and shutdown the DAI before you can change configurations. So you can still do run-time changes, but not after the hw_params were selected - if you do you have to go back to square1.

@plbossart Ok, we are on the same page. I only need to switch configuration when PCM is opened and that's all. If i need another configuration I will close the PCM and re-open it with new params.

This will work for us for PCM streams. We will see how things will be going with the compress interface where there I think there is a need to change the params without reopening the interface. But that's another story.

My concern now is about the initial configuration of the pipeline. Because on FW the pipeline is created just only once when the sound card is created on the Linux kernel side. When pipeline is created one needs to know how big the buffers will be, for example.

So, when sound card is created the kernel will look at PCM card capabilities and extract the maximum possible parameters.

If the sound card on the Linux kernel is advertised to support the following config:

rate: 44100, 48000
chanels: 2, 4, 16
rate: S16_LE, S24_LE, S32_LE

Then the kernel needs to figure out that, it needs to support:
maxrate: 48000
maxchan: 16
maxformat: S32_LE

and then create the pipeline on the firmware with exact these parameters.

On the PCM/host side, you can have a lot more variations as long as you have the proper converter for the DAI format. For the DAIs you should really have 3 or 4 configs max.

Note that the pipeline creation on startup is what we do today, at some point we should really need to create the pipeline when it's needed. That would really allow for more optimal uses of the hardware resources.

dbaluta · 2020-02-12T19:51:01Z

@plbossart is there an equivalent PR for Linux kernel. Sorry, I haven't been paying attention to this feature until now :).

plbossart · 2020-02-12T20:01:05Z

@plbossart is there an equivalent PR for Linux kernel. Sorry, I haven't been paying attention to this feature until now :).

thesofproject/linux#1785 but that's just a structure change, no new functionality

plbossart · 2020-02-12T20:03:40Z

@plbossart is there an equivalent PR for Linux kernel. Sorry, I haven't been paying attention to this feature until now :).

thesofproject/linux#1785 but that's just a structure change, no new functionality

this was suggested earlier and @dragosht also had some changes related to this multi-config part:
thesofproject/linux#1029

bardliao · 2020-02-13T05:50:36Z

 	/* reserved for future use */
-	uint32_t reserved[8];
+	uint32_t reserved16;
+	uint32_t reserved[7];


Should uint32_t reserved16; be uint16_t reserved16;?

ranj063 · 2020-02-13T06:26:55Z

@plbossart I just realized that we'd have not 2 but 3 flex arrays in the dai_config since the dmic_params already has a flex array for the pdm controller configs. It seems more and more like a bad idea to me to have such a definition.

As an alternative, can we consider a flex array of dai configs per dai instead of a flex array params per dai config?

plbossart · 2020-02-13T17:22:29Z

@plbossart I just realized that we'd have not 2 but 3 flex arrays in the dai_config since the dmic_params already has a flex array for the pdm controller configs. It seems more and more like a bad idea to me to have such a definition.

As an alternative, can we consider a flex array of dai configs per dai instead of a flex array params per dai config?

Yes, I realized that adding the stream_map after the flex array is complete nonsense. There's no way to use any compiler-generated offset, the code would need to manually go past the variable array and do a cast. It's really horrible.

@slawblauciak is this too late to change the definition of the stream map and somehow add it to the dai config?

ranj063 · 2020-02-13T17:25:06Z

@plbossart I just realized that we'd have not 2 but 3 flex arrays in the dai_config since the dmic_params already has a flex array for the pdm controller configs. It seems more and more like a bad idea to me to have such a definition.
As an alternative, can we consider a flex array of dai configs per dai instead of a flex array params per dai config?

Yes, I realized that adding the stream_map after the flex array is complete nonsense. There's no way to use any compiler-generated offset, the code would need to manually go past the variable array and do a cast. It's really horrible.

@slawblauciak is this too late to change the definition of the stream map and somehow add it to the dai config?

@plbossart in general it is a bad idea to have a struct with a flex array as a member of another structure. We've already made the mistake of adding the dmic_params as a member of dai_config in the first place. Do you think it makes sense to go back and fix that first before we make any more changes to dai config?

plbossart · 2020-02-13T17:31:57Z

@plbossart I just realized that we'd have not 2 but 3 flex arrays in the dai_config since the dmic_params already has a flex array for the pdm controller configs. It seems more and more like a bad idea to me to have such a definition.
As an alternative, can we consider a flex array of dai configs per dai instead of a flex array params per dai config?

Yes, I realized that adding the stream_map after the flex array is complete nonsense. There's no way to use any compiler-generated offset, the code would need to manually go past the variable array and do a cast. It's really horrible.
@slawblauciak is this too late to change the definition of the stream map and somehow add it to the dai config?

@plbossart in general it is a bad idea to have a flex array as a member of another structure. We've already made the mistake of adding the dmic_params as a member of dai_config in the first place. Do you think it makes sense to go back and fix that first before we make any more changes to dai config?

I realized this as well, there's currently no way we can support multiple DMIC configurations indeed.
That would be a compatibility break though and more tests at the kernel level to deal with older firmware/topology. I am still leaning on cleaning this up before it's really too late.

ranj063 · 2020-02-13T17:35:59Z

I realized this as well, there's currently no way we can support multiple DMIC configurations indeed.
That would be a compatibility break though and more tests at the kernel level to deal with older firmware/topology. I am still leaning on cleaning this up before it's really too late.

@plbossart we're kind of stuck with this problem because we've been consciously trying to avoid pointers in the IPC structs. What is the downside of having dynamic arrays in these structs?

juimonen · 2020-02-13T17:58:20Z

@plbossart @ranj063 as a side note, demux/mux will have exactly same problem with nested flex arrays. Easy to parse from topology, but horrible to copy to ipc...

ranj063 · 2020-02-13T18:02:43Z

@plbossart @ranj063 as a side note, demux/mux will have exactly same problem with nested flex arrays. Easy to parse from topology, but horrible to copy to ipc...

@juimonen please lets not make the mistake with nested struct members with flex arrays. We have to find a more suitable alternative.

plbossart · 2020-02-13T18:52:47Z

@plbossart @ranj063 as a side note, demux/mux will have exactly same problem with nested flex arrays. Easy to parse from topology, but horrible to copy to ipc...

@juimonen please lets not make the mistake with nested struct members with flex arrays. We have to find a more suitable alternative.

agree, we have PDM, dai configs, demux configs, stream configs... Let's do this right!

bardliao · 2020-02-14T06:36:15Z

@plbossart @slawblauciak I have an idea. Can we send normal ALH dai configs followed by a MULTIDAI type dai config when there is a multi dai created? And the MULTIDAI config contains stream map and subdai's type and index only.
e.g. A multi dai which connects to ALH0x102 and ALH0x202 dais. We can send two ipcs for the two ALH dais, and a ipc for the multi dai.
So we don't need to use flex arrays to store all subdai's information. We can create struct sof_ipc_dai_multi_params{} which include stream_map, dai_type and dai_index[]. where dai_index can be fixed size. Basically, sof_ipc_dai_multi_params{} is only used to identify which dais will be enabled at the same time.
What do you think?

slawblauciak · 2020-02-14T07:37:20Z

@bardliao that was my very first proposal but @plbossart mentioned it would be too hard to implement on the kernel side. I think it was something to do with DAI power management, I don't recall exactly.

slawblauciak · 2020-02-14T07:43:20Z

@plbossart I believe there are other parts of the code (mux/demux) that use the stream_map. Regardless, they use the serialization API for the stream map, so if we do change it, it should be a smooth fix.

Anyway, I don't quite follow what you mean by adding the stream map to the DAI config. And what exactly do you have in mind in terms of changing stream map's definition?

bardliao · 2020-02-14T09:00:33Z

@bardliao that was my very first proposal but @plbossart mentioned it would be too hard to implement on the kernel side. I think it was something to do with DAI power management, I don't recall exactly.

I could miss something but I didn't see it will be hard to implement on the kernel side. On kernel side, we send ALH dai config ipc on topology loading and passing stream id. However, I think we only need to send a multi dai config ipc on topology loading. FW can create the connection at that moment. We just need to send a normal ALH dai config ipc when passing stream id. @plbossart Please correct me if I am wrong.

plbossart · 2020-02-14T16:18:16Z

@bardliao that was my very first proposal but @plbossart mentioned it would be too hard to implement on the kernel side. I think it was something to do with DAI power management, I don't recall exactly.

I could miss something but I didn't see it will be hard to implement on the kernel side. On kernel side, we send ALH dai config ipc on topology loading and passing stream id. However, I think we only need to send a multi dai config ipc on topology loading. FW can create the connection at that moment. We just need to send a normal ALH dai config ipc when passing stream id. @plbossart Please correct me if I am wrong.

@bardliao we pass the configuration for ALH at hw_params time, not topology load.

My problem is that a dai-link needs to be mapped with a CPU dai. So that CPU dai is a multi-dai one, so there's really nothing that will allow me to deal with individual dais at the kernel level. I don't know how to represent a multi-dai made of subdais, and all these runtime/stream objects. The only things we can do is represent an abstract multi-dai and pass a configuration.

Either we deal with the 'multi-cpu' solution at the kernel level, and we don't need the notion of multi-dai, or we use the 'multi-dai' solution and firmware takes care of everything. The mixed version seems like asking for trouble.

Quite frankly for us the kernel multi-cpu dai is way simpler overall. I know @slawblauciak mentioned we might have synchronization issues but honestly at this point it looks more and more like the path of least resistance.

bardliao · 2020-02-17T10:20:11Z

@bardliao we pass the configuration for ALH at hw_params time, not topology load.

Yeah we pass stream id at hw_params time but we also send dai configuration ipc on sof_link_alh_load()
https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/topology.c#L3087

My point is that we can create sof_link_multi_load() and send a multi dai configuration ipc to tell FW that which dais will be tied together.

My problem is that a dai-link needs to be mapped with a CPU dai. So that CPU dai is a multi-dai one, so there's really nothing that will allow me to deal with individual dais at the kernel level. I don't know how to represent a multi-dai made of subdais, and all these runtime/stream objects. The only things we can do is represent an abstract multi-dai and pass a configuration.

Actually, I never think CPU dai will map to multi-dai before.
My idea is always based on the multi-cpu solution. i.e. A dai link with two CPU dais which map to physical dais.
In my opinion, the role of multi dai is to connect the stream and the physical dai.
For example, currently, we connect ALH2.OUT to SDW0-Playback. and ALH0x102 to SDW1-Playback.
We can connect MULTI0.OUT to SDW0-Playback and ALH2.OUT and ALH0x102.OUT to MULTI0.OUT.
However, the CPU dais are always map to physical dais no matter what dai is connected to SDW0-Playback.
Obviously, we will need topology change to support connecting ALH2.OUT to MULTI0.OUT.

Either we deal with the 'multi-cpu' solution at the kernel level, and we don't need the notion of multi-dai, or we use the 'multi-dai' solution and firmware takes care of everything. The mixed version seems like asking for trouble.

It would be the best if we can deal with the 'multi-cpu' solution only. But my question is how do we enable the ALH dais on FW. Currently, FE dai and BE dai is 1:1 mapping. I think multi-dai can make it to 1:N mapping and that's what we need. To me the only purpose of multi dai is to create an virtual dai which can be connected to the dai link and nothing else. So it is also good to me if we can handle it on kernel side without multi dai solution.

Quite frankly for us the kernel multi-cpu dai is way simpler overall. I know @slawblauciak mentioned we might have synchronization issues but honestly at this point it looks more and more like the path of least resistance.

Totally agree.

bardliao · 2020-02-19T07:10:51Z

@plbossart @slawblauciak I created a PR to explain my idea thesofproject/linux#1804 and #2406. Could you take a look?

plbossart · 2020-02-20T21:08:02Z

Abandonned for now. First patch pushed as #2417, the rest will be re-opened when we've removed variable length arrays for DMIC thesofproject/linux#1801

plbossart requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and slawblauciak February 11, 2020 00:03

plbossart requested a review from a team as a code owner February 11, 2020 00:03

plbossart force-pushed the fix/dai-ipc branch from f17354e to 23c95b6 Compare February 11, 2020 00:06

paulstelian97 reviewed Feb 11, 2020

View reviewed changes

Comment thread src/include/ipc/dai-imx.h Outdated

slawblauciak approved these changes Feb 11, 2020

View reviewed changes

Comment thread src/include/ipc/dai-imx.h Outdated

paulstelian97 reviewed Feb 11, 2020

View reviewed changes

Comment thread src/include/ipc/dai.h Outdated

lgirdwood requested changes Feb 11, 2020

View reviewed changes

Comment thread src/include/ipc/dai-imx.h Outdated

Comment thread src/include/ipc/dai.h Outdated

ipc: dai: remove unused hdr field in params

735ad88

We don't use this field, move back to reserved. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart force-pushed the fix/dai-ipc branch from 23c95b6 to 11bf7b8 Compare February 11, 2020 17:58

plbossart requested review from lgirdwood, paulstelian97 and slawblauciak February 11, 2020 17:59

ranj063 reviewed Feb 12, 2020

View reviewed changes

Comment thread src/include/ipc/dai.h Outdated

ranj063 reviewed Feb 12, 2020

View reviewed changes

Comment thread src/include/ipc/dai.h Outdated

slawblauciak approved these changes Feb 12, 2020

View reviewed changes

paulstelian97 reviewed Feb 12, 2020

View reviewed changes

Comment thread src/include/ipc/dai.h Outdated

paulstelian97 approved these changes Feb 12, 2020

View reviewed changes

lgirdwood approved these changes Feb 12, 2020

View reviewed changes

plbossart force-pushed the fix/dai-ipc branch from 11bf7b8 to 027d0c1 Compare February 12, 2020 17:24

plbossart requested review from jajanusz, singalsu and tlauda as code owners February 12, 2020 17:24

plbossart force-pushed the fix/dai-ipc branch from 027d0c1 to c360ffa Compare February 12, 2020 18:22

plbossart changed the title ~~[RFC] alternate multi-dai proposal~~ alternate multi-dai and multi-config proposal Feb 12, 2020

plbossart requested review from bardliao, lgirdwood, paulstelian97, ranj063 and slawblauciak February 12, 2020 18:24

ranj063 reviewed Feb 12, 2020

View reviewed changes

plbossart mentioned this pull request Feb 12, 2020

ASoC: SOF: add support for multi-dai and multi-configs per dai thesofproject/linux#1785

Closed

dbaluta reviewed Feb 12, 2020

View reviewed changes

bardliao reviewed Feb 13, 2020

View reviewed changes

plbossart closed this Feb 20, 2020

		@@ -688,7 +688,7 @@ static int dai_config(struct comp_dev dev, struct sof_ipc_dai_config config)
		switch (config->type) {

Conversation

plbossart commented Feb 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart commented Feb 11, 2020

Uh oh!

Uh oh!

Uh oh!

slawblauciak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulstelian97 left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented Feb 12, 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!

dbaluta Feb 12, 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!

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!

dbaluta Feb 12, 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!

Choose a reason for hiding this comment

Uh oh!

dbaluta commented Feb 12, 2020

Uh oh!

plbossart commented Feb 12, 2020

Uh oh!

plbossart commented Feb 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Feb 13, 2020

Uh oh!

plbossart commented Feb 13, 2020

Uh oh!

ranj063 commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Feb 13, 2020

Uh oh!

ranj063 commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juimonen commented Feb 13, 2020

Uh oh!

ranj063 commented Feb 13, 2020

Uh oh!

plbossart commented Feb 13, 2020

Uh oh!

bardliao commented Feb 14, 2020

Uh oh!

slawblauciak commented Feb 14, 2020

Uh oh!

slawblauciak commented Feb 14, 2020

Uh oh!

bardliao commented Feb 14, 2020

Uh oh!

plbossart commented Feb 14, 2020

Uh oh!

bardliao commented Feb 17, 2020

dbaluta Feb 12, 2020 •

edited

Loading

dbaluta Feb 12, 2020 •

edited

Loading

ranj063 commented Feb 13, 2020 •

edited

Loading

ranj063 commented Feb 13, 2020 •

edited

Loading