| Message ID | 20231128165638.757665-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Accepted |
| Commit | d32bac9cb09cce4dc3131ec5d0b6ba3c277502ac |
| Headers | show |
| Series | [1/2] ASoC: qcom: Add helper for allocating Soundwire stream runtime | expand |
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>
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
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);
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(-)