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

[RTNR] Pass binary data to RTNR #5544

Closed
wants to merge 1 commit into from
Closed

[RTNR] Pass binary data to RTNR #5544

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
38 changes: 33 additions & 5 deletions src/audio/rtnr/rtnr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
#define RTNR_BLK_LENGTH_MASK (RTNR_BLK_LENGTH - 1)
#define RTNR_MAX_SOURCES 1 /* Microphone stream */

#define SOF_RTNR_CONFIG 0

static const struct comp_driver comp_rtnr;

/** \brief RTNR processing functions map item. */
Expand Down Expand Up @@ -399,26 +401,52 @@ static int rtnr_cmd_get_data(struct comp_dev *dev,
return ret;
}

static int rtnr_set_bin_data(struct comp_dev *dev,
struct sof_ipc_ctrl_data *cdata)
{
struct comp_data *cd = comp_get_drvdata(dev);
int ret = 0;

assert(cd);

comp_dbg(dev, "rtnr_set_bin_data() msg_index = %d, num_elems = %d, remaining = %d ",
cdata->msg_index, cdata->num_elems,
cdata->elems_remaining);

if (cdata->data->type == SOF_RTNR_CONFIG) {
comp_dbg(dev, "rtnr_set_bin_data(): SOF_RTNR_CONFIG");
ret = comp_data_blob_set_cmd(cd->model_handler, cdata);
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 two parameters be passed at the same time? Just wondering if the same model handler can be reused or if they have to be separate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the model_handler is used for settings only( sample rate/ on&off ). For other data we would like to modify the internal data in RTNR directly.

Copy link
Member

Choose a reason for hiding this comment

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

@jsarha is updated this API at, @jsarha can we support multiple large blobs per module/component ?
@MingJenTai I think the data would have to be serialized so that we only send one at a time from the host, @jsarha pls correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't work as such, since the handler only maintains one blob per com_data_blob_handler. I think the easiest would adding multiple handlers to the comp_data, one for each blob, and and select the correct one based on component specific type.

if (ret >= 0)
ret = rtnr_check_config_validity(dev, cd);
} else {
comp_dbg(dev, "rtnr_set_bin_data(): SOF_RTNR_DATA type: %d", cdata->data->type);
ret = RTKMA_API_Set_Data(cd->rtk_agl,
cdata->data->data,
cdata->msg_index,
cdata->num_elems,
cdata->elems_remaining,
cdata->data->type);
}

return ret;
}

static int rtnr_cmd_set_data(struct comp_dev *dev,
struct sof_ipc_ctrl_data *cdata)
{
struct comp_data *cd = comp_get_drvdata(dev);
int ret = 0;

switch (cdata->cmd) {
case SOF_CTRL_CMD_BINARY:
comp_info(dev, "rtnr_cmd_set_data(), SOF_CTRL_CMD_BINARY");
ret = comp_data_blob_set_cmd(cd->model_handler, cdata);
ret = rtnr_set_bin_data(dev, cdata);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you stop using comp_data_blob_set_cmd() for some cases and implement the message handling for those your self, you should do the same thing symmetrically for comp_data_blob_get_cmd(). One alternative would not to use the comp_data_blob_handler at all and write all message fragment handling your self.

break;
default:
comp_err(dev, "rtnr_cmd_set_data() error: invalid command %d", cdata->cmd);
ret = -EINVAL;
break;
}

if (ret >= 0)
ret = rtnr_check_config_validity(dev, cd);

return ret;
}

Expand Down
6 changes: 6 additions & 0 deletions src/include/sof/audio/rtnr/rtklib/include/RTK_MA_API.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ int RTKMA_API_Set(void *Context, const void *pParameters, int size, unsigned int

int RTKMA_API_Get(void *Context, void *pParameters, int size, unsigned int IDs);

int RTKMA_API_Set_Data(void *Context, unsigned int *data,
unsigned int msg_index,
unsigned int num_elms,
unsigned int elems_remaining,
unsigned int data_type);

Copy link
Member

Choose a reason for hiding this comment

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

When should we use 1 and when should we use 2 ? can the be better named so users can understand the usage better. I would also put some comments explaining the difference and usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cujomalainey @lgirdwood

Thanks for the review. We'll combine those 2 APIs into 1 to avoid confusion.

Another question is what's the best practice to send binary configuration file to sof component during system startup? The config is about 2KB, and should only be set once.

We've been using sof-ctl which works well, but the same configuration will be sent every time before recording. Besides, using sof-ctl might not be a proper way for final product.

We've also tried using cset-tlv to send config just like the control blobs for the UI. However the config is much larger than control blobs and I have no success during my experiments. Is there file size limit for this?

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just force-pushed a commit to simplify the Set_Data API.

There could be different types of the data, but yes they should be handled internally in library.

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

You could have a the config data as a static data block as part of the RTNT. This would be build into the DATA section and could be used by default ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes currently there is a default config stored in the static table inside the firmware. The proposed method provides a way to overwrite the default config so that we don't have to re-build firmware each time when a new config is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using my PR #5509 you could use the new comp_data_blob_handler_new_ext() with single_blob = true and tap into alloc() and free() callbacks to return always the pointer to the static buffer (and return an error if the requested size is bigger than the buffer).

With this approach you should keep using the comp_data_blob_handler and friends mostly the way the other components do. The IPC fragment aggregating would be handled by data_blob.c functions. Just before processing call you should check with comp_is_current_data_blob_valid() if the configuration buffer is currently valid, or if it is in the middle of an update.

#ifdef __cplusplus
}
#endif
Expand Down
3 changes: 2 additions & 1 deletion src/include/user/rtnr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

