Skip to content

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

Closed
Closed
[BUG] nocodec (SSP) topology with PASSTHROUGH=yes is broken.#9979
Assignees
ujfalusi
Labels
bugSomething isn't working as expected

Description

@ujfalusi
ujfalusi
opened on Apr 29, 2025

Describe the bug
A nocodec topology which is built using the PASSTHROUGH=yes will result incorrect SSP configurations.

The topology will have one hw_config:
48KHz, S32_LE, stereo

three SSP blobs:
48KHz, S32_LE, stereo
96KHz, S32_LE, stereo
192KHz, S32_LE, stereo

The dai and host copier is supporting three formats on their inputs and outputs:
48KHz, S16_LE, stereo
48KHz, S24_LE, stereo
48KHz, S32_LE, stereo

This only works if the PCM is opened with 48KHz, S32_LE, stereo. All other configurations are broken.

To Reproduce
Use any of the nocodec-fpga topology or build one for TGL (upx) for example:

"cavs-nocodec\;sof-tgl-nocodec-pass\;NUM_DMICS=4,PDM1_MIC_A_ENABLE=1,PDM1_MIC_B_ENABLE=1,\
PREPROCESS_PLUGINS=nhlt,NHLT_BIN=nhlt-sof-tgl-nocodec-pass.bin,\
PLATFORM=tgl,PASSTHROUGH=true"

Reproduction Rate
10/10

Expected behavior
Not broken topology, but I don't know how that should look like...

Impact
A bit more than annoyance as it is not expected to face with a topology which provides broken configuration.

