[BUG] IPC4 fails to set up stream metadata correctly

Component stream state in IPC4 doesn't seem to be set up reliably. When the sof-mtl-max98357a-rt5682-ssp2-ssp0 topology (built form the mtl-007-drop-stable branch), the Google AEC component is being presented with a reference/playback input source that claims 4 channels, but in fact is only providing enough data for two.

Similarly, the computed "alignment constants" on a buffer's audio stream start out with (I think) garbage, and require that something somewhere call source/sink_set_alignment_constants() with arbitrary (and even incorrect, e.g. a byte alignment of 1 was being used!) arguments just to get the fields initialized.

The component has always had code present (see this commit for the current form: bab124d) to fix this at prepare() time. But it's fragile, undocumented, got dropped during refactoring and took me DAYS to figure out; the symptoms were just ring buffer overruns in the downstream components that produced "mild distortion".

This just isn't right. Components don't own their sinks/sources and shouldn't ever be responsible for configuring them. The pipeline/IPC layer needs to do this correctly from topology, and in the event that the data is wrong or mismatched, it needs to fail gracefully. We shouldn't ever be in a situation like this where a source promises 8 byte frames per sample period but delivers 4.

@marcinszkudlinski

@marcinszkudlinski can you comment on a solution ?

Yes, I know there are inconsistences at number of channels.
Other parameters are set properly

that's why I introduced a WA to mtl 007 branch:

sof/src/audio/google/google_rtc_audio_processing.c

Line 96 in 6d3061c

/* TODO - it does not work, to be checked before merging!!

problems are being investigated

@andrula-song

Add @andrula-song as assignee given expert-seeming comments in #8571

@ujfalusi

@ujfalusi can you give this some priority. Thanks. @plbossart fyi, driver may be sending incorrect IPC4 data, we are not sure yet.

@andyross

@andyross, can you attach kernel and firmware logs with pointers where things go south? I'm not sure if I follow the description.
If the sof-mtl-max98357a-rt5682-ssp2-ssp0 represents the Google AEC with 4 channels and it should not be like that then the issue is in topology?

pipelines are prepared in sequence which means that one side of the module likely will not prepared when it is prepared. I'm not sure what is the direction in this case.

The code for src/audio/google/google_rtc_audio_processing.c differs quite a bit between main, mtl-007-drop-stable and wherever bab124d is.

@cujomalainey

Adding @cujomalainey @johnylin76 since @andyross out till Friday.

@lkoenig

@lkoenig is the other person working on this whom might have logs.

@cujomalainey

I am afraid there is nothing in the kernel that deals with the number of channels. All information on configurations are provided by topology and sent to firmware as is.

I do remember talking with @cujomalainey on the AEC/DMIC integration. The premise for DMICs since GLK is that we ALWAYS deal with 4 channels, and userspace/UCM extract the useful channels based on platform-specific channel maps.

But if the DMIC input is provided to AEC, then how would the firmware know which channels are to be dealt with? Conversely the echo reference is two channels, so this really begs the question on how the echo reference is mapped to which input channels for processing.

So yes there's clearly a 2:4 mismatch in the AEC integration...

@marcinszkudlinski

@marcinszkudlinski @plbossart it sounds like we could add a topology tuple(s) for AEC DMIC channel mapping. i.e. AEC echo ref is channels mask 0x3

@48khz

To clarify: the DMIC source to AEC does indeed represent itself correctly as 4 channels. And that's indeed a problem that needs solving (right now AEC simply drops the second two). But at the source/sink metadata level, everything is correct. It's promising us 4x s16 @48khz and delivering the expected 8 bytes per 1/48000 seconds.

This bug is that the reference input (demuxed from the playback pipeline by the copier) is also listing 4 channels, even though it's clearly only providing two. And AFAICT the topology shows two there. I don't know where the "4" is coming from.

@johnylin76

But if the DMIC input is provided to AEC, then how would the firmware know which channels are to be dealt with? Conversely the echo reference is two channels, so this really begs the question on how the echo reference is mapped to which input channels for processing.

@johnylin76 has been doing some excellent work in CRAS to enable easy dynamic configuration of SOF. The main target is DRC/EQ configs from CRAS format. That being said, it would not be hard to extend that to a channel map which we have on hand already and write that down to the firmware. We could even update it for WF vs UF mics.

@andyross

In addition to #8672 @andyross submitted, there's #8575 to address point (3) in upstream.

audio_stream: Fix uninitialized data in alignment_constants

Been working on that, actually. See #8685 for IMHO a much cleaner take on the "base_cfg_ext" problem that doesn't require module code do weird gymnastics managing memory in the core module layers. It is, admittedly, pretty opinionated. But it's a lot smaller, much simpler for component code, and as mentioned I do feel like we've gotten stuck with a really bad API paradigm here.

With that, the big workaround patch in the AEC series linked above is down to just one line that looks like:

#ifdef CONFIG_IPC_MAJOR_4
	ipc4_update_source_format(sources[0], &mod->priv.cfg.input_pins[1].audio_fmt);
#endif

And I'm pretty sure I see where the problem is there and hope to have a fix out tomorrow.

audio_stream: Fix uninitialized data in alignment_constants

audio_stream: Persist requirements and lazy-recalculate alignment

google_rtc_audio_processing: Work around IPC4 stream setup issues

audio_stream: Persist requirements and lazy-recalculate alignment

@andyross

@andyross is this issue good to close now?

Let's leave this open for another week or two while I get the AEC rework rebased. The actual stream format for multi-output copier objects is still being set incorrectly, because the copier doesn't get the cfg_ext data that would contain the correct value, because it has its own config struct in the protocol where cfg_ext would go. It's a protocol bug basically, the topology isn't transmitted to the firmware in a single format or a single place, every component is rolling its own.

Basically I need to write up the protocol stuff in a new bug before closing this.

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

Navigation Menu

Repository navigation

[BUG] IPC4 fails to set up stream metadata correctly #8639

10 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[BUG] IPC4 fails to set up stream metadata correctly #8639

Description

Activity

lgirdwood commented on Dec 18, 2023

marcinszkudlinski commented on Dec 18, 2023

andyross commented on Dec 19, 2023

lgirdwood commented on Dec 19, 2023

ujfalusi commented on Dec 19, 2023

lgirdwood commented on Dec 19, 2023

cujomalainey commented on Dec 19, 2023

plbossart commented on Dec 20, 2023

lgirdwood commented on Dec 20, 2023

andyross commented on Dec 20, 2023

cujomalainey commented on Dec 20, 2023

10 remaining items

kv2019i commented on Jan 2, 2024

andyross commented on Jan 3, 2024

abonislawski commented on Jan 23, 2024

andyross commented on Jan 26, 2024

github-actions commented on Jun 19, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions