Message-ID: <f960757f-ec8b-6d3f-f00e-27242c687926@linux.intel.com>
Date:   Fri, 22 Jan 2021 09:32:58 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        vkoul@...nel.org, yung-chuan.liao@...ux.intel.com
Cc:     gregkh@...uxfoundation.org, sanyog.r.kale@...el.com,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] soundwire: add support for static port mapping



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.

Powered by blists - more mailing lists