On 8/27/21 4:33 AM, Sameer Pujar wrote:
In some cases, multiple FE components have the same BE component in their respective DPCM paths. One such example would be a mixer component, which can receive two or more inputs and sends a mixed output. In such cases, to avoid reconfiguration of already active DAI (mixer output DAI in this case), check the BE stream state to filter out the redundancy.
In summary, allow connection of BE if the respective current stream state is either NEW or CLOSED.
This patch breaks our SOF CI tests, ironically in all cases where we have a mixer with a 'Deep buffer' port! The tests with multiple streams all fail with this error:
[ 124.366400] Port0 Deep Buffer: ASoC: no backend DAIs enabled for Port0 Deep Buffer [ 124.366406] Port0 Deep Buffer: ASoC: dpcm_fe_dai_prepare() failed (-22)
Reverting this patch restore the mixer functionality.
I see multiple problems with this patch:
At a high-level, there's at least a race condition where this BE state is checked without any protection. That was already a problem that I highlighted in a recent RFC and still working on, when we have multiple FEs we can have START/STOP triggers happening concurrently and it's necessary to serialize these triggers when checking the state, and this patch increases the 'surface' for this race condition.
But in addition we'd need to agree on what an 'active BE' is. Why can't we connect a second stream while the first one is already in HW_PARAMS or PAUSED or STOP? It's perfectly legal in ALSA/ASoC to have multiple HW_PARAMS calls, and when we reach STOP we have to do a prepare again.
And more fundamentally, the ability to add a second FE on a 'active' BE in START state is a basic requirement for a mixer, e.g. to play a notification on one FE while listening to music on another. What needs to happen is only to make sure that the FE and BE are compatible in terms of HW_PARAMS and not sending a second TRIGGER_STOP, only checking the BE NEW or CLOSE state is way too restrictive.
I will agree this sort of mixer use cases is not well supported in soc-pcm.c, but let's not make it worse than it was before this patch, shall we?
I can send a revert with the explanations in the commit message if there is a consensus that this patch needs to be revisited.
[1] https://github.com/thesofproject/linux/pull/3177 [2] https://sof-ci.01.org/linuxpr/PR3177/build6440/devicetest/
Signed-off-by: Sameer Pujar spujar@nvidia.com
sound/soc/soc-pcm.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 48f71bb..e30cb5a 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1395,6 +1395,10 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, if (!fe->dpcm[stream].runtime && !fe->fe_compr) continue;
if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_NEW) &&(be->dpcm[stream].state != SND_SOC_DPCM_STATE_CLOSE))continue;- /* newly connected FE and BE */ err = dpcm_be_connect(fe, be, stream); if (err < 0) {