ASoC: Intel: sof_sdw: Divide channels by num_cpus in capture

[bardliao]

The captured data will be combined from each cpu dais if the dai link has more than one cpu dais. Therefore, the channel number should divide by num_cpus for each cpu/codec dai.

Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

[bardliao]

@plbossart This PR is to fix the distortion issue of aggregated feedback. The issue is that when we do 4 channels capture, the 4 channels format will be applied to each cpu/codec dai. In other words, we will have 4 channels data on the 1st sdw link AND the 2nd sdw link.
This PR modifies the channel format in machine driver and will be pass to cpu and codec drivers. The FE format will not be impacted.

[plbossart]

Humm, yes that makes sense @bardliao

But I wonder if this isn't an ASoC framework issue really. maybe the dailink definition should have something like a flag or a mask that says how the channels are handled, which btw might be a way to solve the M:N mapping between cpu and codec dais that isn't allowed today.

[ranj063]

nit-pick: "each CPU DAI" not "dais"

[bardliao]

Humm, yes that makes sense @bardliao

But I wonder if this isn't an ASoC framework issue really. maybe the dailink definition should have something like a flag or a mask that says how the channels are handled, which btw might be a way to solve the M:N mapping between cpu and codec dais that isn't allowed today.

hmm, adding a flag to inform ASoC framework may be a good idea. Let me see if I can find a good way to handle it.

[bardliao]

Humm, yes that makes sense @bardliao
But I wonder if this isn't an ASoC framework issue really. maybe the dailink definition should have something like a flag or a mask that says how the channels are handled, which btw might be a way to solve the M:N mapping between cpu and codec dais that isn't allowed today.

hmm, adding a flag to inform ASoC framework may be a good idea. Let me see if I can find a good way to handle it.

@plbossart Please see #4136

[ujfalusi]

@bardliao, @plbossart, this is confusing and I'm not sure how to interpret the setup.
Why we cannot use tdm_mask to describe the channels each DAI is handling on the link? I have not seen a clear (or at all) description of a multi-cpu - multi-codec dai_link, what is the "default" mapping of things?
Are everything runs on the same number of channels? Does one of the CPU dai's channels are first, followed by the second's and so forth?
Let say for simplicity (and I think we have this as a case as well):
2 CPU dai
2 CODEC dai
one link. The link is 4 channel.

Out of box when you play 4 channel, what does that means?
each CPU dai sends four channels, so on bus they are 4|4=4?
how this can even work?
each sends 2 channels and they are in different slots, so on bus they are 2+2=4 ?
I understand that the codecs have kcontrol to select which tdm_slot they should listen to pick the samples, so the codecs can see all the slots and it is user space decision to pick which one to grab? Do they (need to) support four channel mode, or 2 channel is what they can handle?

On capture of 4 channel I see similar issue just the reverse.

I think in ASoC the single CPU dai - multi-codec dai is well defined, tested and it is using tdm_mask to specify the number of channels and the place of those channels for each codec on the link, so the CPU send 4 channel and each CPU 'receives' two, but takes it from different slots. Same for capture, codecs sending 2 channels, placing them to specific slots and the CPU receives these combined channels.

I have not used the multi-cpu - single codec type, but I think in that case ASoC would treat it that all CPU dai receives the same number of channels as a whole and I don't think there is a tdm_mask for the CPU side to be able to extract portions, like the other setup.

I might over-simplify this, but probably a tdm_mask support on the CPU side is what we are missing to be able to describe the issue correctly in the core and handle it?
I don't recall if tdm_mask can be used or adapted to SDW, so that is really a question for you, but adding flags like divide the number of channels by number of cpus is not really generic since you could have a setup where one CPU should be using 4 channel, the other 2 on the same link with 8 channels. How would that be flagged out?

[bardliao]

@bardliao, @plbossart, this is confusing and I'm not sure how to interpret the setup. Why we cannot use tdm_mask to describe the channels each DAI is handling on the link? I have not seen a clear (or at all) description of a multi-cpu - multi-codec dai_link, what is the "default" mapping of things? Are everything runs on the same number of channels? Does one of the CPU dai's channels are first, followed by the second's and so forth? Let say for simplicity (and I think we have this as a case as well): 2 CPU dai 2 CODEC dai one link. The link is 4 channel.

