ASoC: SOF: change trigger sequence to fix pop noise when stopping playback on sdw platforms #1897

RanderWang · 2020-03-16T06:25:37Z

Now the trigger sequence is set to SND_SOC_DPCM_TRIGGER_POST for
SOF. This means FE will be stopped before BE, so BE will consume
invalid data and this generates huge pop noise. Now set trigger
sequence to SND_SOC_DPCM_TRIGGER_PRE to let FE to be stopped after
BE to avoid this issue.

Thanks @YvonneYang2 Fully tested on Comet Lake for a few cycles. She didn't find any issue on Comet Lake U. On Comet Lake H, she found a clock stop issue when doing S3 stress test, but she can't reproduce it again. I think is not connect to this PR directly.

Fixes #1867 #1869 #1865 #1866

ClarexZhou · 2020-03-16T07:31:42Z

Error is much improved with PR#1897. But still has very light pop via scenario "pause aplay first then Ctl+c to release aplay". Can observe this pop sound when increase speaker volume to Max via PGA3.0 3
Tested with kernel sof-dev, commit: 0f90705 + PR #1897 and FW: https://github.com/thesofproject/sof/tree/cml-010-hot-fix, commit thesofproject/sof@0ed0dba + commit thesofproject/sof@f73332e
And with same kernel sof-dev, commit: 0f90705 + PR #1897 and FW: Master, commit: thesofproject/sof@afd9770,

lyakh · 2020-03-16T09:11:00Z

sound/soc/sof/pcm.c

+			fe->dai_link->trigger[SNDRV_PCM_STREAM_PLAYBACK] =
+				SND_SOC_DPCM_TRIGGER_PRE;
+			fe->dai_link->trigger[SNDRV_PCM_STREAM_CAPTURE] =
+				SND_SOC_DPCM_TRIGGER_PRE;


@RanderWang a question: this PR aims at reducing the "pop" noise for playback. Do we need to change capture too?

@lyakh I just restore the default behavior, please check (fd274c2). I will communicate with QA to get more info.

Will this be called multi times?

I confirmed with QA, they didn't find noise in wav data.

Will this be called multi times?

just like hw_params

@RanderWang just wondering why not do this in sof_link_load()?

oh I guess we dont know if this is connected to an ALH BE during link_load.

kv2019i · 2020-03-16T13:14:24Z

Good fine @RanderWang ! Because we have been switching the orders before, I'd want to have an explicit ack from @ranj063 that this is ok. We obviously need to do something due ot the many bugs, but let's still make sure we are fixing at the right place. Also FYI @tlauda I would expect the BE to run out of data (xrun) in case FE is stopped first, but not to play out garbage, but the tests do point to different behaviour. Please comment.

plbossart · 2020-03-16T13:48:17Z

I am also not comfortable with this change. We had a very long discussion on this in the context of HDaudio, and now we would revert the trigger order only for ALH - but what about SSP/DMIC which also use the GPDMA.
For playback I still think triggering the FE before the BE makes sense, I never really understood why for HDaudio we had to use a reverse order.

RanderWang · 2020-03-17T07:49:55Z

I am also not comfortable with this change. We had a very long discussion on this in the context of HDaudio, and now we would revert the trigger order only for ALH - but what about SSP/DMIC which also use the GPDMA.
For playback I still think triggering the FE before the BE makes sense, I never really understood why for HDaudio we had to use a reverse order.

Last week I just reverted the commit to use PRE sequence for all and @YvonneYang2 tested the patch. She found IPC timeout and DSP panic caused by HDMI, which uses link DMA. So it seems FW require POST sequence for link dma. @tlauda can you help to explain this ?

RanderWang · 2020-03-17T07:56:54Z

Good fine @RanderWang ! Because we have been switching the orders before, I'd want to have an explicit ack from @ranj063 that this is ok. We obviously need to do something due ot the many bugs, but let's still make sure we are fixing at the right place. Also FYI @tlauda I would expect the BE to run out of data (xrun) in case FE is stopped first, but not to play out garbage, but the tests do point to different behaviour. Please comment.

