Skip to content

P20 eq iir - convert the eq_iir module to only use the sink/source api #11234

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

Merged
lgirdwood merged 1 commit into from
Oct 13, 2026
Merged

P20 eq iir - convert the eq_iir module to only use the sink/source api #11234

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
122 changes: 93 additions & 29 deletions src/audio/eq_iir/eq_iir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <sof/audio/buffer.h>
#include <sof/audio/format.h>
#include <sof/audio/pipeline.h>
#include <sof/audio/sink_source_utils.h>
#include <sof/audio/ipc-config.h>
#include <sof/common.h>
#include <rtos/panic.h>
Expand Down Expand Up @@ -45,7 +46,7 @@ static int eq_iir_init(struct processing_module *mod)
struct module_data *md = &mod->priv;
struct comp_dev *dev = mod->dev;
struct comp_data *cd;
int i;
unsigned int i;

comp_info(dev, "entry");

Expand Down Expand Up @@ -113,15 +114,32 @@ static int eq_iir_get_config(struct processing_module *mod,
}

static int eq_iir_process(struct processing_module *mod,
struct input_stream_buffer *input_buffers, int num_input_buffers,
struct output_stream_buffer *output_buffers, int num_output_buffers)
struct sof_source **sources, int num_input_buffers,
struct sof_sink **sinks, int num_output_buffers)
{
struct comp_data *cd = module_get_private_data(mod);
struct audio_stream *source = input_buffers[0].data;
struct audio_stream *sink = output_buffers[0].data;
uint32_t frame_count = input_buffers[0].size;
struct sof_source *source;
struct sof_sink *sink;
struct cir_buf_source source_buf;
struct cir_buf_sink sink_buf;
size_t source_frame_bytes;
size_t sink_frame_bytes;
size_t source_bytes;
size_t sink_bytes;
size_t source_buf_size;
size_t sink_buf_size;

@softwarecki softwarecki Oct 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

source_frame_bytes and sink_frame_bytes seem unnecessary, as they are used only once. The corresponding expressions could be inlined at the call sites, which would reduce the number of temporary variables.

source_buf_size and sink_buf_size could be merged into a single variable.

@lgirdwood lgirdwood Oct 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@piotrhoppeintel can you do this as a followup, I'd like to give full e2e P20 pipelines a good run in CI to shake out any integration issues over next few weeks.

size_t frame_count;
int ret;

if (num_input_buffers != 1 || num_output_buffers != 1) {

@lyakh lyakh Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we commonly check this in all / most modules? This is a hot path, wouldn't it be enough to check this where buffers / connections are configured?

@lgirdwood lgirdwood Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ack, we could actually put this in a cold path and have a simpler helper too. This shouldn't block this, @piotrhoppeintel we should follow up in another PR as I suspect we have copy and paste for this in more than 1 place.

@softwarecki softwarecki Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We dont need to check it every process call. One check in prepare should be fine.

comp_err(mod->dev, "EQ IIR supports one source and one sink");
return -EINVAL;
}

source = sources[0];
sink = sinks[0];
cd->frame_bytes = source_get_frame_bytes(source);

/* Check for changed configuration. The IPC-time validator installed
* in eq_iir_init() has already structurally validated the blob, so
* only NULL needs to be guarded here.
Expand All @@ -131,67 +149,111 @@ static int eq_iir_process(struct processing_module *mod,
if (!cd->config)
return -EINVAL;

ret = eq_iir_new_blob(mod, audio_stream_get_frm_fmt(source),
audio_stream_get_frm_fmt(sink),
audio_stream_get_channels(source));
ret = eq_iir_new_blob(mod, source_get_frm_fmt(source),
sink_get_frm_fmt(sink),
source_get_channels(source));
if (ret)
return ret;
}

if (frame_count) {
cd->eq_iir_func(mod, &input_buffers[0], &output_buffers[0], frame_count);
module_update_buffer_position(&input_buffers[0], &output_buffers[0], frame_count);
if (!cd->eq_iir_func)
return -EINVAL;

frame_count = source_sink_avail_frames_aligned(source, sink);
if (!frame_count)
return 0;

source_frame_bytes = source_get_frame_bytes(source);
sink_frame_bytes = sink_get_frame_bytes(sink);
source_bytes = frame_count * source_frame_bytes;
sink_bytes = frame_count * sink_frame_bytes;

ret = source_get_data(source, source_bytes, &source_buf.ptr,
&source_buf.buf_start, &source_buf_size);
if (ret < 0)
return ret;
source_buf.buf_end = (const char *)source_buf.buf_start + source_buf_size;

ret = sink_get_buffer(sink, sink_bytes, &sink_buf.ptr,
&sink_buf.buf_start, &sink_buf_size);
if (ret < 0) {
source_release_data(source, 0);
return ret;
}
return 0;
sink_buf.buf_end = (char *)sink_buf.buf_start + sink_buf_size;

cd->eq_iir_func(mod, &source_buf, &sink_buf, frame_count);

ret = source_release_data(source, source_bytes);
if (ret < 0) {
sink_commit_buffer(sink, 0);
return ret;
}

return sink_commit_buffer(sink, sink_bytes);
}

/**
* \brief Set EQ IIR frames alignment limit.
* \param[in,out] source Structure pointer of source.
* \param[in,out] sink Structure pointer of sink.
*/
static void eq_iir_set_alignment(struct audio_stream *source,
struct audio_stream *sink)
static int eq_iir_set_alignment(struct sof_source *source,
struct sof_sink *sink)
{
const uint32_t byte_align = SOF_FRAME_BYTE_ALIGN;
const uint32_t frame_align_req = 2;
int ret;

ret = source_set_alignment_constants(source, byte_align, frame_align_req);
if (ret < 0)
return ret;

audio_stream_set_align(byte_align, frame_align_req, source);
audio_stream_set_align(byte_align, frame_align_req, sink);
return sink_set_alignment_constants(sink, byte_align, frame_align_req);
}

static int eq_iir_prepare(struct processing_module *mod,
struct sof_source **sources, int num_of_sources,
struct sof_sink **sinks, int num_of_sinks)
{
struct comp_data *cd = module_get_private_data(mod);
struct comp_buffer *sourceb, *sinkb;
struct comp_dev *dev = mod->dev;
struct sof_source *source;
struct sof_sink *sink;
enum sof_ipc_frame source_format;
enum sof_ipc_frame sink_format;
int channels;
unsigned int channels;
int ret = 0;

comp_dbg(dev, "entry");

/* EQ component will only ever have 1 source and 1 sink buffer */
sourceb = comp_dev_get_first_data_producer(dev);
sinkb = comp_dev_get_first_data_consumer(dev);
if (!sourceb || !sinkb) {
comp_err(dev, "no source or sink buffer");
/* EQ component will only ever have 1 source and 1 sink buffer. */
if (num_of_sources != 1 || num_of_sinks != 1) {
comp_err(dev, "EQ IIR supports one source and one sink");
return -ENOTCONN;
}

source = sources[0];
sink = sinks[0];

ret = eq_iir_prepare_sub(mod);
if (ret < 0)
return ret;

eq_iir_set_alignment(&sourceb->stream, &sinkb->stream);
ret = eq_iir_set_alignment(source, sink);
if (ret < 0)
return ret;

/* get source and sink data format */
channels = audio_stream_get_channels(&sinkb->stream);
source_format = audio_stream_get_frm_fmt(&sourceb->stream);
sink_format = audio_stream_get_frm_fmt(&sinkb->stream);
channels = source_get_channels(source);
source_format = source_get_frm_fmt(source);
sink_format = sink_get_frm_fmt(sink);
if (channels != sink_get_channels(sink)) {
comp_err(dev, "source and sink channel counts do not match");
return -EINVAL;
}
cd->channels = channels;

@softwarecki softwarecki Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

cd is filled here, so doing it in process is redundant.

@piotrhoppeintel piotrhoppeintel Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. The redundant in process function has been removed.

cd->frame_bytes = source_get_frame_bytes(source);

cd->config = comp_get_data_blob(cd->model_handler, &cd->config_size, NULL);

Expand Down Expand Up @@ -224,6 +286,8 @@ static int eq_iir_reset(struct processing_module *mod)
eq_iir_free_delaylines(mod);

cd->eq_iir_func = NULL;
cd->channels = 0;
cd->frame_bytes = 0;
for (i = 0; i < PLATFORM_MAX_CHANNELS; i++)
iir_reset_df1(&cd->iir[i]);

Expand All @@ -233,7 +297,7 @@ static int eq_iir_reset(struct processing_module *mod)
static const struct module_interface eq_iir_interface = {
.init = eq_iir_init,
.prepare = eq_iir_prepare,
.process_audio_stream = eq_iir_process,
.process = eq_iir_process,
.set_configuration = eq_iir_set_config,
.get_configuration = eq_iir_get_config,
.reset = eq_iir_reset,
Expand Down
32 changes: 16 additions & 16 deletions src/audio/eq_iir/eq_iir.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,17 @@

#include <stdint.h>
#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/audio_stream.h>
#include <sof/math/iir_df2t.h>
#include <sof/math/iir_df1.h>

/** \brief Macros to convert without division bytes count to samples count */
#define EQ_IIR_BYTES_TO_S16_SAMPLES(b) ((b) >> 1)
#define EQ_IIR_BYTES_TO_S32_SAMPLES(b) ((b) >> 2)

struct audio_stream;
struct comp_dev;

/** \brief Type definition for processing function select return value. */
typedef void (*eq_iir_func)(struct processing_module *mod, struct input_stream_buffer *bsource,
struct output_stream_buffer *bsink, uint32_t frames);
typedef void (*eq_iir_func)(struct processing_module *mod,
const struct cir_buf_source *source,
struct cir_buf_sink *sink, size_t frames);

/** \brief IIR EQ processing functions map item. */
struct eq_iir_func_map {
Expand All @@ -41,35 +39,37 @@ struct comp_data {
int32_t *iir_delay; /**< pointer to allocated RAM */
size_t config_size; /**< configuration size */
size_t iir_delay_size; /**< allocated size */
unsigned int channels; /**< number of channels */
size_t frame_bytes; /**< source frame size */
eq_iir_func eq_iir_func; /**< processing function */
};

#ifdef UNIT_TEST
void sys_comp_module_eq_iir_interface_init(void);
#endif

void eq_iir_s16_default(struct processing_module *mod, struct input_stream_buffer *bsource,
struct output_stream_buffer *bsink, uint32_t frames);
void eq_iir_s16_default(struct processing_module *mod, const struct cir_buf_source *source,
struct cir_buf_sink *sink, size_t frames);

void eq_iir_s24_default(struct processing_module *mod, struct input_stream_buffer *bsource,
struct output_stream_buffer *bsink, uint32_t frames);
void eq_iir_s24_default(struct processing_module *mod, const struct cir_buf_source *source,
struct cir_buf_sink *sink, size_t frames);

void eq_iir_s32_default(struct processing_module *mod, struct input_stream_buffer *bsource,
struct output_stream_buffer *bsink, uint32_t frames);
void eq_iir_s32_default(struct processing_module *mod, const struct cir_buf_source *source,
struct cir_buf_sink *sink, size_t frames);

int eq_iir_new_blob(struct processing_module *mod, enum sof_ipc_frame source_format,
enum sof_ipc_frame sink_format, int channels);
enum sof_ipc_frame sink_format, unsigned int channels);

void eq_iir_set_passthrough_func(struct comp_data *cd,
enum sof_ipc_frame source_format,
enum sof_ipc_frame sink_format);

int eq_iir_prepare_sub(struct processing_module *mod);

void eq_iir_pass(struct processing_module *mod, struct input_stream_buffer *bsource,
struct output_stream_buffer *bsink, uint32_t frames);
void eq_iir_pass(struct processing_module *mod, const struct cir_buf_source *source,
struct cir_buf_sink *sink, size_t frames);

int eq_iir_setup(struct processing_module *mod, int nch);
int eq_iir_setup(struct processing_module *mod, unsigned int nch);

int eq_iir_validate_config(struct comp_dev *dev, void *new_data, uint32_t new_data_size);

Expand Down
Loading
Loading