ASoC: SOF: ipc4-pcm: support multiple configs in BE DAIs

[kv2019i]

Depends on: thesofproject/sof#7091

ASoC: SOF: ipc4-pcm: support multiple configs for BE DAIs

Backend DAIs may support multiple audio formats. Modify pipeline
setup to select a suitable configuration based on topology and
frontend DAI runtime configuration.

For sampling rate, if one of the BE DAI configurations has
a sampling rate matching that of FE DAI, configure BE DAI to
this rate.

For sample format, the current code hardcodes DAI copier sample format
to 32bit for both playback and capture pipelines. This is not always
desired, so lift the limitation and set the sample format based on
topology definitions for the copiers. For capture pipelines, we want to
set the BE DAI pipeline format based on topology instead of using the FE
DAI format. This covers the common use-case where BE DAI outputs data at
a higher sample precision and sample width is reduced later in the
pipeline. Instead of hardcoding to 32bit, use the BE DAI copier output
format defined in topology.

[kv2019i]

@juimonen FYI, this is needed to have the hwconfig selection work with IPC4.

[kv2019i]

V2 (Jan 24th) update:

• updated implementation and tested with all Bluetooth offload configurations
• cleaned up the code so it is ready for review

[juimonen]

for me it looks good, although can't predict if it has other consequences, driver experts can chime in on that.

[kv2019i]

Hmm, test results are brutal https://sof-ci.01.org/linuxpr/PR4137/build2829/devicetest/index.html so this will need more work. Back to draft and tagging with DNM.

[kv2019i]

Update on Jan 25th, but results are still bad -> https://sof-ci.01.org/linuxpr/PR4137/build2847/devicetest/index.html

There's a problem in deciding which copier instance does the format conversions if FE and BE formats don't match. Now in CI, the failed cases have S16_LE in FE and a topology where DAI/BE copier only supports S32_LE.

In current code, the format of DAI copier is just overridden to S32.

Not sure yet whether to fix in topology, or in code, and how.

[kv2019i]

V4 (25th Jan) update:

• a potential fix to format negotiation for capture pipelines: instead of hardcoding to S32, limit pipeline parameters to format of DAI copier

[kv2019i]

A few comments to help review.

[kv2019i]

With current topologies, this is needed. Not sure whether to keep this in. I'll add a link once I have the tplg PR.

[ranj063]

why would we not set the bit_depth? this seems like a miss

[kv2019i]

@ranj063 thesofproject/sof#6994

Large number of tests will fail without this. If #6994 was a one-off error, I can remove the "case 0" and make it a fatal error.

[ranj063]

@kv2019i I am having a hard time understanding the logic with the loop here. For every available format, you first set the format mask based on the input format and then override it with the output format. Can you please add some explanation as to what we're trying to do here?

[kv2019i]

@ranj063 The "set" verb is misleading but I'm sticking to ALSA semantics where "snd_mask_set()" actually does a logical OR to the mask, i.e. formats are incrementally added to the mask.

I do agree this is still confusing. This code is in dai_link_fixup, and the dailink is really bidirectional. What we want here is to limit the DAI link to the formats declared in the topology. Unfortunately it is possible to have different formats as copier inputs and outputs. Here I'm declaring all formats (both at input and outputs). This defines a superset (e.g. both S16 and S32), even if for particular direction (like capture) only a a subset is actually supported (S32).

This still seems improvement over existing code where we just hardcode to S32 with no input from topology.

But, but, one option I'm considering whether it would be better not to limit the formats in dai_link_fixup at all. Having the format negotiation done in the path enabling code (e.g. in copier_prepare), might be sufficient and having this in one place is probably easier to understand.
UPDATE: I tried a version that removes modifying fmt mask at all, and seems to work for both old and new usages. So this is an option as well.

In IPC3 code we do look up more constraints from tplg (e.g. format from private->comp_dai->config.frame_fmt) and set them in dai_link_fixup. And thus I tried to do the same for IPC4.

[ranj063]

why would we not set the bit_depth? this seems like a miss

[ranj063]

@kv2019i this is also very hard to follow. Basically, the reason for limiting the input format to 32 bits in the case of the DAI copier was that we can match and retreive the format based on the output format along from the list of available formats. Now, if you want to change the logic to not limit to 32 bits, then what you need is to change the matching function (which is sof_ipc4_init_audio_fmt) to include 2 references not one. The current function will match only the output format based on pipeline_params. But in your case you need to match the output to match pipeline_params and input to also match the runtime params.