#include <stdint.h>

#define SOF_RTNR_MAX_SIZE 256
#define SOF_RTNR_MAX_SIZE 8192

struct sof_rtnr_params {
/* 1 to enable RTNR, 0 to disable it */
Expand All @@ -26,6 +26,7 @@ struct sof_rtnr_config {
uint32_t reserved[4];

struct sof_rtnr_params params;
int32_t data[];
Copy link
Member

@lgirdwood lgirdwood Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Wont you need a size here to tell client code how big data[] is ? or maybe this in fully handled in blob API @jsarha please comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The data is with variable length. Actual size is stored in the size flag.

} __attribute__((packed, aligned(4)));

#endif /* __USER_RTNR_H__ */
Expand Down
47 changes: 45 additions & 2 deletions tools/topology/topology1/sof/pipe-rtnr-capture-16khz.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ ifdef(`RTNR_BUFFER_SIZE_MAX',`', define(RTNR_BUFFER_SIZE_MAX, `65536'))

define(DEF_RTNR_PRIV, concat(`rtnr_priv_', PIPELINE_ID))
define(DEF_RTNR_BYTES, concat(`rtnr_bytes_', PIPELINE_ID))
define(DEF_RTNR_DATA_1, concat(`rtnr_data_1', PIPELINE_ID))
define(DEF_RTNR_DATA_1_BYTES, concat(`rtnr_data_1_bytes_', PIPELINE_ID))
define(DEF_RTNR_DATA_2, concat(`rtnr_data_2', PIPELINE_ID))
define(DEF_RTNR_DATA_2_BYTES, concat(`rtnr_data_2_bytes_', PIPELINE_ID))

CONTROLBYTES_PRIV(DEF_RTNR_PRIV,
` bytes "0x53,0x4f,0x46,0x00,0x00,0x00,0x00,0x00,'
Expand All @@ -41,10 +45,45 @@ C_CONTROLBYTES(DEF_RTNR_BYTES, PIPELINE_ID,
CONTROLBYTES_OPS(bytes, 258 binds the mixer control to bytes get/put handlers, 258, 258),
CONTROLBYTES_EXTOPS(258 binds the mixer control to bytes get/put handlers, 258, 258),
, , ,
CONTROLBYTES_MAX(, 256),
CONTROLBYTES_MAX(, 8192),
,
DEF_RTNR_PRIV)

CONTROLBYTES_PRIV(DEF_RTNR_DATA_1,
` bytes "0x53,0x4f,0x46,0x00,0x01,0x00,0x00,0x00,'
` 0x10,0x00,0x00,0x00,0x00,0x30,0x01,0x03,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x52,0x54,0x4b,0x00,0x00,0x00,0x00,0x00,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00"'
)

# Bytes control for RTNR Data 1 blob
C_CONTROLBYTES(DEF_RTNR_DATA_1_BYTES, PIPELINE_ID,
CONTROLBYTES_OPS(bytes, 258 binds the mixer control to bytes get/put handlers, 258, 258),
CONTROLBYTES_EXTOPS(258 binds the mixer control to bytes get/put handlers, 258, 258),
, , ,
CONTROLBYTES_MAX(, 10240),
,
DEF_RTNR_DATA_1)

CONTROLBYTES_PRIV(DEF_RTNR_DATA_2,
` bytes "0x53,0x4f,0x46,0x00,0x02,0x00,0x00,0x00,'
` 0x10,0x00,0x00,0x00,0x00,0x30,0x01,0x03,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x52,0x54,0x4b,0x00,0x00,0x00,0x00,0x00,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00"'
)

# Bytes control for RTNR Data 2 blob
C_CONTROLBYTES(DEF_RTNR_DATA_2_BYTES, PIPELINE_ID,
CONTROLBYTES_OPS(bytes, 258 binds the mixer control to bytes get/put handlers, 258, 258),
CONTROLBYTES_EXTOPS(258 binds the mixer control to bytes get/put handlers, 258, 258),
, , ,
CONTROLBYTES_MAX(, 524288),
,
DEF_RTNR_DATA_2)

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

# "RTNR 0" has 2 sink period and 2 source periods
W_RTNR(0, PIPELINE_FORMAT, 2, DAI_PERIODS, DMIC_PIPELINE_16k_CORE_ID, LIST(` ', "DEF_RTNR_BYTES"))
W_RTNR(0, PIPELINE_FORMAT, 2, DAI_PERIODS, DMIC_PIPELINE_16k_CORE_ID, LIST(` ', "DEF_RTNR_BYTES", "DEF_RTNR_DATA_1_BYTES", "DEF_RTNR_DATA_2_BYTES"))

# Capture Buffers
W_BUFFER(0, COMP_BUFFER_SIZE(4,
Expand Down Expand Up @@ -88,3 +127,7 @@ PCM_CAPABILITIES(Capture PCM_ID, CAPABILITY_FORMAT_NAME(PIPELINE_FORMAT),

undefine(`DEF_RTNR_PRIV')
undefine(`DEF_RTNR_BYTES')
undefine(`DEF_RTNR_DATA_1')
undefine(`DEF_RTNR_DATA_1_BYTES')
undefine(`DEF_RTNR_DATA_2')
undefine(`DEF_RTNR_DATA_2_BYTES')
47 changes: 45 additions & 2 deletions 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 @@ -24,6 +24,10 @@ ifdef(`RTNR_BUFFER_SIZE_MAX',`', define(RTNR_BUFFER_SIZE_MAX, `65536'))

define(DEF_RTNR_PRIV, concat(`rtnr_priv_', PIPELINE_ID))
define(DEF_RTNR_BYTES, concat(`rtnr_bytes_', PIPELINE_ID))
define(DEF_RTNR_DATA_1, concat(`rtnr_data_1', PIPELINE_ID))
define(DEF_RTNR_DATA_1_BYTES, concat(`rtnr_data_1_bytes_', PIPELINE_ID))
define(DEF_RTNR_DATA_2, concat(`rtnr_data_2', PIPELINE_ID))
define(DEF_RTNR_DATA_2_BYTES, concat(`rtnr_data_2_bytes_', PIPELINE_ID))

CONTROLBYTES_PRIV(DEF_RTNR_PRIV,
` bytes "0x53,0x4f,0x46,0x00,0x00,0x00,0x00,0x00,'
Expand All @@ -41,10 +45,45 @@ C_CONTROLBYTES(DEF_RTNR_BYTES, PIPELINE_ID,
CONTROLBYTES_OPS(bytes, 258 binds the mixer control to bytes get/put handlers, 258, 258),
CONTROLBYTES_EXTOPS(258 binds the mixer control to bytes get/put handlers, 258, 258),
, , ,
CONTROLBYTES_MAX(, 256),
CONTROLBYTES_MAX(, 8192),
,
DEF_RTNR_PRIV)

CONTROLBYTES_PRIV(DEF_RTNR_DATA_1,
` bytes "0x53,0x4f,0x46,0x00,0x01,0x00,0x00,0x00,'
` 0x10,0x00,0x00,0x00,0x00,0x30,0x01,0x03,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x52,0x54,0x4b,0x00,0x00,0x00,0x00,0x00,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00"'
)

# Bytes control for RTNR Data 1 blob
C_CONTROLBYTES(DEF_RTNR_DATA_1_BYTES, PIPELINE_ID,
CONTROLBYTES_OPS(bytes, 258 binds the mixer control to bytes get/put handlers, 258, 258),
CONTROLBYTES_EXTOPS(258 binds the mixer control to bytes get/put handlers, 258, 258),
, , ,
CONTROLBYTES_MAX(, 10240),
,
DEF_RTNR_DATA_1)

CONTROLBYTES_PRIV(DEF_RTNR_DATA_2,
` bytes "0x53,0x4f,0x46,0x00,0x02,0x00,0x00,0x00,'
` 0x10,0x00,0x00,0x00,0x00,0x30,0x01,0x03,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,'
` 0x52,0x54,0x4b,0x00,0x00,0x00,0x00,0x00,'
` 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00"'
)

# Bytes control for RTNR Data 2 blob
C_CONTROLBYTES(DEF_RTNR_DATA_2_BYTES, PIPELINE_ID,
CONTROLBYTES_OPS(bytes, 258 binds the mixer control to bytes get/put handlers, 258, 258),
CONTROLBYTES_EXTOPS(258 binds the mixer control to bytes get/put handlers, 258, 258),
, , ,
CONTROLBYTES_MAX(, 524288),
,
DEF_RTNR_DATA_2)

#
# Components and Buffers
Expand All @@ -55,7 +94,7 @@ 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", "DEF_RTNR_DATA_1_BYTES", "DEF_RTNR_DATA_2_BYTES"))

# Capture Buffers
W_BUFFER(0, COMP_BUFFER_SIZE(4,
Expand Down Expand Up @@ -88,3 +127,7 @@ PCM_CAPABILITIES(Capture PCM_ID, CAPABILITY_FORMAT_NAME(PIPELINE_FORMAT),

undefine(`DEF_RTNR_PRIV')
undefine(`DEF_RTNR_BYTES')
undefine(`DEF_RTNR_DATA_1')
undefine(`DEF_RTNR_DATA_1_BYTES')
undefine(`DEF_RTNR_DATA_2')
undefine(`DEF_RTNR_DATA_2_BYTES')