Repository navigation
Topology: Apply high-pass for HDA capture streams, Volume: Force gain ramp - #2109
Conversation
|
@bardliao can you try this with a change to the rt715 capture pipeline to solve thesofproject/linux#1480 it should be just a change in the pipeline name |
Not lucky. This PR doesn't solve thesofproject/linux#1480 |
|
The DC pulse becomes shorter, about 0.04s(previously 0.35s) |
|
@aiChaoSONG Can you please check if there's volume ramp happening at all in the capture? There's also the suspending volume jump issue that makes the driver suspect of not doing un-mute in beginning. Normally the volume ramp should conceal the beginning and once the ramp has completed the IIR has settled mostly. Also I can still increase a bit the cut-off frequency to settle the DC level a bit faster than 0.04s. |
@bardliao Do you have a captured sample or wav editor screenshot of the result? |
|
@singalsu I think you are talking about #1463 Output very loud at the beginning ..., as a matter of fact, this issue exist on almost every device we have. the volume ramp only exists at the begin of the recording, because the dsp never goes into suspended after I pause(SPACE)/stop(CTRL+Z) the recording pipeline and wait for a long time. now I suspect #2102 and #1463 are the same issue. |
|
@aiChaoSONG Thanks for info! The bugs were assigned to me so I will try to fix these. Sounds indeed they are the same issues. |
@singalsu dmic.zip is the captured sample and highpass.diff.txt is the change I did. Thanks @bardliao ! It's as expected and the next version of this PR should fix this. |
23db6a2 to
ed0a9d5
Compare
singalsu
changed the title
|
@bardliao @aiChaoSONG Can please re-check with this PR version the capture waveform. There's now a FW change into FW component as well. |
|
|
The volume of the capture pipeline will influence the waveform at the begin of recording. I tried vol=30 and vol =50, please find the waveform in the file below |
Thanks, yep it seems to do. The request volume should not impact the start ramp time. Looks like the ramp time calculation fails somehow. I'll look into it and fix it. What is strange is that it looks like the high-pass IIR fails. The DC settling time of 3 seconds looks like it's not working. |
This does not look as I intended. There's no unmute ramp happening in the beginning, strange. But I'll anyway do a new version, need to fix and clean up the ramp calculation. |
|
I fixed bugs / rewrote the ramp step calculation since I no more understood why the code was like that. Also not sure if it was the case but when testing this PR please ensure both topology and firmware commits from this PR are included. |
|
@zrombel @lgirdwood @lbetlej One idea to address the KPB fail could be to alter the gain ramp length for the volume instance instance for KPB, e.g. change ramp length from 250 ms to 10 ms. Would be OK for testing such approach? |
|
@singalsu I've investigated fails in that particular test (KD + DMIC) and it seems that there is a bug in KD on APL platform. It's rather sporadic but as we can see it affects CI results. I've removed that test from CI scope and I'm about to report the bug on github, so it shouldn't be a problem any more. |
|
@zrombel Thanks, it's excellent news for this PR and hope the issue gets a fix soon too! |
|
@singalsu cant we just change the ramp time for KPB only in topology ? |
|
@lgirdwood Yep, that was exactly my thought. If there's still problem in some specific usage I can make ramp time 0 ms for them via topology to get the previous behavior. DMIC does in HW an unmute ramp when started so the volume ramp is not a must. But also it doesn't hurt. |
|
@singalsu ack, lets keep KPB ramp at 0 then. |
OK, I'll add the topology change to this PR then too. |
@lgirdwood There's no need to update this PR. I just checked that all topologies those use pipe-kfbm-capture.m4 macro (sof-cml-rt5682-kwd.m4, sof-apl-keyword-detect.m4, sof-glk-da7219-kwd.m4) have no more volume component in the pipeline. The KPB fails must be a totally separate issue since the components impacted by this PR are not included. |
|
SOFCI TEST |
This patch replaces in topology sof-hda-generic.m4 the use of macro pipe-volume-capture.m4 usage with a new macro pipe-highpass-capture.m4. The used 40 Hz cut-off is sufficient to suppress the DC pulse in the beginning of capture from headset with minimal impact to other applications such as recording music. The capture volume ramp length is changed from 250 ms to 400 ms. The longer ramp is feasible because the microphone path seems to open at 500 ms from capture. It helps to attenuate the remaining noises in the beginning of capture. The patch also adds additional high-pass responses macros for topology with different 20-50 Hz cut-off frequencies into SOF/tools/topology/m4. The changes for Matlab/Octave script example_iir_eq.m that generated the filter coefficients are included here as well. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch fixes the calculation of macro VOL_RAMP_STEP_CONST that is used in linear step size calculation. The previous version calculated incorrectly the value as inverse. However with used ramp update rate of 1000 us there were no issues seen because the value and inverse were both 1.0. The bug would have triggered with with a faster or slower ramp update rate. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch ensures that a ramp will happen in beginning of streaming even if the driver control sequence would not launch it by successive volume requests for mute and actual volume. Also a check is added into volume ramp update to prevent a volume command in component idle state to schedule repeatedly ramp updates. In that case the volume can be set immediately to ramp end value. Also the ramp is updated for active channels only. There's no need to update for the maximum channels count. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The earlier version of ramp step calculation was more complex than necessary and there was no way to ensure the max. topology defined ramp time happens in beginning of streaming so the step equation was rewritten. The step value is rounded for better match with topology defined ramp length. Normally the ramp is constant rate type where the topology define time happens for vol min-max transition. A smaller transition is completed in less time. The constant rate mode is not used for the first copy() triggered ramp to ensure the ramp from vol min to used volume takes the entire topology defined time. This mode is also as earlier used for legacy kernels where the vol min/max information is not passed. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch removes a trivial function vol_update() and alters vol_sync_host() to update all channels the same time into the host data structure instead of per channel updates. There is no other change to component operation. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
f044876 to
9c184c2
Compare
|
New version, no other changes than the conflict fix with PR #2272. |
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
CI seems to missed this update lets rerun
|
SOFCI TEST |
|
@zrombel @lgirdwood I wonder why TestPtpSimpleLoopback48000Hz32b32b8ch fails on ICL. Could the forced volume unmute ramp or IIR high-pass cause it? There's no same test for other HDA codec based platform. Though there's a pass for similar case for APL but maybe the tested pipelines are not similar? |
For me it looks like it recorded silence, maybe @zrombel can tell more |
|
@lgirdwood The CI fail is still mystery to me if it's real or not. I wonder if I should split the volume changes into another PR to proceed with the high-pass part that cannot impact the loop-back test. We need the high-pass filters set also to address a similar issue with a SDW codec and I can't proceed while they are not merged. |
|
@singalsu @lgirdwood I've checked this PR test history and it looks like there were some random errors. We have enhanced some of test to prevent this issues, but some was just hardware failures which sadly we cannot avoid. Sorry for that. We will continue to work to increase CI stability. |
11 participants
This pull request consists of five commits. The purpose is to mitigate the DC pulse noise in the beginning of audio capture from headset.
The core of the fix is in commits 1 and 3. Commit 4 makes the initial ramp consistent. Commit 2 fixes a mistake that luckily haven't triggered any issues. Commit 5 is additional cleanup of code that was requested in review or this PR.