P20 eq fir convert the eq_fir module to only use the sink/source api by piotrhoppeintel · Pull Request #11221 · thesofproject/sof

piotrhoppeintel · 2026-09-22T09:17:22Z

Rework the eq_fir module to only use the sink/source api to
prepare the SOF for the full transition to pipeline 2.0.

Copilot

Copilot review overview

🟡 Changes recommended

eq_fir_process() returns -ENOSPC for fatal buffer-capacity mismatches, which can be silently treated as non-fatal by module_process_sink_src(), masking real errors and risking stalled processing.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Avoid masking undersized buffer errors as non-fatal ENOSPC · New
Update PR description to reflect EQ FIR module scope · New

What changed in this PR

Converts the eq_fir audio module (and its cmocka coverage) from legacy audio_stream buffer handling to the sink/source API, aligning it with the pipeline 2.0 direction.

Changes:

Switch eq_fir processing entrypoint to module_interface.process() and implement processing using sof_source/sof_sink acquire/commit APIs.
Update FIR inner-loop implementations (generic + HiFi variants) to operate on cir_buf_source/cir_buf_sink views.
Extend cmocka tests to prepare/process via sink/source APIs and add negative/config/alignment-related test cases.

File	Description
test/cmocka/src/audio/eq_fir/eq_fir_process.c	Updates unit tests to use sink/source prepare + processing, and adds new validation/alignment tests.
src/audio/eq_fir/eq_fir.h	Updates FIR function signatures to take circular buffer views and explicit channel count.
src/audio/eq_fir/eq_fir.c	Reworks module processing + prepare to use sink/source APIs and passthrough via `source_to_sink_copy()`.
src/audio/eq_fir/eq_fir_hifi3.c	Adapts HiFi3 optimized FIR processing to circular buffer view API.
src/audio/eq_fir/eq_fir_hifi2ep.c	Adapts HiFi2EP optimized FIR processing to circular buffer view API.
src/audio/eq_fir/eq_fir_generic.c	Adapts generic FIR processing to circular buffer view API.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

piotrhoppeintel · 2026-09-24T19:01:02Z

+	if (buffer_size < source_bytes) {
+		source_release_data(source, 0);
+		return -ENOSPC;
+	}


lgirdwood

@piotrhoppeintel can you resolve the GH comments too. Thanks !

lgirdwood · 2026-09-23T14:47:44Z

+				fir_get_lrshifts(f, &lshift, &rshift);
+				fir_hifiep_setup_circular(f);
+				y0 = snk + ch;
+				fir_32x16(f, src[ch], y0, lshift, rshift);


looks like the indentation is wrong here unless its the GH diff rendering ?

kv2019i

First commit looks good, I'd like @singalsu to sign of the variable naming change in second commit.

kv2019i · 2026-09-25T12:32:49Z

+	int max_samples;
+	int chunk_samples;
+	int sample_index;
+	int channel;


@singalsu Can you quickly check just the generic naming approach?

Replace legacy input/output buffer processing with the source/sink API. Handle circular-buffer wrapping in FIR kernels and use direct source-to-sink copy for pass-through operation. Validate matching source and sink formats and add tests for invalid configurations and odd frame counts. Signed-off-by: Piotr Hoppe <piotr.hoppe@intel.com>

Replace abbreviated local variable names with descriptive names across the generic, HiFi2EP, and HiFi3 EQ FIR implementations. Clarify channel, sample, pointer, filter, and stride handling without changing processing behavior. Signed-off-by: Piotr Hoppe <piotr.hoppe@intel.com>

lgirdwood · 2026-09-28T19:27:46Z

@piotrhoppeintel just 1 CI open.

lyakh · 2026-09-29T08:04:20Z


-	return 0;
+	if (!cd->eq_fir_func)
+		return -EINVAL;


this is actually impossible, right? cd->eq_fir_func == NULL is only possible if cd->fir_delay_size == 0 and then you'd take one of returns in lines 425 or 427. If you really want you could just use an assertion here.

lyakh · 2026-09-29T08:14:39Z

+		return ret;
+	}
+
+	return sink_commit_buffer(sink, sink_bytes);


would you be converting them all to use your new release_source_and_commit_sink()?

lyakh · 2026-09-29T08:23:39Z

 	int nmax, n, i, j;
-	int nch = audio_stream_get_channels(source);
-	int remaining_samples = frames * nch;
+	int remaining_samples = frames * channels;


...consistent across modules types would be nice...

lyakh · 2026-09-29T08:34:53Z

+				fir_32x16_2x(f, *x0, *x1, y0, y1, lshift, rshift);
+				x0 += 2 * nch;
+				y0 += 2 * nch;
+			}


would it be possible to just add here the handling of the "left-over" odd frame and remove the whole block in lines 53-65 above and line 52? Maybe just

if (chunk_frames & 1) fir_32x16(f, src[ch], y0, lshift, rshift);

would be enough then. Same in other cases too.

lyakh · 2026-09-29T08:38:51Z

+	abi->size = sizeof(*config);
+	config->size = abi->size;
+	ret = eq_fir_send_blob(mod, abi, sizeof(blob_copy));
+	assert_int_equal(ret, -EINVAL);


a short comment explaining which specific invalid case each block is testing would be nice...

kv2019i

@lgirdwood Please note the local variable naming change. I want @singalsu to ack.

Add -1 just to make sure @singalsu has a change to check

Copilot AI lite review requested due to automatic review settings September 22, 2026 09:17

piotrhoppeintel requested review from dbaluta, kv2019i, lbetlej, lgirdwood, mmaka1, plbossart and singalsu as code owners September 22, 2026 09:17

Copilot started reviewing on behalf of piotrhoppeintel September 22, 2026 09:20 View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

lgirdwood reviewed Sep 23, 2026

View reviewed changes

piotrhoppeintel force-pushed the p20-eq-fir branch from 527602c to f9ad41f Compare September 24, 2026 18:56

kv2019i reviewed Sep 25, 2026

View reviewed changes

piotrhoppeintel force-pushed the p20-eq-fir branch from f9ad41f to 586bfc9 Compare September 28, 2026 08:50

piotrhoppeintel force-pushed the p20-eq-fir branch from 586bfc9 to 2969fb7 Compare September 28, 2026 08:51

lgirdwood approved these changes Sep 28, 2026

View reviewed changes

lyakh reviewed Sep 29, 2026

View reviewed changes

kv2019i requested changes Sep 29, 2026

View reviewed changes

Navigation Menu

P20 eq fir convert the eq_fir module to only use the sink/source api - #11221

P20 eq fir convert the eq_fir module to only use the sink/source api#11221
piotrhoppeintel wants to merge 2 commits into
thesofproject:mainfrom
piotrhoppeintel:p20-eq-fir

piotrhoppeintel commented Sep 22, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

piotrhoppeintel Sep 24, 2026

Uh oh!

Uh oh!

lgirdwood left a comment

Uh oh!

lgirdwood Sep 23, 2026

Uh oh!

kv2019i left a comment

Uh oh!

kv2019i Sep 25, 2026

Uh oh!

lgirdwood commented Sep 28, 2026

Uh oh!

lyakh Sep 29, 2026

Uh oh!

lyakh Sep 29, 2026

Uh oh!

lyakh Sep 29, 2026

Uh oh!

lyakh Sep 29, 2026

Uh oh!

lyakh Sep 29, 2026

Uh oh!

kv2019i left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

piotrhoppeintel commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Sep 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

piotrhoppeintel commented Sep 22, 2026 •

edited

Loading