We've been adding a 'deep buffer' PCM device to several SOF topologies in order to reduce power consumption. The typical use-case would be music playback over a headset: this additional PCM device provides more buffering and longer latencies, leaving the rest of the system sleep for longer periods. Notifications and 'regular' low-latency audio playback would still use the 'normal' PCM device and be mixed with the 'deep buffer' before rendering on the headphone endpoint. The tentative direction would be to expose this alternate device to PulseAudio/PipeWire/CRAS via the UCM SectionModifier definitions. That seemed a straightforward topology change until our automated validation stress tests started reporting issues on SoundWire platforms, when e.g. two START triggers might be send and conversely the STOP trigger is never sent. The SoundWire stream state management flagged inconsistent states when the two 'normal' and 'deep buffer' devices are used concurrently with rapid play/stop/pause monkey testing. Looking at the soc-pcm.c code, it seems that the BE state management needs a lot of love. a) there is no consistent protection for the BE state. In some parts of the code, the state updates are protected by a spinlock but in the trigger they are not. When we open/play/close the two PCM devices in stress tests, we end-up testing a state that is being modified. That can't be good. b) there is a conceptual deadlock: on stop we check the FE states to see if a shared BE can be stopped, but since we trigger the BE first the FE states have not been modified yet, so the TRIGGER_STOP is never sent. This patchset suggests the removal of the dedicated 'dpcm_lock', and the use of the FE PCM lock before walking through the BE list, a mutual exclusion between triggers using the BE PCM lock, and the use of a refcount to decide when to trigger the BE. With these patches I am able to run our entire validation suite without any issues with this new 'deep buffer' topology, and no regressions on existing solutions [1] One might ask 'how come we didn't see this earlier'? The answer is probably that the .trigger callbacks in most implementations seems to perform DAPM operations, and sending the triggers multiple times is not an issue. In the case of SoundWire, we do use the .trigger callback to reconfigure the bus using the 'bank switch' mechanism. It could be acceptable to tolerate a trigger multiple times, but the deadlock on stop cannot be fixed at the SoundWire layer alone. I chose to send this patchset as an RFCv3 to gather more feedback and make use others know about DPCM issues. We're going to spend more time on this but if others can provide feedback/test results it would be greatly appreciated. The change in the locking model could be problematic on other platforms so we do want more time to comment/test before even considering a merge. Opens: 1) is this the right solution? The DPCM code is far from simple, has notions such as SND_SOC_DPCM_UPDATE_NO and 'trigger_pending' that I have no background on. It's not clear if these cases are still needed with the locking changes. 2) There are other reports of kernel oopses [2] that seem related to the lack of protection. I'd be good to confirm if this patchset solve these problems as well. [1] https://github.com/thesofproject/linux/pull/3146 [2] https://lore.kernel.org/alsa-devel/002f01d7b4f5$c030f4a0$4092dde0$@samsung.com/ changes since RFCv2: Removal of dpcm_lock to use FE PCM locks (credits to Takashi Iwai for the suggestion). The FE PCM lock is now used before each use of for_each_dpcm_be() - with the exception of the trigger where the lock is already taken. This change is also applied in drivers which make use of this loop (compress, SH, FSL). Addition of BE PCM lock to deal with mutual exclusion between triggers for the same BE. Alignment of the BE atomicity on the FE on connections, this is required to avoid sleeping in atomic context. Additional cleanups (indentation, static functions) changes since RFC v1: Removed unused function Removed exported symbols only used in soc-pcm.c, used static instead Use a mutex instead of a spinlock Protect all for_each_dpcm_be() loops Fix bugs introduced in the refcount Pierre-Louis Bossart (13): ASoC: soc-pcm: remove snd_soc_dpcm_fe_can_update() ASoC: soc-pcm: don't export local functions, use static ASoC: soc-pcm: use proper indentation on 'continue' ASoC: soc-pcm: introduce snd_soc_dpcm_fe_lock_irq/unlock_irq() ASoC: soc-pcm: align BE 'atomicity' with that of the FE ASoC: soc-pcm: remove dpcm spin_lock, use PCM stream lock ASoC: soc-pcm: protect for_each_dpcm_be() loops ASoC: soc-compress: protect for_each_dpcm_be() loops ASoC: sh: rcar: protect for_each_dpcm_be() loops ASoC: fsl: asrc_dma: protect for_each_dpcm_be() loops ASoC: soc-pcm: serialize BE triggers ASoC: soc-pcm: test refcount before triggering ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE include/sound/soc-dpcm.h | 20 +-- include/sound/soc.h | 2 - sound/soc/fsl/fsl_asrc_dma.c | 2 + sound/soc/sh/rcar/core.c | 2 + sound/soc/soc-compress.c | 4 + sound/soc/soc-core.c | 1 - sound/soc/soc-pcm.c | 253 ++++++++++++++++++++++++----------- 7 files changed, 187 insertions(+), 97 deletions(-) -- 2.25.1
