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

[Pierre-Louis Bossart]

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.