The pop noise should be caused by this case: the value of last data sample should be much bigger than zero and suddenly followed by zero-value data, so a sound like "bang"

plbossart · 2020-03-17T21:02:00Z

@tlauda @ranj063 please chime in. We are now in the unfortunate case where what is required for the HDaudio link breaks the GPDMA/ALH case. Please don't tell me we need to have a trigger order that depends on the endpoint :-(

ranj063 · 2020-03-17T22:33:20Z

@tlauda @ranj063 please chime in. We are now in the unfortunate case where what is required for the HDaudio link breaks the GPDMA/ALH case. Please don't tell me we need to have a trigger order that depends on the endpoint :-(

@plbossart unfortunately from the results it looks like that might indeed be the case.

But I have a suggestion. Instead of setting the trigger order to POST during topology loading, I think we should we should leave it as default during topology loading and only set the trigger order to POST in the BE hw_params() ioctl for HDA link.

plbossart · 2020-03-18T18:17:01Z

@ranj063 @tlauda rather than discussing where the order change is handled, can we please have clarity on why the HDaudio link DMA requires a specific order and SoundWire another?

That is the big question to me, and I am not willing to merge anything until we understand what the directions are. It seems like a firmware dependency that's not properly handled or documented

ranj063 · 2020-03-18T18:20:14Z

@ranj063 @tlauda rather than discussing where the order change is handled, can we please have clarity on why the HDaudio link DMA requires a specific order and SoundWire another?

That is the big question to me, and I am not willing to merge anything until we understand what the directions are. It seems like a firmware dependency that's not properly handled or documented

@plbossart I am just as much in the dark about this as you are. I was under the impression that when we made the change the last time around it was going to be blanket fix for all types of BEs. I'll have to punt this to the FW team @tlauda , please could you chime in?

tlauda · 2020-03-19T14:00:51Z

@plbossart @ranj063 @RanderWang I'm not very familiar with the differences between FE and BE triggers, so maybe I will just describe why the order has been changed in the first place. When you are sending TRIGGER_START/RELEASE IPC to the FW, the DSP starts the pipeline immediately. Pipelines are scheduled on timer, so ticks are happening asynchronously to the IPCs. Since you first triggered the pipeline and HDA Link was started after that, sometimes there was a race happening, where we expected to copy the data, but the data wasn't yet there. It often caused xruns. The same thing is for the STOP sequence, but for the stop it gets even worse, since the wrong sequence between FW and host can hang everything.
For DMIC and SSP there is probably no difference, since FW is controlling the majority of the interface.
I'm not an expert for SDW, but if the pop noise is at the beginning of the stream, shouldn't we just empty ALH FIFO, zero out the DMA buffer and that should fix the problem?

ranj063 · 2020-03-19T16:37:55Z

@plbossart @ranj063 @RanderWang I'm not very familiar with the differences between FE and BE triggers, so maybe I will just describe why the order has been changed in the first place. When you are sending TRIGGER_START/RELEASE IPC to the FW, the DSP starts the pipeline immediately. Pipelines are scheduled on timer, so ticks are happening asynchronously to the IPCs. Since you first triggered the pipeline and HDA Link was started after that, sometimes there was a race happening, where we expected to copy the data, but the data wasn't yet there. It often caused xruns. The same thing is for the STOP sequence, but for the stop it gets even worse, since the wrong sequence between FW and host can hang everything.
For DMIC and SSP there is probably no difference, since FW is controlling the majority of the interface.
I'm not an expert for SDW, but if the pop noise is at the beginning of the stream, shouldn't we just empty ALH FIFO, zero out the DMA buffer and that should fix the problem?

@tlauda the xruns we saw was for the capture case where starting the BE before the FE makes sense because you dont want to be in a situation where there's nothing to copy. Shouldnt the order be the reverse then for playback? But that would make it even more crazy to have 2 different trigger orders for playback and capture. Does it make sense?

