-
Notifications
You must be signed in to change notification settings - Fork 349
Audio: Multiband-DRC: Convert to module adapter #7698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Audio: Multiband-DRC: Convert to module adapter #7698
Conversation
b946d98 to
6af4202
Compare
6af4202 to
4e7e6ed
Compare
4e7e6ed to
25ab7db
Compare
|
Without firmware (due to the build failure) and combination of other serious CI issues, https://sof-ci.01.org/sofpr/PR7698/build9948/devicetest uselessly hogged 3 MTL devices for more than 200 minutes :-( |
Sorry for that, would [SKIP CI] in this PR title or in commit message title prevent CI to run? I've been thinking sharing early and often is good - had no idea the tests continue after failed build. There's a problem with component dependencies to crossover and drc those don't build correctly in current git main. |
Yes, or the newer and clearer [SKIP SOF-TEST]. Both work the same.
Yes and drafts are great for that.
This is a CI design issue that is being worked on. |
1cfc34e to
63705e1
Compare
|
First working code version for IPC3, and IPC4. |
Still draft ? |
Need to wait for crossover merge to remove the duplicated patch. Also will split this into three PRs with topology and tools separately. Topology part needs DRC topology PR merge. I think I will add a configuration macro to select for EFX topology DRC vs. multiband-DRC. Multiband-DRC is more versatile but also it has a much higher load. Optimizations would be useful for this component. |
63705e1 to
7e4dd98
Compare
| CONFIG_COMP_SRC_IPC4_FULL_MATRIX=y | ||
| CONFIG_COMP_DRC=y | ||
| CONFIG_COMP_CROSSOVER=y | ||
| CONFIG_COMP_MULTIBAND_DRC=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lgirdwood @kv2019i Do you have opinion about this? If we are not having issues with .text size this might be acceptable. But if we should be more compact these could be default no. DRC is essential as are the EQs, these are more like optional for more advanced speaker enhance. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If left to default no, we should find a way to get them tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets have these in default Linux Kconfig.
792468d to
77a4914
Compare
| static int crossover_process_audio_stream(struct processing_module *mod, | ||
| struct input_stream_buffer *input_buffers, | ||
| int num_input_buffers, | ||
| struct output_stream_buffer *output_buffers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Fixes: 5bcbcf1 ("Audio: Crossover: Convert to module adapter") to commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not good,
/zep_workspace/sof/src/audio/crossover/crossover.c:656:58: warning: incorrect type in argument 2 (different address spaces)
Seems avoiding sparse errors needs a different approach
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need the ALSA kcontrol updates ?
IPC3 version works as before, with IPC4 the control is not available. It now works with IPC4 specific addition in module to force internally the processing switch on. The processing can be controlled on/off with user space sent bytes control blob (in #8049). Currently there is no other way. I plan to submit another PR to make the blob controlled on/off switch as MCPS consumption efficient as in IPC3. Currently even in bypass the band split filters remain running that is not happening with switch control in IPC3. The idea is to turn the processing switch off if all band filters and emphasis/de-emphasis are switched off in the blob. What do you think of this @google ? |
77a4914 to
1cf6c84
Compare
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we trending for making v2.7 code freeze ?
| comp_err(dev, | ||
| "crossover_assign_sinks(), multiple sinks from pipeline %d are assigned", | ||
| pipeline_id); | ||
| sink_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same string fix needed as previous PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I need to rebase this and move the crossover fixes to other PR since they are no more critical for this component.
6f73d92 to
7eb7b60
Compare
|
The new error with sparse cache in crossover.c was caused by regression in my other PR. The error appears with this PR since the required crossover filter is enabled in kconfig by this component. The fix for sparse cache is addressed in #8088. |
|
@singalsu i landed your other PR, please rebase this one. |
This patch replaces legacy component API with new module adapter API. There are no changes to functionality. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch enables for TGL and MTL platforms as default to enable CI build tests, etc. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
7eb7b60 to
63d8f39
Compare
kv2019i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One typoe in log print, maybe you can fix this up in a follow-up. Otherwise looks good.
| } | ||
|
|
||
| return ret; | ||
| comp_err(dev, "tdfb_cmd_get_value() error: invalid cdata->cmd"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tdfb, copy-paste?
No description provided.