[kv2019i]

@ranj063 I did consider extending sof_ipc4_init_audio_fmt(), but that's used by all modules and this special handling really applies to "edge" copier. In short, when we start the traversal for capture, prepare is called with hw_params initialized to FE. Now the problem is, if the BE supports multiple formats, which should be used. Example:

• arecord opens FE with S16
  tplg defines capture pipeline with multiple processing components - configured with S32 input/output
• the BE supports both S16 and S32

With current code, the above works as BE capabilities are ignored and the format is just hardcoded to S32 at the BE DAI (for capture).

The above does not work if we want to have anything else than S32 flowing from a capture BE DAI. And e.g. for Bluetooth offload we specifically want this (and probably for many other cases as well).

So then we reach the problem:
which format to pick in copier_prepare for the capture DAI walk?

I think we really want to push this decision to tplg definition and not hardcode it in kernel. Given it is very difficult to try out all combinations (as you'd need to do the full graph walk to see whether all modules on the path can be configured e.g. if copier starts to output S24), what my patch does is to pick the first format defined for the DAI copier as output format.

This allows tplg creators to defined how the data flows out from copiers in capture pipelines and make the format selection more predictable.

So that's the thought process. If this seems feasible, I can try to map that to better code comments.

FYI to @ujfalusi and @plbossart as well.

[ranj063]

Hi @kv2019i I'm a bit confused as to how this works. So you want to allow 16-bit capture in the DAI but here you dont change the ref_audio_fmt or the ref_params? Shouldn't you set the ref_params to fe_params and the ref_audio_fmt to the input_params?

[kv2019i]

@ranj063 More specifically, I'd want to give control of how DAI copier is configured here to topology. The copier can do conversions and in current code, kernel hardcodes this to S32 irrespectively of what is defined in topology for this copier. With this patch, we take the DAI copier output format, and limit the pipeline parameters to its output format.

One confusion is that "fmt" is a mask pointing to pipeline_params and on line 1386 this is copied to ref_params. So we do modify the ref_params which is passed to sof_ipc4_init_audio_fmt().

In sof_ipc4_init_audio_fmt() we have a bit of redundancy as the format selection is already limited, but this is a) the case already as current code overrides fmt to S32, and b) sof_ipc4_init_audio_fmt() is used for all modules so it needs to be more generic.

[kv2019i]

V11 (Feb 14th, commit 0d0051652a53)

• reverted to stricter policy on tplg definitions for dai_out
• above change will result in CI failures with current SOF main, at least topology2: use passthrough-capture-be instead of passthrough-be  sof#7091 will be needed (possibly more PRs to SOF)

[ujfalusi]

V11 (Feb 14th, commit 0d00516)

* reverted to stricter policy on tplg definitions for dai_out

* above change will result in CI failures with current SOF main, at least [202302 tplg add capture be pipe sof#7091](https://github.com/thesofproject/sof/pull/7091) will be needed (possibly more PRs to SOF)

and thesofproject/sof#7091 needs this PR to pass CI or the dependency is not circular?

[kv2019i]

@ujfalusi wrote:

and thesofproject/sof#7091 needs this PR to pass CI or the dependency is not circular?

One way only, the FW 7091 should work with current kernel. But once kernel starts being stricter about the tplg definitions, then FW needs to be ready.

[ujfalusi]

It took me some time to get this, but the comment does not helped...
And I'm still not sure if I got it...

available_fmt->input_pin_fmts[0].audio_fmt.sampling_frequency is be_rate, right?

no, I don't get what this is doing.

What happens if the available_fmt->input_pin_fmts[] only contains single rates, but different sample sizes, channels and this rate does not match for the fe_rate?
We will fail somewhere else?

[kv2019i]

@ujfalusi Right, now that I moved (in V11) this piece of code to same scope where 'be_rate' is defined, yes, I could assign to 'be_rate' now. Will fix the comment.

For the bigger question of what happens if rate is not limited at all, that's a good question. I've been just going by what the existing code does and what we've done with ipc3-pcm.c in the past. For the specific need of enabling multiple BE configs, I've not wanted to reopen the wider arch.

But there is certainly overlap. The dai-fixup is called in hw_params flow. This is standard ASoC stuff. Then in sof_pcm_hw_params() we run the sof_pcm_setup_connected_widgets() path which will do a more complex path enablement and this logic is more specific to SOF. E.g. for capture we traverse all widgets on the path, and set formats for each.

Now whether all constraints could be moved to sof_pcm_setup_connected_widgets(), I don't really know. I don't really see a big obstacle really. @ranj063 @plbossart ? Both paths of logic are done at hw_params epoch, so I don't see a big difference, but I'm sure there is some subtle dependencies.

[kv2019i]

@ujfalusi Dug the history of this code a bit more and the key reason to have the "rate->min/max" lookup in dai fixup is that we do the SSP blob selection here as well (via ipc4_ssp_dai_config_pcm_params_match()). So that's one option to hide this logic behind the SSP specific code and only limit the rate if we need to choose a blob. At least this would be hidden deeper in vendor/DAI specific code and not have this in generic fixup code for IPC4....

[RanderWang]

@kv2019i Another reason is for SRC, .e.g. 44.1k stream for FE, we do SRC in fw to convert it to 48k supported by BE, so we need to use this fixup to change rate for audio bus controller and codec driver.

[RanderWang]

Maybe it is caused my first patch, now I prefer to use output format available_fmt->output_pin_fmts[0].audio_fmt.sampling_frequency since output is for audio bus and codec. we may have different input valid bit from internal pipeline but only one output valid bits by audio dsp

[kv2019i]

@RanderWang fixup is called for both directions (capture/playback) so use of copier pins configs is problematic as e.g. for playback case, copier input may be different than what is actually sent to the DAI. Especially to evaluate sample formats (as copier can do conversions and input/output formats may be different), this is a real problem. For sampling rate (as used in this PR) and channel count, it should not matter whether we look at input or output pins, as both must be the same (as copier cannot convert). This is harder to follow in general than IPC3, where we had an explicit object to describe DAI properties.

[RanderWang]

so now if (use_chain_dma) we do nothing but the original code set rate->min and rate->max. If we don't need to set rate min and max, we can do it as

if (use_chain_dma)
     return 0

[ujfalusi]

We have a switch for dai_type later on, yes it is only handling INTEL_SSP and the ChainDMA is only supported (for now) via HDA, but that can change, I think. Probably a goto then?

[kv2019i]

@RanderWang I was wondering about this but in theory we could use chain-dma for SSP as well, and in that case, we may need to run the DAI-type specific logic later in the function. But yeah, this code could be more reabable, let me split this segment to a separate subfunction in next version.

[ranj063]

We cannot use chain DMA for SSP. It uses gpdma

[ujfalusi]

@ranj063, #4158 gives a glimpse into the future ;)
But the ChainDMA FW implementation as it is does not rule out use of GPDMA on the peripheral side, with some effort it can be made working.

[ujfalusi]

Having said that, for now a simple return 0 in case of chain_dma would make the code nicer. If there will be a need for a change, it can be done when it is due.

[kv2019i]

@ranj063 @ujfalusi As it's more a FW side development whether chain_dma can work with SSP, I'd rather keep the kernel code neutral on this.

[kv2019i]

V12 (Feb 15th, commit 2af0d033fbd1):

• reworked the dai_fixup code and code comments for better readability (based on feedback from @ujfalusi and @RanderWang )
• raise an error if copier config in tplg does not provide sufficient information to pick up correct sampling rate (feedback from @ujfalusi )
• the FW PR updated topology2: use passthrough-capture-be instead of passthrough-be  sof#7091 and now passes the PR test plan together with this PR (FW PR needs to be merged first)

[ujfalusi]

We have a switch for dai_type later on, yes it is only handling INTEL_SSP and the ChainDMA is only supported (for now) via HDA, but that can change, I think. Probably a goto then?

[ranj063]

Looks good @kv2019i

[kv2019i]

V13 (Feb 16th, commit 599f71dcd626):

• harmonized the code style in sof_ipc4_pcm_dai_link_fixup_rate() and ipc4_copier_set_capture_fmt()
• other fixes to comments raised in V12 review
• the FW dependency is now merged, so expectation is that CI will no pass for this PR

[ujfalusi]

@kv2019i, from code point of view looks nice and polished, thank you!
Let's see what CI thinks now.

[ujfalusi]

@kv2019i, well, it is not even compiling :o

[kv2019i]

V14 (Feb 16th, commit f37e1d2)

• fix compiler warning (raised as error in CI build), no other changes

[plbossart]

I am not sure what happens if we have multi stages of mixers/demux and changes of frequency and formats on intermediate branches, but for now this looks good enough. Nice work @kv2019i