audio: Add Crossover Filter component #3263

johnylin76 · 2020-07-31T07:23:36Z

Note: this PR contains the upstreamed commits from PR: #2802

This patch adds a Crossover Filter component to SOF. A crossover can be used to split an input signal into different frequency bands. It's a one input, multiple output component. The number of outputs supported are 2,3 and 4. Supports s16_le, s24_le and s32_le.

To maximize code reuse, the biquad processing function was separated from iir_df2t(). This allows to process the biquads more freely. The motivation behind this change was to reuse the coefficients of the LR4 biquads. An LR4 filter is the basic building block of a crossover, and it is made of two biquads in series with same coefficients. Therefore to save memory, we can store one copy of each set of coeffcients, and pass those to the biquad processing function.

The tool to generate the coefficients are found under tools/tune/crossover, and you just need to run:

octave example_crossover.m
If you want tweak the parameters (number of outputs, frequency cutoffs or sink assignments) just modify example_crossover.m.

tools/topology/sof/pipe-crossover-playback.m4

src/audio/crossover/crossover.c

singalsu

There's some commit merge need plus it seems that there's a build error with TGL platform.

cujomalainey · 2020-07-31T17:03:39Z

Thanks for the review @singalsu, I agree we should squash fixes into their original commit.

johnylin76 · 2020-08-04T09:05:47Z

Thanks for review, commits are updated.

singalsu

Thanks, looks good to me! What's the preferred order of merging, this PR or IIR library move PR first?

singalsu · 2020-08-11T16:49:04Z

SOFCI TEST

cujomalainey · 2020-08-11T20:49:10Z

@singalsu we are working in #3270 to remove the inlining of the iir function, that way we can use the function as is without any changes and also select the SIMD variants easier, need to resolve that first before landing this

johnylin76 · 2020-08-13T05:06:23Z

@singalsu @cujomalainey I have updated this PR to have iir_df2t functions in src/math. The previous inline implementation by Seb is removed.

lgirdwood

Looks good - mostly minor thing to change or check. Some of the loops could be potentially improved too.

src/audio/crossover/crossover.c

src/audio/crossover/crossover_generic.c

lgirdwood · 2020-08-13T10:13:51Z

src/include/ipc/topology.h

SOF_COMP_PROCESS should be used here. @keyonjie does your UUID PRs ass this type ? @plbossart fyi

SOF_COMP_PROCESS doesn't seem to exist in tip-of-tree code. Maybe it is still un-merged?

No need to add a new comp type enum, just keep this part intact should work to me. @johnylin76

@johnylin76 can you delete this change. I think we are good to merge after this.