Out of box when you play 4 channel, what does that means? each CPU dai sends four channels, so on bus they are 4|4=4? how this can even work? each sends 2 channels and they are in different slots, so on bus they are 2+2=4 ? I understand that the codecs have kcontrol to select which tdm_slot they should listen to pick the samples, so the codecs can see all the slots and it is user space decision to pick which one to grab? Do they (need to) support four channel mode, or 2 channel is what they can handle?

For playback, we send identical data to each cpu/codec dai pair. So if we play 4 channels, they are 4 channels for each codec.
The codecs can see all the slots and it is user space decision to pick which one to grab. They need to support four channel mode. But the common use case is to play 2 channels, not 4 channels. The existing topology supports 2 channel playback only.

On capture of 4 channel I see similar issue just the reverse.

I think in ASoC the single CPU dai - multi-codec dai is well defined, tested and it is using tdm_mask to specify the number of channels and the place of those channels for each codec on the link, so the CPU send 4 channel and each CPU 'receives' two, but takes it from different slots. Same for capture, codecs sending 2 channels, placing them to specific slots and the CPU receives these combined channels.

I have not used the multi-cpu - single codec type, but I think in that case ASoC would treat it that all CPU dai receives the same number of channels as a whole and I don't think there is a tdm_mask for the CPU side to be able to extract portions, like the other setup.

I might over-simplify this, but probably a tdm_mask support on the CPU side is what we are missing to be able to describe the issue correctly in the core and handle it? I don't recall if tdm_mask can be used or adapted to SDW, so that is really a question for you, but adding flags like divide the number of channels by number of cpus is not really generic since you could have a setup where one CPU should be using 4 channel, the other 2 on the same link with 8 channels. How would that be flagged out?

That would be an issue indeed.

[plbossart]

@ujfalusi the notion of TDM mask is based on the notion of the same type of data multiplexed. There's nothing that tells us that the mask is related to a channel, just that the slot is used. Also in SoundWire the channels for different streams may have different depth, e.g. the capture channel may use 24 buts but the IV feedback 12.

Think of SoundWire as an extension of TDM where the limitation to have the same data type is removed. Also you don't need to have the same number of slots on TX and RX.

So basically I don't think we can use TDM notions here.

[ujfalusi]

@plbossart, playback and capture tdm_slot is separate, they don't need to be identical.
The data inside can in theory different. The slot width will tell the max size (the steps hetween slots) and it is possible that one codec sends 16bit inside it's slots while the other sends 24bits, with slot width of 32 for example.
For me a definition of some part of.the stream is going here, the other is there sounds too open to interpretation and for sure going to put into creative use...

I'm coming from i2s world, yes, but I think the very same issue present there with just a different terminology.

[plbossart]

The data inside can in theory different. The slot width will tell the max size (the steps hetween slots) and it is possible that one codec sends 16bit inside it's slots while the other sends 24bits, with slot width of 32 for example.

not really, there is no way for the 2 parties to agree on what the contents of each slot might be.

[ujfalusi]

not really, there is no way for the 2 parties to agree on what the contents of each slot might be.

you have 2 CPU dais and 2 codecs. They all share the same signal lines, one of them is the clock provider (or it is something which always runs, does not matter).
sub_link1: cpu1: listens on slot0/1 - codec1: sends data on slot0/1
sub_link2: cpu2: listens on slot2/3 - codec2: sends data on slot2/3
The TDM is configured as 32bit words, 4 channel.
sub_link1 works with 16bit samples
sub_link2 works with 24bit samples
This is valid setup from signal and hardware pow, it is a different question that we don't have support for this, but that is just work needed to be done if someone needs it.

Sure, in I2S the components cannot 'agree', they are told to use this and that format (by the link description, machine driver).
Sure #2, SDW is more than audio data, I know, it is in a different level of complexity, all I'm saying is that the concept of what this is trying to solve can be abstracted away to be a generic issue and solved in a more transparent way than solve this very specific hardware configuration that we have on front of us.

