| Message ID | 20200317095351.15582-2-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: sdm845: fix soundwire stream handling | expand |
> @@ -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? > 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/arch/arm64/boot/dts/qcom/sdm845-db845c.dts?h=next-20200318#n538 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/arch/arm64/boot/dts/qcom/sdm845-db845c.dts?h=next-20200318#n538 > > > > 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>
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, };
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(-)