[PATCH 0/2] ASoC: sdm845: fix soundwire stream handling
Recent addition of SoundWire stream state-machine checks in linux-next have shown an existing issue with handling soundwire streams in codec drivers.
In general soundwire stream prepare/enable/disable can be called from either codec/machine/controller driver. However calling it in codec driver means that if multiple instances(Left/Right speakers) of the same codec is connected to the same stream then it will endup calling stream prepare/enable/disable more than once. This will mess up the stream state-machine checks in the soundwire core.
Moving this stream handling to machine driver would fix this issue and also allow board/platform specfic power sequencing.
Srinivas Kandagatla (2): ASoC: qcom: sdm845: handle soundwire stream ASoC: codecs: wsa881x: remove soundwire stream handling
sound/soc/codecs/wsa881x.c | 44 +----------------------- sound/soc/qcom/Kconfig | 2 +- sound/soc/qcom/sdm845.c | 69 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 44 deletions(-)
In existing setup WSA881x codec handles soundwire stream, however DB845c and other machines based on SDM845c have 2 instances for WSA881x codec. This will force soundwire stream to be prepared/enabled twice or multiple times. Handling SoundWire Stream in machine driver would fix this issue.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/qcom/Kconfig | 2 +- sound/soc/qcom/sdm845.c | 69 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig index 6530d2462a9e..f51b28d1b94d 100644 --- a/sound/soc/qcom/Kconfig +++ b/sound/soc/qcom/Kconfig @@ -99,7 +99,7 @@ config SND_SOC_MSM8996
config SND_SOC_SDM845 tristate "SoC Machine driver for SDM845 boards" - depends on QCOM_APR && CROS_EC && I2C + depends on QCOM_APR && CROS_EC && I2C && SOUNDWIRE select SND_SOC_QDSP6 select SND_SOC_QCOM_COMMON select SND_SOC_RT5663 diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c index 3ac02204a706..82ec71a1e3bd 100644 --- a/sound/soc/qcom/sdm845.c +++ b/sound/soc/qcom/sdm845.c @@ -11,6 +11,7 @@ #include <sound/pcm_params.h> #include <sound/jack.h> #include <sound/soc.h> +#include <linux/soundwire/sdw.h> #include <uapi/linux/input-event-codes.h> #include "common.h" #include "qdsp6/q6afe.h" @@ -31,10 +32,12 @@ struct sdm845_snd_data { struct snd_soc_jack jack; bool jack_setup; + bool stream_prepared[SLIM_MAX_RX_PORTS]; struct snd_soc_card *card; uint32_t pri_mi2s_clk_count; uint32_t sec_mi2s_clk_count; uint32_t quat_tdm_clk_count; + struct sdw_stream_runtime *sruntime[SLIM_MAX_RX_PORTS]; };
static unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28}; @@ -45,11 +48,20 @@ static int sdm845_slim_snd_hw_params(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dai *codec_dai; + struct sdm845_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); u32 rx_ch[SLIM_MAX_RX_PORTS], tx_ch[SLIM_MAX_TX_PORTS]; + struct sdw_stream_runtime *sruntime; u32 rx_ch_cnt = 0, tx_ch_cnt = 0; int ret = 0, i;
for_each_rtd_codec_dais(rtd, i, codec_dai) { + sruntime = snd_soc_dai_get_sdw_stream(codec_dai, + substream->stream); + if (sruntime != ERR_PTR(-ENOTSUPP)) + pdata->sruntime[cpu_dai->id] = sruntime; + else + pdata->sruntime[cpu_dai->id] = NULL; + ret = snd_soc_dai_get_channel_map(codec_dai, &tx_ch_cnt, tx_ch, &rx_ch_cnt, rx_ch);
@@ -425,8 +437,65 @@ static void sdm845_snd_shutdown(struct snd_pcm_substream *substream) } }
+static int sdm845_snd_prepare(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct sdm845_snd_data *data = snd_soc_card_get_drvdata(rtd->card); + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id]; + int ret; + + if (!sruntime) + return 0; + + if (data->stream_prepared[cpu_dai->id]) { + sdw_disable_stream(sruntime); + sdw_deprepare_stream(sruntime); + data->stream_prepared[cpu_dai->id] = false; + } + + ret = sdw_prepare_stream(sruntime); + if (ret) + return ret; + + /** + * NOTE: there is a strict hw requirement about the ordering of port + * enables and actual WSA881x PA enable. PA enable should only happen + * after soundwire ports are enabled if not DC on the line is + * accumulated resulting in Click/Pop Noise + * PA enable/mute are handled as part of codec DAPM and digital mute. + */ + + ret = sdw_enable_stream(sruntime); + if (ret) { + sdw_deprepare_stream(sruntime); + return ret; + } + data->stream_prepared[cpu_dai->id] = true; + + return ret; +} + +static int sdm845_snd_hw_free(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct sdm845_snd_data *data = snd_soc_card_get_drvdata(rtd->card); + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id]; + + if (sruntime && data->stream_prepared[cpu_dai->id]) { + sdw_disable_stream(sruntime); + sdw_deprepare_stream(sruntime); + data->stream_prepared[cpu_dai->id] = false; + } + + return 0; +} + static const struct snd_soc_ops sdm845_be_ops = { .hw_params = sdm845_snd_hw_params, + .hw_free = sdm845_snd_hw_free, + .prepare = sdm845_snd_prepare, .startup = sdm845_snd_startup, .shutdown = sdm845_snd_shutdown, };
@@ -45,11 +48,20 @@ static int sdm845_slim_snd_hw_params(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dai *codec_dai;
struct sdm845_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); u32 rx_ch[SLIM_MAX_RX_PORTS], tx_ch[SLIM_MAX_TX_PORTS];
struct sdw_stream_runtime *sruntime; u32 rx_ch_cnt = 0, tx_ch_cnt = 0; int ret = 0, i;
for_each_rtd_codec_dais(rtd, i, codec_dai) {
sruntime = snd_soc_dai_get_sdw_stream(codec_dai,substream->stream);if (sruntime != ERR_PTR(-ENOTSUPP))pdata->sruntime[cpu_dai->id] = sruntime;elsepdata->sruntime[cpu_dai->id] = NULL;
Can you explain this part? The get_sdw_stream() is supposed to return what was set by set_sdw_stream(), so if it's not supported isn't this an error?
ret = snd_soc_dai_get_channel_map(codec_dai, &tx_ch_cnt, tx_ch, &rx_ch_cnt, rx_ch);@@ -425,8 +437,65 @@ static void sdm845_snd_shutdown(struct snd_pcm_substream *substream) } }
+static int sdm845_snd_prepare(struct snd_pcm_substream *substream) +{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct sdm845_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
- struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
- struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id];
- int ret;
- if (!sruntime)
return 0;
same here, isn't this an error?
- if (data->stream_prepared[cpu_dai->id]) {
sdw_disable_stream(sruntime);sdw_deprepare_stream(sruntime);data->stream_prepared[cpu_dai->id] = false;- }
- ret = sdw_prepare_stream(sruntime);
- if (ret)
return ret;- /**
* NOTE: there is a strict hw requirement about the ordering of port* enables and actual WSA881x PA enable. PA enable should only happen* after soundwire ports are enabled if not DC on the line is* accumulated resulting in Click/Pop Noise* PA enable/mute are handled as part of codec DAPM and digital mute.*/- ret = sdw_enable_stream(sruntime);
- if (ret) {
sdw_deprepare_stream(sruntime);return ret;- }
- data->stream_prepared[cpu_dai->id] = true;
- return ret;
+}
+static int sdm845_snd_hw_free(struct snd_pcm_substream *substream) +{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct sdm845_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
- struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
- struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id];
- if (sruntime && data->stream_prepared[cpu_dai->id]) {
and here?
Really wondering where the stream is actually allocated and set.
sdw_disable_stream(sruntime);sdw_deprepare_stream(sruntime);data->stream_prepared[cpu_dai->id] = false;- }
- return 0;
+}
- static const struct snd_soc_ops sdm845_be_ops = { .hw_params = sdm845_snd_hw_params,
- .hw_free = sdm845_snd_hw_free,
- .prepare = sdm845_snd_prepare, .startup = sdm845_snd_startup, .shutdown = sdm845_snd_shutdown, };
On 17/03/2020 13:07, Pierre-Louis Bossart wrote:
@@ -45,11 +48,20 @@ static int sdm845_slim_snd_hw_params(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dai *codec_dai; + struct sdm845_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); u32 rx_ch[SLIM_MAX_RX_PORTS], tx_ch[SLIM_MAX_TX_PORTS]; + struct sdw_stream_runtime *sruntime; u32 rx_ch_cnt = 0, tx_ch_cnt = 0; int ret = 0, i; for_each_rtd_codec_dais(rtd, i, codec_dai) { + sruntime = snd_soc_dai_get_sdw_stream(codec_dai, + substream->stream); + if (sruntime != ERR_PTR(-ENOTSUPP)) + pdata->sruntime[cpu_dai->id] = sruntime; + else + pdata->sruntime[cpu_dai->id] = NULL;
Can you explain this part? The get_sdw_stream() is supposed to return what was set by set_sdw_stream(), so if it's not supported isn't this an error?
In this case its not an error because we have totally 3 codecs in this path. One wcd934x Slimbus codec and two wsa881x Soundwire Codec.
wcd934x codec side will return ENOTSUPP which is not an error.
ret = snd_soc_dai_get_channel_map(codec_dai, &tx_ch_cnt, tx_ch, &rx_ch_cnt, rx_ch); @@ -425,8 +437,65 @@ static void sdm845_snd_shutdown(struct snd_pcm_substream *substream) } } +static int sdm845_snd_prepare(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct sdm845_snd_data *data = snd_soc_card_get_drvdata(rtd->card); + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id]; + int ret;
+ if (!sruntime) + return 0;
same here, isn't this an error?
These callbacks are used for other dais aswell in this case HDMI, Slimbus and Soundwire, so sruntime being null is not treated as error.
+ if (data->stream_prepared[cpu_dai->id]) { + sdw_disable_stream(sruntime); + sdw_deprepare_stream(sruntime); + data->stream_prepared[cpu_dai->id] = false; + }
+ ret = sdw_prepare_stream(sruntime); + if (ret) + return ret;
+ /** + * NOTE: there is a strict hw requirement about the ordering of port + * enables and actual WSA881x PA enable. PA enable should only happen + * after soundwire ports are enabled if not DC on the line is + * accumulated resulting in Click/Pop Noise + * PA enable/mute are handled as part of codec DAPM and digital mute. + */
+ ret = sdw_enable_stream(sruntime); + if (ret) { + sdw_deprepare_stream(sruntime); + return ret; + } + data->stream_prepared[cpu_dai->id] = true;
+ return ret; +}
+static int sdm845_snd_hw_free(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct sdm845_snd_data *data = snd_soc_card_get_drvdata(rtd->card); + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id];
+ if (sruntime && data->stream_prepared[cpu_dai->id]) {
and here?
Really wondering where the stream is actually allocated and set.
Controller is allocating the runtime in this case.
+ sdw_disable_stream(sruntime); + sdw_deprepare_stream(sruntime); + data->stream_prepared[cpu_dai->id] = false; + }
+ return 0; +}
static const struct snd_soc_ops sdm845_be_ops = { .hw_params = sdm845_snd_hw_params, + .hw_free = sdm845_snd_hw_free, + .prepare = sdm845_snd_prepare, .startup = sdm845_snd_startup, .shutdown = sdm845_snd_shutdown, };
for_each_rtd_codec_dais(rtd, i, codec_dai) { + sruntime = snd_soc_dai_get_sdw_stream(codec_dai, + substream->stream); + if (sruntime != ERR_PTR(-ENOTSUPP)) + pdata->sruntime[cpu_dai->id] = sruntime; + else + pdata->sruntime[cpu_dai->id] = NULL;
Can you explain this part? The get_sdw_stream() is supposed to return what was set by set_sdw_stream(), so if it's not supported isn't this an error?
In this case its not an error because we have totally 3 codecs in this path. One wcd934x Slimbus codec and two wsa881x Soundwire Codec.
wcd934x codec side will return ENOTSUPP which is not an error.
I must admit I am quite confused here. After reading your response, I thought this was a case of codec-to-codec dailink, but then you also have a notion of cpu_dai?
ret = snd_soc_dai_get_channel_map(codec_dai, &tx_ch_cnt, tx_ch, &rx_ch_cnt, rx_ch); @@ -425,8 +437,65 @@ static void sdm845_snd_shutdown(struct snd_pcm_substream *substream) } } +static int sdm845_snd_prepare(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct sdm845_snd_data *data = snd_soc_card_get_drvdata(rtd->card); + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct sdw_stream_runtime *sruntime = data->sruntime[cpu_dai->id]; + int ret;
+ if (!sruntime) + return 0;
same here, isn't this an error?
These callbacks are used for other dais aswell in this case HDMI, Slimbus and Soundwire, so sruntime being null is not treated as error.
Same comment, how does the notion of cpu_dai come in the picture for a SoundWire dailink? Would you mind listing what the components of the dailinks are?
On 18/03/2020 15:26, Pierre-Louis Bossart wrote:
Same comment, how does the notion of cpu_dai come in the picture for a SoundWire dailink? Would you mind listing what the components of the dailinks are?
dais that I was referring here are all codec dais from backend-dai.
Device tree entries from https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arc...
Frontend-dai: mm1-dai-link { link-name = "MultiMedia1"; cpu { sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA1>; }; };
Backend-dai: slim-dai-link { link-name = "SLIM Playback"; cpu { sound-dai = <&q6afedai SLIMBUS_0_RX>; };
platform { sound-dai = <&q6routing>; };
codec { sound-dai = <&left_spkr>, <&right_spkr>, <&swm 0>, <&wcd9340 0>; }; };
--srini
On 3/18/20 10:57 AM, Srinivas Kandagatla wrote:
On 18/03/2020 15:26, Pierre-Louis Bossart wrote:
Same comment, how does the notion of cpu_dai come in the picture for a SoundWire dailink? Would you mind listing what the components of the dailinks are?
dais that I was referring here are all codec dais from backend-dai.
Device tree entries from https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arc...
Frontend-dai: mm1-dai-link { link-name = "MultiMedia1"; cpu { sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA1>; }; };
Backend-dai: slim-dai-link { link-name = "SLIM Playback"; cpu { sound-dai = <&q6afedai SLIMBUS_0_RX>; };
platform { sound-dai = <&q6routing>; };
codec { sound-dai = <&left_spkr>, <&right_spkr>, <&swm 0>, <&wcd9340 0>; };
Thanks, I didn't realize this and now understand your point.
I guess that means we've officially stretched the limits of the DPCM model though, lumping all codec dais from separate devices into the same 'backend' doesn't seem like a very good path forward, we'd really need a notion of domain to represent such bridges.
For now for the series
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
There could be multiple instances of this codec on any platform, so handling stream directly in this codec driver can lead to multiple calls to prepare/enable/disable on the same SoundWire stream. Move this stream handling to machine driver to fix this issue.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/codecs/wsa881x.c | 44 +------------------------------------- 1 file changed, 1 insertion(+), 43 deletions(-)
diff --git a/sound/soc/codecs/wsa881x.c b/sound/soc/codecs/wsa881x.c index 1810e0775efe..d39d479e2378 100644 --- a/sound/soc/codecs/wsa881x.c +++ b/sound/soc/codecs/wsa881x.c @@ -680,7 +680,6 @@ struct wsa881x_priv { int active_ports; bool port_prepared[WSA881X_MAX_SWR_PORTS]; bool port_enable[WSA881X_MAX_SWR_PORTS]; - bool stream_prepared; };
static void wsa881x_init(struct wsa881x_priv *wsa881x) @@ -958,41 +957,6 @@ static const struct snd_soc_dapm_widget wsa881x_dapm_widgets[] = { SND_SOC_DAPM_OUTPUT("SPKR"), };
-static int wsa881x_prepare(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) -{ - struct wsa881x_priv *wsa881x = dev_get_drvdata(dai->dev); - int ret; - - if (wsa881x->stream_prepared) { - sdw_disable_stream(wsa881x->sruntime); - sdw_deprepare_stream(wsa881x->sruntime); - wsa881x->stream_prepared = false; - } - - - ret = sdw_prepare_stream(wsa881x->sruntime); - if (ret) - return ret; - - /** - * NOTE: there is a strict hw requirement about the ordering of port - * enables and actual PA enable. PA enable should only happen after - * soundwire ports are enabled if not DC on the line is accumulated - * resulting in Click/Pop Noise - * PA enable/mute are handled as part of DAPM and digital mute. - */ - - ret = sdw_enable_stream(wsa881x->sruntime); - if (ret) { - sdw_deprepare_stream(wsa881x->sruntime); - return ret; - } - wsa881x->stream_prepared = true; - - return ret; -} - static int wsa881x_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) @@ -1020,12 +984,7 @@ static int wsa881x_hw_free(struct snd_pcm_substream *substream, { struct wsa881x_priv *wsa881x = dev_get_drvdata(dai->dev);
- if (wsa881x->stream_prepared) { - sdw_disable_stream(wsa881x->sruntime); - sdw_deprepare_stream(wsa881x->sruntime); - sdw_stream_remove_slave(wsa881x->slave, wsa881x->sruntime); - wsa881x->stream_prepared = false; - } + sdw_stream_remove_slave(wsa881x->slave, wsa881x->sruntime);
return 0; } @@ -1056,7 +1015,6 @@ static int wsa881x_digital_mute(struct snd_soc_dai *dai, int mute, int stream)
static struct snd_soc_dai_ops wsa881x_dai_ops = { .hw_params = wsa881x_hw_params, - .prepare = wsa881x_prepare, .hw_free = wsa881x_hw_free, .mute_stream = wsa881x_digital_mute, .set_sdw_stream = wsa881x_set_sdw_stream,
Hi Srinivas,
On 3/17/20 4:53 AM, Srinivas Kandagatla wrote:
Recent addition of SoundWire stream state-machine checks in linux-next have shown an existing issue with handling soundwire streams in codec drivers.
In general soundwire stream prepare/enable/disable can be called from either codec/machine/controller driver. However calling it in codec driver means that if multiple instances(Left/Right speakers) of the same codec is connected to the same stream then it will endup calling stream prepare/enable/disable more than once. This will mess up the stream state-machine checks in the soundwire core.
That's a known issue that we've fixed on the Intel side a month or two ago. Unfortunately the review cycle is so slow that you don't benefit immediately from our fixes, what can I say.
Moving this stream handling to machine driver would fix this issue and also allow board/platform specfic power sequencing.
It's fine but that's unnecessary, and if you start having multiple machine drivers with the same codecs you'll duplicate the stream handling code.
All you need to ensure in a multi-codec or multi-cpu dai case is that the stream is allocated once, and yes that's typically done as part of the dailink .startup callback.
Then it's fine to call the stream prepare or enable multiple times from the individual dai level, and only do the transition for the first call.
See patch "soundwire: stream: only change state if needed" that I just shared. We've used it both in 'TDM' mode with two codecs hanging off of the same link and in 'aggregated' mode with two codecs on separate links.
Srinivas Kandagatla (2): ASoC: qcom: sdm845: handle soundwire stream ASoC: codecs: wsa881x: remove soundwire stream handling
sound/soc/codecs/wsa881x.c | 44 +----------------------- sound/soc/qcom/Kconfig | 2 +- sound/soc/qcom/sdm845.c | 69 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 44 deletions(-)
participants (2)
-
Pierre-Louis Bossart -
Srinivas Kandagatla