Side note: there are I2S codecs 'abusing' the TDM bus in a funky way that they reserve a slot or two for commands in the audio stream, some goes as far is to allow firmware download via TDM... much faster than i2c would be, or allow IQ param updates sent alongside the audio stream.

[ujfalusi]

The idea of let's mix audio and commands in a TDM stream gone away pretty quickly, never got upstream support.

[plbossart]

@ujfalusi when we use multiple cpu-dais for a SoundWire stream, it's because the codecs are connected on separate links - which do not share clock or data lines. Example: two separate amplifiers on two different links

[bardliao]

@ujfalusi when we use multiple cpu-dais for a SoundWire stream, it's because the codecs are connected on separate links - which do not share clock or data lines. Example: two separate amplifiers on two different links

@ujfalusi To be more specific, a clock line per cpu dai. So, for the 2 cpu/codec dais case, the cpu/codec will use all slots on the link.
sub_link1 and sub_link2 are separated physically.

[bardliao]

@ujfalusi I seriously considered your's proposal to use snd_soc_dai_set_tdm_slot, but the problem is that we don't know the channel mapping until sof_ipc4_prepare_copier_module is called. What we really need is to update the params_channels(params) in sdw cpu dai .hw_params ops. sof_ipc4_prepare_copier_module is triggered by sof_pcm_hw_params
snd_soc_pcm_component_hw_params -> sof_pcm_hw_params -> ... -> sof_ipc4_prepare_copier_module
And snd_soc_pcm_component_hw_params will be called AFTER cpu dai hw_params. See
__soc_pcm_hw_params
Actually, we should upgrade params_channels(params) for codec dais, too although they are not used by codec drivers for now.

Given that we don't know the channel mapping when we call snd_soc_dai_set_tdm_slot, I don't see any benefit of using snd_soc_dai_set_tdm_slot. If we don't find a suitable flag in ASoC to indicate we need to split channels, setting it in machine driver probably be the best way to do so.
In fact, I am thinking maybe we should do it in ASoC unconditionally when a BE link has more than one cpu dais. What do you think? @plbossart

[plbossart]

set_tdm_slot() is not a good match for SoundWire capabilities. We should not even try to use it, it's be an horrible overload of both concept and code.

[bardliao]

set_tdm_slot() is not a good match for SoundWire capabilities. We should not even try to use it, it's be an horrible overload of both concept and code.

@plbossart How about current solution? Or #4136?

[plbossart]

@bardliao I think we should solve multiple problems at the same time:
a) deal with the mapping of N cpu-DAIs to M codec-DAIs, which must involve some sort of channel map information set at the machine driver level
b) as a bonus, use the same infrastructure to figure how channels are split even in the N:N case. we're making the assumption that on playback all channels are duplicated, but it would be nice for testing purposes to have the ability to split channels as well. And for capture we also need the ability to combine channels, possibly in a symmetrical way but it would also be nice to have the ability to reorder, e.g. L1 L2 R1 R2 instead of L1 R1 L2 R2 if we have two stereo capture devices.

The mapping at the dailink level should be implemented in ASoC soc-pcm.c, and the configuration in sof_sdw.c IMHO.

[bardliao]

@bardliao I think we should solve multiple problems at the same time: a) deal with the mapping of N cpu-DAIs to M codec-DAIs, which must involve some sort of channel map information set at the machine driver level b) as a bonus, use the same infrastructure to figure how channels are split even in the N:N case. we're making the assumption that on playback all channels are duplicated, but it would be nice for testing purposes to have the ability to split channels as well. And for capture we also need the ability to combine channels, possibly in a symmetrical way but it would also be nice to have the ability to reorder, e.g. L1 L2 R1 R2 instead of L1 R1 L2 R2 if we have two stereo capture devices.

The mapping at the dailink level should be implemented in ASoC soc-pcm.c, and the configuration in sof_sdw.c IMHO.

I will implement the N:M mapping. Close this PR for now.