topology2: sof-mtl-rt5650: add SSP FMT 24 bits support

[macchian]

use 24 bits format on playback and capture pipelines if codec needs, e.g. RT5650 HS/SPK codec.

[plbossart]

This needs to be reviewed by @ranj063, some parts don't look quite right.

[plbossart]

... but here you do have a 'false' case. So that makes me wonder if this whole patch is consistent.

I guess we'll have to ask @ranj063 for feedback, this whole format selection remains a mystery to the rest of us....

[macchian]

The same overwriting input, output formats with 24 bits/32 bits from io-gateway-capture.conf. The patch is consistent.

[bardliao]

@plbossart I think that depends on whether the original code set the audio format or not. If audio_format is set in the original code, then we should have a false case to set the audio_format as before. If not, we don't need to set the the audio_format since default audio_format worked before. Make sense?

[plbossart]

no, I don't like this sort of undocumented behavior that seems to depends on factors such as the phase of the moon and levels of inheritance. We shouldn't write topologies that are so hard to review and require a PhD in topology to change a format.

And in this case I don't understand how the copier format configurations influences the format on the wire, IOW why can't we just change the SSP configuration?

[plbossart]

Can we please go back to the problem statement?

"use 24 bits format on playback and capture pipelines if codec needs, e.g. RT5650 HS/SPK codec."

What does the codec need, if as I read "codec dai no need to change the sample bits 24 bits"

[macchian]

@plbossart ,

What does the codec need, if as I read "codec dai no need to change the sample bits 24 bits"

Sorry for the confusion. My description is not so precisely. I mean no need to change additional attribute "sample_bits" in dai.ssp. It will influence the bclk freq.

Detail problem statement:
#aplay -r 4800 -c 2 -f s32 /dev/zero
snd_pcm_hw_params: Invalid argument because the rt5645/50 codec not support S32_LE, but support S24_LE.

kernel: [ 9845.213552] rt5645 i2c-10EC5650:00: ASoC: error at snd_soc_dai_hw_params on rt5645-aif2: -22

That is because default DSP/Copier format is 32 valid bits in 32 bits container transmission to SSP. In order to support other specific format codec, we config DSP/Copier as 24 valid bits(LSB) in 32 bits container. By sending the valid_bits ipc token, ipc4 can process and pass S24_LE request to backend codec. In the end, rt5650 receives the 24 bits data from SSP bus.

