Skip to content

Conversation

@ujfalusi
Copy link
Collaborator

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

bardliao
bardliao previously approved these changes Apr 30, 2025
Copy link

@jsarha jsarha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. LGTM

@ujfalusi
Copy link
Collaborator Author

ujfalusi commented May 9, 2025

@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
Copy link
Collaborator Author

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

@ujfalusi
Copy link
Collaborator Author

SOFCI TEST

kv2019i
kv2019i previously approved these changes May 15, 2025
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

SOFCI TEST

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One open.

lgirdwood
lgirdwood previously approved these changes May 28, 2025
@ujfalusi ujfalusi dismissed stale reviews from lgirdwood, kv2019i, and bardliao via 6546623 June 23, 2025 13:40
@ujfalusi ujfalusi force-pushed the peter/sof/pr/ipc4-tplg-ssp-playback-fix-01 branch from afa0d6a to 6546623 Compare June 23, 2025 13:40
@ujfalusi
Copy link
Collaborator Author

Changes since v1:

  • rebased on top of 8-bit format support pastch

bardliao
bardliao previously approved these changes Jun 23, 2025
lgirdwood
lgirdwood previously approved these changes Jun 26, 2025
@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Aug 7, 2025

SOFCI TEST

@ujfalusi ujfalusi dismissed stale reviews from lgirdwood and bardliao via 4e89c0d October 1, 2025 09:39
@ujfalusi ujfalusi force-pushed the peter/sof/pr/ipc4-tplg-ssp-playback-fix-01 branch from 6546623 to 4e89c0d Compare October 1, 2025 09:39
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 <[email protected]>
…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 <[email protected]>
@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Oct 1, 2025

Changes since v2:

  • rebased on topic/sof-dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants