[PATCH v2 0/1] ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive
It's reasonable that DAI parameters should be cleared if current DAI becomes inactive.
Only check DAI active status in soc_pcm_hw_free() is not enough since there's the risk that DAI parameters never be cleared if there're more than one stream [see A]. Only check DAI active status in soc_pcm_close() is also not enough since it will cause the cleanup just happening in soc_pcm_close() [see B].
[A] For example, we have stream1 and stream2 for DAI about to stop. stream2 executes soc_pcm_hw_free() before stream1 executes soc_pcm_close(). At the moment, stream2 should clear current DAI parameters because stream1 passed the cleanup stage in soc_pcm_hw_free() and stream2 in fact is the only active stream which has a chance to do clean up. Since DAI active status is not yet updated by stream1 in soc_pcm_close(), stream2 doesn't know itself should clear DAI parameters. In result both stream1 and stream2 don't clear the parameters.
[B] Suppose a case: aplay -Dhw:0 44100.wav 48000.wav In this case, we call soc_pcm_open()->soc_pcm_hw_params()->soc_pcm_hw_free() ->soc_pcm_hw_params()->soc_pcm_hw_free()->soc_pcm_close() in order. The DAI parameters would be remained in the system even if the playback of 44100.wav is finished.
In conclusion, it's better to check DAI active status in both soc_pcm_hw_free() and soc_pcm_close() which makes sure DAI parameters cleared if the DAI becomes inactive.
changes in v2: - Patch v1 link: https://lore.kernel.org/all/20230112065834.580192-1-chancel.liu@nxp.com/ Patch v1 tries to introduce a usage count called hw_params_count to fix issue on DAI parameters cleanup. However it's not a good fix because not considering hw_params() and hw_free() are not symmetrical and hw_params() might be called multilpe times by user. - Both check DAI active status in soc_pcm_hw_free() and soc_pcm_close() which makes sure DAI parameters cleared if the DAI becomes inactive.
Chancel Liu (1): ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive
sound/soc/soc-pcm.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
-- 2.25.1
The commit 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after stream_active is updated") tries to make sure DAI parameters can be cleared properly through moving the cleanup to the place where stream active status is updated. However, it will cause the cleanup only happening in soc_pcm_close().
Suppose a case: aplay -Dhw:0 44100.wav 48000.wav. The case calls soc_pcm_open()->soc_pcm_hw_params()->soc_pcm_hw_free()-> soc_pcm_hw_params()->soc_pcm_hw_free()->soc_pcm_close() in order. The parameters would be remained in the system even if the playback of 44100.wav is finished.
The case requires us clearing parameters in phase of soc_pcm_hw_free(). However, moving the DAI parameters cleanup back to soc_pcm_hw_free() has the risk that DAIs parameters never be cleared if there're more than one stream, see commit 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after stream_active is updated") for more details.
To meet all these requirements, in addition to do DAI parameters cleanup in soc_pcm_hw_free(), also check it in soc_pcm_close() to make sure DAI parameters cleared if the DAI becomes inactive.
Fixes: 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after stream_active is updated") Signed-off-by: Chancel Liu chancel.liu@nxp.com --- sound/soc/soc-pcm.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 3aa6b988cb4b..6cf4cd667d03 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -698,14 +698,12 @@ static int soc_pcm_clean(struct snd_soc_pcm_runtime *rtd,
if (!rollback) { snd_soc_runtime_deactivate(rtd, substream->stream); - /* clear the corresponding DAIs parameters when going to be inactive */ - for_each_rtd_dais(rtd, i, dai) { - if (snd_soc_dai_active(dai) == 0) - soc_pcm_set_dai_params(dai, NULL);
- if (snd_soc_dai_stream_active(dai, substream->stream) == 0) - snd_soc_dai_digital_mute(dai, 1, substream->stream); - } + /* Make sure DAI parameters cleared if the DAI becomes inactive */ + for_each_rtd_dais(rtd, i, dai) + if (snd_soc_dai_active(dai) == 0 && + (dai->rate || dai->channels || dai->sample_bits)) + soc_pcm_set_dai_params(dai, NULL); }
for_each_rtd_dais(rtd, i, dai) @@ -936,6 +934,15 @@ static int soc_pcm_hw_clean(struct snd_soc_pcm_runtime *rtd,
snd_soc_dpcm_mutex_assert_held(rtd);
+ /* clear the corresponding DAIs parameters when going to be inactive */ + for_each_rtd_dais(rtd, i, dai) { + if (snd_soc_dai_active(dai) == 1) + soc_pcm_set_dai_params(dai, NULL); + + if (snd_soc_dai_stream_active(dai, substream->stream) == 1) + snd_soc_dai_digital_mute(dai, 1, substream->stream); + } + /* run the stream event */ snd_soc_dapm_stream_stop(rtd, substream->stream);
On Wed, Sep 20, 2023 at 11:36:21PM +0800, Chancel Liu wrote:
The commit 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after stream_active is updated") tries to make sure DAI parameters can be cleared properly through moving the cleanup to the place where stream active status is updated. However, it will cause the cleanup only happening in soc_pcm_close().
Suppose a case: aplay -Dhw:0 44100.wav 48000.wav. The case calls soc_pcm_open()->soc_pcm_hw_params()->soc_pcm_hw_free()-> soc_pcm_hw_params()->soc_pcm_hw_free()->soc_pcm_close() in order. The parameters would be remained in the system even if the playback of 44100.wav is finished.
The case requires us clearing parameters in phase of soc_pcm_hw_free(). However, moving the DAI parameters cleanup back to soc_pcm_hw_free() has the risk that DAIs parameters never be cleared if there're more than one stream, see commit 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after stream_active is updated") for more details.
To meet all these requirements, in addition to do DAI parameters cleanup in soc_pcm_hw_free(), also check it in soc_pcm_close() to make sure DAI parameters cleared if the DAI becomes inactive.
Fixes: 1da681e52853 ("ASoC: soc-pcm.c: Clear DAIs parameters after stream_active is updated") Signed-off-by: Chancel Liu chancel.liu@nxp.com
For the record, this change incidentally also fixed the remaining click sounds I heard when stopping pulseaudio (e.g. on reboot) with the Lenovo ThinkPad X13s, which have also been discussed here:
https://lore.kernel.org/lkml/ZTukaxUhgY4WLgEs@hovoldconsulting.com/
Johan
On Wed, Sep 20, 2023 at 11:36:20PM +0800, Chancel Liu wrote:
It's reasonable that DAI parameters should be cleared if current DAI becomes inactive.
Only check DAI active status in soc_pcm_hw_free() is not enough since there's the risk that DAI parameters never be cleared if there're more than one stream [see A]. Only check DAI active status in soc_pcm_close() is also not enough since it will cause the cleanup just happening in soc_pcm_close() [see B].
Please don't send cover letters for single patches, if there is anything that needs saying put it in the changelog of the patch or after the --- if it's administrative stuff. This reduces mail volume and ensures that any important information is recorded in the changelog rather than being lost.
On Wed, 20 Sep 2023 23:36:20 +0800, Chancel Liu wrote:
It's reasonable that DAI parameters should be cleared if current DAI becomes inactive.
Only check DAI active status in soc_pcm_hw_free() is not enough since there's the risk that DAI parameters never be cleared if there're more than one stream [see A]. Only check DAI active status in soc_pcm_close() is also not enough since it will cause the cleanup just happening in soc_pcm_close() [see B].
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: soc-pcm.c: Make sure DAI parameters cleared if the DAI becomes inactive commit: 3efcb471f871cc095841d411f98c593228ecbac6
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Chancel Liu -
Johan Hovold -
Mark Brown