Skip to content

Conversation

@sebcarlucci
Copy link
Contributor

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.

Assigning Pipelines to Crossover Outputs:

Refer to src/include/user/crossover.h for how the pipelines are routed to the crossover sinks.

Signed-off-by: Sebastiano Carlucci scarlucci@google.com

@sebcarlucci
Copy link
Contributor Author

fyi @cujomalainey

@lgirdwood
Copy link
Member

@singalsu fyi

Copy link
Collaborator

@singalsu singalsu left a comment

Choose a reason for hiding this comment

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

It's nice clean code for a pretty complicated filters network processing component. Can you check checkpatch output, there's some long lines to fix.

@kv2019i
Copy link
Collaborator

kv2019i commented Apr 22, 2020

As this is modifying an enum that is part of the FW ABI, needs to be classified before merging.

@sebcarlucci sebcarlucci force-pushed the crossover-filter-test branch 2 times, most recently from 418d4ba to ac3ff0a Compare April 27, 2020 23:23
@singalsu
Copy link
Collaborator

singalsu commented Apr 28, 2020

@tlauda It looks like APL build fails for this PR. Do you have still around the code snippet (for volume) to adjust .bss in APL to fit more code?

Edit: Also it's a possibility to reduce a lot the SRC code size by switching to low quality option. I don't think we have official use cases for SRC so it could be done (via src/include/sof/audio/src/src_config.h). Currently the high quality SRC coefficients tables those are enabled for all HiFi3 builds consume a lot of code size.

@tlauda
Copy link
Contributor

tlauda commented Apr 28, 2020

@tlauda It looks like APL build fails for this PR. Do you have still around the code snippet (for volume) to adjust .bss in APL to fit more code?

diff --git a/src/platform/apollolake/include/platform/lib/memory.h b/src/platform/apollolake/include/platform/lib/memory.h
index 6197e05..e30e3e2 100644
--- a/src/platform/apollolake/include/platform/lib/memory.h
+++ b/src/platform/apollolake/include/platform/lib/memory.h
@@ -286,7 +286,7 @@
 #define HEAP_BUFFER_BLOCK_SIZE	0x100
 #define HEAP_BUFFER_COUNT	(HEAP_BUFFER_SIZE / HEAP_BUFFER_BLOCK_SIZE)
 
-#define HEAP_SYSTEM_M_SIZE		0x8000	/* heap master core size */
+#define HEAP_SYSTEM_M_SIZE		0x7000	/* heap master core size */
 #define HEAP_SYSTEM_S_SIZE		0x6000	/* heap slave core size */
 #define HEAP_SYSTEM_T_SIZE \
 	(HEAP_SYSTEM_M_SIZE + ((PLATFORM_CORE_COUNT - 1) * HEAP_SYSTEM_S_SIZE))

@cujomalainey
Copy link
Contributor

We can disable for APL like we did the DCblocker for CHT if need be.

@cujomalainey cujomalainey requested a review from singalsu April 28, 2020 18:42
@juimonen
Copy link

@singalsu I don't get how this PR is needing more memory? Am I missing something here? Or is this PR increasing the size of total sof firmware so much that you can't load it?

@sebcarlucci sebcarlucci force-pushed the crossover-filter-test branch from ac3ff0a to e49202f Compare April 29, 2020 06:34
@cujomalainey
Copy link
Contributor

@juimonen i believe that is correct and .text is simply growing too much

SOF_COMP_DEMUX,
SOF_COMP_ASRC, /**< Asynchronous sample rate converter */
SOF_COMP_DCBLOCK,
SOF_COMP_CROSSOVER,
Copy link
Member

Choose a reason for hiding this comment

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

@plbossart @kv2019i @mmaka1 is the driver still accepting new types or should all new stuff be using a generic "PROCESS" type now ? If so what's the ETA on this new process type landing in the kernel ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lgirdwood can we land this then fix this up later? There are more PRs coming in using the old comp_type (see #2925) and this is functionally correct with the current state of the system. I will own updating this if need be to the new process type. As mentioned above, any changes now would require this PR to be closed and a new one to be opened therefore losing all history of comments.

Copy link
Member

Choose a reason for hiding this comment

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

@plbossart are you still accepting new types currently (or is the process type ready for merging) ? we have 3 PRs blocking on new types atm iirc

@cujomalainey
Copy link
Contributor

This will need to be rebased and fixed up once #2920 lands. Also Ideally we will need to fixup the used of the biquards as we are restricted right now the generic model.

@singalsu
Copy link
Collaborator

@cujomalainey If there's tasks where you would need and I could help please let me know.

@lgirdwood
Copy link
Member

@singalsu it looks like the UUID should land next week, all the opens are mostly resolved now. I guess this wont take long to convert to UUID.

@cujomalainey
Copy link
Contributor

@singalsu Thanks for the offer, another member of my team will be taking this over then starting the DRC integration. I will make sure to mention you as someone to reach out to if they get stuck.

@singalsu
Copy link
Collaborator

singalsu commented Jun 15, 2020

@cujomalainey Thanks, sounds good!

@singalsu singalsu removed their assignment Jun 15, 2020
@lgirdwood
Copy link
Member

@singalsu who's doing this, please assign them.

@cujomalainey
Copy link
Contributor

@lgirdwood I will get their github username

@lgirdwood lgirdwood added Notable Enhancement is significant or notable for release. ABI ABI change involved labels Jun 15, 2020
@lgirdwood lgirdwood added this to the ABI-3.17 milestone Jun 15, 2020
@lgirdwood
Copy link
Member

@deb-intel fyi

@cujomalainey
Copy link
Contributor

Does this require and ABI change once we go to the UUID model? @lgirdwood I thought that was the whole point of UUID that there was no need for an ABI change

@paulstelian97
Copy link
Collaborator

Does this require and ABI change once we go to the UUID model? @lgirdwood I thought that was the whole point of UUID that there was no need for an ABI change

Can the configuration itself and everything pass through via existing ABI and provide all required functionalities of this component? Until then assume ABI change is needed.

@cujomalainey
Copy link
Contributor

@lgirdwood can you add @johnylin76 as a sof dev so we can assign this to him?

@cujomalainey
Copy link
Contributor

@paulstelian97 everything is through byte controls, the only change prior was adding the enum if i recall correctly

@lgirdwood
Copy link
Member

@cujomalainey it's just a reminder to me that we are blocking on the ABI update for UUID in 3.17 (so I dont mistakenly merge beofre the UUID).
@johnylin76 invite sent, please reply here and I will assign once you are a member. Thanks

@johnylin76
Copy link
Contributor

johnylin76 commented Jun 16, 2020 via email

@zrombel
Copy link

zrombel commented Jul 31, 2020

Due to recent PR #3194 merge, please rebase for TGL Multicore tests to Pass on Internal CI.

@cujomalainey
Copy link
Contributor

@zrombel thanks, this PR is now #3263. Also, please use # notation so Github links correctly

@zrombel
Copy link

zrombel commented Aug 1, 2020

@cujomalainey comment edited, I will keep that in mind :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ABI ABI change involved Notable Enhancement is significant or notable for release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.