[PATCH 1/2] ASoC: qcom: Add helper for allocating Soundwire stream runtime

[PATCH 1/2] ASoC: qcom: Add helper for allocating Soundwire stream runtime

[Krzysztof Kozlowski]

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);
-- 
2.34.1

[PATCH 2/2] ASoC: qcom: Move Soundwire runtime stream alloc to soundcards

[Krzysztof Kozlowski]

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@linaro.org/
---
 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,
-- 
2.34.1

Re: [PATCH 2/2] ASoC: qcom: Move Soundwire runtime stream alloc to soundcards

[Krzysztof Kozlowski]

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@linaro.org/

... 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

Re: [PATCH 2/2] ASoC: qcom: Move Soundwire runtime stream alloc to soundcards

[Pierre-Louis Bossart]

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@linaro.org/
> 
> ... 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.

Re: [PATCH 2/2] ASoC: qcom: Move Soundwire runtime stream alloc to soundcards

[Pierre-Louis Bossart]

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@linaro.org/

LGTM

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Re: [PATCH 1/2] ASoC: qcom: Add helper for allocating Soundwire stream runtime

[Pierre-Louis Bossart]

> +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;
> +}

Re: [PATCH 1/2] ASoC: qcom: Add helper for allocating Soundwire stream runtime

[Krzysztof Kozlowski]

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

Re: [PATCH 1/2] ASoC: qcom: Add helper for allocating Soundwire stream runtime

[Pierre-Louis Bossart]

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'...

Re: [PATCH 1/2] ASoC: qcom: Add helper for allocating Soundwire stream runtime

[Mark Brown]

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