| Message ID | 20210120180110.8357-2-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | soundwire: add static port mapping support | expand |
Thanks Pierre for your inputs, On 20/01/2021 22:15, Pierre-Louis Bossart wrote: > > > On 1/20/21 12:01 PM, Srinivas Kandagatla wrote: >> Some of the soundwire controllers can have static functions assigned >> to each port, like some ports can only do PCM or PDM. This is the >> situation >> with some of the Qualcomm Controllers. >> >> In such cases its not correct to assign/map any free port on master >> during streaming. >> >> So, this patch provides a way to pass mapped port number along >> with the port config, so that master can assign correct ports based >> on the provided static mapping. > > I am not sure I understand the problem or what's different between Intel > and Qualcomm. > > On the Intel side we also have fixed-function ports, some for PDM and > some for PCM. They are not interchangeable, and they are also dedicated > for each link. > That is good to know! > We don't dynamically allocate ports on the master side, the mapping is > defined by the dai->id and is static. There is a 1:1 relationship > between dai->id and port_number. See intel_register_dai() and > intel_hw_params() in drivers/soundwire/intel.c > > In the machine driver we make use of specific master DAIs in the dailink > definitions, just like regular ASoC solutions, so which DAIs you use in > the machine driver defines what ports end-up being used. There is > nothing fancy or dynamic here, the dai/port allocation is defined by the > dailinks. This is a static/worst-case allocation, we don't reassign > ports depending on use-cases, etc. > > The only thing that is dynamic is that the programming of each port is > handled based on the bandwidth needs of that port, i.e if you play 16 or > 24 bits you'd get fewer or more bitSlots allocated to that dai/port, and > the DPn registers are updated if you have concurrent streaming on other > ports. If you only have a fixed set of payloads, as in the existing > amplifier cases, you can hard-code this allocation as well. Yes, it will work for the existing WSA881x amplifier case. Am preparing patches for a new QCOM codec driver WCD938x (TX and RX) connected via Soundwire, Port allocations are something like this: RX: (Simple) Port 1 -> HPH L/R Port 2 -> CLASS H Amp Port 3 -> COMP Port 4 -> DSD. TX: (This get bit more complicated) Port 1: PCM Port 2: ADC 1 & 2 Port 3: ADC 3 & 4 Port 4: DMIC-0, DMIC-1, DIMC-2 , DMIC-3 and MBHC Port 5: DMIC-4, DMIC-5, DMIC-6 and DMIC-7 We handle the port allocation dynamically based on mixer and dapm widgets in my code! Also channel allocations are different for each function! > > Does this help and can you align on what Intel started with? Firstly, This is where the issue comes, if we go with the suggested(dai->id) solution, we would end up with a long list of dai-links with different combinations of both inputs/output connections and usecases. Again we have to deal with limited DSP resources too! Secondly, The check [1] in stream.c will not allow more than one master port config to be added to master runtime. Ex: RX Port 1, 2, 3 is used for Headset Playback. But if we have a static mapping table of the ports then this will provide more flexibility to codec driver! And we can dynamically select ports based on the usecase or active dapm path! --srini [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soundwire/stream.c?h=v5.11-rc4#n1294 >
> Port allocations are something like this: > > RX: (Simple) > Port 1 -> HPH L/R > Port 2 -> CLASS H Amp > Port 3 -> COMP > Port 4 -> DSD. > > TX: (This get bit more complicated) > Port 1: PCM > Port 2: ADC 1 & 2 > Port 3: ADC 3 & 4 > Port 4: DMIC-0, DMIC-1, DIMC-2 , DMIC-3 and MBHC > Port 5: DMIC-4, DMIC-5, DMIC-6 and DMIC-7 > > We handle the port allocation dynamically based on mixer and dapm > widgets in my code! Also channel allocations are different for each > function! Sorry, I am not following here. What is dynamic here and use-case dependent? And is this a mapping on the master or the codec sides that you want to modify? >> Does this help and can you align on what Intel started with? > > Firstly, This is where the issue comes, if we go with the > suggested(dai->id) solution, we would end up with a long list of > dai-links with different combinations of both inputs/output connections > and usecases. Again we have to deal with limited DSP resources too! > > Secondly, The check [1] in stream.c will not allow more than one master > port config to be added to master runtime. Ex: RX Port 1, 2, 3 is used > for Headset Playback. I am confused here, we do have examples in existing codec drivers where we use multiple ports for a single stream, e.g. for IV feedback we use 2 ports. In your "RX Port 1, 2, 3" example, are you referring to the codec or the master side? If it's for the codec, it's already supported, see e.g. https://github.com/thesofproject/linux/pull/2514, we use DP2 and DP4 for the same stream. This is done with the port_config capability.
On 21/01/2021 14:56, Pierre-Louis Bossart wrote: > > >> Port allocations are something like this: >> >> RX: (Simple) >> Port 1 -> HPH L/R >> Port 2 -> CLASS H Amp >> Port 3 -> COMP >> Port 4 -> DSD. >> >> TX: (This get bit more complicated) >> Port 1: PCM >> Port 2: ADC 1 & 2 >> Port 3: ADC 3 & 4 >> Port 4: DMIC-0, DMIC-1, DIMC-2 , DMIC-3 and MBHC >> Port 5: DMIC-4, DMIC-5, DMIC-6 and DMIC-7 >> >> We handle the port allocation dynamically based on mixer and dapm >> widgets in my code! Also channel allocations are different for each >> function! > > Sorry, I am not following here. What is dynamic here and use-case > dependent? And is this a mapping on the master or the codec sides that > you want to modify? [SLAVE]-------[MASTER] NA-------------Port 1: PCM Port 1---------Port 2: ADC 1 & 2 Port 2---------Port 3: ADC 3 & 4 Port 3---------Port 4: DMIC-0, DMIC-1, DIMC-2 , DMIC-3 and MBHC Port 4---------Port 5: DMIC-4, DMIC-5, DMIC-6 and DMIC-7 Mapping is still static however Number of ports selection and channel mask will be dynamic here. Example: for Headset MIC usecase we will be using Slv Port1, Slv Port3 along with Mstr Port2 and Master Port4 Similarly for usecases like Digital MIC or other Analog MICs. > >>> Does this help and can you align on what Intel started with? >> >> Firstly, This is where the issue comes, if we go with the >> suggested(dai->id) solution, we would end up with a long list of >> dai-links with different combinations of both inputs/output >> connections and usecases. Again we have to deal with limited DSP >> resources too! >> >> Secondly, The check [1] in stream.c will not allow more than one >> master port config to be added to master runtime. Ex: RX Port 1, 2, 3 >> is used for Headset Playback. > > I am confused here, we do have examples in existing codec drivers where > we use multiple ports for a single stream, e.g. for IV feedback we use 2 > ports. Is this on multi_link? which is why it might be working for you. > Currently we have below check in sdw_stream_add_master(). if (!bus->multi_link && stream->m_rt_count > 0) { dev_err(bus->dev, "Multilink not supported, link %d\n", bus->link_id); ret = -EINVAL; goto unlock; } If we have single master(like my case) and dai-links which have more then one port will be calling sdw_stream_add_master() for each port, so m_rt_count above check will fail for the second call! > In your "RX Port 1, 2, 3" example, are you referring to the codec or the > master side? If it's for the codec, it's already supported, see e.g. Master side. > https://github.com/thesofproject/linux/pull/2514, we use DP2 and DP4 for This fine on slave side! Issue is on the master side! > the same stream. This is done with the port_config capability. > >
On 1/21/21 9:41 AM, Srinivas Kandagatla wrote: > > > On 21/01/2021 14:56, Pierre-Louis Bossart wrote: >> >> >>> Port allocations are something like this: >>> >>> RX: (Simple) >>> Port 1 -> HPH L/R >>> Port 2 -> CLASS H Amp >>> Port 3 -> COMP >>> Port 4 -> DSD. >>> >>> TX: (This get bit more complicated) >>> Port 1: PCM >>> Port 2: ADC 1 & 2 >>> Port 3: ADC 3 & 4 >>> Port 4: DMIC-0, DMIC-1, DIMC-2 , DMIC-3 and MBHC >>> Port 5: DMIC-4, DMIC-5, DMIC-6 and DMIC-7 >>> >>> We handle the port allocation dynamically based on mixer and dapm >>> widgets in my code! Also channel allocations are different for each >>> function! >> >> Sorry, I am not following here. What is dynamic here and use-case >> dependent? And is this a mapping on the master or the codec sides that >> you want to modify? > > [SLAVE]-------[MASTER] > NA-------------Port 1: PCM > Port 1---------Port 2: ADC 1 & 2 > Port 2---------Port 3: ADC 3 & 4 > Port 3---------Port 4: DMIC-0, DMIC-1, DIMC-2 , DMIC-3 and MBHC > Port 4---------Port 5: DMIC-4, DMIC-5, DMIC-6 and DMIC-7 > > > Mapping is still static however Number of ports selection and channel > mask will be dynamic here. > > > Example: for Headset MIC usecase we will be using Slv Port1, Slv Port3 > along with Mstr Port2 and Master Port4 > > Similarly for usecases like Digital MIC or other Analog MICs. Sorry, I must be thick here, but in my experience the choice of Digital or analog mics is a hardware design level not a use-case one. Using ADC 1 & 2 at the same time as DMICs is very surprising to me. You'd have different sensitivities/performance, not sure how you would combine the results. I also don't see how a headset mic can both use Analog and digital, unless we have a different definition of what a 'headset' is. >>>> Does this help and can you align on what Intel started with? >>> >>> Firstly, This is where the issue comes, if we go with the >>> suggested(dai->id) solution, we would end up with a long list of >>> dai-links with different combinations of both inputs/output >>> connections and usecases. Again we have to deal with limited DSP >>> resources too! >>> >>> Secondly, The check [1] in stream.c will not allow more than one >>> master port config to be added to master runtime. Ex: RX Port 1, 2, 3 >>> is used for Headset Playback. >> >> I am confused here, we do have examples in existing codec drivers >> where we use multiple ports for a single stream, e.g. for IV feedback >> we use 2 ports. > > Is this on multi_link? which is why it might be working for you. no, this is done at the codec driver level, which has no notion of multi-link. we pass a port_config as a array of 2. > Currently we have below check in sdw_stream_add_master(). > > if (!bus->multi_link && stream->m_rt_count > 0) { > dev_err(bus->dev, "Multilink not supported, link %d\n", bus->link_id); > ret = -EINVAL; > goto unlock; > } > > If we have single master(like my case) and dai-links which have more > then one port will be calling sdw_stream_add_master() for each port, > so m_rt_count above check will fail for the second call! if you use multiple ports in a given master for the same stream, you should have the m_rt_count == 1. That's a feature, not a bug. A port is not a stream... You cannot call sdw_stream_add_master() for each port, that's not what the concept was. You allocate ONE master_rt per master, and that master_rt deals with one or more ports - your choice. A 'stream' is an abstract data transport which can be split across multiple masters/sales and for each master/slave use multiple ports. When calling sdw_stream_add_master/slave, you need to provide a port_config/num_ports to state which ports will be used on that master/slave when using the stream. That's how we e.g. deal with 4ch streams that are handled by two ports on each side. To up-level a bit, the notion of 'stream' is actually very very similar to the notion of dailink. And in fact, the 'stream' is actually created for Intel in the dailink .startup callback, so I am quite in the dark on what you are trying to accomplish.
On 21/01/2021 18:00, Pierre-Louis Bossart wrote: > > > On 1/21/21 9:41 AM, Srinivas Kandagatla wrote: >> >> >> On 21/01/2021 14:56, Pierre-Louis Bossart wrote: >>> >>> >>>> Port allocations are something like this: >>>> >>>> RX: (Simple) >>>> Port 1 -> HPH L/R >>>> Port 2 -> CLASS H Amp >>>> Port 3 -> COMP >>>> Port 4 -> DSD. >>>> >>>> TX: (This get bit more complicated) >>>> Port 1: PCM >>>> Port 2: ADC 1 & 2 >>>> Port 3: ADC 3 & 4 >>>> Port 4: DMIC-0, DMIC-1, DIMC-2 , DMIC-3 and MBHC >>>> Port 5: DMIC-4, DMIC-5, DMIC-6 and DMIC-7 >>>> >>>> We handle the port allocation dynamically based on mixer and dapm >>>> widgets in my code! Also channel allocations are different for each >>>> function! >>> >>> Sorry, I am not following here. What is dynamic here and use-case >>> dependent? And is this a mapping on the master or the codec sides >>> that you want to modify? >> >> [SLAVE]-------[MASTER] >> NA-------------Port 1: PCM >> Port 1---------Port 2: ADC 1 & 2 >> Port 2---------Port 3: ADC 3 & 4 >> Port 3---------Port 4: DMIC-0, DMIC-1, DIMC-2 , DMIC-3 and MBHC >> Port 4---------Port 5: DMIC-4, DMIC-5, DMIC-6 and DMIC-7 >> >> >> Mapping is still static however Number of ports selection and channel >> mask will be dynamic here. >> >> >> Example: for Headset MIC usecase we will be using Slv Port1, Slv Port3 >> along with Mstr Port2 and Master Port4 >> >> Similarly for usecases like Digital MIC or other Analog MICs. > > Sorry, I must be thick here, but in my experience the choice of Digital > or analog mics is a hardware design level not a use-case one. Using ADC > 1 & 2 at the same time as DMICs is very surprising to me. You'd have > different sensitivities/performance, not sure how you would combine the > results. In this particular case, ADC2 on Port2 is used along with the MBHC(Multi Button and Headset Detection) channel on Master Port4. This is intended for Headset Button Click Suppression!. This again can be dynamically selected based on if headset button Click Suppression is enabled or not. So this is not really mixing ADC with DMICs here! > > I also don't see how a headset mic can both use Analog and digital, > unless we have a different definition of what a 'headset' is. > >>>>> Does this help and can you align on what Intel started with? >>>> >>>> Firstly, This is where the issue comes, if we go with the >>>> suggested(dai->id) solution, we would end up with a long list of >>>> dai-links with different combinations of both inputs/output >>>> connections and usecases. Again we have to deal with limited DSP >>>> resources too! >>>> >>>> Secondly, The check [1] in stream.c will not allow more than one >>>> master port config to be added to master runtime. Ex: RX Port 1, 2, >>>> 3 is used for Headset Playback. >>> >>> I am confused here, we do have examples in existing codec drivers >>> where we use multiple ports for a single stream, e.g. for IV feedback >>> we use 2 ports. >> >> Is this on multi_link? which is why it might be working for you. > > no, this is done at the codec driver level, which has no notion of > multi-link. we pass a port_config as a array of 2. > Am referring to sdw_stream_add_master() not sdw_stream_add_slave(). >> Currently we have below check in sdw_stream_add_master(). >> >> if (!bus->multi_link && stream->m_rt_count > 0) { >> dev_err(bus->dev, "Multilink not supported, link %d\n", >> bus->link_id); >> ret = -EINVAL; >> goto unlock; >> } >> >> If we have single master(like my case) and dai-links which have more >> then one port will be calling sdw_stream_add_master() for each port, >> so m_rt_count above check will fail for the second call! > > if you use multiple ports in a given master for the same stream, you > should have the m_rt_count == 1. That's a feature, not a bug. > > A port is not a stream... You cannot call sdw_stream_add_master() for > each port, that's not what the concept was. You allocate ONE master_rt Am looking at intel_hw_params(). Isn't sdw_stream_add_master() called for every dai in the dai link. > per master, and that master_rt deals with one or more ports - your choice. > > A 'stream' is an abstract data transport which can be split across > multiple masters/sales and for each master/slave use multiple ports. > When calling sdw_stream_add_master/slave, you need to provide a > port_config/num_ports to state which ports will be used on that > master/slave when using the stream. That's how we e.g. deal with 4ch > streams that are handled by two ports on each side. > > To up-level a bit, the notion of 'stream' is actually very very similar > to the notion of dailink. And in fact, the 'stream' is actually created > for Intel in the dailink .startup callback, so I am quite in the dark on > what you are trying to accomplish. In qcom case stream is also allocated for in dai startup(). I think we are talking about two different issues, 1>one is the failure I see in sdw_stream_add_master() when I try to use dai-link dai-id style approach suggested. I will dig this bit more and collect more details! 2>(Main issue) Ability for slave to select different combination of ports at runtime based on the mixer setting or active dapm. All this patch is trying do is the pass this *CURRENT/ACTIVE* static port mapping between slave and master while setting up the stream. With the dailink approach number of ports are pretty much static and may not be required for particular use case. As above example if we have a headset with button click suppression we would need 2 Ports and similarly without we only need one port. This is not possible with dai-link approach, unless we create two different dai links for the above example usecase! Hopefully this adds some light to the issue :-) --srini
>>> [SLAVE]-------[MASTER] >>> NA-------------Port 1: PCM >>> Port 1---------Port 2: ADC 1 & 2 >>> Port 2---------Port 3: ADC 3 & 4 >>> Port 3---------Port 4: DMIC-0, DMIC-1, DIMC-2 , DMIC-3 and MBHC >>> Port 4---------Port 5: DMIC-4, DMIC-5, DMIC-6 and DMIC-7 >>> >>> >>> Mapping is still static however Number of ports selection and channel >>> mask will be dynamic here. >>> >>> >>> Example: for Headset MIC usecase we will be using Slv Port1, Slv >>> Port3 along with Mstr Port2 and Master Port4 >>> >>> Similarly for usecases like Digital MIC or other Analog MICs. >> >> Sorry, I must be thick here, but in my experience the choice of >> Digital or analog mics is a hardware design level not a use-case one. >> Using ADC 1 & 2 at the same time as DMICs is very surprising to me. >> You'd have different sensitivities/performance, not sure how you would >> combine the results. > > In this particular case, ADC2 on Port2 is used along with the MBHC(Multi > Button and Headset Detection) channel on Master Port4. This is intended > for Headset Button Click Suppression!. This again can be dynamically > selected based on if headset button Click Suppression is enabled or not. The question is whether the ADC2 and MBHC ports convey data for the same 'stream'. If they need to be synchronous, they have to be part of the same stream and triggered at the same time. we don't have the ability to change the stream definition at run-time when an ALSA control value changes. The only thing you could do is enabled it always, and drop the data on the floor inside of the master if/when the control value changes. >>>>> Firstly, This is where the issue comes, if we go with the >>>>> suggested(dai->id) solution, we would end up with a long list of >>>>> dai-links with different combinations of both inputs/output >>>>> connections and usecases. Again we have to deal with limited DSP >>>>> resources too! Like I said above, your ability to reconfigure is limited, and you may have to stop/start streaming if you want to optimize allocation. >>>>> Secondly, The check [1] in stream.c will not allow more than one >>>>> master port config to be added to master runtime. Ex: RX Port 1, 2, >>>>> 3 is used for Headset Playback. >>>> >>>> I am confused here, we do have examples in existing codec drivers >>>> where we use multiple ports for a single stream, e.g. for IV >>>> feedback we use 2 ports. >>> >>> Is this on multi_link? which is why it might be working for you. >> >> no, this is done at the codec driver level, which has no notion of >> multi-link. we pass a port_config as a array of 2. >> > > Am referring to sdw_stream_add_master() not sdw_stream_add_slave(). It doesn't matter, it's the same concept that for a given stream, you tell the device which ports will be used. The API is quasi-identical, in the master case the bus/master/link are the same concept. int sdw_stream_add_master(struct sdw_bus *bus, struct sdw_stream_config *stream_config, struct sdw_port_config *port_config, unsigned int num_ports, struct sdw_stream_runtime *stream) int sdw_stream_add_slave(struct sdw_slave *slave, struct sdw_stream_config *stream_config, struct sdw_port_config *port_config, unsigned int num_ports, struct sdw_stream_runtime *stream) >>> Currently we have below check in sdw_stream_add_master(). >>> >>> if (!bus->multi_link && stream->m_rt_count > 0) { >>> dev_err(bus->dev, "Multilink not supported, link %d\n", >>> bus->link_id); >>> ret = -EINVAL; >>> goto unlock; >>> } >>> >>> If we have single master(like my case) and dai-links which have more >>> then one port will be calling sdw_stream_add_master() for each >>> port, so m_rt_count above check will fail for the second call! >> >> if you use multiple ports in a given master for the same stream, you >> should have the m_rt_count == 1. That's a feature, not a bug. >> >> A port is not a stream... You cannot call sdw_stream_add_master() for >> each port, that's not what the concept was. You allocate ONE master_rt > > Am looking at intel_hw_params(). Isn't sdw_stream_add_master() called > for every dai in the dai link. Yes, that's correct, but again a dai may use one or more ports. if you defined each port as a dai, and want to call sdw_stream_add_master() for each port you are doing something the API was not designed for. There is a 'num_ports' argument for a reason :-) >> per master, and that master_rt deals with one or more ports - your >> choice. > >> A 'stream' is an abstract data transport which can be split across >> multiple masters/sales and for each master/slave use multiple ports. >> When calling sdw_stream_add_master/slave, you need to provide a >> port_config/num_ports to state which ports will be used on that >> master/slave when using the stream. That's how we e.g. deal with 4ch >> streams that are handled by two ports on each side. >> >> To up-level a bit, the notion of 'stream' is actually very very >> similar to the notion of dailink. And in fact, the 'stream' is >> actually created for Intel in the dailink .startup callback, so I am >> quite in the dark on what you are trying to accomplish. > In qcom case stream is also allocated for in dai startup(). > > I think we are talking about two different issues, > > 1>one is the failure I see in sdw_stream_add_master() when I try to use > dai-link dai-id style approach suggested. I will dig this bit more and > collect more details! > > 2>(Main issue) Ability for slave to select different combination of > ports at runtime based on the mixer setting or active dapm. > > All this patch is trying do is the pass this *CURRENT/ACTIVE* static > port mapping between slave and master while setting up the stream. > With the dailink approach number of ports are pretty much static and may > not be required for particular use case. As above example if we have a > headset with button click suppression we would need 2 Ports and > similarly without we only need one port. As I said above you cannot enable the button click suppression dynamically *after* the headset capture hw_params/prepare. > This is not possible with dai-link approach, unless we create two > different dai links for the above example usecase! The current approach is a worst-case one, where you would create a single 'headset capture' dailink. We never envisioned a case where you modify what the definition of 'headset capture' is based on control events, and I really challenge the fact that it is feasible/realistic. This is really about streaming data across a bus, and we are limited on what we can do. It's the same problem that we never modify the number of channels dynamically on a PCM device.
On 21/01/2021 21:30, Pierre-Louis Bossart wrote: >> >> Am looking at intel_hw_params(). Isn't sdw_stream_add_master() called >> for every dai in the dai link. > > Yes, that's correct, but again a dai may use one or more ports. > > if you defined each port as a dai, and want to call > sdw_stream_add_master() for each port you are doing something the API > was not designed for. There is a 'num_ports' argument for a reason :-) > >>> per master, and that master_rt deals with one or more ports - your >>> choice. > >>> A 'stream' is an abstract data transport which can be split across >>> multiple masters/sales and for each master/slave use multiple ports. >>> When calling sdw_stream_add_master/slave, you need to provide a >>> port_config/num_ports to state which ports will be used on that >>> master/slave when using the stream. That's how we e.g. deal with 4ch >>> streams that are handled by two ports on each side. >>> >>> To up-level a bit, the notion of 'stream' is actually very very >>> similar to the notion of dailink. And in fact, the 'stream' is >>> actually created for Intel in the dailink .startup callback, so I am >>> quite in the dark on what you are trying to accomplish. >> In qcom case stream is also allocated for in dai startup(). >> >> I think we are talking about two different issues, >> >> 1>one is the failure I see in sdw_stream_add_master() when I try to >> use dai-link dai-id style approach suggested. I will dig this bit more >> and collect more details! >> >> 2>(Main issue) Ability for slave to select different combination of >> ports at runtime based on the mixer setting or active dapm. >> >> All this patch is trying do is the pass this *CURRENT/ACTIVE* static >> port mapping between slave and master while setting up the stream. >> With the dailink approach number of ports are pretty much static and >> may not be required for particular use case. As above example if we >> have a headset with button click suppression we would need 2 Ports and >> similarly without we only need one port. > > As I said above you cannot enable the button click suppression > dynamically *after* the headset capture hw_params/prepare. That is not true, the ports in this case are selected based on mixer setting or register state even before stream is setup/started in hw_params/prepare. WSA881x codec has pretty much similar setup. > >> This is not possible with dai-link approach, unless we create two >> different dai links for the above example usecase! > > The current approach is a worst-case one, where you would create a > single 'headset capture' dailink. > Are you suggesting that we have dailink for each usecase like: "headset capture" "Analog MIC1 capture" "Analog MIC2 Capture" ... "Analog MIC4 Capture" ... "DMIC0 capture" "DMIC1 Capture" "DMIC2 Capture" ... "DMIC7 Capture" .. "Headset Playback" "Ear Playback" .. "Aux Playback" ... this is not really doable! All am saying is that codec can decide which ports it has to select based on mixer setting before the stream is setup/started. This updated mapping between slv port and master ports is passed as part of the port_config in sdw_stream_add_slave(). --srini > We never envisioned a case where you modify what the definition of > 'headset capture' is based on control events, and I really challenge the > fact that it is feasible/realistic. This is really about streaming data > across a bus, and we are limited on what we can do. It's the same > problem that we never modify the number of channels dynamically on a PCM > device. > > >
On 1/22/21 1:05 AM, Srinivas Kandagatla wrote: > > > On 21/01/2021 21:30, Pierre-Louis Bossart wrote: >>> >>> Am looking at intel_hw_params(). Isn't sdw_stream_add_master() called >>> for every dai in the dai link. >> >> Yes, that's correct, but again a dai may use one or more ports. >> >> if you defined each port as a dai, and want to call >> sdw_stream_add_master() for each port you are doing something the API >> was not designed for. There is a 'num_ports' argument for a reason :-) >> >>>> per master, and that master_rt deals with one or more ports - your >>>> choice. > >>>> A 'stream' is an abstract data transport which can be split across >>>> multiple masters/sales and for each master/slave use multiple ports. >>>> When calling sdw_stream_add_master/slave, you need to provide a >>>> port_config/num_ports to state which ports will be used on that >>>> master/slave when using the stream. That's how we e.g. deal with 4ch >>>> streams that are handled by two ports on each side. >>>> >>>> To up-level a bit, the notion of 'stream' is actually very very >>>> similar to the notion of dailink. And in fact, the 'stream' is >>>> actually created for Intel in the dailink .startup callback, so I am >>>> quite in the dark on what you are trying to accomplish. >>> In qcom case stream is also allocated for in dai startup(). >>> >>> I think we are talking about two different issues, >>> >>> 1>one is the failure I see in sdw_stream_add_master() when I try to >>> use dai-link dai-id style approach suggested. I will dig this bit >>> more and collect more details! >>> >>> 2>(Main issue) Ability for slave to select different combination of >>> ports at runtime based on the mixer setting or active dapm. >>> >>> All this patch is trying do is the pass this *CURRENT/ACTIVE* static >>> port mapping between slave and master while setting up the stream. >>> With the dailink approach number of ports are pretty much static and >>> may not be required for particular use case. As above example if we >>> have a headset with button click suppression we would need 2 Ports >>> and similarly without we only need one port. >> >> As I said above you cannot enable the button click suppression >> dynamically *after* the headset capture hw_params/prepare. > > That is not true, the ports in this case are selected based on mixer > setting or register state even before stream is setup/started in > hw_params/prepare. > WSA881x codec has pretty much similar setup. we are saying the same thing, the configuration provided is only taken into account when setting-up the stream in hw_params. mixer or configuration changes after that step are ignored. If you follow what we've done at Intel with the sdw_stream_add_master() called in the .hw_params phase, and conversely call sdw_stream_remove_master() in .hw_free, you should be good to go. You will note that we have a notification to the DSP, so you can manage resources in your firmware, there is no need to oversubscribe but only allocate what is required for a given use case. >>> This is not possible with dai-link approach, unless we create two >>> different dai links for the above example usecase! >> >> The current approach is a worst-case one, where you would create a >> single 'headset capture' dailink. >> > > Are you suggesting that we have dailink for each usecase like: > > "headset capture" > "Analog MIC1 capture" > "Analog MIC2 Capture" > > ... > > "Analog MIC4 Capture" > > ... > > "DMIC0 capture" > "DMIC1 Capture" > "DMIC2 Capture" > > ... > > "DMIC7 Capture" > .. > "Headset Playback" > "Ear Playback" > .. > "Aux Playback" > ... > > this is not really doable! No, what I was saying is that you need to define multiple streams e.g. - headset capture (configured with or without click suppression) - mic capture (configured with AMICs or DMICs) - playback (or possibly different endpoint specific streams depending on whether concurrency between endpoint is possible) if you change the configuration, you have to tear down the stream and reconfigure it - and for this we already have the required API and you can guarantee that the configuration for that stream is consistent between master and slave(s). > All am saying is that codec can decide which ports it has to select > based on mixer setting before the stream is setup/started. This updated > mapping between slv port and master ports is passed as part of the > port_config in sdw_stream_add_slave(). if you completely remove the stream and re-add it with updated configuration things should work.
On 22/01/2021 15:32, Pierre-Louis Bossart wrote: >> >> Are you suggesting that we have dailink for each usecase like: >> >> "headset capture" >> "Analog MIC1 capture" >> "Analog MIC2 Capture" >> >> ... >> >> "Analog MIC4 Capture" >> >> ... >> >> "DMIC0 capture" >> "DMIC1 Capture" >> "DMIC2 Capture" >> >> ... >> >> "DMIC7 Capture" >> .. >> "Headset Playback" >> "Ear Playback" >> .. >> "Aux Playback" >> ... >> >> this is not really doable! > > No, what I was saying is that you need to define multiple streams e.g. > - headset capture (configured with or without click suppression) > - mic capture (configured with AMICs or DMICs) > - playback (or possibly different endpoint specific streams depending on > whether concurrency between endpoint is possible) > > if you change the configuration, you have to tear down the stream and > reconfigure it - and for this we already have the required API and you > can guarantee that the configuration for that stream is consistent > between master and slave(s). Yes, we make sure that new configuration is only applied before the stream is started, and not in middle of already started stream. > >> All am saying is that codec can decide which ports it has to select >> based on mixer setting before the stream is setup/started. This >> updated mapping between slv port and master ports is passed as part of >> the port_config in sdw_stream_add_slave(). > > if you completely remove the stream and re-add it with updated > configuration things should work. That's exactly what we do currently! The updated ports due to new configuration ex: for "mic capture" dailink needs to be communicated from slave(codec) to master so that it can allocate correct ports. That is what this patch is trying to do (share current port map information). --srini
>> No, what I was saying is that you need to define multiple streams e.g. >> - headset capture (configured with or without click suppression) >> - mic capture (configured with AMICs or DMICs) >> - playback (or possibly different endpoint specific streams depending >> on whether concurrency between endpoint is possible) >> >> if you change the configuration, you have to tear down the stream and >> reconfigure it - and for this we already have the required API and you >> can guarantee that the configuration for that stream is consistent >> between master and slave(s). > > Yes, we make sure that new configuration is only applied before the > stream is started, and not in middle of already started stream. ok, we are almost in agreement but... >>> All am saying is that codec can decide which ports it has to select >>> based on mixer setting before the stream is setup/started. This >>> updated mapping between slv port and master ports is passed as part >>> of the port_config in sdw_stream_add_slave(). >> >> if you completely remove the stream and re-add it with updated >> configuration things should work. > > That's exactly what we do currently! > > The updated ports due to new configuration ex: for "mic capture" dailink > needs to be communicated from slave(codec) to master so that it can > allocate correct ports. That is what this patch is trying to do (share > current port map information). .. we have a disconnect on how to do this configuration update. The 'stream' support was designed so that a stream can be split across multiple devices (both masters and slaves). With this design we need to have a central configuration and distribute the information to all devices taking part of the stream. It seems you are in a different solution-space, where the codec driver needs to notify the master of which ports it needs to use? I also don't see where the mapping is actually set. Patch 2 uses a mapping but there's no codec driver change that defines the mapping? Do you actually call sdw_stream_add_slave() with a new mapping? It feels we are missing the codec part to really see what you are trying to do?
On 22/01/2021 16:42, Pierre-Louis Bossart wrote: >>> >>> if you completely remove the stream and re-add it with updated >>> configuration things should work. >> >> That's exactly what we do currently! >> >> The updated ports due to new configuration ex: for "mic capture" >> dailink needs to be communicated from slave(codec) to master so that >> it can allocate correct ports. That is what this patch is trying to do >> (share current port map information). > > .. we have a disconnect on how to do this configuration update. > > The 'stream' support was designed so that a stream can be split across > multiple devices (both masters and slaves). With this design we need to > have a central configuration and distribute the information to all > devices taking part of the stream. > > It seems you are in a different solution-space, where the codec driver > needs to notify the master of which ports it needs to use? Correct! As Codec is the place where we have mixer controls ant it can clearly tell which master ports should be used for that particular configuration. > > I also don't see where the mapping is actually set. Patch 2 uses a > mapping but there's no codec driver change that defines the mapping? > > Do you actually call sdw_stream_add_slave() with a new mapping? > Yes, currently am working on a codec driver for WCD938x Codec, which I will posting very soon! > It feels we are missing the codec part to really see what you are trying > to do? My WIP code is at https://git.linaro.org/people/srinivas.kandagatla/linux.git/tree/sound/soc/codecs/wcd938x.c?h=wcd938x/wip#n4526 Currently the master ports are hardcoded in the driver for now, but these will come from DT. --srini
On 25-01-21, 16:23, Srinivas Kandagatla wrote: > > > On 22/01/2021 16:42, Pierre-Louis Bossart wrote: > > > > > > > > if you completely remove the stream and re-add it with updated > > > > configuration things should work. > > > > > > That's exactly what we do currently! > > > > > > The updated ports due to new configuration ex: for "mic capture" > > > dailink needs to be communicated from slave(codec) to master so that > > > it can allocate correct ports. That is what this patch is trying to > > > do (share current port map information). > > > > .. we have a disconnect on how to do this configuration update. > > > > The 'stream' support was designed so that a stream can be split across > > multiple devices (both masters and slaves). With this design we need to > > have a central configuration and distribute the information to all > > devices taking part of the stream. That is correct, but in this case a stream consists of one master and one or more slave devices. This is not a multi-master design. The adding of multiple masters should not be done here... that does not seem logically right in this situation > > It seems you are in a different solution-space, where the codec driver > > needs to notify the master of which ports it needs to use? > > Correct! As Codec is the place where we have mixer controls ant it can > clearly tell which master ports should be used for that particular > configuration. And that should come from firmware (DT etc) and driver should pass on this info > > I also don't see where the mapping is actually set. Patch 2 uses a > > mapping but there's no codec driver change that defines the mapping? > > > > Do you actually call sdw_stream_add_slave() with a new mapping? > > > Yes, currently am working on a codec driver for WCD938x Codec, which I will > posting very soon! > > > It feels we are missing the codec part to really see what you are trying > > to do? > My WIP code is at https://git.linaro.org/people/srinivas.kandagatla/linux.git/tree/sound/soc/codecs/wcd938x.c?h=wcd938x/wip#n4526 > > Currently the master ports are hardcoded in the driver for now, but these > will come from DT. > > --srini
On 19/02/2021 19:52, Pierre-Louis Bossart wrote: > > >>>>> It seems you are in a different solution-space, where the codec driver >>>>> needs to notify the master of which ports it needs to use? >>>> Correct! As Codec is the place where we have mixer controls ant it can >>>> clearly tell which master ports should be used for that particular >>>> configuration. >>> And that should come from firmware (DT etc) and driver should pass on >>> this info >> >> Are you okay with the patch as it is, provided this information is >> populated from DT? > > I am fine with the direction at a high-level. The premise for SoundWire > is that the bus is simple enough that it can be used in different > contexts and architectures, so if Qualcomm need something that differs > from what is needed for Intel we are really not in a position to object. > > That said, I could use more explanations on how the mapping is defined: > I don't think we have the same definition of 'static port mapping'. For > SDCA integration, we plan to have a static mapping in some sort of ACPI > table that will describe which port on the Manager side is connected to > which ports on Peripheral XYZ. That's static as in set in stone in > platform firmware. I understand the reference to DT settings as the same > idea. Yes, we are talking about the same static mapping here! > > But if the mapping depends on the value of mixer controls as you > describe it, then it's not static and defined by DT settings, but > run-time defined. I think there is some miss understanding here, the mapping is static but the port selection is based on the mixer controls! > > Also maybe we'd want to have a more opaque way of passing the > information, maybe with a stream private data or a callback, instead of > hard-coding fields that are only used by Qualcomm. Let me try the callback way and see how it will endup! thanks, srini > >
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index 2e049d39c6e5..e812557c3293 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -85,6 +85,8 @@ int sdw_find_col_index(int col); * @num: Port number. For audio streams, valid port number ranges from * [1,14] * @ch_mask: Channel mask + * @mapped_port_num: Port number to map on Master or Slave in Static Configuration + * @is_static_map: true for static port mapping * @transport_params: Transport parameters * @port_params: Port parameters * @port_node: List node for Master or Slave port_list @@ -95,6 +97,8 @@ int sdw_find_col_index(int col); struct sdw_port_runtime { int num; int ch_mask; + unsigned int mapped_port_num; + bool is_static_map; struct sdw_transport_params transport_params; struct sdw_port_params port_params; struct list_head port_node; diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 1099b5d1262b..eab3bc0c95ed 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1202,6 +1202,10 @@ static struct sdw_port_runtime p_rt->ch_mask = port_config[port_index].ch_mask; p_rt->num = port_config[port_index].num; + p_rt->is_static_map = port_config[port_index].is_static_map; + + if (p_rt->is_static_map) + p_rt->mapped_port_num = port_config[port_index].mapped_port_num; return p_rt; } diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index f0b01b728640..a523f062993d 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -894,10 +894,14 @@ void sdw_bus_master_delete(struct sdw_bus *bus); * * @num: Port number * @ch_mask: channels mask for port + * @mapped_port_num: Port number to map on Master or Slave in Static Configuration + * @is_static_map: true for static port mapping */ struct sdw_port_config { unsigned int num; unsigned int ch_mask; + unsigned int mapped_port_num; + bool is_static_map; }; /**
Some of the soundwire controllers can have static functions assigned to each port, like some ports can only do PCM or PDM. This is the situation with some of the Qualcomm Controllers. In such cases its not correct to assign/map any free port on master during streaming. So, this patch provides a way to pass mapped port number along with the port config, so that master can assign correct ports based on the provided static mapping. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/soundwire/bus.h | 4 ++++ drivers/soundwire/stream.c | 4 ++++ include/linux/soundwire/sdw.h | 4 ++++ 3 files changed, 12 insertions(+)