ASoC: SOF: ipc4-topology: Improve playback dai copier in/out format selection by ujfalusi · Pull Request #5396 · thesofproject/linux

ujfalusi · 2025-04-29T08:04:22Z

ASoC: SOF: ipc4-topology: Improve playback dai copier in/out format selection

The dai copier configuration for playback and capture needs to be separated
because it is not correct to configure the dai copier as part of the input
format selection for both direction.
The input format is the dai format for capture, but it is not for playback,
for playback the dai format is on the output side.

Currently we configure and adjust the params based on the DAI supported
formats when configuring the input side of the copier but right after the
format has been adjusted we reset it for playback and loose this
information.

When using a nocodec passthrough topology (which is a bug) we have SSP
blobs supporting 32bit only, copier supporting 16/24/32 bit then on
playback the dai and copier will be incorrectly configured:
host.copier.in: S16_LE
host.copier.out: S16_LE
dai.copier.in: S16_LE
SSP.blob: S32_LE (we only have S32_LE blobs)
dai.copier.out: S16_LE (the dai constraint is ignored)

To handle such case the handling of capture and playback streams must be
changed:
The input format (no changes to previous implementation):
for playback it is the pipeline_params
for capture it is the adjusted fe_params

The output format (no change for capture direction):
for playback it is the adjusted fe_params
for capture it is the fe_params

with this change path format configuration will be correct:
host.copier.in: S16_LE
host.copier.out: S16_LE
dai.copier.in: S16_LE
SSP.blob: S32_LE (we only have S32_LE blobs)
dai.copier.out: S32_LE

jsarha

Makes sense. LGTM

ujfalusi · 2025-05-09T06:08:42Z