tlauda · 2020-03-19T16:40:50Z

@ranj063 We saw underruns on playback pipes: #1160.

ranj063 · 2020-03-19T16:44:26Z

@ranj063 We saw underruns on playback pipes: #1160.

@tlauda oh yeah. dont know why I have it as capture in my mind. So in the case of HDA link DMA< even for playback the trigger order makes sense.

So now to @plbossart 's question. Why should it change for SDW?

tlauda · 2020-03-19T16:47:17Z

@ranj063 We saw underruns on playback pipes: #1160.

@tlauda oh yeah. dont know why I have it as capture in my mind. So in the case of HDA link DMA< even for playback the trigger order makes sense.

So now to @plbossart 's question. Why should it change for SDW?

@RanderWang Have you tried what I proposed? Emptying ALH/SDW FIFO (in kernel) and zeroing DMA buffer (in FW)? Maybe it will help for the pop without changing any order.

RanderWang · 2020-03-20T03:19:30Z

@ranj063 We saw underruns on playback pipes: #1160.

@tlauda oh yeah. dont know why I have it as capture in my mind. So in the case of HDA link DMA< even for playback the trigger order makes sense.
So now to @plbossart 's question. Why should it change tfor SDW?

@RanderWang Have you tried what I proposed? Emptying ALH/SDW FIFO (in kernel) and zeroing DMA buffer (in FW)? Maybe it will help for the pop without changing any order.

