[RFC] audio: Add DC Blocking Filter Component#2462
[RFC] audio: Add DC Blocking Filter Component#2462lgirdwood merged 5 commits intothesofproject:masterfrom
Conversation
sebcarlucci
requested review from
a team,
akloniex,
bkokoszx,
dbaluta,
jajanusz,
lbetlej,
lgirdwood,
mmaka1,
plbossart,
tlauda and
xiulipan
as code owners
|
@bzhg FYI |
paulstelian97
left a comment
paulstelian97
left a comment
There was a problem hiding this comment.
Another comment (I do agree with the already present ones as well), commit message should have the subsystem in it as well. Something like: "sof: component: Add DC blocking filter component".
|
@singalsu can you provide your comments. thanks. |
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Just a comment on the control byte tooling from me.
There was a problem hiding this comment.
Can we have a comment on the tool used to generate this byte sequence.
There was a problem hiding this comment.
@lgirdwood I added the scripts to generate the byte sequences. Could you please review them?
There was a problem hiding this comment.
Here also use a pipeline specific defined name for "DCBLOCK". Then refer later with that name. If you use "DCBLOCK_priv" below as macro name it's OK.
I noticed that for EQs it was a mistake to try to name the X_priv with some response specific names. I will do a PR later to fix them to the scheme I'm now proposing for DCBLOCK. For IIR e.g. the tuning tuning should create blobs with just name "EQIIR_PRIV". And that is defined to be pipeline specific.
|
@sebcarlucci you will need to bump the ABI version to make checkpatch happy. |
|
As for Travis. It looks like CHT is just out of space. |
sebcarlucci
force-pushed
the
dcblock-filter
branch
from
4eb3bca to
e82752b
Compare
|
@lgirdwood can we just ignore CHT? Or do we need something done in the CI? Like maybe disable processing elements in CHT by default? |
|
@cujomalainey we can make this a Kconfig default n for BYT and CHT. That wont stop it being used on those platforms, it's just that the user will need to disable other components on this platforms if they want DC blocker since memory is small. |
|
@sebcarlucci can you add another commit to adjust the default configs for those platforms accordingly? |
|
SOFCI TEST |
|
@sebcarlucci #2510 has landed, want to add another commit here to add your component? You will likely need to rebase your branch. Not sure what is going on with CI, @lgirdwood do you know what is happening? |
sebcarlucci
force-pushed
the
dcblock-filter
branch
from
c96c2d3 to
6e8485a
Compare
|
SOFCI TEST |
|
@cujomalainey there were some issues at the end of last week, kicking CI again |
src/include/kernel/abi.h
Outdated
There was a problem hiding this comment.
Should be 14, almost missed this !
There was a problem hiding this comment.
checkpatch complained that it needed an ABI bump, are you saying we don't?
There was a problem hiding this comment.
No bump needed, as we have open space in 14. checkpatch is really there to remind us to check the ABI.
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Lets run CI again, to rule out CI issue, but it does look like the platforms where we disable this component are OK. This probably means we need to grow the heap.
|
@cujomalainey I've logged issue #2622 since this PR has uncovered a FW crash. |
sebcarlucci
force-pushed
the
dcblock-filter
branch
from
6e8485a to
fe32c12
Compare
src/audio/dcblock/dcblock.c
Outdated
There was a problem hiding this comment.
Please change to:
comp_register(platform_shared_get(&comp_dcblock_info, sizeof(comp_dcblock_info)));
This should fix the crash.
There was a problem hiding this comment.
@tlauda looks like we should do the platform_shared_get() inside comp_register() and perhaps for other comp calls ?
There was a problem hiding this comment.
ah i didn't even have that in my local repo. Thanks @tlauda
This change provides a DC Blocking Filter (DCB) component. A DCB is a high-pass filter used to remove any DC offset from a signal. The coefficients for the component can be set individually for each channel. Currently, only the following formats are supported: s16_le s24_le s32_le Signed-off-by: Sebastiano Carlucci <scarlucci@google.com>
This commits adds the scripts to generate the bytes control blob for the DCB. It also provides some generic scripts to convert valid binary blob to a csv or binary file (usable by sof-ctl) or an m4 byte control topology file. This commit adds two new folders under tools/tune - dcblock: scripts to generate the blob in Octave - common: scripts to convert any generic blob to a config file. Signed-off-by: Sebastiano Carlucci <scarlucci@google.com>
This commit includes a new test case in tplg-build.sh. It also includes the new simple pipelines pipe-dcblock-capture/playback. Those new pipelines are used by tplg-build.sh to generate the test pipelines. Signed-off-by: Sebastiano Carlucci <scarlucci@google.com>
The memory size for cht and byt is too small to include the DC Block module. If the user wants to use it, they will have to disable some other components. Signed-off-by: Sebastiano Carlucci <scarlucci@google.com>
This commit adds support for running test for dc blocker. Signed-off-by: Sebastiano Carlucci <scarlucci@google.com>
sebcarlucci
force-pushed
the
dcblock-filter
branch
from
fe32c12 to
ada5748
Compare
|
Travis all green, Jenkins known failures. |
|
This was actually an ABI change needed and the corresponding linux PR should be provided. @sebcarlucci @lgirdwood @cujomalainey @singalsu |
| * \brief Retrieives DC Blocking processing function. | ||
| * \param[in,out] dev DC Blocking Filter base component device. | ||
| */ | ||
| static inline dcblock_func dcblock_find_func(enum sof_ipc_frame src_fmt) |
There was a problem hiding this comment.
Doxygen error, @sebcarlucci can you please fix?
sof/src/include/sof/audio/dcblock/dcblock.h:58: warning: argument 'dev' of command @param is not found in the argument list of dcblock_find_func(enum sof_ipc_frame src_fmt)
sof/src/include/sof/audio/dcblock/dcblock.h:61: warning: The following parameters of dcblock_find_func(enum sof_ipc_frame src_fmt) are not documented:
parameter 'src_fmt'
To run doxygen:
cd sof/
cmake -S doc -B doxy
make -C doxy doc VERBOSE=1
Longer story:
https://thesofproject.github.io/latest/contribute/process/docbuild.html
If/when PR #2741 is merged doxygen issues will be reported by CI automatically.
There was a problem hiding this comment.
PR #2741 was merged faster than I expected. So this doxygen error is now the only one turning Travis to red on every PR.
@sebcarlucci if you're busy I can fix this myself, however I just need you (or anyone really) to tell me what to write for src_fmt.
There was a problem hiding this comment.
I can work on this today
There was a problem hiding this comment.
Already fixed by @sebcarlucci in PR #2746, thx!
9 participants
This change provides a DC Blocking Filter (DCB) component. A DCB is a
high-pass filter used to remove any DC offset from a signal. The
coefficients for the component can be set individually for each channel.
Currently, only the following formats are supported:
Signed-off-by: Sebastiano Carlucci scarlucci@google.com