Repository navigation
Conversation
ujfalusi
requested review from
bardliao,
dbaluta,
kv2019i,
lgirdwood,
plbossart,
ranj063 and
singalsu
as code owners
November 9, 2026 12:51
There was a problem hiding this comment.
🟡 Changes recommended
The topology changes can propagate incorrect formats across fanout connections, inflate pipeline resource accounting during repeated preparation, and accept formats incompatible with the bound input pin.
3 open issues
What changed in this PR
This PR makes SOF IPC4 format selection and propagation pin-aware, separating module configuration from the format passed along each connection.
Changes:
- Passes output-pin identity through widget preparation and isolates parameters across branching paths.
- Selects and propagates process and copier formats per pin, including secondary-pin handling.
- Validates complete pin bindings, checks ambiguous formats, and fixes process-format cleanup.
| File | Description |
|---|---|
sound/soc/sof/topology.c |
Rejects partially defined pin-binding arrays. |
sound/soc/sof/sof-audio.h |
Adds output-pin identifiers and extends the preparation interface. |
sound/soc/sof/sof-audio.c |
Prepares each connection with pin identity and independent branch parameters. |
sound/soc/sof/ipc4-topology.h |
Clarifies that parameter-change detection concerns pin 0. |
sound/soc/sof/ipc4-topology.c |
Implements pin-aware format handling, validation, and error-path cleanup. |
Review effort: Balanced
kv2019i
reviewed
Nov 10, 2026
kv2019i
left a comment
kv2019i
left a comment
Collaborator
There was a problem hiding this comment.
Complicated series, but commit messages look good and explain the changes. There is unfortunately lot of special logic around pin0, but that is fundamental of the firmware interface and kernel can do nothing about it. Minor comment(s) inline.
| } | ||
|
|
||
| copier_data->gtw_cfg.node_id &= ~SOF_IPC4_NODE_INDEX_MASK; | ||
| copier_data->gtw_cfg.node_id |= SOF_IPC4_NODE_INDEX(host_dma_id); |
Collaborator
There was a problem hiding this comment.
Not sure how to explain this simpler, but I think this commit has complexities:
""Select the copier output format among the pin 0 formats only and modify
the params for the next widget with the format of the output pin feeding
the path. The format of a pin above 0 is selected from its formats in
topology""
This seems selfcontracticting. First it is said selecting format amont pin 0 formats only, but then in next sentence, format selection is done based on other factors. Was the intention that for pin 0, only formats in pin 0 are considered?
Collaborator
Author
There was a problem hiding this comment.
Yes, exactly, the pin 0 restriction applies to the copier’s initialization format. Downstream parameter propagation separately uses the format of the pin feeding that connection. I’ll clarify it.
sof_ipc4_widget_setup_comp_process() allocates the input and output pin
format arrays via sof_ipc4_get_audio_fmt(). When the following
sof_ipc4_widget_setup_msg() fails, e.g. because the firmware does not
provide a module matching the widget UUID, the error path jumps to the
'err' label, which frees the process struct and clears swidget->private
but skips sof_ipc4_free_audio_fmt(). The arrays are then unreachable and
are reported by kmemleak:
sof_ipc4_get_audio_fmt+0xc9/0x420 [snd_sof]
sof_ipc4_widget_setup_comp_process+0x8e/0x200 [snd_sof]
snd_sof_load_topology+0x71a/0x840 [snd_sof]
sof_pcm_probe+0x77/0x120 [snd_sof]
snd_soc_component_probe+0x21/0x60 [snd_soc_core]
...
Jump to 'free_available_fmt' instead, as the other process widget error
paths after sof_ipc4_get_audio_fmt() already do.
The other IPC4 widget setup functions (pga, mixer, src, asrc) free the
audio formats in their common error label and are not affected.
Fixes: f9efae9 ("ASoC: SOF: ipc4-topology: Add support for base config extension")
Cc: stable@vger.kernel.org
Signed-off-by: Baoli Zhang <baoli.zhang@linux.intel.com>
The pin binding array is only stored if the binding of pin 0 is defined in topology. The bindings of the other pins are assumed to be present and they are compared with strcmp() when looking for the pin a widget is connected to, which results in a NULL pointer dereference if the topology does not define the binding for all pins. If the binding of pin 0 is not defined, the parsed bindings of the other pins are leaked. The pin binding must be defined for all pins, with "NotConnected" for the unused pins as documented in struct snd_sof_widget. Reject the topology if the binding is only defined for some of the pins. Fixes: 3b3aced ("ASoC: SOF: Add support to parse pin binding array from topology") Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Move the output format selection of process modules to sof_ipc4_process_select_output_fmt(). Pass the selected input format, or NULL for modules without input formats. Keep the prepared-widget shortcut and pin 0-only parameter propagation unchanged. This prepares for separating module configuration from the output format propagated along each path. No functional change. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…repare The ipc_prepare op of a widget is called once, before its sinks are walked, without information about which output pin the path continues on. A widget with multiple output pins, each with its own format, cannot tell which pin format to use as the input params of the next widget. The pin cannot be looked up once for the whole PCM walk either: a widget can feed several sinks of the same PCM graph via different pins, like the KPB in the Multi-WOV topology, with its two outputs reconverging at the WOV arbiter. Call ipc_prepare once per outgoing connection in the PCM widget list and pass the output pin feeding that sink: - 0 for widgets with a single output pin, - the pin bound to the sink in topology for widgets with multiple pins, - SOF_OUTPUT_PIN_UNKNOWN if the widget has no output pin bindings, - SOF_OUTPUT_PIN_NONE for widgets without a downstream connection in the PCM, which still receive one call. A connection from a widget without output pins or to a sink missing from the bindings is rejected. Virtual widgets (input, output, out_drv) are not connected in the firmware, the walk does not continue to them and they are not counted as connections. The first call initializes the widget and marks it prepared, subsequent calls only propagate the format of the given pin. When the widget fans out to multiple sinks each branch is walked with its own copy of the incoming params, so that the params propagated along one branch do not leak into the others. The source is unprepared if any of its connections fails. The IPC4 prepare callbacks are updated for the new parameter, the output pin is not used yet. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The output format lookup selects among the formats of all output pins of a module. Modules with multiple output pins need to select the format among the formats of a given pin. Add a pin parameter to sof_ipc4_init_output_audio_fmt() and sof_ipc4_is_single_format() to only consider the formats of that pin. SOF_OUTPUT_PIN_ANY considers the formats of all pins as before. Also allow selecting an output format without updating the obs of a base config, for pins which are not configured via the base config. Factor out the format comparison to sof_ipc4_is_same_fmt() and move the pin format helpers next to sof_ipc4_fmt_cfg_to_type(), so that they can also be used while the topology is parsed. All callers pass SOF_OUTPUT_PIN_ANY, no functional change. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
… single format The formats of a pin are treated as a single format if they have the same rate, channel count and valid bit depth. In that case the first format is used without matching the formats against the params. Formats which only differ in their sample type, like 32-bit integer and 32-bit float, or unsigned 8-bit and A-law/mu-law, are not the same format: the first one is used even if the params ask for the other one. Compare the sample type as well, the same way as the formats are matched against the params. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…mats The formats of the pins of a process module are fixed when the module is initialized: pin 0 via the base config, the other pins via the base config extension or by the module itself. Unlike the copier, which can change the format of its output pins with SET_SINK_FORMAT, there is no message to change them afterwards. On a side with multiple pins the module can be initialized via a path using any of the pins. The format of the other pins can only be known if they have a single format, there are no params to select one of their formats from. Warn at topology load if a pin on a side with multiple pins has more than one distinct format. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…ocess modules A process module with multiple output pins can be shared by paths of several PCMs. Selecting among all output formats with the FE params guesses the pin used by the path. The prepared-widget shortcut always propagates process->output_format, which describes pin 0 and may not have been set if the first path selected another pin. The module is configured with the output pin 0 format and buffer size in the base config. Other pins use their topology format via the base config extension or are configured by the module itself. Unlike a copier, a process module cannot change pin formats after initialization. Use the output pin feeding the path to separate module configuration from downstream propagation. Select the base config format only among pin 0 formats. If the path leaves on another pin, pin 0 must have a single format since the path params cannot select it. Propagate the configured format of the pin feeding the path on first and subsequent preparation. Pins without topology formats use the pin 0 format. Reject ambiguous secondary pin formats, including when building the base config extension. Modules with input formats are configured once, the pin formats are not selected again with the FE params of a subsequent path. Without output pin bindings the output pin of a sink is only allocated at bind time and the pin 0 format is propagated to it. Accept this only if all output pins are configured with the pin 0 format: pins without formats in topology use it, the others must have it as their single format. When there is no downstream connection, configure pin 0 without propagating an output format. Zero-output consumers skip output format selection entirely. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
A copier has a single input pin and up to 4 output pins. The copier is
configured with the output pin 0 format at init, the format of the other
output pins is set with SET_SINK_FORMAT when they are bound.
The copier prepare ignores the output pins:
- the output format is looked up among the formats of all output pins,
so a format of another pin can be picked as the pin 0 format,
- the params for the next widget are always taken from the pin 0
format, even if the path is fed from another output pin.
With a DAI copier fanning out to two pipelines:
| pin0 (48K/16bit/2ch) -> B1 -> PCM1
DAI -> A ---|
| pin1 (48K/32bit/2ch) -> B2 -> PCM2
if the copier input is 32-bit, the pin 1 format is selected as out_format
and B1 receives 32-bit data. When PCM2 is started, B2 is prepared with
the pin 0 format, while the copier will output 32-bit on pin 1.
Select the output format used to initialize the copier exclusively from
the formats declared for output pin 0.
Separately, propagate the format of the output pin feeding each
downstream connection. For pin 0, use the initialization format. For
higher-numbered pins, select from that pin's topology formats using the
copier input format as reference. If the pin has no declared formats,
propagate the pin 0 format, as before.
When setting the sink format of an output pin, check that the input
format of the sink is one of the formats listed for the pin in topology.
With no downstream connection, skip output parameter propagation.
Without output pin bindings the pin is allocated at bind time: propagate
the pin 0 format and let the bind set the format of the allocated pin
from the input format of the sink.
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…n > 0 The input format of a module is looked up among its pin 0 input formats with the params of the source widget in the path being prepared. This assumes that the path enters the module on input pin 0. A module with multiple input pins can be reached first via another pin, depending on the order of the widgets in the widget list. For example the ECNS module receives 16kHz/32bit/4ch from dmic16k on pin 0 and 48kHz/32bit/4ch from dmic01 on pin 1. If the path from dmic01 is walked first the 48kHz params are looked up among the pin 0 formats and the prepare fails, even though they are valid for pin 1. It only works today because the dmic16k path happens to be walked first. The base config of the module is always set from the pin 0 input format. If the params do not match any pin 0 format but match the format of another input pin, the path enters the module on that pin: use the pin 0 format for the base config. This is only possible if pin 0 has a single format, otherwise the format to configure is unknown: modules with multiple pin 0 formats must be prepared first via the path entering on pin 0. Fail the prepare with an explicit error if this is not the case. The fallback is only taken where the lookup used to fail, the selection for paths entering on pin 0 is not changed. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…nput process modules The pipeline params are updated with the output format of a process module based on the changed_params mask, which is evaluated between the input and output formats of the module. When the path enters a multi-input module on an input pin other than pin 0 the pipeline params carry the format of that pin while the output format is derived from the pin 0 format of the module. The params not flagged as changed would be propagated from the wrong pin. This only worked because the changed_params is evaluated over the formats of all pins and the format of the other input pin usually differs from the output format. Update all params with the output format for modules with multiple input pins. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…s only The changed_params mask is evaluated by comparing all input formats with all output formats of a module regardless of the pin they belong to. For modules with multiple pins this results in params flagged as changed which are not changed by the module on its data path. For example the google-rtc-aec has the 2 channel echo reference on input pin 1 and with 4 channel microphones it has a 4 channel format on input pin 0 and on the output, but the channels are flagged as changed. Only compare the formats of input pin 0 and output pin 0. The params of a path leaving a multi-output module on other pins or entering a multi-input module are updated with the full output format, they are not affected by the mask. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
ujfalusi
force-pushed
the
peter/sof/pr/multi-pin-multi-format
branch
from
November 10, 2026 13:37
aa84a33 to
ba430ff
Compare
Collaborator
Author
|
Changes since v1:
|
There was a problem hiding this comment.
🟡 Changes recommended
The secondary copier output lookup can reject a bind-compatible mono connection by requiring the output to match the four-channel input, rather than the connected sink’s constraints.
1 open issue
3 resolved since last review
Review effort: Balanced
Comment on lines
+2355
to
+2359
| channels = SOF_IPC4_AUDIO_FORMAT_CFG_CHANNELS_COUNT(in_fmt->fmt_cfg); | ||
| valid_bits = SOF_IPC4_AUDIO_FORMAT_CFG_V_BIT_DEPTH(ref_fmt->fmt_cfg); | ||
| i = sof_ipc4_init_output_audio_fmt(sdev, swidget, NULL, available_fmt, out_pin, | ||
| in_fmt->sampling_frequency, channels, | ||
| valid_bits, |
kv2019i
approved these changes
Nov 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Separate module format configuration from the format propagated along each connection. Previously, format lookup could consider formats belonging to unrelated pins, while already-prepared modules propagated the pin 0 format regardless of the output feeding the path.
This series:
Passes the connection’s output pin to widget preparation.
Preserves independent parameters across branching paths, including Multi-WOV branches that reconverge.
Configures process modules using pin 0 and propagates the configured format of the actual output pin.
Handles copier secondary outputs separately, preserving bind-time format configuration.
Supports paths entering modules through secondary input pins.
Restricts parameter-change detection to pin 0 formats and includes sample type in format-equivalence checks.
Validates complete pin-binding arrays and warns about ambiguous process-module pin formats.
Also includes the upstream process-widget pin-format leak fix.
Validation
Focused W=1 builds and strict checkpatch checks.
DUT: ECNS capture and silent speaker/deepbuffer playback passed.
Multi-WOV PCM12 prepares and reaches RUNNING without kernel warnings/errors; frame delivery remains unverified because capture stayed in the listening state.