Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Audio: DRC: Add processing enable switch control #8474

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Audio: DRC: Add processing enable switch control #8474

merged 2 commits into from
Dec 21, 2023

Conversation

singalsu
Copy link
Collaborator

No description provided.

@singalsu
Copy link
Collaborator Author

@johnylin76 @cujomalainey Does adding this control and the logic of letting the control to impact to look OK to you? The operation should be as before if topology is not defining a switch - the blob will determine enabled / disabled.

When there is a switch control in topology, processing is enabled only if blob has enable set and user space is set to enabled. This is the behaviour what I'm not fully sure about. Other way would be to enable the processing with the blob definitions whenever user requests it even if blob has enable set to false.

For lowest disabled MCPS I was testing also a version that uses bypass copy function (drc_default_pass) for disabled but there would be a click when enabled/disabled so I dropped for now the idea.

Note that multiband-DRC is somewhat different in enable switch control handling. It's not changing enabled/disable during streaming. I'd like to update it to be similar as what is defined for DRC to they would be kind of plug-in replacements for each other. DRC would be the light alternative for multiband-DRC (that runs min. 2 band filterbank always).

@singalsu singalsu marked this pull request as ready for review November 10, 2023 16:25
lyakh
lyakh reviewed Nov 13, 2023
View reviewed changes
src/audio/drc/drc.c Outdated Show resolved Hide resolved
src/audio/drc/drc.c Outdated Show resolved Hide resolved
johnylin76
johnylin76 reviewed Nov 13, 2023
View reviewed changes
Copy link
Contributor

@johnylin76 johnylin76 left a comment

Choose a reason for hiding this comment

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

It looks good to me only with one code suggestion. Thanks

src/audio/drc/drc.c Outdated Show resolved Hide resolved
btian1
btian1 reviewed Nov 20, 2023
View reviewed changes
src/audio/drc/drc.c Outdated
@@ -235,6 +266,16 @@ static void drc_set_alignment(struct audio_stream *source,
audio_stream_init_alignment_constants(1, 1, sink);
}

static bool drc_has_config(struct drc_comp_data *cd)
Copy link
Contributor

Choose a reason for hiding this comment

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

"static inline"

Copy link
Collaborator

Choose a reason for hiding this comment

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

on ACE and cAVS the compiler is now allowed to inline automatically. If this is an issue on other platforms, I'd rather enable CONFIG_COMPILER_INLINE_FUNCTION_OPTION on them too, than adding inline to individual functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what to do, maybe leaving like this since the inline happens. Checked it from zephyr.lst.

lgirdwood
lgirdwood reviewed Nov 23, 2023
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

LGTM when all current comments resolved.

@lgirdwood lgirdwood added this to the v2.9 milestone Nov 23, 2023
lyakh
lyakh reviewed Dec 14, 2023
View reviewed changes
src/audio/drc/drc.c Outdated

switch (param_id) {
case SOF_IPC4_SWITCH_CONTROL_PARAM_ID:
if (ctl->id == 0 && ctl->num_elems == 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't control IDs be macros?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, it's a good idea

@singalsu singalsu requested a review from lyakh December 18, 2023 13:39
@cujomalainey
Copy link
Member

@johnylin76 will adding additional controls like this to EQ/MDRC cause any overhead in your offload logic?

@lgirdwood
Copy link
Member

@wszypelt @lrudyX CI been pending a while. Tests passing ? (dont think DRC is tested on Internal CO today).

btian1
btian1 reviewed Dec 20, 2023
View reviewed changes
src/audio/drc/drc.c Show resolved Hide resolved
src/audio/drc/drc.c Show resolved Hide resolved
src/audio/drc/drc.c Outdated
@@ -261,6 +298,9 @@ static int drc_process(struct processing_module *mod,
}
}

/* Control pass-though in processing function with switch control */
cd->enabled = drc_get_config_enabled(cd) && cd->enable_switch;
Copy link
Contributor

Choose a reason for hiding this comment

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

better to directly as:
cd->enabled = cd->config && cd->config->params.enabled
&& cd->enable_switch;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, will change. The function has no other usage.

@wszypelt
Copy link

@lgirdwood
Our current CI is all green.
As for DRC, the module's integration was not synchronized with validation so unfortunately at the moment we do not have any scenarios for this module.

@lgirdwood
Copy link
Member

@singalsu one conflict.

@singalsu
Audio: DRC: Add processing enable switch control
a8bd16a 
This patch adds to DRC component in IPC4 mode a control to
switch processing on/off. The control is useful for DRC
pipeline that is used for both headphone (unprocessed) and
speaker (processed). It also allows the user to switch off
DRC processing if desired.

If a blob has enable set to false the processing cannot be
enabled with the switch control. If the blob enables processing,
the user space can control processing on or off.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu
Tools: Topology2: Add switch control to topologies with DRC
b79eb20 
The mixer control for switch is added to widget definition
of drc.conf.

In cavs-mixin-mixout-efx-hda.conf the existing control name
is changed to have "bytes" similarly as multiband-drc has. The
switch control is added for the widget to implement the switch.

The controls definitions files in benchmark topologies are replaced
to new format from current .conf generator script. The bytes control
is same as before, and the mixer control for switch is added.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu singalsu requested a review from btian1 December 20, 2023 15:32
@lgirdwood lgirdwood merged commit dabf0a6 into thesofproject:main Dec 21, 2023
43 of 44 checks passed
@singalsu singalsu deleted the drc_add_switch_ctrl branch January 4, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@btian1 btian1 btian1 approved these changes

@lyakh lyakh lyakh approved these changes

@ranj063 ranj063 Awaiting requested review from ranj063 ranj063 is a code owner

@jsarha jsarha Awaiting requested review from jsarha jsarha is a code owner

@plbossart plbossart Awaiting requested review from plbossart

@mmaka1 mmaka1 Awaiting requested review from mmaka1

@lbetlej lbetlej Awaiting requested review from lbetlej

@dbaluta dbaluta Awaiting requested review from dbaluta

@kv2019i kv2019i Awaiting requested review from kv2019i

@lgirdwood lgirdwood Awaiting requested review from lgirdwood

@johnylin76 johnylin76 Awaiting requested review from johnylin76

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.9
Development

Successfully merging this pull request may close these issues.
7 participants
@singalsu @cujomalainey @lgirdwood @wszypelt @lyakh @btian1 @johnylin76