static int sof_ipc4_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd,
struct snd_pcm_hw_params *params)
{
...
valid_bits = SOF_IPC4_AUDIO_FORMAT_CFG_V_BIT_DEPTH(ipc4_fmt->fmt_cfg);
dev_dbg(component->dev, "Set %s to %d bit format\n", dai->name, valid_bits);
}

    /* Set format if it is specified */
    switch (valid_bits) {
    case 16:
            snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
            break;
    case 24:
            snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
            break;
    case 32:
            snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S32_LE);
            break;
    default:
            break;
    }

    switch (ipc4_copier->dai_type) {
    case SOF_DAI_INTEL_SSP:
            ipc4_ssp_dai_config_pcm_params_match(sdev, (char *)rtd->dai_link->name, params);
            break;

[kv2019i]

Some comments inline.

[kv2019i]

Hmm, with MSB_INTEGER, the data sent to SSP will be basicly same as before, 32bit containers with just the lower 8 bits zeroed out. Not sure if this makes a different to the codec. I'm I missing something, does the codec DAI specification also need a change ("sample_bits 24"), so that we change what is sent over I2S:.?

[macchian]

I'm I missing something, does the codec DAI specification also need a change ("sample_bits 24"), so that we change what is sent over I2S:.?

@kv2019i , codec dai no need to change the sample bits 24 bits. Because the valid bit 24 bits can be in 32 container. Thanks for pointing out, I think the SOF FW currently set LSB_INTEGER and it will use sample type to choose format conversion. It makes no difference to the codec with MSB_INTEGER. Remove it, any concerns?

[bardliao]

I guess we don't need to overwrite out_fmt_cfg if we don't set out_sample_type here right? @kv2019i @ranj063

[kv2019i]

Right, but I think the problem is what we want to achieve (see Pierre's comment above). While ALH (for which we have tplg examples for 24bit output) takes MSB-format 32bit values, SSP behaves differently. We don't have any examples for 24bit SSP in upstream, so this is not so obvious, but SSP hw requires 24bit in the lower bits, so if the valid bits is 24bits in the SSP configuration, then copier needs to be configured to SAMPLE_TYPE_LSB_INTEGER here, and SSP configuration needs to set to have a 24bit configuration.

DSP/Copier will send 32bit containers with audio in the lower 24bit, SSP will send the 24bit over SSP bus.

But yeah, @macchian , is above the correct configuration? The current cavs-rt5682.conf only defines 32bit SSP config. And if that is correct, then you don't need anything special in copier configuration. It outputs 32/32bit by default (MSB/LSB makes no differences if valid_bits==container_bits).

[macchian]

@kv2019i ,

But yeah, @macchian , is above the correct configuration?

yes, correct! It's what we'd like to do. As the exact as you mentioned, we want to achieve valid 24 bits / 32 bits container support.
"DSP/Copier will send 32bit containers with audio in the lower 24bit, SSP will send the 24bit over SSP bus."

[kv2019i]

@macchian Right, so then your patch is correct but the sample type needs to be "out_sample_type $SAMPLE_TYPE_LSB_INTEGER". This is very confusing, but hopefully having more examples will help. The reason for confusion stems from fact that historically the FW has "fixed up" some bad configurations. See e.g. commit 383d17a . So even if host/topology sets copier to input/output 24bit in a certain format, the FW has based on the link type (.e.g whether it's SSP/DMIC/ALH) overwritten the configuration and done the right thing. There are some legacy cases FW still needs to support, but we are trying to get rid of this in topology and have the correct formats in topology and not rely on the FW to fixup the values. Problem is that one must still enter some value in topology and having values in topology than in practise will be ignored by FW, is source for a lot of confusion. Hopefully this clarifies. So some short term pain to adapt, but in longer term this should be easier for both tplg and FW development.

[macchian]

@macchian Right, so then your patch is correct but the sample type needs to be "out_sample_type $SAMPLE_TYPE_LSB_INTEGER".

@kv2019i , yes, it's clear to me. Add in/out_sample_type $SAMPLE_TYPE_LSB_INTEGER for easier review by everyone.

[kv2019i]

Looks good now. @macchian Do you need to change "Object.Dai.SSP" definitions in cavs-rt5682.conf. Now the SSP is still configured to 32bit/32bit, so that's what will be sent to I2S bus. But audio will be in lower 24bits. Is this correct?

@kv2019i , no need I think. Because the ssp configuration keeps the 32/32 bits could both satisfy valid_bits 24 or 32 bits from DSP/Copier in 32bits container separately without changing bclk(64fs x 48k).

[kv2019i]

Thanks @macchian !

[plbossart]

ok for the topology but the cmake stuff has problems, see below @macchian

[marc-hb]

topology2: cavs-mixin-mixout-ssp: cleanup whitespaces to use tabs

Nit: I have no problem with that change and thank you for keeping whitespace changes in a separate commit but it's not really a "cleanup" because there seems to be more inconsistencies left in that file. I think what is important and what you're doing in this particular case is fixing the indentation so the file looks better and is more readable. Whether indentation is getting fixed by tabs or spaces seems secondary. So "fix indentation of..." would be a more accurate title IMHO

[macchian]

topology2: cavs-mixin-mixout-ssp: cleanup whitespaces to use tabs

Nit: I have no problem with that change and thank you for keeping whitespace changes in a separate comit but it's not really a "cleanup" because there seems to be more inconsistencies left in that file. I think what is important and what you're doing in this particular case is fixing the indentation so the file looks better and is more readable. Whether indentation is getting fixed by tabs or spaces seems secondary. So "fix indentation of..." would be a more accurate title IMHO

@marc-hb got it!

[plbossart]

LGTM, thanks for these updates @macchian