[PATCH 1/2] ASoC: qcom: Add helper for allocating Soundwire stream runtime
Newer Qualcomm SoC soundcards will need to allocate Soundwire stream runtime in their startup op. The code will be exactly the same for all soundcards, so add a helper for that.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- sound/soc/qcom/sdw.c | 45 +++++++++++++++++++++++++++++++++++++++++++- sound/soc/qcom/sdw.h | 1 + 2 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/sound/soc/qcom/sdw.c b/sound/soc/qcom/sdw.c index dd275123d31d..77dbe0c28b29 100644 --- a/sound/soc/qcom/sdw.c +++ b/sound/soc/qcom/sdw.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 -// Copyright (c) 2018, Linaro Limited. +// Copyright (c) 2018-2023, Linaro Limited. // Copyright (c) 2018, The Linux Foundation. All rights reserved.
#include <dt-bindings/sound/qcom,q6afe.h> @@ -7,6 +7,49 @@ #include <sound/soc.h> #include "sdw.h"
+/** + * qcom_snd_sdw_startup() - Helper to start Soundwire stream for SoC audio card + * @substream: The PCM substream from audio, as passed to snd_soc_ops->startup() + * + * Helper for the SoC audio card (snd_soc_ops->startup()) to allocate and set + * Soundwire stream runtime to each codec DAI. + * + * The shutdown() callback should call sdw_release_stream() on the same + * sdw_stream_runtime. + * + * Return: 0 or errno + */ +int qcom_snd_sdw_startup(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); + struct sdw_stream_runtime *sruntime; + struct snd_soc_dai *codec_dai; + int ret, i; + + sruntime = sdw_alloc_stream(cpu_dai->name); + if (!sruntime) + return -ENOMEM; + + for_each_rtd_codec_dais(rtd, i, codec_dai) { + ret = snd_soc_dai_set_stream(codec_dai, sruntime, + substream->stream); + if (ret < 0 && ret != -ENOTSUPP) { + dev_err(rtd->dev, "Failed to set sdw stream on %s\n", + codec_dai->name); + goto err_set_stream; + } + } + + return 0; + +err_set_stream: + sdw_release_stream(sruntime); + + return ret; +} +EXPORT_SYMBOL_GPL(qcom_snd_sdw_startup); + int qcom_snd_sdw_prepare(struct snd_pcm_substream *substream, struct sdw_stream_runtime *sruntime, bool *stream_prepared) diff --git a/sound/soc/qcom/sdw.h b/sound/soc/qcom/sdw.h index d74cbb84da13..392e3455f1b1 100644 --- a/sound/soc/qcom/sdw.h +++ b/sound/soc/qcom/sdw.h @@ -6,6 +6,7 @@
#include <linux/soundwire/sdw.h>
+int qcom_snd_sdw_startup(struct snd_pcm_substream *substream); int qcom_snd_sdw_prepare(struct snd_pcm_substream *substream, struct sdw_stream_runtime *runtime, bool *stream_prepared);
Currently the Qualcomm Soundwire controller in its DAI startup op allocates the Soundwire stream runtime. This works fine for existing designs, but has limitations for stream runtimes with multiple controllers, like upcoming Qualcomm X1E80100 SoC with four WSA8840 speakers on two Soundwire controllers.
When two Soundwire controllers are added to sound card codecs, Soundwire startup() is called twice, one for each Soundwire controller, and second execution overwrites what was set before. During shutdown() this causes double free.
It is expected to have only one Soundwire stream runtime, thus it should be allocated from SoC soundcard context startup(), not from each Soundwire startup(). Such way will properly handle both cases: one and two Soundwire controllers in the stream runtime.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
---
This is an entirely different approach than my previous try here: https://lore.kernel.org/all/20231025144601.268645-1-krzysztof.kozlowski@lina... --- drivers/soundwire/qcom.c | 33 +-------------------------------- sound/soc/qcom/sc8280xp.c | 13 +++++++++++++ sound/soc/qcom/sm8250.c | 15 ++++++++++++++- 3 files changed, 28 insertions(+), 33 deletions(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index f42c83c390ff..ac9176f714bf 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -1291,10 +1291,7 @@ static int qcom_swrm_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev); - struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct sdw_stream_runtime *sruntime; - struct snd_soc_dai *codec_dai; - int ret, i; + int ret;
ret = pm_runtime_get_sync(ctrl->dev); if (ret < 0 && ret != -EACCES) { @@ -1305,33 +1302,7 @@ static int qcom_swrm_startup(struct snd_pcm_substream *substream, return ret; }
- sruntime = sdw_alloc_stream(dai->name); - if (!sruntime) { - ret = -ENOMEM; - goto err_alloc; - } - - ctrl->sruntime[dai->id] = sruntime; - - for_each_rtd_codec_dais(rtd, i, codec_dai) { - ret = snd_soc_dai_set_stream(codec_dai, sruntime, - substream->stream); - if (ret < 0 && ret != -ENOTSUPP) { - dev_err(dai->dev, "Failed to set sdw stream on %s\n", - codec_dai->name); - goto err_set_stream; - } - } - return 0; - -err_set_stream: - sdw_release_stream(sruntime); -err_alloc: - pm_runtime_mark_last_busy(ctrl->dev); - pm_runtime_put_autosuspend(ctrl->dev); - - return ret; }
static void qcom_swrm_shutdown(struct snd_pcm_substream *substream, @@ -1340,8 +1311,6 @@ static void qcom_swrm_shutdown(struct snd_pcm_substream *substream, struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
swrm_wait_for_wr_fifo_done(ctrl); - sdw_release_stream(ctrl->sruntime[dai->id]); - ctrl->sruntime[dai->id] = NULL; pm_runtime_mark_last_busy(ctrl->dev); pm_runtime_put_autosuspend(ctrl->dev);
diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c index d93b18f07be5..7c20b25ba3de 100644 --- a/sound/soc/qcom/sc8280xp.c +++ b/sound/soc/qcom/sc8280xp.c @@ -31,6 +31,17 @@ static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd) return qcom_snd_wcd_jack_setup(rtd, &data->jack, &data->jack_setup); }
+static void sc8280xp_snd_shutdown(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); + struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); + struct sdw_stream_runtime *sruntime = pdata->sruntime[cpu_dai->id]; + + pdata->sruntime[cpu_dai->id] = NULL; + sdw_release_stream(sruntime); +} + static int sc8280xp_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd, struct snd_pcm_hw_params *params) { @@ -91,6 +102,8 @@ static int sc8280xp_snd_hw_free(struct snd_pcm_substream *substream) }
static const struct snd_soc_ops sc8280xp_be_ops = { + .startup = qcom_snd_sdw_startup, + .shutdown = sc8280xp_snd_shutdown, .hw_params = sc8280xp_snd_hw_params, .hw_free = sc8280xp_snd_hw_free, .prepare = sc8280xp_snd_prepare, diff --git a/sound/soc/qcom/sm8250.c b/sound/soc/qcom/sm8250.c index 9cc869fd70ac..f298167c2a23 100644 --- a/sound/soc/qcom/sm8250.c +++ b/sound/soc/qcom/sm8250.c @@ -66,7 +66,19 @@ static int sm8250_snd_startup(struct snd_pcm_substream *substream) default: break; } - return 0; + + return qcom_snd_sdw_startup(substream); +} + +static void sm2450_snd_shutdown(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); + struct sm8250_snd_data *data = snd_soc_card_get_drvdata(rtd->card); + struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id]; + + data->sruntime[cpu_dai->id] = NULL; + sdw_release_stream(sruntime); }
static int sm8250_snd_hw_params(struct snd_pcm_substream *substream, @@ -103,6 +115,7 @@ static int sm8250_snd_hw_free(struct snd_pcm_substream *substream)
static const struct snd_soc_ops sm8250_be_ops = { .startup = sm8250_snd_startup, + .shutdown = sm2450_snd_shutdown, .hw_params = sm8250_snd_hw_params, .hw_free = sm8250_snd_hw_free, .prepare = sm8250_snd_prepare,
On 28/11/2023 17:56, Krzysztof Kozlowski wrote:
Currently the Qualcomm Soundwire controller in its DAI startup op allocates the Soundwire stream runtime. This works fine for existing designs, but has limitations for stream runtimes with multiple controllers, like upcoming Qualcomm X1E80100 SoC with four WSA8840 speakers on two Soundwire controllers.
When two Soundwire controllers are added to sound card codecs, Soundwire startup() is called twice, one for each Soundwire controller, and second execution overwrites what was set before. During shutdown() this causes double free.
It is expected to have only one Soundwire stream runtime, thus it should be allocated from SoC soundcard context startup(), not from each Soundwire startup(). Such way will properly handle both cases: one and two Soundwire controllers in the stream runtime.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
This is an entirely different approach than my previous try here: https://lore.kernel.org/all/20231025144601.268645-1-krzysztof.kozlowski@lina...
... and I forgot to thank you Pierre-Louis for patient explanation of the case in my previous try. Your review was much appreciated!
Thank you.
Best regards, Krzysztof
On 11/28/23 10:59, Krzysztof Kozlowski wrote:
On 28/11/2023 17:56, Krzysztof Kozlowski wrote:
Currently the Qualcomm Soundwire controller in its DAI startup op allocates the Soundwire stream runtime. This works fine for existing designs, but has limitations for stream runtimes with multiple controllers, like upcoming Qualcomm X1E80100 SoC with four WSA8840 speakers on two Soundwire controllers.
When two Soundwire controllers are added to sound card codecs, Soundwire startup() is called twice, one for each Soundwire controller, and second execution overwrites what was set before. During shutdown() this causes double free.
It is expected to have only one Soundwire stream runtime, thus it should be allocated from SoC soundcard context startup(), not from each Soundwire startup(). Such way will properly handle both cases: one and two Soundwire controllers in the stream runtime.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
This is an entirely different approach than my previous try here: https://lore.kernel.org/all/20231025144601.268645-1-krzysztof.kozlowski@lina...
... and I forgot to thank you Pierre-Louis for patient explanation of the case in my previous try. Your review was much appreciated!
You're welcome. It's good if we have multiple platforms using the 'stream' concept in similar ways.
On 11/28/23 10:56, Krzysztof Kozlowski wrote:
Currently the Qualcomm Soundwire controller in its DAI startup op allocates the Soundwire stream runtime. This works fine for existing designs, but has limitations for stream runtimes with multiple controllers, like upcoming Qualcomm X1E80100 SoC with four WSA8840 speakers on two Soundwire controllers.
When two Soundwire controllers are added to sound card codecs, Soundwire startup() is called twice, one for each Soundwire controller, and second execution overwrites what was set before. During shutdown() this causes double free.
It is expected to have only one Soundwire stream runtime, thus it should be allocated from SoC soundcard context startup(), not from each Soundwire startup(). Such way will properly handle both cases: one and two Soundwire controllers in the stream runtime.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
This is an entirely different approach than my previous try here: https://lore.kernel.org/all/20231025144601.268645-1-krzysztof.kozlowski@lina...
LGTM
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
+int qcom_snd_sdw_startup(struct snd_pcm_substream *substream) +{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
- struct sdw_stream_runtime *sruntime;
- struct snd_soc_dai *codec_dai;
- int ret, i;
- sruntime = sdw_alloc_stream(cpu_dai->name);
- if (!sruntime)
return -ENOMEM;- for_each_rtd_codec_dais(rtd, i, codec_dai) {
ret = snd_soc_dai_set_stream(codec_dai, sruntime,substream->stream);if (ret < 0 && ret != -ENOTSUPP) {
I know this is existing code moved into a helper, but out of curiosity why is -ENOTSUPP ignored? Isn't this problematic?
dev_err(rtd->dev, "Failed to set sdw stream on %s\n",codec_dai->name);goto err_set_stream;}- }
Also should the CPU DAIs also be used to set the stream information? it's not clear to me why only the CODEC DAIs are used.
- return 0;
+err_set_stream:
- sdw_release_stream(sruntime);
- return ret;
+}
On 28/11/2023 18:39, Pierre-Louis Bossart wrote:
+int qcom_snd_sdw_startup(struct snd_pcm_substream *substream) +{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
- struct sdw_stream_runtime *sruntime;
- struct snd_soc_dai *codec_dai;
- int ret, i;
- sruntime = sdw_alloc_stream(cpu_dai->name);
- if (!sruntime)
return -ENOMEM;- for_each_rtd_codec_dais(rtd, i, codec_dai) {
ret = snd_soc_dai_set_stream(codec_dai, sruntime,substream->stream);if (ret < 0 && ret != -ENOTSUPP) {I know this is existing code moved into a helper, but out of curiosity why is -ENOTSUPP ignored? Isn't this problematic?
This is for all DAI links, so if some don't have set_stream callback, we assume it is not needed. For example few codecs do not need it because they are not on Soundwire bus at all and they don't care about the stream.
dev_err(rtd->dev, "Failed to set sdw stream on %s\n",codec_dai->name);goto err_set_stream;}- }
Also should the CPU DAIs also be used to set the stream information? it's not clear to me why only the CODEC DAIs are used.
I don't know, we never did. As you pointed out, I am just moving things around, so I don't really know the original intention.
Best regards, Krzysztof
On 11/29/23 10:35, Krzysztof Kozlowski wrote:
On 28/11/2023 18:39, Pierre-Louis Bossart wrote:
+int qcom_snd_sdw_startup(struct snd_pcm_substream *substream) +{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
- struct sdw_stream_runtime *sruntime;
- struct snd_soc_dai *codec_dai;
- int ret, i;
- sruntime = sdw_alloc_stream(cpu_dai->name);
- if (!sruntime)
return -ENOMEM;- for_each_rtd_codec_dais(rtd, i, codec_dai) {
ret = snd_soc_dai_set_stream(codec_dai, sruntime,substream->stream);if (ret < 0 && ret != -ENOTSUPP) {I know this is existing code moved into a helper, but out of curiosity why is -ENOTSUPP ignored? Isn't this problematic?
This is for all DAI links, so if some don't have set_stream callback, we assume it is not needed. For example few codecs do not need it because they are not on Soundwire bus at all and they don't care about the stream.
Humm, it was my understanding that the substream is mapped 1:1 with a dailink, so not sure how SoundWire and non-SoundWire DAIs could be part of the same dailink?
I am not saying this test is silly, just wondering if there is any case where this error code is returned. Worst-case it's always false but harmless.
dev_err(rtd->dev, "Failed to set sdw stream on %s\n",codec_dai->name);goto err_set_stream;}- }
Also should the CPU DAIs also be used to set the stream information? it's not clear to me why only the CODEC DAIs are used.
I don't know, we never did. As you pointed out, I am just moving things around, so I don't really know the original intention.
Fair enough, I've been in your shoes :-) Not always easy to grade 3+ yr code as 'miss', 'bug', 'optimization' or 'feature'...
On Tue, 28 Nov 2023 17:56:37 +0100, Krzysztof Kozlowski wrote:
Newer Qualcomm SoC soundcards will need to allocate Soundwire stream runtime in their startup op. The code will be exactly the same for all soundcards, so add a helper for that.
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/2] ASoC: qcom: Add helper for allocating Soundwire stream runtime commit: d32bac9cb09cce4dc3131ec5d0b6ba3c277502ac [2/2] ASoC: qcom: Move Soundwire runtime stream alloc to soundcards commit: 15c7fab0e0477d7d7185eac574ca43c15b59b015
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)
-
Krzysztof Kozlowski -
Mark Brown -
Pierre-Louis Bossart