Activity

  1. ujfalusi
    added
    bugSomething isn't working as expected
    on Apr 29, 2025

  2. ujfalusi commented on Apr 29, 2025

    @ujfalusi
    ujfalusi
    on Apr 29, 2025
    ContributorAuthor

    @kv2019i, @singalsu, @jsarha, @ranj063, @lgirdwood, FYI

  3. ranj063 commented on Apr 29, 2025

    @ranj063
    ranj063
    on Apr 29, 2025
    Collaborator

    @ujfalusi what is your thinking about fixing this? You want to expand SSP hw_config in topology or would you prefer converting to 32-bit at the input of the DAI copier always?

  4. ujfalusi commented on Apr 30, 2025

    @ujfalusi
    ujfalusi
    on Apr 30, 2025
    ContributorAuthor

    I'm leaning forward of a fixed single format on DAI side, but on the other hand I think the ability to test S32_LE/S24_LE/S16_LE without loading different topology would have been the best aim.

    Fixed rate, channels, but blobs and hw_config for 32/24/16 bit on the SSP side. In theory it should work with thesofproject/linux#5395 and thesofproject/linux#5396 might be needed as well.

  5. kv2019i commented on Apr 30, 2025

    @kv2019i
    kv2019i
    on Apr 30, 2025
    Collaborator

    Yeah, I think this is collateral damage from overloading nocodec for R&D/test purposes (still probably makes sense, but you get issues like this). For product use, the DAI side format is always fixed (and host side is variable). Only exception is BT offload where there are multiple formats on the DAI (one for each BT profile), but in the BT offload topologies the host/FE side is fixed to same selection (same enumeration of BT profiles). I don't see any need to have both FE and BE variable and opening this can of worms (kernel will then start to do policy choices on where to convert and IMHO this is wrong).

    This leaves the R&D question of how to test multiple formats in our test beds. Maybe have nocodec variants for different DAI side configurations? Anyways this is more of a test asset scalability/maintainability question.

  6. lgirdwood commented on Apr 30, 2025

    @lgirdwood
    lgirdwood
    on Apr 30, 2025
    Member

    This only works if the PCM is opened with 48KHz, S32_LE, stereo. All other configurations are broken.

    This should be rejected by FW at hw_params() i.e. I've no way to convert requested sample rate to 48kHz in this pipeline.

  7. ujfalusi commented on May 2, 2025

    @ujfalusi
    ujfalusi
    on May 2, 2025
    ContributorAuthor

    For any topology containing SSP we must have matching host and SSP configs, the internal format can be 32bit but the formats on both sides must have identical formats, this includes hw_configs.
    for example:
    Playback (capture is just the reverse naturally)
    three host inputs (PCM params): 48K, 2ch with S16_LE, S24_LE, S32_LE
    one host out: 48K, 2ch S32_LE
    ...
    one dai input: 48K, 2ch, S32_LE
    three dai outputs: 48K, 2ch with S16_LE, S24_LE, S32_LE

    three hw_configs: 48K, 2ch with S16_LE, S24_LE, S32_LE

    The DAI format is picked based on the fe_params.

    If we support different rates, then we need to add these as well. I think the DAI side can support multiple formats in every case, like:
    one host inputs (PCM params): 48K, 2ch with S16_LE
    one host out: 48K, 2ch S32_LE
    ...
    one dai input: 48K, 2ch, S32_LE
    three dai outputs: 48K, 2ch with S16_LE, S24_LE, S32_LE

    three hw_configs: 48K, 2ch with S16_LE, S24_LE, S32_LE

    The PCM can only be S16_LE and the DAI will be configured to S16_LE as well all the time to match with the FE.

    What matters is that we must have the format on the "host input" supported on the "dai output" and "hw config", if we list more on the dai side, it does not matter.

  8. ujfalusi commented on May 2, 2025

    @ujfalusi
    ujfalusi
    on May 2, 2025
    ContributorAuthor

    But for SSP this is not possible, we cannot have two hw_config for some reason and even so, the SSP definition does not allow 16 and 32 bit config to be present at the same time because:

    Object.Dai.SSP."N" {
	direction		"duplex" # playback, capture or duplex
	dai_name	"NoCodec-0"
	id 			0
	quirks		"lbm_mode"
	sample_bits	32					<===== THIS (32 or 16 ???)
	Object.hw_config."0" {
		mclk_freq	24000000
		bclk_freq	4800000
		tdm_slot_width	32				<===== VS THIS
	}
	Object.hw_config."1" {
		mclk_freq	24000000
		bclk_freq	4800000
		tdm_slot_width	16				<===== VS THIS
	}
	Object.dai."playback" {
	}
	Object.dai."capture" {
	}

    So, single format for dai for sure. I'm not sure how BT offload works then if it does need different formats... I think it works because it only uses 16bit, no 24, 32 or 8 bit, just difference in rate and/or channels (mono vs stereo)

  9. lgirdwood commented on May 2, 2025

    @lgirdwood
    lgirdwood
    on May 2, 2025
    Member

    For any topology containing SSP we must have matching host and SSP configs, the internal format can be 32bit but the formats on both sides must have identical formats, this includes hw_configs.

    Different formats should also be rejected by FW if pipeline does not permit format change, although my understanding is that teh copier should be able to do this (but for some reason its not). @singalsu any insights here why the copier would not align formats for the SSP blob ?

  10. ujfalusi commented on May 2, 2025

    @ujfalusi
    ujfalusi
    on May 2, 2025
    ContributorAuthor

    @lgirdwood, it does, but with a broken topology the kernel cannot really find anything which is usable, it does not have that level of insights and I doubt we want to build a huge validation machinery in kernel to cover all cases and then go back and rewrite the topology information to correct things.
    Basically we find things alright, but the things we find are not meant to be used together.

  11. ujfalusi commented on May 2, 2025

    @ujfalusi
    ujfalusi
    on May 2, 2025
    ContributorAuthor

    the two kernel patch will handle this better, but the topology remains broken.

  12. ujfalusi commented on May 2, 2025

    @ujfalusi
    ujfalusi
    on May 2, 2025
    ContributorAuthor

    @lgirdwood, I don't think the firmware is parsing the blob. The kernel sends the wrong blob (the only one available) with not matching copier config. My PRs are fixing this, but the topology itself remains broken.

  13. ujfalusi
    added a commit that references this issue on May 2, 2025

    tools: topology2: cavs-nocodec: Fix PASSTHROUGH configuration for SSPs

    f41b320
  14. kv2019i
    added a commit that references this issue on May 5, 2025

    tools: topology2: cavs-nocodec: Fix PASSTHROUGH configuration for SSPs

    fa18fe5

  15. abonislawski commented on May 6, 2025

    @abonislawski
    abonislawski
    on May 6, 2025
    Member

    @ujfalusi please close it if verified

  16. abonislawski
    assigned
    ujfalusi
    on May 6, 2025
  17. ujfalusi
    closed this as completedon May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working as expected

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions