| Message ID | 20230323164403.6654-4-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: qcom: fixes for Click/Pop Noise | expand |
On Thu, Mar 23, 2023 at 04:44:02PM +0000, Srinivas Kandagatla wrote: > In the current setup the PA is left unmuted even when the > Soundwire ports are not started streaming. This can lead to click > and pop sounds during start. > There is a same issue in the reverse order where in the PA is > left unmute even after the data stream is stopped, the time > between data stream stopping and port closing is long enough > to accumulate DC on the line resulting in Click/Pop noise > during end of stream. Wow, that hardware sounds *super* fragile. > Moving the mute/unmute to trigger stop/start respectively seems to > help a lot with this Click/Pop issues reported on this Codec. > +static int wsa883x_trigger(struct snd_pcm_substream *s, int cmd, > + struct snd_soc_dai *dai) > +{ > + switch (cmd) { > + case SNDRV_PCM_TRIGGER_START: > + case SNDRV_PCM_TRIGGER_RESUME: > + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > + wsa883x_digital_mute(dai, false, 0); > + break; > static const struct snd_soc_dai_ops wsa883x_dai_ops = { > + .startup = wsa883x_startup, > .hw_params = wsa883x_hw_params, > .hw_free = wsa883x_hw_free, > - .mute_stream = wsa883x_digital_mute, > + .trigger = wsa883x_trigger, The trigger is run in atomic context, can you really write safely to a SoundWire device there? This feels like we should be doing it at the framework level, either tightening up where the mute happens in general or having some option that devices can select if they really need it.
>> +static int wsa883x_trigger(struct snd_pcm_substream *s, int cmd, >> + struct snd_soc_dai *dai) >> +{ >> + switch (cmd) { >> + case SNDRV_PCM_TRIGGER_START: >> + case SNDRV_PCM_TRIGGER_RESUME: >> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: >> + wsa883x_digital_mute(dai, false, 0); >> + break; > >> static const struct snd_soc_dai_ops wsa883x_dai_ops = { >> + .startup = wsa883x_startup, >> .hw_params = wsa883x_hw_params, >> .hw_free = wsa883x_hw_free, >> - .mute_stream = wsa883x_digital_mute, >> + .trigger = wsa883x_trigger, > > The trigger is run in atomic context, can you really write safely to a > SoundWire device there? Mark, I've seen that comment from you several times, and I wonder if I am missing something: the triggers for SoundWire managers and dailinks are typically nonatomic - at least for the Cadence-based solution the trigger is based on a bank switch that may happen with a delay and with a wait_for_completion(). Sending a command over the SoundWire channel is also typically not atomic, there's usually a wait_for_completion() as well.
On Thu, Mar 23, 2023 at 01:11:11PM -0500, Pierre-Louis Bossart wrote: > > The trigger is run in atomic context, can you really write safely to a > > SoundWire device there? > Mark, I've seen that comment from you several times, and I wonder if I > am missing something: the triggers for SoundWire managers and dailinks > are typically nonatomic - at least for the Cadence-based solution the > trigger is based on a bank switch that may happen with a delay and with > a wait_for_completion(). Sending a command over the SoundWire channel is > also typically not atomic, there's usually a wait_for_completion() as well. Ah, you're setting the nonatomic flag on your links to disable the locking. The default for trigger operations is to run them with local interrupts disabled. It looks like at least some of the Qualcomm stuff does that too.
On 24/03/2023 00:14, Mark Brown wrote: > On Thu, Mar 23, 2023 at 01:11:11PM -0500, Pierre-Louis Bossart wrote: > >>> The trigger is run in atomic context, can you really write safely to a >>> SoundWire device there? > >> Mark, I've seen that comment from you several times, and I wonder if I >> am missing something: the triggers for SoundWire managers and dailinks >> are typically nonatomic - at least for the Cadence-based solution the >> trigger is based on a bank switch that may happen with a delay and with >> a wait_for_completion(). Sending a command over the SoundWire channel is >> also typically not atomic, there's usually a wait_for_completion() as well. > > Ah, you're setting the nonatomic flag on your links to disable the > locking. The default for trigger operations is to run them with local > interrupts disabled. It looks like at least some of the Qualcomm stuff > does that too. Yes, by default dailinks are marked as nonatomic in Qualcomm case aswell. --srini
On 23/03/2023 17:07, Mark Brown wrote: > On Thu, Mar 23, 2023 at 04:44:02PM +0000, Srinivas Kandagatla wrote: >> In the current setup the PA is left unmuted even when the >> Soundwire ports are not started streaming. This can lead to click >> and pop sounds during start. >> There is a same issue in the reverse order where in the PA is >> left unmute even after the data stream is stopped, the time >> between data stream stopping and port closing is long enough >> to accumulate DC on the line resulting in Click/Pop noise >> during end of stream. > > Wow, that hardware sounds *super* fragile. > >> Moving the mute/unmute to trigger stop/start respectively seems to >> help a lot with this Click/Pop issues reported on this Codec. > >> +static int wsa883x_trigger(struct snd_pcm_substream *s, int cmd, >> + struct snd_soc_dai *dai) >> +{ >> + switch (cmd) { >> + case SNDRV_PCM_TRIGGER_START: >> + case SNDRV_PCM_TRIGGER_RESUME: >> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: >> + wsa883x_digital_mute(dai, false, 0); >> + break; > >> static const struct snd_soc_dai_ops wsa883x_dai_ops = { >> + .startup = wsa883x_startup, >> .hw_params = wsa883x_hw_params, >> .hw_free = wsa883x_hw_free, >> - .mute_stream = wsa883x_digital_mute, >> + .trigger = wsa883x_trigger, > > The trigger is run in atomic context, can you really write safely to a > SoundWire device there? > > This feels like we should be doing it at the framework level, either > tightening up where the mute happens in general or having some option > that devices can select if they really need it. That makes more sense, I can give that a try. --srini
diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c index c609cb63dae6..b83b5b0d4bab 100644 --- a/sound/soc/codecs/wsa883x.c +++ b/sound/soc/codecs/wsa883x.c @@ -1204,9 +1204,6 @@ static int wsa883x_spkr_event(struct snd_soc_dapm_widget *w, break; } - snd_soc_component_write_field(component, WSA883X_DRE_CTL_1, - WSA883X_DRE_GAIN_EN_MASK, - WSA883X_DRE_GAIN_FROM_CSR); if (wsa883x->port_enable[WSA883X_PORT_COMP]) snd_soc_component_write_field(component, WSA883X_DRE_CTL_0, WSA883X_DRE_OFFSET_MASK, @@ -1219,9 +1216,6 @@ static int wsa883x_spkr_event(struct snd_soc_dapm_widget *w, snd_soc_component_write_field(component, WSA883X_PDM_WD_CTL, WSA883X_PDM_EN_MASK, WSA883X_PDM_ENABLE); - snd_soc_component_write_field(component, WSA883X_PA_FSM_CTL, - WSA883X_GLOBAL_PA_EN_MASK, - WSA883X_GLOBAL_PA_ENABLE); break; case SND_SOC_DAPM_PRE_PMD: @@ -1341,10 +1335,38 @@ static int wsa883x_digital_mute(struct snd_soc_dai *dai, int mute, int stream) return 0; } +static int wsa883x_trigger(struct snd_pcm_substream *s, int cmd, + struct snd_soc_dai *dai) +{ + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_RESUME: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + wsa883x_digital_mute(dai, false, 0); + break; + case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + wsa883x_digital_mute(dai, true, 0); + break; + default: + break; + } + + return 0; +} + +static int wsa883x_startup(struct snd_pcm_substream *stream, + struct snd_soc_dai *dai) +{ + return wsa883x_digital_mute(dai, true, 0); +} + static const struct snd_soc_dai_ops wsa883x_dai_ops = { + .startup = wsa883x_startup, .hw_params = wsa883x_hw_params, .hw_free = wsa883x_hw_free, - .mute_stream = wsa883x_digital_mute, + .trigger = wsa883x_trigger, .set_stream = wsa883x_set_sdw_stream, };
In the current setup the PA is left unmuted even when the Soundwire ports are not started streaming. This can lead to click and pop sounds during start. There is a same issue in the reverse order where in the PA is left unmute even after the data stream is stopped, the time between data stream stopping and port closing is long enough to accumulate DC on the line resulting in Click/Pop noise during end of stream. Moving the mute/unmute to trigger stop/start respectively seems to help a lot with this Click/Pop issues reported on this Codec. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- sound/soc/codecs/wsa883x.c | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-)