Hi @lgirdwood , my only concern is that testbench doesn't seem to have uuid token parsing now, which I think should be ported from Linux topology driver.
(mentioned in https://thesofproject.github.io/latest/developer_guides/uuid/index.html#linux-topology-driver)

Then it will cause unsupported comp type error while running testbench for crossover. Shouldn't we port the uuid token parsing to testbench first?

Hi @lgirdwood , my only concern is that testbench doesn't seem to have uuid token parsing now, which I think should be ported from Linux topology driver.
(mentioned in https://thesofproject.github.io/latest/developer_guides/uuid/index.html#linux-topology-driver)

Then it will cause unsupported comp type error while running testbench for crossover. Shouldn't we port the uuid token parsing to testbench first?

@johnylin76 @cujomalainey if nobody is using testbench for crossover ATM, it's fine to remove the adding of SOF_COMP_CROSSOVER and leave the tplg_parser gap as TODO?

lgirdwood

OK, LGTM - we just pend on the SOF_COMP_PROCESS. @keyonjie are you doing this update ?

keyonjie · 2020-08-14T15:43:11Z

tools/topology/m4/crossover.m4

don't need this token if you add UUID for it, e.g. refer to detect.m4

After I removed this token, the error messages will appear while "./scripts/build-tools.sh -t"

ALSA lib data.c:700:(build_tuples) cannot find tuples CROSSOVER1.0_crossover_process_tuples_str
ALSA lib parser.c:285:(tplg_build) failed to check topology integrity

It sounds like process_tuples_str is still necessary?

if you remove the whole definition of "'N_CROSSOVER($1)`_crossover_process_tuples_str" here, you need to remove the reference to this in line 34 below also.

Thanks, that works.
And I guess it still need to define a tuple_uuid type with UUID in crossover.m4 file?
(according to https://github.com/thesofproject/sof/blob/master/tools/tplg_parser/tplg_parser.c#L1395)
Or we should just leave process->comp->type as NONE in tplg_load_process?
(according to https://github.com/thesofproject/sof/blob/master/tools/tplg_parser/tplg_parser.c#L1058)

Ah please neglect my last comment because the answer is already in detect.m4 (after I rebased my project)

However, according to https://thesofproject.github.io/latest/developer_guides/uuid/index.html
I think something is still missing (e.g. parse uuid token) on running testbench. I will investigate more.

@keyonjie I think the code of parsing uuid token should be ported to tools/tplg_parser from Linux topology driver
(mentioned in https://thesofproject.github.io/latest/developer_guides/uuid/index.html#linux-topology-driver)
if we need to run testbench for topologies with UUID token (and without process_type token).

lgirdwood

We have UUID so dont need the comp enum now. Good to go otherwise.

lgirdwood · 2020-08-25T09:17:04Z

src/include/ipc/topology.h

@johnylin76 can you delete this change. I think we are good to merge after this.

Moved iir_df2t() from src/audio/eq_iir to src/math as a common library for IIR and Crossover usage. Signed-off-by: Pin-chih Lin <johnylin@google.com>

johnylin76 · 2020-08-26T01:01:16Z

Hi @lgirdwood , my only concern is that testbench doesn't seem to have uuid token parsing now, which I think should be ported from Linux topology driver.
(mentioned in https://thesofproject.github.io/latest/developer_guides/uuid/index.html#linux-topology-driver)

Then it will cause unsupported comp type error while running testbench for crossover. Shouldn't we port the uuid token parsing to testbench first?

lgirdwood · 2020-08-26T16:34:40Z

@johnylin76 good point - @keyonjie or @singalsu any update here for support in testbench ?

johnylin76 · 2020-09-01T05:22:41Z

Hi, any suggestion?

keyonjie · 2020-09-01T06:07:22Z

@johnylin76 good point - @keyonjie or @singalsu any update here for support in testbench ?

Yes, we need add this support to testbench, I can try to add it this week.

keyonjie · 2020-09-01T06:13:50Z

@johnylin76 good point - @keyonjie or @singalsu any update here for support in testbench ?

Yes, we need add this support to testbench, I can try to add it this week.

@singalsu @lgirdwood just looking into tplg_parser/topology.h and find that we actually already have significant gap with Linux deriver sof/topology.c, we need to fill up those gaps first.

juimonen · 2020-09-01T06:18:44Z

@keyonjie and others: I find it really bad we have 2 different parsers in linux driver and testbench. There should be some way of detaching the exactly same parsing from the driver to test bench. Now, having tried this way back, it is pretty difficult as the kernel C types don't match user space. But maybe some kind of semi automatic scripting could work?

In essence: if we do for example fuzzing in testbench, it could tell something about the driver's functionality, or it might not tell anything...

cujomalainey · 2020-09-01T06:36:31Z

@bzhg FYI

cujomalainey · 2020-09-01T06:37:34Z

@juimonen I agree, this isn't ideal, it makes fuzzing harder too as we are likely patching irrelevant memory bugs.

keyonjie · 2020-09-01T06:46:05Z

@juimonen exactly. @xiulipan @aiChaoSONG please be noticed that the tplg_parser used in our CI could be out-of-date and not aligned with the Linux driver one. This worth a topic to discuss about how we should proceed.

cujomalainey · 2020-09-01T06:48:22Z

@lgirdwood given the hefty work to get tplg_parser fixed can we land as is for the time being an leave a TODO to remove the enum once tplg_parser is fixed?

src/audio/crossover/crossover.c

aiChaoSONG · 2020-09-01T07:09:39Z

@juimonen exactly. @xiulipan @aiChaoSONG please be noticed that the tplg_parser used in our CI could be out-of-date and not aligned with the Linux driver one. This worth a topic to discuss about how we should proceed.

Does this PR changes the topology from ABI level, for example, snd_soc_tplg_damp_widget structure ,etc? if not, then we are good. Add a new widget type will not influence the topology parser.

lgirdwood · 2020-09-01T10:28:23Z

@juimonen @keyonjie @cujomalainey @johnylin76 I think there are actually 3 parsers.

Kernel - GPLv2
tplg_parser - Testebench/Fuzzer - BSD
CI - python - ???

tplg_parser already has UUID support APIs, but it does need those APIs glued to component matching code which is currently using comp_type only. It's not a big job, probably a day or two.

@johnylin76 can you remove the new comp_type form this PR and I will merge. I will find someone to fix parser so we can use Cross Over in the pasrer before v1.6.

This commit adds Crossover to the list of SOF components. A crossover filter can be used to split an input to different frequency bands. The number of outputs should be set statically in the topology. The user then uses the control bytes to route the frequency bands to different outputs. (similar to the demux component). This commit adds support for the following formats: - S16_LE - S24_LE - S32_LE Signed-off-by: Sebastiano Carlucci <scarlucci@google.com>

This commit adds the topology files for the crossover component. The control bytes are generated by the tools in tune/crossover. Signed-off-by: Sebastiano Carlucci <scarlucci@google.com>

This commit adds the tools to generate the control bytes for the crossover component. To generate the control bytes, run the example_crossover.m script. The parameters of the crossover components are: - number of outputs - sink assignments (routing crossover output to different pipelines) - frequency cutoffs To tweak the parameters modify the values in example_crossover.m and run it. Refer to sof/src/include/user/crossover.h for more information on how the crossover config is structured and how sink assignments are done. Signed-off-by: Sebastiano Carlucci <scarlucci@google.com>

Add Crossover component for tplg_parser. Signed-off-by: Pin-chih Lin <johnylin@google.com>

Crossover component is added as multi-output playback tests for testbench. Signed-off-by: Pin-chih Lin <johnylin@google.com>

johnylin76 · 2020-09-02T05:26:22Z

Hi @lgirdwood ,
New comp_type SOF_COMP_CROSSOVER and the related change are removed in this PR, thanks.

johnylin76 · 2020-09-02T05:48:21Z

We have UUID so dont need the comp enum now. Good to go otherwise.

Done, thanks.

lgirdwood · 2020-09-02T13:31:20Z

Jenkins has BSW DUT HW failure.
@johnylin76 testbench and fuzzer UUID support is being worked now by @juimonen

cujomalainey · 2020-09-02T18:42:05Z

@sebcarlucci fyi

johnylin76 requested review from cujomalainey and singalsu July 31, 2020 07:23

johnylin76 requested review from a team, dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart and tlauda as code owners July 31, 2020 07:23

singalsu reviewed Jul 31, 2020

View reviewed changes

tools/topology/sof/pipe-crossover-playback.m4 Outdated Show resolved Hide resolved

singalsu reviewed Jul 31, 2020

View reviewed changes

src/audio/crossover/crossover.c Outdated Show resolved Hide resolved

singalsu reviewed Jul 31, 2020

View reviewed changes

src/audio/crossover/crossover.c Outdated Show resolved Hide resolved

singalsu requested changes Jul 31, 2020

View reviewed changes

cujomalainey mentioned this pull request Jul 31, 2020

audio: Add Crossover Filter component #2802

Closed

johnylin76 mentioned this pull request Aug 4, 2020

WIP: Testbench Support For Crossover Component #3139

Closed

johnylin76 force-pushed the upstream_crossover branch from 7ee7567 to 746adf1 Compare August 4, 2020 08:52

singalsu approved these changes Aug 11, 2020

View reviewed changes

johnylin76 force-pushed the upstream_crossover branch from 746adf1 to 4f9dbd5 Compare August 12, 2020 07:39

johnylin76 requested a review from ranj063 as a code owner August 12, 2020 07:39

johnylin76 mentioned this pull request Aug 12, 2020

WIP: audio: optimize biquad filter by arch #3270

Closed

lgirdwood requested changes Aug 13, 2020

View reviewed changes

johnylin76 force-pushed the upstream_crossover branch from 4f9dbd5 to c3ea31a Compare August 14, 2020 10:25

lgirdwood reviewed Aug 14, 2020

View reviewed changes

keyonjie reviewed Aug 14, 2020

View reviewed changes

lgirdwood requested changes Aug 25, 2020

View reviewed changes

src/include/ipc/topology.h Outdated

Copy link

Member

lgirdwood Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnylin76 can you delete this change. I think we are good to merge after this.

sof: math: move iir_df2t function to src/math

e41e639

Moved iir_df2t() from src/audio/eq_iir to src/math as a common library for IIR and Crossover usage. Signed-off-by: Pin-chih Lin <johnylin@google.com>

keyonjie reviewed Sep 1, 2020

View reviewed changes

src/audio/crossover/crossover.c Outdated Show resolved Hide resolved

johnylin76 force-pushed the upstream_crossover branch from 8ddcd02 to 26d56a4 Compare September 1, 2020 09:13

sebcarlucci and others added 5 commits September 2, 2020 13:22

tools: topology: Add crossover topology files

1c3e546

This commit adds the topology files for the crossover component. The control bytes are generated by the tools in tune/crossover. Signed-off-by: Sebastiano Carlucci <scarlucci@google.com>

tools: tplg_parser: Add Crossover component

903e466

Add Crossover component for tplg_parser. Signed-off-by: Pin-chih Lin <johnylin@google.com>

tools: testbench: add Crossover component tests

bddf48e

Crossover component is added as multi-output playback tests for testbench. Signed-off-by: Pin-chih Lin <johnylin@google.com>

johnylin76 force-pushed the upstream_crossover branch from 26d56a4 to bddf48e Compare September 2, 2020 05:24

lgirdwood approved these changes Sep 2, 2020

View reviewed changes

lgirdwood merged commit bb6565d into master Sep 2, 2020

kv2019i mentioned this pull request Sep 23, 2020

ASoC: SOF: topology: Add support for Crossover thesofproject/linux#2030

Closed

lgirdwood deleted the upstream_crossover branch January 27, 2021 09:25

audio: Add Crossover Filter component #3263

audio: Add Crossover Filter component #3263

Uh oh!

Conversation

johnylin76 commented Jul 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

cujomalainey commented Jul 31, 2020

Uh oh!

johnylin76 commented Aug 4, 2020

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

singalsu commented Aug 11, 2020

Uh oh!

cujomalainey commented Aug 11, 2020

Uh oh!

johnylin76 commented Aug 13, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

lgirdwood left a comment

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!

lgirdwood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnylin76 commented Aug 26, 2020

Uh oh!

lgirdwood commented Aug 26, 2020

Uh oh!

johnylin76 commented Sep 1, 2020

Uh oh!

keyonjie commented Sep 1, 2020

Uh oh!

keyonjie commented Sep 1, 2020

Uh oh!

lgirdwood left a comment •

edited

Loading