rtnr: Decouple switch-enable control from params blob

[johnylin76]

The ideal implementation for the feature component control is to have:

• A switch-typed control for enabling/disabling the feature processing
• A byte-typed control for the feature parameters

Typically the feature parameters should be set only once on DSP init. Updating
new parameters is supported but we should minimize such occurences. A decoupled
switch control provides the perfect solution to enable/disable the feature
without touching params, which is also less cost and error-prone.

Moreover, due to the security request there should be a bypass path under the
switch control, which routes around the feature library for audio samples as
the passthrough.

Hi Realtek folks,
Please help to verify this patch (I only verified the functionality of the switch control).
The example to set the switch value by UCM config:

cset "name='RTNR10.0 rtnr_enable_10' off"
cset "name='RTNR10.0 rtnr_enable_10' on"

You can also set/get via amixer for testing purposes:

amixer -c0 cset name='RTNR10.0 rtnr_enable_10' 1
amixer -c0 cget name='RTNR10.0 rtnr_enable_10'

[johnylin76]

In the aspect of UCM, currently it requires the param blob setting per enabling/disabling. The On/Off.bin is 72 bytes and only one bit different between them.

SectionModifier."Internal Mic Noise Cancellation".0 {
        EnableSequence [
                cdev "hw:sofrt5682"
		cset-tlv "name='RTNR10.0 rtnr_bytes_10' /opt/rtnr/RTNR_On.bin"
        ]
        DisableSequence [
                cdev "hw:sofrt5682"
		cset-tlv "name='RTNR10.0 rtnr_bytes_10' /opt/rtnr/RTNR_Off.bin"
        ]
}

With this PR, we set the param blob once, and enable/disable the component via switch control:

SectionVerb {
	Value {
		FullySpecifiedUCM "1"
	}
	EnableSequence [
		cdev "hw:sofrt5682"
		cset-tlv "name='RTNR10.0 rtnr_bytes_10' /opt/rtnr/RTNR.bin"
        ]
}

SectionModifier."Internal Mic Noise Cancellation".0 {
        EnableSequence [
                cdev "hw:sofrt5682"
		cset "name='RTNR10.0 rtnr_enable_10' on"
        ]
        DisableSequence [
                cdev "hw:sofrt5682"
		cset "name='RTNR10.0 rtnr_enable_10' off"
        ]
}

[cujomalainey]

if this is the only purpose of the bytes control I would suggest we just remove it.

[johnylin76]

Could you rephrase for me? I think both bytes and switch control are still required, aren't they?

[MingJenTai]

Sampling rate is also part of the bytes control.

[johnylin76]

Sorry I get more confused. The comment at line 71

# "RTNR 0" has 2 sink period and 2 source periods

has no relation to bytes control. "sink period" and "source period" are the 3rd and 4th arguments of W_RTNR.
line 73 is the 6th argument for bytes-control, line 74 is the 7th argument for switch-control (mixer). Both of them are required for W_RTNR (refer to m4/rtnr.m4)

[cujomalainey]

just return the function call

[MingJenTai]

@johnylin76 Thanks for the PR!
@cujomalainey Do we have to support toggling on/off on-the-fly during recording? If so I think we should move this part to rtnr_set_value() since copy function is assigned before recording and couldn't be changed on-the-fly.

[johnylin76]

Toggling on/off on-the-fly during recording should be supported. The fact is that it will be handled on CRAS side.
The input device node (e.g. iternal microphone) will be closed and then re-opened transiently while NR state is toggled.

In the aspect of SOF side, RTNR, when the state is being toggled, it will be triggered to stop (comp_reset) and then start (comp_prepare, comp_trigger). So, it is sufficient to make the config and state update just during comp_prepare.

And here I mean is for the toggle of the real use cases, not including the toggle by amixer command because it will not be across CRAS.

[MingJenTai]

Thanks for the description. For developers like us this might be helpful for debugging. Would there be a chance that sof+RTNR is running on the device without NS UI integrated in the OS?

[johnylin76]

Some components would add a config-changed check in comp_copy(), for example:
https://github.com/thesofproject/sof/blob/main/src/audio/eq_iir/eq_iir.c#L825
which makes it able to "dynamically" update config on-the-fly. This is for config blob

I am not sure for switch-typed value change, with the concern that whether the setter (comp_cmd) and the getter (comp_copy) are synchronized. The former is from IPC while the latter is from scheduler. To be honest I am not sure whether the config blob had handled synchronization either.
@singalsu hi Seppo could you help to comment on this?

[singalsu]

There's no controls synchronization in the framework from cmd() to copy(). If a control could create a hazard if it changes in the middle of processing the component should implement some two phase control change. The binary blob is updated in two steps by the model handler so it doesn't need work in the component side. Except if @jsarha 's new single blob feature (for very large ones) is used.

[fuyuntsuo]

@johnylin76 @cujomalainey AFAIK there is a fuzzer stress testing binary control widget. Is it same for switch control widget?

[MingJenTai]

This is because RTNR requires a default sample rate while creating instance.

[johnylin76]

If so I would suggest to defer RTKMA_API_Context_Create() to rtnr_params(), where we could get the sample rate from source and sink buffer without config blob, unless the required sample rate is neither of them. Even so, I am still of the opinion against fetching config in rtnr_new(). Could we use a constant value for the default sample rate?
@cujomalainey WDYT?

[cujomalainey]

Could we simply instantiate on the first call to params and then only recreate as needed if the next params call has a different rate?

[MingJenTai]

Hi @cujomalainey and @johnylin76,

I've tested this patch and it works well on my device. With some modifications, I created another PR #5618 that should complete the proposed feature.

Thanks!

[cujomalainey]

Lets continue on #5618