@ranj063, @kv2019i, can you take a look at this and perhaps test it as well? In my testing it does not introduce regression or change in behavior when the topology (and it's blobs, config) is correct.

ujfalusi · 2025-05-15T05:25:47Z

@kv2019i, @ranj063, can you take a look at this change? Thanks!

ujfalusi · 2025-05-15T05:25:54Z

SOFCI TEST

kv2019i

I'm juggling betweeen whether this is going too far in terms of adding intelligence in the kernel code, or whether this is doing mandatory things, and I'm leaning towards the latter.

Did you test with the BT offload test (with nocodec), I think it would be good to do a sanity test before merging.

Minor nit in comimt message: "When using a nocodec passthrough topology (which is a bug)". I had to read this a few time, what do you mean this is a bug? I guess this whole configuration is invalid... (with current code), but with your PR it actually works correctly, so not so clear is it invalid anymore. NHLT and topology can come via different routes, so I think this needs clarify how we handle if topology has more formats and than the NHLT blob.
wether this

ujfalusi · 2025-05-15T07:32:16Z

I'm juggling betweeen whether this is going too far in terms of adding intelligence in the kernel code, or whether this is doing mandatory things, and I'm leaning towards the latter.

Did you test with the BT offload test (with nocodec), I think it would be good to do a sanity test before merging.

Minor nit in comimt message: "When using a nocodec passthrough topology (which is a bug)". I had to read this a few time, what do you mean this is a bug? I guess this whole configuration is invalid... (with current code), but with your PR it actually works correctly, so not so clear is it invalid anymore. NHLT and topology can come via different routes, so I think this needs clarify how we handle if topology has more formats and than the NHLT blob. wether this

I have fixed now the nocodec passthrough topology, but there were a bug that it contained only S32_LE blob and config, but advertised support for S16_LE, S24_LE as well. That did not went well and brought this kernel bug in surface, that we were doing the blob selection wrongly for playback, only the capture was correct and this worked because all of our other topologies were correct.

ujfalusi · 2025-05-16T05:32:43Z

SOFCI TEST

lgirdwood

One open.

ujfalusi · 2025-06-23T13:41:15Z

Changes since v1:

rebased on top of 8-bit format support pastch

ujfalusi · 2025-08-07T11:58:24Z

SOFCI TEST

ujfalusi · 2025-10-01T09:39:49Z

Changes since v2:

rebased on topic/sof-dev

ranj063 · 2026-03-24T17:24:47Z

-		if (dir == SNDRV_PCM_STREAM_PLAYBACK)
-			memcpy(ref_params, pipeline_params, sizeof(*ref_params));
+			ret = sof_ipc4_prepare_dai_copier(sdev, dai, ref_params, dir);
+			if (ret < 0)


@ujfalusi do we have a memory leak for the cllocated memory for ref_params here and possibly all error paths below?

No, we don't because of:

struct snd_pcm_hw_params *ref_params __free(kfree) = NULL;

it will be freed on function exit.

lgirdwood

LGTM, only 1 comment around change of data source

lgirdwood · 2026-05-15T13:34:35Z

+
+		if (params_width(params) != bit_depth) {
+			format_change = true;
+			dev_dbg(sdev->dev, "SSP format change from %d to %d\n",


the thing I would add here "...change from user blob %d to NHLT %d" or vice versa. This way its obvious in the logs

This has nothing to do with NHLT at this point. the format change is caused by the topology's hw_config for ssp.
The case is: SSP has a single format stored in the topology hw_config array, thus it only have support in copier for this format, but the params contains different format -> we need a format change.
I will replace the format with sample width for clarity.

ujfalusi · 2026-05-19T05:30:58Z

Changes since v3:

replace the misleading format with more appropriate sample width in debug print.

kv2019i

Would be good to test this with BT offload scenario. Otherwise looks very good change, the copier setup needs fixing.

kv2019i · 2026-05-20T11:35:08Z

+			format_change = true;
+			dev_dbg(sdev->dev, "SSP sample width change from %d to %d\n",
+				params_width(params), bit_depth);
+		}


I'm wondering if this safe for the BT offload case where SSP DAI supports multiple formats and we should be not using NHLT info for this. I guess this is. On current devices where we support BT offload, there is no NHLT in BIOS, so I guess this is good, but some potential for confusing bugs.

at this point we did not queried NHLT at all. This is all topology. The topology contains 'hw_config' array of supported configurations on the copier, this corresponds with the copier formats (and if topology have NHLT, then the blobs).

If the copier does not support the sample width of the params, then we need to pick the one which is supported to force a conversion.

Later we will ask the NHLT blob for the configuration that is actually supported by the copier.

@kv2019i, the issue is that if we don't do this then we would have silent incorrect configuration as described by the commit message. copier running in 16 bit mode while the SSP is configured in 32 bit mode or other permutation.
The nice thing is that the NHLT blob and copier format / hw_config all comes from different source and it is a topology bug if they don't match.

kv2019i · 2026-05-20T11:40:26Z

@@ -2175,28 +2175,32 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget,
 		copier_data = &ipc4_copier->data;
 		available_fmt = &ipc4_copier->available_fmt;


The commit message "When using a nocodec passthrough topology (which is a bug) " is a bit confusing. What is the bug here, the fact we are using nocodec passthrough topology? Not sure what is the bug. It seems legal to only provide S32 blobs and a copier configuration that supports at least S32.

OK, I will drop the note. I added it after a long internal discussion that using the nocodec passthrough is really something which is not to be used, it is even more experimental than nocodec and it has been not tested at all (I happened to needed it for testing something else)

@ujfalusi Right but kernel shouldn't care if some topology should be used or not. The topology is either correct (and it should work), or it's not and kernel should return an error. I guess this falls a bit in the grey are and poses the question should kernel try to cope with "almost correct" topology. But based on your description, this topology actually seems correct.

We need to adjust the params based on the available and picked SSP blob in the similar way we do for DMIC. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

…election The dai copier configuration for playback and capture needs to be separated because it is not correct to configure the dai copier as part of the input format selection for both direction. The input format is the dai format for capture, but it is not for playback, for playback the dai format is on the output side. Currently we configure and adjust the params based on the DAI supported formats when configuring the input side of the copier but right after the format has been adjusted we reset it for playback and loose this information. When using a nocodec passthrough topology (which is a bug) we have SSP blobs supporting 32bit only, copier supporting 16/24/32 bit then on playback the dai and copier will be incorrectly configured: host.copier.in: S16_LE host.copier.out: S16_LE dai.copier.in: S16_LE SSP.blob: S32_LE (we only have S32_LE blobs) dai.copier.out: S16_LE (the dai constraint is ignored) To handle such case the handling of capture and playback streams must be changed: The input format (no changes to previous implementation): for playback it is the pipeline_params for capture it is the adjusted fe_params The output format (no change for capture direction): for playback it is the adjusted fe_params for capture it is the fe_params with this change path format configuration will be correct: host.copier.in: S16_LE host.copier.out: S16_LE dai.copier.in: S16_LE SSP.blob: S32_LE (we only have S32_LE blobs) dai.copier.out: S32_LE Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi · 2026-07-23T11:13:52Z

Changes since v4:

rebased to resolve conflicts

Copilot

Pull request overview

This PR updates SOF IPC4 topology handling to correctly select DAI copier input vs. output formats for playback vs. capture, so that DAI-side constraints (e.g., SSP NHLT blobs that only support 32-bit) aren’t lost during playback configuration.

Changes:

Track SSP sample-width mismatches as a format change when selecting NHLT endpoint blobs, enabling proper params updates when the blob forces a different width.
Split DAI copier parameter selection so playback uses pipeline_params for input format selection while using DAI-adjusted params for output format selection (and keeps capture behavior consistent with the described intent).
Update output-format reference selection to use the appropriate params source per direction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+		memcpy(ref_params, fe_params, sizeof(*ref_params));
+		if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
+			ret = sof_ipc4_prepare_dai_copier(sdev, dai, ref_params, dir);
+			if (ret < 0)
+				return ret;
+		}


ujfalusi requested a review from singalsu April 29, 2025 08:04

ujfalusi requested review from bardliao, dbaluta, kv2019i, lgirdwood, plbossart and ranj063 as code owners April 29, 2025 08:04

bardliao previously approved these changes Apr 30, 2025

View reviewed changes

ujfalusi mentioned this pull request Apr 30, 2025

[BUG] nocodec (SSP) topology with PASSTHROUGH=yes is broken. thesofproject/sof#9979

Closed

jsarha previously approved these changes May 2, 2025

View reviewed changes

kv2019i previously approved these changes May 15, 2025

View reviewed changes

lgirdwood reviewed May 26, 2025

View reviewed changes

Comment thread sound/soc/sof/ipc4-topology.c Outdated

lgirdwood previously approved these changes May 28, 2025

View reviewed changes

ujfalusi dismissed stale reviews from lgirdwood, kv2019i, and bardliao via 6546623 June 23, 2025 13:40

ujfalusi force-pushed the peter/sof/pr/ipc4-tplg-ssp-playback-fix-01 branch from afa0d6a to 6546623 Compare June 23, 2025 13:40

bardliao previously approved these changes Jun 23, 2025

View reviewed changes

lgirdwood previously approved these changes Jun 26, 2025

View reviewed changes

ujfalusi dismissed stale reviews from lgirdwood and bardliao via 4e89c0d October 1, 2025 09:39

ujfalusi force-pushed the peter/sof/pr/ipc4-tplg-ssp-playback-fix-01 branch from 6546623 to 4e89c0d Compare October 1, 2025 09:39

bardliao previously approved these changes Oct 1, 2025

View reviewed changes

ranj063 reviewed Mar 24, 2026

View reviewed changes

lgirdwood reviewed May 15, 2026

View reviewed changes

ujfalusi dismissed bardliao’s stale review via 26f4500 May 19, 2026 05:29

ujfalusi force-pushed the peter/sof/pr/ipc4-tplg-ssp-playback-fix-01 branch from 4e89c0d to 26f4500 Compare May 19, 2026 05:29

kv2019i reviewed May 20, 2026

View reviewed changes

ujfalusi dismissed jsarha’s stale review via 26f4500 June 21, 2026 05:59

Copilot AI review requested due to automatic review settings July 23, 2026 11:13

ujfalusi force-pushed the peter/sof/pr/ipc4-tplg-ssp-playback-fix-01 branch from 26f4500 to 7836cb7 Compare July 23, 2026 11:13

ujfalusi added 2 commits July 23, 2026 14:13

ASoC: SOF: ipc4-topology: Propagate change caused by NHLT data for SSP

a599fc4

We need to adjust the params based on the available and picked SSP blob in the similar way we do for DMIC. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Copilot started reviewing on behalf of ujfalusi July 23, 2026 11:13 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread sound/soc/sof/ipc4-topology.c

Comment on lines +2320 to +2325

memcpy(ref_params, fe_params, sizeof(*ref_params));

if (dir == SNDRV_PCM_STREAM_PLAYBACK) {

ret = sof_ipc4_prepare_dai_copier(sdev, dai, ref_params, dir);

if (ret < 0)

return ret;

}

		@@ -2175,28 +2175,32 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget,
		copier_data = &ipc4_copier->data;
		available_fmt = &ipc4_copier->available_fmt;

Uh oh!

Conversation

ujfalusi commented Apr 29, 2025

Uh oh!

jsarha left a comment

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented May 9, 2025

Uh oh!

ujfalusi commented May 15, 2025

Uh oh!

ujfalusi commented May 15, 2025

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented May 15, 2025

Uh oh!

ujfalusi commented May 16, 2025

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ujfalusi commented Jun 23, 2025

Uh oh!

ujfalusi commented Aug 7, 2025

Uh oh!

ujfalusi commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

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!

ujfalusi commented May 19, 2026

Uh oh!

kv2019i left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants