Uh oh!
There was an error while loading. Please reload this page.
ASoC: SOF: ipc4-topology: Always validate the audio format on fmt init - #5435
Conversation
b7f545c to
71da2a6Compareujfalusi
commented
Jun 2, 2025
Changes since v1:
|
bardliao
commented
Jun 23, 2025
SOFCI TEST |
71da2a6 to
15ace47Compareujfalusi
commented
Jun 23, 2025
Changes since v2:
|
ranj063
commented
Jun 24, 2025
@ujfalusi are the LNL/pTL failures related to the PR? |
ujfalusi
commented
Jun 24, 2025
They are, but not sure why... 4 channels???? |
it is not the 4 channels, but the 24bits, the alh-copier.SDW2-Capture.0 only supports 32bit and single format, so now that we check the input fmt for single format as well we see that 24bit is asked and we would have used 32bit, so the fmt is not supported: @singalsu , any idea? |
singalsu
commented
Jun 24, 2025
Is that a topology issue, would it work if we add to alh copier the 24 bit format? |
ujfalusi
commented
Jun 24, 2025
Yes, that would fix the issue |
ujfalusi
commented
Jun 24, 2025
[ 1038.199172] [peter] sof_ipc4_init_input_audio_fmt: 48000Hz/24bit/4ch/1type VS 48000Hz/24bit/4ch/0type The type differs for the same format: In topologies the type is set to MSB mostly, but some random places uses LSB. So, this PR is correct |
…mt init Even if there is a single format supported by the module on it's input, it must be validated that it is matching with the reference parameters. The DAI copier's DAI side reference is adjusted to the formats it supports, but modules deep within the path might have incorrect configuration in topology (single format which is not matching with the previous module's output for example). This should be reported as errors and not silently accepted. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
15ace47 to
fbea2d8Compareujfalusi
commented
Jun 26, 2025
Changes since v3:
|
singalsu
commented
Jun 27, 2025
This patch is needed to be able to support channels up/downmix in firmware. It's needed for thesofproject/sof#10071 . |
| if (params_rate(params) == rate && params_channels(params) == channels && | ||
| sample_valid_bits == valid_bits && sample_type == type) | ||
| break; | ||
| goto in_fmt; |
There was a problem hiding this comment.
stylistically I think the commonly used pattern, where goto is only used for error handling is clearer. IIUC this change is using goto to jump to a success case continuation, while handling the error on the main code path. Also it doesn't seem to be related to the fix. The actual fix seems to be removing the lines 1579-1582 (and removing the jump label), that would also make the patch smaller
There was a problem hiding this comment.
In fact the code - including the return statement - after in_fmt: label could be added here in side this if statement, and the loop would still be reasonable size. But IMO not a big deal.
There was a problem hiding this comment.
right, I converted this function to work and look like the coutnerpart sof_ipc4_init_output_audio_fmt().
I can keep the two function different, but I cannot change the output function when touching the input.
| if (params_rate(params) == rate && params_channels(params) == channels && | ||
| sample_valid_bits == valid_bits && sample_type == type) | ||
| break; | ||
| goto in_fmt; |
There was a problem hiding this comment.
In fact the code - including the return statement - after in_fmt: label could be added here in side this if statement, and the loop would still be reasonable size. But IMO not a big deal.
ranj063
commented
Jun 27, 2025
@bardliao please feel free to merge if you're good |
bardliao
commented
Jun 30, 2025
@ujfalusi Could you take a look at the NOCODEC CI issues? |
something is not right, but it is not this patch, it is correctly flagging that
The bug happens at |
ujfalusi
commented
Jun 30, 2025
@bardliao , found in daily: arecord -D hw:0,12 -c 2 -r 48000 -f S16_LE /dev/null -q If we go back: So, the bug is at |
ujfalusi
commented
Jun 30, 2025
sof-tgl-nocodec.tplg is working fine, but it looks identical. |
ujfalusi
commented
Jun 30, 2025
OK, I can reproduce this on topic/sof-dev. log: The 0,12 will misconfigure: like this: |
ujfalusi
commented
Jun 30, 2025
Right, I think this has never worked correctly and I do have a fix for this rolled since May 2024.... Basically the issue is that we don't really support this type of branch.
|
ujfalusi
commented
Jun 30, 2025
@bardliao, this fixes the issue on topology level: thesofproject/sof#10081 |
ujfalusi
commented
Jun 30, 2025
OK, so that is not fixing the issue, we will need to: #5471 |
ujfalusi
commented
Aug 7, 2025
SOFCI TEST |
Uh oh!
There was an error while loading. Please reload this page.
Even if there is a single format supported by the module on it's input or output, it must be validated that it is matching with the reference parameters.
The DAI copier's DAI side reference is adjusted to the formats it supports, but modules deep within the path might have incorrect configuration in topology (single format which is not matching with the previous module's output for example). This should be reported as errors and not silently accepted.