thesofproject
ASoC: SOF: ipc4-topology: Fix logic for input format selection for DA…
…I copier capture
Check if all available input audio formats for a DAI copier have the same sample format and use that for reference when selecting the input format. This is needed for the case when a DAI copier for the DMIC supports only 32-bit capture but has multiple input audio formats in topology in order to support different channel counts and rates.
@ranj063 can you remind me why this is needed and what this fixes? We just merged DMIC related stuff, no following if this is related or a new problem?
@ranj063 can you remind me why this is needed and what this fixes? We just merged DMIC related stuff, no following if this is related or a new problem?
@plbossart the DMIC PR that was merged was to deal with NHLT blobs. Typically in our topologies and the BIOS NHLT, we only have 32-bit format blobs and in order to support 16-bit capture, the PR proposed using the 32-bit blob so that we can use 32-bit format for the DMIC DAI copier and convert to 16-bit at the host copier.
But apart from this , the other problem is that our topology has multiple input formats for the DMIC DAI copier in order to support both 2ch and 4ch capture even though the input sample format in both cases in 32-bit. So, in this case, we need to restrict the reference sample format to 32-bit when trying to find a match for the input.
@ranj063 can you remind me why this is needed and what this fixes? We just merged DMIC related stuff, no following if this is related or a new problem?
@plbossart the DMIC PR that was merged was to deal with NHLT blobs. Typically in our topologies and the BIOS NHLT, we only have 32-bit format blobs and in order to support 16-bit capture, the PR proposed using the 32-bit blob so that we can use 32-bit format for the DMIC DAI copier and convert to 16-bit at the host copier.
But apart from this , the other problem is that our topology has multiple input formats for the DMIC DAI copier in order to support both 2ch and 4ch capture even though the input sample format in both cases in 32-bit. So, in this case, we need to restrict the reference sample format to 32-bit when trying to find a match for the input.
You left me in the dust, I have no idea what this is about, sorry.
@plbossart We want to keep capture DAI 32 bits even if capture format to user space is 16 bits. We can't provide to user space 16 bit quality with 16 bit pre-processing pipelines since they all add quantization noise (iir/dcblock, volume, in future more processing). With 32 bits pipelines the noise floor due to processing is much less than 16 bits. Also if need to add gain to capture like in DMIC case or similar lower sensitivity (and high-SPL capable) analog microphone we need the extra precision bits to "shift" to upper parts of the word.
Earlier version of this patch plus now merged #4840 has worked for me to fix the 16 bit capture fail.
@plbossart We want to keep capture DAI 32 bits even if capture format to user space is 16 bits. We can't provide to user space 16 bit quality with 16 bit pre-processing pipelines since they all add quantization noise (iir/dcblock, volume, in future more processing). With 32 bits pipelines the noise floor due to processing is much less than 16 bits. Also if need to add gain to capture like in DMIC case or similar lower sensitivity (and high-SPL capable) analog microphone we need the extra precision bits to "shift" to upper parts of the word.
Earlier version of this patch plus now merged #4840 has worked for me to fix the 16 bit capture fail.
Okay, this makes sense @singalsu, but the commit message doesn't even begin to explain how this is related to 16 bits and that the choice of 32 bits is intentional to improve headroom/avoid quantization.
Honestly this looked like ChatGPT-speak with the prompt: "generate a commit message that will confuse reviewers" :-)
Okay, this makes sense @singalsu, but the commit message doesn't even begin to explain how this is related to 16 bits and that the choice of 32 bits is intentional to improve headroom/avoid quantization.
Honestly this looked like ChatGPT-speak with the prompt: "generate a commit message that will confuse reviewers" :-)
Yep, the problem statement is missing or well hidden from commit text. I pressed already accept but good to improve it.
I think I'm lost with this with the description of if all the input formats are the same (why we have multiple if they are the same?) then select from the formats (from the identical ones) the one which is the only one?
@ujfalusi what I mean is multiple input audio format objects in topology but with different channels/rates but same sample format
@plbossart We want to keep capture DAI 32 bits even if capture format to user space is 16 bits. We can't provide to user space 16 bit quality with 16 bit pre-processing pipelines since they all add quantization noise (iir/dcblock, volume, in future more processing). With 32 bits pipelines the noise floor due to processing is much less than 16 bits. Also if need to add gain to capture like in DMIC case or similar lower sensitivity (and high-SPL capable) analog microphone we need the extra precision bits to "shift" to upper parts of the word. Earlier version of this patch plus now merged #4840 has worked for me to fix the 16 bit capture fail.
Okay, this makes sense @singalsu, but the commit message doesn't even begin to explain how this is related to 16 bits and that the choice of 32 bits is intentional to improve headroom/avoid quantization.
Honestly this looked like ChatGPT-speak with the prompt: "generate a commit message that will confuse reviewers" :-)
@plbossart ive updated the commit message now. Let me know if it still looks like chatgpt-generated.
Looks better @ranj063, but it can be improved further. The problem is that you use 'format' in two separate contexts
" In topology the DAI copier may have multiple input audio format objects that have different channel counts/rates with the same 32-bits for sample format across all of them. "
the input audio format object contains a sample format?
Can we try to find another term so that 'format' is used in a unique way.
Looks better @ranj063, but it can be improved further. The problem is that you use 'format' in two separate contexts
" In topology the DAI copier may have multiple input audio format objects that have different channel counts/rates with the same 32-bits for sample format across all of them. "
the input audio format object contains a sample format?
Can we try to find another term so that 'format' is used in a unique way.
I've replaced sample format with bit depth.
@ranj063, I think this and #4840 should be combined as a fixup for
ASoC: SOF: ipc4-topology: Look for the 32-bit DMIC blob firstBasically drop the
ASoC: SOF: ipc4-topology: Look for the 32-bit DMIC blob firstand do theref_paramsrefinement in case of single input format bit_depth earlier, that will pass the only available params_width to snd_sof_get_nhlt_endpoint_data()If we don't have correct blob for the only supported depth then we anyway going to fail, no? I mean: if we only have blob for 16bit but the copier only have support for 32bit then what we will should do?
Atm, we would use t he 16bit blob but a 32bit copier config, is that going to work?
The problem is that we use two 'conditions' for the two things which depend on each other and there could be a disconnect in the configuration.
@ujfalusi these 2 patches are both requirements for 16-bit capture to work but I'm not sure squashing them up together makes sense. They address 2 separate problem areas and I think it makes sense to keep them separate as they fix 2 different kinds of errors
