-
Notifications
You must be signed in to change notification settings - Fork 349
Add support for hostless/dailess pipelines #10329
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
base: main
Are you sure you want to change the base?
Conversation
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 support for hostless pipelines using a tone generator as an example. The changes enable pipelines to run without a host connection by introducing direction awareness and implementing the tone generator as a modular component.
Key changes:
- Implemented pipeline direction tracking in IPC4 to support hostless operation
- Refactored tone generator from legacy component to module adapter architecture
- Added configuration support for tone generator in SDW platform topologies
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/topology/topology2/platform/intel/sdw-amp-generic.conf | Adds tone generator pipeline configuration with PCM definition and routing |
| tools/topology/topology2/include/pipelines/virtual-siggen-mixin-playback.conf | Defines new virtual signal generator to mixer pipeline for tone playback |
| tools/topology/topology2/include/components/virtual.conf | Adds stream_name attribute to virtual widget component |
| tools/topology/topology2/include/components/siggen.conf | New signal generator component definition with configuration attributes |
| tools/topology/topology2/cavs-sdw.conf | Includes new pipeline and adds SPK_TONE_PLAYBACK configuration flag |
| src/ipc/ipc4/helper.c | Extracts and stores pipeline direction from IPC extension data |
| src/include/sof/audio/pipeline.h | Adds direction tracking fields to pipeline structure |
| src/include/ipc4/pipeline.h | Defines direction bitfields in IPC4 pipeline extension structure |
| src/audio/tone.toml | Adds module configuration manifest for tone generator |
| src/audio/tone.c | Refactors tone generator to module adapter interface with sink API |
| src/audio/module_adapter/module_adapter.c | Propagates pipeline direction to module devices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # A generic siggen widget. All attributes defined herein are namespaced | ||
| # by alsatplg to "Object.Widget.siggen.N.attribute_name" | ||
| # | ||
| # Usage: this component can be used by declaring int a parent object. i.e. |
Copilot
AI
Oct 25, 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.
Corrected spelling of 'int' to 'in'.
| # Usage: this component can be used by declaring int a parent object. i.e. | |
| # Usage: this component can be used by declaring in a parent object. i.e. |
| # Attribute categories | ||
| attributes { | ||
| # | ||
| # The sighen widget name would be constructed using the index and instance attributes. |
Copilot
AI
Oct 25, 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.
Corrected spelling of 'sighen' to 'siggen'.
| # The sighen widget name would be constructed using the index and instance attributes. | |
| # The siggen widget name would be constructed using the index and instance attributes. |
src/audio/tone.c
Outdated
| int ret; | ||
|
|
||
| /* tone generator only ever has 1 sink */ | ||
| output_frames = sink_get_free_frames(sinks[0]); |
Copilot
AI
Oct 25, 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.
Inconsistent array indexing. Line 395 uses sinks[0] while line 389 assigns sink = sinks[0]. Consider using the sink variable consistently throughout the function (e.g., sink_get_free_frames(sink)).
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.
One question on design.
| if (dev->pipeline->direction_set) { | ||
| dev->direction_set = true; | ||
| dev->direction = dev->pipeline->direction; | ||
| } |
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, ipc_comp_connect can propagates direction both ways, so shouldn't it be possible to set component directions based on "DAI copier"? The direction is passed to DAI copier already from host "get_gateway_direction(node_id.f.dma_type)", so direction is known.
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.
@kv2019i ipc_comp_connect checks both sides but the trigger is always propagated from source->sink. So if there's no host copier, the tone pipeline wont have a source direction to hinge upon
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.
@ranj063 Right but can we extend this? Seems FW has enough information to do the right thing without any new information from host (even if it doesn't do it now).
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.
@kv2019i I dont want to extend it at all, in fact Its quite a convoluted logic having to walk through the entire pipeline just to set the component's direction. This should be simplified
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.
@ranj063 Couldn't this module be converted into some virtual/fake DAI and replace the host copier with it?
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.
@ranj063 Ack. I guess this is more inline with the IPC4 philosophy of letting host provide all programming. It does still feel a bit redundant to me. The whole notion of direction is a bit redundant in describing a graph when you already provide have a description of a graph and how nodes are connected. It seems only entities that have some special functionality around playback/capture are the copier/endpoint modules and for these there's already a direction flag passed (e.g. in copier config blob). So if "direction" is only related to the endpoints, looking up the direction by traversin the graph topology, does not seem so convoluted.
| uint32_t core_id : 4; | ||
| uint32_t rsvd2 : 6; | ||
| uint32_t direction_set : 1; | ||
| uint32_t direction : 1; | ||
| uint32_t rsvd2 : 4; | ||
| uint32_t _reserved_2 : 2; |
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.
Changes in these structures require prior synchronization and acceptance from the architecture side.
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.
@tmleman ack. How do I go about this?
src/include/sof/audio/pipeline.h
Outdated
| uint32_t time_domain; /**< scheduling time domain */ | ||
| uint32_t attributes; /**< pipeline attributes from IPC extension msg/ */ | ||
| uint32_t direction_set; /**< flag indicating if direction set from IPC extension msg? */ | ||
| uint32_t direction; /**< pipeline direction from IPC extension msg */ |
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.
bool
| dev->ipc_config = *config; | ||
| comp_info(dev, "tone_new()"); | ||
|
|
||
| cd = rzalloc(SOF_MEM_FLAG_USER, sizeof(*cd)); |
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.
mod_alloc() while at it to save @jsarha some work :-)
| static SHARED_DATA struct comp_driver_info comp_tone_info = { | ||
| .drv = &comp_tone, | ||
| }; | ||
| #if CONFIG_COMP_TONE_MODULE |
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.
it isn't tristate yet, it's boolean, LLEXT isn't supported for the tone generator yet, no need for this branch yet
src/audio/tone.toml
Outdated
| @@ -0,0 +1,21 @@ | |||
| #ifndef LOAD_TYPE | |||
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.
this file is probably unneeded yet - nobody includes it
| # | ||
| ## Stream name - maps to the DAPM widget's stream name | ||
| # | ||
| DefineAttribute."stream_name" { |
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.
Should this be virtual_stream_name ? i.e. if if for virtual usage only we best name/state that so its used correctly.
The need to pass direction was discussed, I don't have strong opposition on either path, just wanted to check the options have been considered.
Drop support for IPC3 and update the tone module to work with IPC4. Also, add a couple of new modes in the module to support using the tone module in the echo reference to be able to either passthrough the captured data or generate silence. Signed-off-by: Ranjani Sridharan <[email protected]>
89e64f3 to
2ee7695
Compare
| static void tone_s32_default(struct comp_dev *dev, struct audio_stream *sink, | ||
| uint32_t frames) | ||
| static int tone_s32_passthrough(struct processing_module *mod, struct sof_sink *sink, | ||
| struct sof_source *source) |
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.
why does a tone generator have to support pass-through? Was it also possible with IPC3?
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 tone generator operates as silence generator in this case, when there is no playback happening. Silence is a kind of audio signal too.
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.
@lyakh with IPC3 we didnt have firmware based echo reference feature. This new addition to do passthrough is required for supporting that with IPC4 for SDW topologies.
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 tone generator operates as silence generator in this case, when there is no playback happening. Silence is a kind of audio signal too.
@singalsu interesting, thanks. I think we also have ad-hoc silence generation in some components. Is it planned to remove those and use "tone" in all cases?
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'd assume those silence generator capabilities remain like now. E.g. mixin/mixout operation is defined by IPC4. While with this component with no Windows OS dependence we can define it as we need.
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.
@ranj063 feel free to fix the spelling in a subsequent PR if CI passes
2ee7695 to
a6f5ed5
Compare
Create a new conf file for the siggen module. Signed-off-by: Ranjani Sridharan <[email protected]>
Add a new token for pipeline direction and a new atttribute for the pipeline component. This change is required to implement the firmware based echo reference feature to inform the kernel of the pipeline direction in order for it to make a decision on which pipelines to set up when the echo reference capture pipeline is set up. Signed-off-by: Ranjani Sridharan <[email protected]>
a6f5ed5 to
f4bbd3a
Compare
|
|
||
| static void tone_s32_default(struct comp_dev *dev, struct audio_stream *sink, | ||
| uint32_t frames) | ||
| static int tone_s32_passthrough(struct processing_module *mod, struct sof_sink *sink, |
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 we use for passthrough the helper source_to_sink_copy(). There's an example in
sof/src/audio/template/template.c
Line 85 in 9a3ae61
| source_to_sink_copy(source, sink, true, frames * cd->frame_bytes); |
|
@ranj063 I wonder how difficult for your change would be to keep the support for IPC3. We want to add this feature to our release too but IMX doesnt support IPC4 yet. |
| #include <sof/audio/buffer.h> | ||
| #include <sof/audio/component.h> | ||
| #include <sof/audio/format.h> | ||
| #include <sof/audio/module_adapter/module/generic.h> |
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 just noticed that this patch misses the .toml patch that was in some earlier version. Because of the module stuff it would also be more convenient to have a new directory for this, e.g. "siggen".
| n_min -= nch; | ||
| for (i = 0; i < nch; i++) { | ||
| switch (cd->mode) { | ||
| case TONE_MDOE_TONEGEN: |
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.
Typo, should be TONE_MODE_TONEGEN.
Preparatory patches with updates to the tone module and topology support for the tone and pipeline direction attribute.