@tlauda We have tested this solution "Emptying ALH/SDW FIFO (in kernel) and zeroing DMA buffer (in FW)"(#1887 & thesofproject/sof#2573), but still have pop noise when stop playback. And you can image when someone is climbing a ladder and the ladder is suddenly removed, what will happen.

tlauda · 2020-03-20T07:14:16Z

@RanderWang So the pop is also when stopping playback? How is that possible if the FW pipeline is stopped first?

RanderWang · 2020-03-20T09:41:06Z

@RanderWang So the pop is also when stopping playback? How is that possible if the FW pipeline is stopped first?

@tlauda please check above bugs, not only start & stop, but also pulse & resume. If FW is stopped first, then the there is no data sent to sdw bus, and there is a abrupt change in data stream, just produce a big bang.

RanderWang · 2020-04-15T05:23:15Z

@ClarexZhou Now I change the trigger sequence of playback for both SDW & I2S, can you help to validate playback on I2S platforms ? thanks!

sinahuang · 2020-04-15T06:35:06Z

@ClarexZhou Now I change the trigger sequence of playback for both SDW & I2S, can you help to validate playback on I2S platforms ? thanks!

Do all sanity test and check all noise scenarios on GLK Chrome with onboard codec DA7219 in I2S mode, found no regression.

RanderWang · 2020-04-15T08:50:36Z

@ClarexZhou Now I change the trigger sequence of playback for both SDW & I2S, can you help to validate playback on I2S platforms ? thanks!

Do all sanity test and check all noise scenarios on GLK Chrome with onboard codec DA7219 in I2S mode, found no regression.

Thanks!

BugLink: https://bugs.launchpad.net/bugs/1872916 Now the trigger sequence is set to SND_SOC_DPCM_TRIGGER_POST for SOF. This means FE will be stopped before BE, so BE will consume invalid data and this generates huge pop noise. Now set trigger sequence to SND_SOC_DPCM_TRIGGER_PRE to let FE to be stopped after BE to avoid this issue. Fully tested on Comet Lake for a few cycles. Signed-off-by: randerwang <rander.wang@linux.intel.com> (backported from thesofproject#1897) Signed-off-by: Hui Wang <hui.wang@canonical.com>

RanderWang · 2020-04-15T13:06:05Z

@plbossart update my PR to unify the trigger sequence with I2S & sdw.

plbossart

Please add more comments or reword them. Thanks @RanderWang

sound/soc/sof/pcm.c

plbossart · 2020-04-15T22:31:03Z

sound/soc/sof/topology.c

+		 * HDA and sdw. Frontend is started before the backend
+		 * when start playback and Frontend is stopped after the
+		 * backend when stop playback. The tigger order is reverse
+		 * for Capture compared to playback.


suggested edit:

set default trigger order for all links. Exceptions to the rule will be handled in sof_pcm_dai_link_fixup()
For playback, the sequence is the following:
start FE, start BE, stop BE, stop FE
For Capture the sequence is inverted
start BE, start FE, stop FE, stop BE

@ranj063 can you please review this since you modified the sequence for HDaudio. Thanks!

…ing playback on sdw platforms Now the trigger sequence is set to SND_SOC_DPCM_TRIGGER_POST for SOF. This means FE will be stopped before BE, so BE will consume invalid data and this generates huge pop noise. This sequence is introduced for HDA DAI which requires SND_SOC_DPCM_TRIGGER_POST for some reasons. Now set default trigger sequence to SND_SOC_DPCM_TRIGGER_PRE for playback with all DAI and fix sequence only for HDA DAI. Fully tested on Comet Lake for a few cycles. Signed-off-by: randerwang <rander.wang@linux.intel.com>

RanderWang · 2020-04-16T00:38:45Z

QA also tested HDMI playback. Thanks!

ClarexZhou

Tested on GLK I2S and CML-U&H SDW. Works OK and no regression.

lyakh

just for my info: this means, with HDA DAIs we will still have pop noises and they are impossible to fix?

RanderWang · 2020-04-16T08:18:34Z

just for my info: this means, with HDA DAIs we will still have pop noises and they are impossible to fix?

@lyakh It also depends on audio IP connected to codecs. Now we don't get pop noise issue with HDA link.

lgirdwood

@RanderWang can you check if this has any impact on capture.

plbossart · 2020-04-16T13:34:10Z

@RanderWang can you check if this has any impact on capture.

capture is not modified by this PR.

lyakh · 2020-04-17T06:48:47Z

just for my info: this means, with HDA DAIs we will still have pop noises and they are impossible to fix?

@lyakh It also depends on audio IP connected to codecs. Now we don't get pop noise issue with HDA link.

@RanderWang got it, thanks!

ranj063

@plbossart @RanderWang I made an interesting discovery today while trying to debug another issue with power management. Basically, what I noticed was the idle_bias_on setting for the component driver has some connection with the xruns that we saw with pause/release in capture. So, to confirm this, I set the trigger order for the DAI's in the SOF driver to the default TRIGGER_PRE and set the "idle_bias_on" in the hdac codec component driver to true.

So my suggestion is try experimenting with the idle_bias_on setting in the rt1308/rt711 codec component drivers and see if this problem can be fixed.

ranj063

idle_bias_off might have PM regressions. so this change is good.

RanderWang · 2020-04-20T02:46:23Z

@kv2019i sorry, I assumed you were reviewer for you commented it, now add you to review this PR. Thanks!

kv2019i · 2020-04-20T09:23:14Z

@RanderWang wrote:

@kv2019i sorry, I assumed you were reviewer for you commented it, now add you to review this

It seems this patch has sufficient review approvals already. So the remaining blockers are @plbossart request-changes. Another are the CML SDW failures in CI, why are these happening:
https://sof-ci.01.org/linuxpr/PR1897/build3587/devicetest/CML_RVP_SDW/check-pause-resume-playback-10/
... ?

plbossart · 2020-04-20T15:40:45Z

@RanderWang wrote:

@kv2019i sorry, I assumed you were reviewer for you commented it, now add you to review this

It seems this patch has sufficient review approvals already. So the remaining blockers are @plbossart request-changes. Another are the CML SDW failures in CI, why are these happening:
https://sof-ci.01.org/linuxpr/PR1897/build3587/devicetest/CML_RVP_SDW/check-pause-resume-playback-10/
... ?

SoundWire pause/resume is currently not supported, so this test should be skipped for now.

RanderWang · 2020-04-21T01:01:12Z

@RanderWang wrote:

@kv2019i sorry, I assumed you were reviewer for you commented it, now add you to review this

It seems this patch has sufficient review approvals already. So the remaining blockers are @plbossart request-changes. Another are the CML SDW failures in CI, why are these happening:
https://sof-ci.01.org/linuxpr/PR1897/build3587/devicetest/CML_RVP_SDW/check-pause-resume-playback-10/
... ?

we need another PR to fix it thesofproject/sof#2673

plbossart · 2020-04-21T22:27:09Z

Let's merge and see what QA reports.

RanderWang requested review from juimonen, keyonjie, lyakh and ranj063 as code owners March 16, 2020 06:25

RanderWang requested review from ClarexZhou and bardliao March 16, 2020 06:25

ClarexZhou previously approved these changes Mar 16, 2020

View reviewed changes

lyakh reviewed Mar 16, 2020

View reviewed changes

RanderWang requested a review from singalsu as a code owner April 15, 2020 05:22

RanderWang force-pushed the sdw_pop_noise branch from a3c3fa6 to d0c07dc Compare April 15, 2020 13:05

plbossart requested changes Apr 15, 2020

View reviewed changes

plbossart changed the title ~~ASoC: SOF: Intel: fix pop noise when stop playback on sdw platforms~~ ASoC: SOF: change trigger sequence to fix pop noise when stopping playback on sdw platforms Apr 15, 2020

plbossart requested review from ClarexZhou and lyakh April 15, 2020 22:33

RanderWang force-pushed the sdw_pop_noise branch from d0c07dc to 360d48c Compare April 16, 2020 00:30

ClarexZhou approved these changes Apr 16, 2020

View reviewed changes

lyakh approved these changes Apr 16, 2020

View reviewed changes

lgirdwood approved these changes Apr 16, 2020

View reviewed changes

ranj063 requested changes Apr 18, 2020

View reviewed changes

ranj063 approved these changes Apr 20, 2020

View reviewed changes

RanderWang requested a review from kv2019i April 20, 2020 02:40

plbossart approved these changes Apr 21, 2020

View reviewed changes

plbossart merged commit 363b960 into thesofproject:topic/sof-dev Apr 21, 2020

ASoC: SOF: change trigger sequence to fix pop noise when stopping playback on sdw platforms #1897

ASoC: SOF: change trigger sequence to fix pop noise when stopping playback on sdw platforms #1897

Uh oh!

Conversation

RanderWang commented Mar 16, 2020

Uh oh!

ClarexZhou commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Mar 16, 2020

Uh oh!

plbossart commented Mar 16, 2020

Uh oh!

RanderWang commented Mar 17, 2020

Uh oh!

RanderWang commented Mar 17, 2020

Uh oh!

plbossart commented Mar 17, 2020

Uh oh!

ranj063 commented Mar 17, 2020

Uh oh!

plbossart commented Mar 18, 2020

Uh oh!

ranj063 commented Mar 18, 2020

Uh oh!

tlauda commented Mar 19, 2020

Uh oh!

ranj063 commented Mar 19, 2020

Uh oh!

tlauda commented Mar 19, 2020

Uh oh!

ranj063 commented Mar 19, 2020

Uh oh!

tlauda commented Mar 19, 2020

Uh oh!

RanderWang commented Mar 20, 2020

Uh oh!

tlauda commented Mar 20, 2020

Uh oh!

RanderWang commented Mar 20, 2020

Uh oh!

RanderWang commented Apr 15, 2020

Uh oh!

sinahuang commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Apr 15, 2020

Uh oh!

RanderWang commented Apr 15, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Apr 16, 2020

Uh oh!

ClarexZhou left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Apr 16, 2020

Uh oh!

ClarexZhou commented Mar 16, 2020 •

edited

Loading

sinahuang commented Apr 15, 2020 •

edited

Loading

ranj063 left a comment •

edited

Loading