| Message ID | 20231025144601.268645-3-krzysztof.kozlowski@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] soundwire: qcom: drop unneeded DAI .set_stream callback | expand |
On 25/10/2023 16:46, Krzysztof Kozlowski wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > If multiple WSA8840 speakers, from two separate Soundwire buses, are > used in one codec DAI link, the set_stream() should ignore calls for > setting stream from other Soundwire controller. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- I should have add a cover letter and explain that this patch depends on previous (2/3). Best regards, Krzysztof
On 10/25/23 09:46, Krzysztof Kozlowski wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > If multiple WSA8840 speakers, from two separate Soundwire buses, are > used in one codec DAI link, the set_stream() should ignore calls for > setting stream from other Soundwire controller. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > sound/soc/codecs/wsa884x.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c > index bee6e763c700..91205e8c96f1 100644 > --- a/sound/soc/codecs/wsa884x.c > +++ b/sound/soc/codecs/wsa884x.c > @@ -1775,6 +1775,12 @@ static int wsa884x_set_stream(struct snd_soc_dai *dai, > void *stream, int direction) > { > struct wsa884x_priv *wsa884x = dev_get_drvdata(dai->dev); > + struct sdw_stream_runtime *sruntime = stream; > + struct sdw_slave *sdw = dev_to_sdw_dev(dai->dev); > + > + /* Check if this belongs to same bus */ > + if (sdw->bus->dev != sruntime->dev) > + return 0; Sorry, maybe I am really thick or need coffee, but I can't figure out why this is necessary. Each amplifier has its own "wsa884x_priv" context and should have its own DAI, not following why the set_stream would mix-up the two dais? We've been using two buses for two amplifiers since CometLake (2019?) and I don't see what's different? > > wsa884x->sruntime = stream; >
On 25/10/2023 17:03, Pierre-Louis Bossart wrote: > > > On 10/25/23 09:46, Krzysztof Kozlowski wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> If multiple WSA8840 speakers, from two separate Soundwire buses, are >> used in one codec DAI link, the set_stream() should ignore calls for >> setting stream from other Soundwire controller. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> --- >> sound/soc/codecs/wsa884x.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c >> index bee6e763c700..91205e8c96f1 100644 >> --- a/sound/soc/codecs/wsa884x.c >> +++ b/sound/soc/codecs/wsa884x.c >> @@ -1775,6 +1775,12 @@ static int wsa884x_set_stream(struct snd_soc_dai *dai, >> void *stream, int direction) >> { >> struct wsa884x_priv *wsa884x = dev_get_drvdata(dai->dev); >> + struct sdw_stream_runtime *sruntime = stream; >> + struct sdw_slave *sdw = dev_to_sdw_dev(dai->dev); >> + >> + /* Check if this belongs to same bus */ >> + if (sdw->bus->dev != sruntime->dev) >> + return 0; > > Sorry, maybe I am really thick or need coffee, but I can't figure out > why this is necessary. Each amplifier has its own "wsa884x_priv" context > and should have its own DAI, not following why the set_stream would > mix-up the two dais? > > We've been using two buses for two amplifiers since CometLake (2019?) > and I don't see what's different? Let me start with some hardware representation in DTS. We have two Soundwire controllers swr0 and swr3, each with two WSA884x speakers (codecs): ------------- &swr0 { status = "okay"; left_woofer: speaker@0,0 { compatible = "sdw20217020400"; reg = <0 0>; // ... }; /* WSA8845, Left Tweeter */ left_tweeter: speaker@0,1 { compatible = "sdw20217020400"; reg = <0 1>; // ... }; }; &swr3 { status = "okay"; /* WSA8845, Right Woofer */ right_woofer: speaker@0,0 { compatible = "sdw20217020400"; reg = <0 0>; // ... }; /* WSA8845, Right Tweeter */ right_tweeter: speaker@0,1 { compatible = "sdw20217020400"; reg = <0 1>; // ... }; }; ------------- Now, for four-speaker playback, we have sound card with links like: ------------- wsa-dai-link { link-name = "WSA Playback"; cpu { sound-dai = <&q6apmbedai WSA_CODEC_DMA_RX_0>; }; codec { sound-dai = <&left_woofer>, <&left_tweeter>, <&swr0 0>, <&lpass_wsamacro 0>, <&right_woofer>, <&right_tweeter>, <&swr3 0>, <&lpass_wsa2macro 0>; }; platform { sound-dai = <&q6apm>; }; }; ------------- We need to prepare the stream for all four speakers and two soundwire controllers (so two different soundwire buses), however without the patches here, the stream (sdw_stream_runtime *sruntime) right woofer/twitter is set to swr0 (the other bus!). But it should stay as swr3 (their bus). Does it help a bit? I hope I am able to properly explain it. Thanks for your feedback and review! Best regards, Krzysztof
> We have two Soundwire controllers swr0 and swr3, each with two WSA884x > speakers (codecs): > > ------------- > &swr0 { > status = "okay"; > > left_woofer: speaker@0,0 { > compatible = "sdw20217020400"; > reg = <0 0>; > // ... > }; > > /* WSA8845, Left Tweeter */ > left_tweeter: speaker@0,1 { > compatible = "sdw20217020400"; > reg = <0 1>; > // ... > }; > }; > > &swr3 { > status = "okay"; > > /* WSA8845, Right Woofer */ > right_woofer: speaker@0,0 { > compatible = "sdw20217020400"; > reg = <0 0>; > // ... > }; > > /* WSA8845, Right Tweeter */ > right_tweeter: speaker@0,1 { > compatible = "sdw20217020400"; > reg = <0 1>; > // ... > }; > }; > ------------- > > Now, for four-speaker playback, we have sound card with links like: > > ------------- > wsa-dai-link { > link-name = "WSA Playback"; > > cpu { > sound-dai = <&q6apmbedai WSA_CODEC_DMA_RX_0>; > }; > > codec { > sound-dai = <&left_woofer>, <&left_tweeter>, > <&swr0 0>, <&lpass_wsamacro 0>, > <&right_woofer>, <&right_tweeter>, > <&swr3 0>, <&lpass_wsa2macro 0>; > }; > > platform { > sound-dai = <&q6apm>; > }; > }; > ------------- > > We need to prepare the stream for all four speakers and two soundwire > controllers (so two different soundwire buses), however without the > patches here, the stream (sdw_stream_runtime *sruntime) right > woofer/twitter is set to swr0 (the other bus!). But it should stay as > swr3 (their bus). > > Does it help a bit? I hope I am able to properly explain it. The configuration seems fine, but the problem is the "sdw_stream_runtime" definition. You need *ONE* sdw_stream_runtime, and multiple m_rt contexts added in the linked lists of this sdw_stream_runtime. In other words, you need to call sdw_stream_add_master() twice, for swr0 and swr3 respectively. Put differently, a sdw_stream_runtime does not point to a specific bus, it provides a top-level structure which can use multiple buses. The best way to allocate the sdw_stream_runtime is to rely on the dailink .startup callback. From the description above that's where you have all the needed information, and then each DAI .startup (or hw_params) can call sdw_stream_add_master() to update the linked lists.
diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c index bee6e763c700..91205e8c96f1 100644 --- a/sound/soc/codecs/wsa884x.c +++ b/sound/soc/codecs/wsa884x.c @@ -1775,6 +1775,12 @@ static int wsa884x_set_stream(struct snd_soc_dai *dai, void *stream, int direction) { struct wsa884x_priv *wsa884x = dev_get_drvdata(dai->dev); + struct sdw_stream_runtime *sruntime = stream; + struct sdw_slave *sdw = dev_to_sdw_dev(dai->dev); + + /* Check if this belongs to same bus */ + if (sdw->bus->dev != sruntime->dev) + return 0; wsa884x->sruntime = stream;