diff mbox series

[RFC,1/2] soundwire: add support for static port mapping

Message ID 20210120180110.8357-2-srinivas.kandagatla@linaro.org (mailing list archive)
State New, archived
Headers show
Series soundwire: add static port mapping support | expand

Commit Message

Srinivas Kandagatla Jan. 20, 2021, 6:01 p.m. UTC
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(+)

Comments

Pierre-Louis Bossart Jan. 20, 2021, 10:15 p.m. UTC | #1
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.

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.

Does this help and can you align on what Intel started with?
Srinivas Kandagatla Jan. 21, 2021, 11:35 a.m. UTC | #2
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
>
Pierre-Louis Bossart Jan. 21, 2021, 2:56 p.m. UTC | #3
> 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.
Srinivas Kandagatla Jan. 21, 2021, 3:41 p.m. UTC | #4
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.
> 
>
Pierre-Louis Bossart Jan. 21, 2021, 6 p.m. UTC | #5
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.
Srinivas Kandagatla Jan. 21, 2021, 6:41 p.m. UTC | #6
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
Pierre-Louis Bossart Jan. 21, 2021, 9:30 p.m. UTC | #7
>>> [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.
Srinivas Kandagatla Jan. 22, 2021, 7:05 a.m. UTC | #8
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.
> 
> 
>
Pierre-Louis Bossart Jan. 22, 2021, 3:32 p.m. UTC | #9
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.
Srinivas Kandagatla Jan. 22, 2021, 3:46 p.m. UTC | #10
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
Pierre-Louis Bossart Jan. 22, 2021, 4:42 p.m. UTC | #11
>> 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?
Srinivas Kandagatla Jan. 25, 2021, 4:23 p.m. UTC | #12
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
Vinod Koul Feb. 1, 2021, 10:27 a.m. UTC | #13
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
Srinivas Kandagatla Feb. 19, 2021, 10:35 a.m. UTC | #14
Hi Pierre/Vinod,

On 01/02/2021 10:27, Vinod Koul 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?



--srini
>
Pierre-Louis Bossart Feb. 19, 2021, 7:52 p.m. UTC | #15
>>>> 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.

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.

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.
Srinivas Kandagatla Feb. 22, 2021, 1:40 p.m. UTC | #16
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 mbox series

Patch

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;
 };
 
 /**