-
Notifications
You must be signed in to change notification settings - Fork 349
Tools: Topology: Add echo reference for SDW speaker and jack #10387
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.
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
Tools: Topology: Add echo reference for SDW speaker and jack #10387
Conversation
482b7a0 to
15a0e37
Compare
| num_input_pins 1 | ||
| num_output_pins 1 | ||
| num_input_audio_formats 1 | ||
| num_output_audio_formats 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the host copier need to support all output formats?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, all s16/24/32 and float are OK, I can add it. The 8-bit formats are not good for the purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I added the formats for objects while this is still with single output format. But for speaker that goes with default rate the output formats could be defined here instead.
15a0e37 to
523ee9d
Compare
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can echo ref be always available in the topology and switch on/off by opening the PCM ?
Yep, the PCMs can be used or ignored. If there is no playback and ref capture is done, it produces silence. |
Perfect, lets do this so its available for general usage. This would need a new PCM ID. I assume this would capture playback if the playback PCM was in use ? |
523ee9d to
12d8f3b
Compare
|
I updated the virtual DAI name to LoopbackCapture_virtual_DAI, and added build for sof-ptl-rt721-echoref.tplg. |
12d8f3b to
f3ab66b
Compare
|
The virtual DAI name is shortened to "Loopback_Virtual". The previous name was too long for kernel name limit. This matches the updated thesofproject/linux#5609 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds echo reference capture PCM functionality for SDW speaker and jack playback paths. The echo reference feature allows capturing the audio being played back, which is typically used for acoustic echo cancellation (AEC) in voice processing applications.
Key Changes:
- Introduces build-time options
SDW_JACK_ECHO_REFandSDW_SPK_ECHO_REFto enable echo reference capture - Adds new pipeline configuration
siggen-host-copier-capture.conffor echo reference processing - Creates echo reference PCM devices with IDs 11 (jack) and 12 (speaker)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/topology/topology2/platform/intel/sdw-jack-generic.conf | Adds jack echo reference routing and PCM configuration with module-copier insertion |
| tools/topology/topology2/platform/intel/sdw-amp-generic.conf | Adds speaker echo reference routing and PCM configuration with module-copier insertion |
| tools/topology/topology2/include/pipelines/cavs/siggen-host-copier-capture.conf | New pipeline definition for siggen-based capture used by echo reference |
| tools/topology/topology2/include/common/common_definitions.conf | Adds build flags for controlling echo reference features |
| tools/topology/topology2/development/tplg-targets.cmake | Adds new topology target with echo reference enabled |
| tools/topology/topology2/cavs-sdw.conf | Includes new siggen-host-copier-capture pipeline configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| in_rate $JACK_RATE | ||
| in_bit_depth 32 | ||
| in_valid_bit_depth 32 | ||
| } | ||
| ] | ||
| Object.Base.output_audio_format [ | ||
| { | ||
| out_rate $JACK_RATE |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The speaker echo reference configuration incorrectly uses $JACK_RATE instead of a speaker-specific rate variable. Since this is in the speaker (amp) configuration file and other parts of this file don't reference JACK_RATE, this should likely use a speaker rate constant or the rate should be omitted if it's handled by the audio format configuration.
| in_rate $JACK_RATE | |
| in_bit_depth 32 | |
| in_valid_bit_depth 32 | |
| } | |
| ] | |
| Object.Base.output_audio_format [ | |
| { | |
| out_rate $JACK_RATE | |
| in_bit_depth 32 | |
| in_valid_bit_depth 32 | |
| } | |
| ] | |
| Object.Base.output_audio_format [ | |
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point to check @singalsu from copilot. The file uses a 48000 rate at least in one of the definitions (for DAI), so not sure if JACK_RATE is correct.
OTOH, this was in the code prior to this PR, so can be done in a separate if a valid concern. Not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix it. It's a copy & paste mistake. It would break with 96 kHz headset builds.
| ] | ||
| Object.Base.output_audio_format [ | ||
| { | ||
| out_rate $JACK_RATE |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The speaker echo reference output configuration incorrectly uses $JACK_RATE. This variable is defined in the jack configuration file but not in the amp configuration, which could cause build failures or incorrect behavior. Either define a speaker-specific rate variable in this file or reference the appropriate rate constant.
f3ab66b to
ba1400c
Compare
kv2019i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments inline, but I don't think any are real blockers.
tools/topology/topology2/include/pipelines/cavs/siggen-host-copier-capture.conf
Show resolved
Hide resolved
| } | ||
| ] | ||
| } | ||
| "true" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@singalsu no need to change this PR, but @ranj063 @jsarha I was wondering if we started adding inline comments to there branches line the C conventions for ifdefs, would that make the nested conditional segments a bit more easy to follow. E.g. here on L702 "# PASSTHROUGH==true " and then the definition. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good. I'm quite confused with long chunks of stuff. And our indents also are not always right.
| in_rate $JACK_RATE | ||
| in_bit_depth 32 | ||
| in_valid_bit_depth 32 | ||
| } | ||
| ] | ||
| Object.Base.output_audio_format [ | ||
| { | ||
| out_rate $JACK_RATE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point to check @singalsu from copilot. The file uses a 48000 rate at least in one of the definitions (for DAI), so not sure if JACK_RATE is correct.
OTOH, this was in the code prior to this PR, so can be done in a separate if a valid concern. Not a blocker.
ba1400c to
ce622a7
Compare
|
Updates: I removed the $JACK_RATE from speaker pipelines. Also I moved the basic host-copier output formats to class definition, so they don't need to be repeated for speaker. Same also for formats for siggen. Also I tried to remove some formats definition stuff from virtual DAI to reduce tplg code amount but seems it's needed. Kernel parsed it but the formats appeared as 0 Hz and 0 bit, so I felt it wasn't safe to do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tools/topology/topology2/include/pipelines/cavs/siggen-host-copier-capture.conf
Outdated
Show resolved
Hide resolved
This patch adds an echo reference capture PCM for speaker and jack playback. The additions are normally disabled but can be enabled with build options SDW_JACK_ECHO_REF=true and/or SDW_SPK_ECHO_REF=true. Signed-off-by: Seppo Ingalsuo <[email protected]>
This patch builds into development directory a version of the topology with echo reference for jack and speaker. The topology name is sof-ptl-rt721-echoref.tplg. Signed-off-by: Seppo Ingalsuo <[email protected]>
ce622a7 to
78e231d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| num_output_audio_formats 1 | ||
| Object.Base.input_audio_format [ | ||
| { | ||
| in_rate $JACK_RATE |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The in_rate field is missing in the alh-copier input_audio_format block (line 627), while it's present in the output_audio_format block (line 634). For consistency and completeness, consider adding the in_rate field here as it is defined in the module-copier blocks (lines 652, 708).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are blind.
| in_bit_depth 32 | ||
| in_valid_bit_depth $SDW_LINK_VALID_BITS | ||
| in_sample_type $SAMPLE_TYPE_MSB_INTEGER | ||
| in_fmt_cfg "$[($in_channels | ($in_valid_bit_depth * 256))]" |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alh-copier input_audio_format block (lines 936-939) is missing the in_rate field, while the output_audio_format block at line 944 includes out_bit_depth and out_valid_bit_depth. For consistency with the jack configuration and to ensure complete audio format specification, consider adding the in_rate field.
| in_fmt_cfg "$[($in_channels | ($in_valid_bit_depth * 256))]" | |
| in_fmt_cfg "$[($in_channels | ($in_valid_bit_depth * 256))]" | |
| in_rate 48000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We operate the speaker pipelines with the default rate (48 kHz) from classes. It should be added everywhere in the large topology fragment, so not doing it unless we add other rates options for speaker.
| ] | ||
| Object.Base.output_audio_format [ | ||
| { | ||
| out_bit_depth 32 |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output_audio_format block (lines 944-945) is missing the out_rate field. For consistency with the jack configuration (line 634) and completeness of the audio format specification, consider adding the out_rate field.
| out_bit_depth 32 | |
| out_bit_depth 32 | |
| out_rate 48000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, not adding rates to speaker.




This patch adds an echo reference capture PCM for speaker and jack playback. The additions are normally disabled but can be enabled with build options SDW_JACK_ECHO_REF=true and/or SDW_SPK_ECHO_REF=true.
This depends on merge of #10329