Skip to content

rtnr: Decouple switch-enable control from params blob #5616

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

Closed
johnylin76 wants to merge 1 commit into from
Closed

rtnr: Decouple switch-enable control from params blob #5616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 93 additions & 27 deletions src/audio/rtnr/rtnr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,21 +181,25 @@ static rtnr_func rtnr_find_func(enum sof_ipc_frame fmt)
return NULL;
}

/* TODO(Realtek): theoretically this is not needed anymore, please remove it after
* verification
*/
static inline void rtnr_set_process(struct comp_dev *dev)
{
comp_info(dev, "rtnr_set_process()");
struct comp_data *cd = comp_get_drvdata(dev);

cd->process_enable = true;
RTKMA_API_Bypass(cd->rtk_agl, 0);
}

/* TODO(Realtek): theoretically this is not needed anymore, please remove it after
* verification
*/
static inline void rtnr_set_bypass(struct comp_dev *dev)
{
comp_info(dev, "rtnr_set_bypass()");
struct comp_data *cd = comp_get_drvdata(dev);

cd->process_enable = false;
RTKMA_API_Bypass(cd->rtk_agl, 1);
}

Expand All @@ -207,6 +211,11 @@ static inline void rtnr_set_process_sample_rate(struct comp_dev *dev, uint32_t s
cd->process_sample_rate = sample_rate;
}

/* TODO(Realtek): since we need to decouple the config and the enabled flag, the RTNR
* param blob shouldn't contain "enabled". If RTNR needs to know the
* enabled state currently, we could use another API to inform RTNR while
* the state is switched.
*/
static int32_t rtnr_check_config_validity(struct comp_dev *dev,
struct comp_data *cd)
{
Expand All @@ -221,11 +230,6 @@ static int32_t rtnr_check_config_validity(struct comp_dev *dev,
p_config->params.enabled,
p_config->params.sample_rate);

if (p_config->params.enabled)
rtnr_set_process(dev);
else
rtnr_set_bypass(dev);

rtnr_set_process_sample_rate(dev, p_config->params.sample_rate);
}

Expand Down Expand Up @@ -270,6 +274,8 @@ static struct comp_dev *rtnr_new(const struct comp_driver *drv,

comp_set_drvdata(dev, cd);

cd->process_enable = false;

/* Handler for configuration data */
cd->model_handler = comp_data_blob_handler_new(dev);
if (!cd->model_handler) {
Expand All @@ -287,7 +293,14 @@ static struct comp_dev *rtnr_new(const struct comp_driver *drv,
/* Component defaults */
cd->source_channel = 0;

/* Check config */
/* TODO(Realtek): it doesn't make sense to fetch config during rtnr_new() because
* new() is always the first function called for the component. The
* fetched config here is the default config specified in topology.
* I suspect that the config blob given in UCM is the one RTNR
* requires, instead of the default one.
* Please move to rtnr_prepare().
*/
/* Check config */
Copy link
Copy Markdown
Contributor

@MingJenTai MingJenTai Mar 30, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@johnylin76 johnylin76 Mar 30, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@cujomalainey cujomalainey Mar 30, 2022

Choose a reason for hiding this comment

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

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?

ret = rtnr_check_config_validity(dev, cd);
if (ret < 0) {
comp_cl_err(&comp_rtnr, "rtnr_new(): rtnr_check_config_validity() failed.");
Expand Down Expand Up @@ -416,6 +429,7 @@ static int rtnr_cmd_set_data(struct comp_dev *dev,
break;
}

/* TODO(Realtek): this should be postponed to rtnr_prepare() */
if (ret >= 0)
ret = rtnr_check_config_validity(dev, cd);

Expand Down Expand Up @@ -450,16 +464,20 @@ static int32_t rtnr_set_value(struct comp_dev *dev, struct sof_ipc_ctrl_data *cd
{
uint32_t val = 0;
int32_t j;
struct comp_data *cd = comp_get_drvdata(dev);

for (j = 0; j < cdata->num_elems; j++) {
val |= cdata->chanv[j].value;
comp_info(dev, "rtnr_set_value(), value = %u", val);
}

if (val)
if (val) {
rtnr_set_process(dev);
else
cd->process_enable = true;
} else {
rtnr_set_bypass(dev);
cd->process_enable = false;
}

return 0;
}
Expand Down Expand Up @@ -522,7 +540,7 @@ static int rtnr_trigger(struct comp_dev *dev, int cmd)
}

/* copy and process stream data from source to sink buffers */
static int rtnr_copy(struct comp_dev *dev)
static int rtnr_process(struct comp_dev *dev)
{
struct comp_buffer *sink;
struct comp_buffer *source;
Expand All @@ -532,7 +550,7 @@ static int rtnr_copy(struct comp_dev *dev)
const struct audio_stream *sources_stream[RTNR_MAX_SOURCES] = { NULL };
struct comp_data *cd = comp_get_drvdata(dev);

comp_dbg(dev, "rtnr_copy()");
comp_dbg(dev, "rtnr_process()");

source = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list);

Expand All @@ -545,7 +563,7 @@ static int rtnr_copy(struct comp_dev *dev)
/* Process integer multiple of RTNR internal block length */
frames = frames & ~RTNR_BLK_LENGTH_MASK;

comp_dbg(dev, "rtnr_copy() source->id: %d, frames = %d", source->id, frames);
comp_dbg(dev, "rtnr_process() source->id: %d, frames = %d", source->id, frames);

if (frames) {
source_bytes = frames * audio_stream_frame_bytes(&source->stream);
Expand Down Expand Up @@ -579,6 +597,46 @@ static int rtnr_copy(struct comp_dev *dev)
return 0;
}

/* copy stream data from source to sink buffers without RTNR involvement */
static int rtnr_passthrough(struct comp_dev *dev)
{
struct comp_copy_limits cl;
struct comp_buffer *source;
struct comp_buffer *sink;

comp_dbg(dev, "rtnr_passthrough()");

source = list_first_item(&dev->bsource_list, struct comp_buffer,
sink_list);
sink = list_first_item(&dev->bsink_list, struct comp_buffer,
source_list);

/* Get source, sink, number of frames etc. to process. */
comp_get_copy_limits_with_lock(source, sink, &cl);

buffer_stream_invalidate(source, cl.source_bytes);

audio_stream_copy(&source->stream, 0, &sink->stream, 0,
source->stream.channels * cl.frames);

buffer_stream_writeback(sink, cl.sink_bytes);
comp_update_buffer_consume(source, cl.source_bytes);
comp_update_buffer_produce(sink, cl.sink_bytes);

return 0;
}

static int rtnr_copy(struct comp_dev *dev)
{
struct comp_data *cd = comp_get_drvdata(dev);
int ret;

comp_dbg(dev, "rtnr_copy()");

ret = cd->rtnr_copy_func(dev);
Copy link
Copy Markdown
Contributor

@cujomalainey cujomalainey Mar 29, 2022

Choose a reason for hiding this comment

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

just return the function call

return ret;
}

static int rtnr_prepare(struct comp_dev *dev)
{
struct comp_data *cd = comp_get_drvdata(dev);
Expand All @@ -594,23 +652,30 @@ static int rtnr_prepare(struct comp_dev *dev)
if (ret == COMP_STATUS_STATE_ALREADY_SET)
return PPL_STATUS_PATH_STOP;

/* Get sink data format */
sinkb = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
cd->sink_format = sinkb->stream.frame_fmt;
/* TODO(Realtek): move the config fetch codes here */

/* Check source and sink PCM format and get copy function */
comp_info(dev, "rtnr_prepare(), sink_format=%d", cd->sink_format);
cd->rtnr_func = rtnr_find_func(cd->sink_format);
if (!cd->rtnr_func) {
comp_err(dev, "rtnr_prepare(): No suitable processing function found.");
goto err;
}
if (cd->process_enable) {
/* Get sink data format */
sinkb = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
cd->sink_format = sinkb->stream.frame_fmt;

/* Default on */
cd->process_enable = true;
/* Check source and sink PCM format and get copy function */
comp_info(dev, "rtnr_prepare(), sink_format=%d", cd->sink_format);
cd->rtnr_copy_func = rtnr_process;
cd->rtnr_func = rtnr_find_func(cd->sink_format);
if (!cd->rtnr_func) {
comp_err(dev, "rtnr_prepare(): No suitable processing function found.");
goto err;
}

/* Clear in/out buffers */
RTKMA_API_Prepare(cd->rtk_agl);
/* Clear in/out buffers */
RTKMA_API_Prepare(cd->rtk_agl);
} else {
/* Bypassthrough path is selected */
comp_info(dev, "rtnr_prepare(), passthrough mode");
cd->rtnr_copy_func = rtnr_passthrough;
cd->rtnr_func = NULL;
}
Copy link
Copy Markdown
Contributor

@MingJenTai MingJenTai Mar 30, 2022

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Contributor Author

@johnylin76 johnylin76 Mar 30, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@MingJenTai MingJenTai Mar 30, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@johnylin76 johnylin76 Mar 30, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Collaborator

@singalsu singalsu Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.


/* Initialize RTNR */
return 0;
Expand All @@ -627,6 +692,7 @@ static int rtnr_reset(struct comp_dev *dev)
comp_info(dev, "rtnr_reset()");
cd->sink_format = 0;
cd->rtnr_func = NULL;
cd->rtnr_copy_func = NULL;
comp_set_state(dev, COMP_TRIGGER_RESET);
return 0;
}
Expand Down
3 changes: 3 additions & 0 deletions src/include/sof/audio/rtnr/rtnr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ typedef void (*rtnr_func)(struct comp_dev *dev,
struct audio_stream *sink,
int frames);

typedef int (*rtnr_copy_func)(struct comp_dev *dev);

/* RTNR component private data */
struct comp_data {
struct comp_data_blob_handler *model_handler;
Expand All @@ -38,6 +40,7 @@ struct comp_data {
int ref_shift;
bool ref_32bits;
bool ref_active;
rtnr_copy_func rtnr_copy_func; /** Processing or passthrough */
rtnr_func rtnr_func; /** Processing function */
void *rtk_agl;
};
Expand Down
3 changes: 3 additions & 0 deletions tools/topology/topology1/m4/rtnr.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ define(`W_RTNR',
` bytes ['
$6
` ]'
` mixer ['
$7
` ]'
`}')

divert(0)dnl
19 changes: 18 additions & 1 deletion tools/topology/topology1/sof/pipe-rtnr-capture.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ include(`buffer.m4')
include(`pcm.m4')
include(`dai.m4')
include(`pipeline.m4')
include(`mixercontrol.m4')
include(`bytecontrol.m4')
include(`rtnr.m4')

Expand Down Expand Up @@ -45,6 +46,19 @@ C_CONTROLBYTES(DEF_RTNR_BYTES, PIPELINE_ID,
,
DEF_RTNR_PRIV)

# RTNR Enable switch
define(DEF_RTNR_ENABLE, concat(`rtnr_enable_', PIPELINE_ID))
define(`CONTROL_NAME', `DEF_RTNR_ENABLE')

C_CONTROLMIXER(DEF_RTNR_ENABLE, PIPELINE_ID,
CONTROLMIXER_OPS(volsw, 259 binds the mixer control to switch get/put handlers, 259, 259),
CONTROLMIXER_MAX(max 1 indicates switch type control, 1),
false,
,
Channel register and shift for Front Center,
LIST(` ', KCONTROL_CHANNEL(FC, 3, 0)),
"1")
undefine(`CONTROL_NAME')

#
# Components and Buffers
Expand All @@ -55,7 +69,9 @@ C_CONTROLBYTES(DEF_RTNR_BYTES, PIPELINE_ID,
W_PCM_CAPTURE(PCM_ID, Capture, 0, 2, SCHEDULE_CORE)

# "RTNR 0" has 2 sink period and 2 source periods
W_RTNR(0, PIPELINE_FORMAT, 2, DAI_PERIODS, SCHEDULE_CORE, LIST(` ', "DEF_RTNR_BYTES"))
W_RTNR(0, PIPELINE_FORMAT, 2, DAI_PERIODS, SCHEDULE_CORE,
LIST(` ', "DEF_RTNR_BYTES"),
Copy link
Copy Markdown
Contributor

@cujomalainey cujomalainey Mar 29, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@johnylin76 johnylin76 Mar 30, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@MingJenTai MingJenTai Mar 30, 2022

Choose a reason for hiding this comment

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

Sampling rate is also part of the bytes control.

Copy link
Copy Markdown
Contributor Author

@johnylin76 johnylin76 Mar 30, 2022

Choose a reason for hiding this comment

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

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)

LIST(` ', "DEF_RTNR_ENABLE"))

# Capture Buffers
W_BUFFER(0, COMP_BUFFER_SIZE(4,
Expand Down Expand Up @@ -86,5 +102,6 @@ PCM_CAPABILITIES(Capture PCM_ID, CAPABILITY_FORMAT_NAME(PIPELINE_FORMAT),
PCM_MIN_RATE, PCM_MAX_RATE, PIPELINE_CHANNELS, PIPELINE_CHANNELS,
2, 16, 192, 16384, RTNR_BUFFER_SIZE_MIN, RTNR_BUFFER_SIZE_MAX)

undefine(`DEF_RTNR_ENABLE')
undefine(`DEF_RTNR_PRIV')
undefine(`DEF_RTNR_BYTES')