Re: [PATCH v3 01/23] ASoC: soc-pcm: cleanup soc_get_playback_capture()

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

Hi Morimoto-san,

> I still wondering why dpcm_xxx flag itself is needed.

That's an excellent question indeed. And since you started a historical
review, we can get a lot of information.

> (A) Before, it checks channels_min for DPCM, same as current non-DPCM.
> This is very clear for me. Let's name this as "validation check"
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		if (cpu_dai->driver->playback.channels_min)
> 			playback = 1;
> 		if (cpu_dai->driver->capture.channels_min)
> 			capture = 1;
> 
> (B) commit 1e9de42f4324b91ce2e9da0939cab8fc6ae93893
> ("Explicitly set BE DAI link supported stream directions") force use to
> dpcm_xxx flag
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		playback = rtd->dai_link->dpcm_playback;
> 		capture = rtd->dai_link->dpcm_capture;

The reason for this (B) addition is very clear from the commit message

"
Some BE DAIs can be "dummy" (when the DSP is controlling the DAI) and as
such wont have set a minimum number of playback or capture channels
required for BE DAI registration (to establish supported stream directions).
"

So (B) introduced these dpcm_xxx flags as override mechanisms, where the
dailink information matters more than the dai information.

> (C) 9b5db059366ae2087e07892b5fc108f81f4ec189
> ("ASoC: soc-pcm: dpcm: Only allow playback/capture if supported")
> checks channels_min (= validation check) again
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> 		...
> 		playback = rtd->dai_link->dpcm_playback &&
> 			snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK);
> 		capture = rtd->dai_link->dpcm_capture &&
> 			snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE);

It helps to look at the commit message in detail:

"
Normally the dpcm_playback/capture parameter should match the
capabilities of the CPU DAI. However, there is no way to set that
parameter from the device tree (e.g. with simple-audio-card or
qcom sound cards). dpcm_playback/capture are always both set to 1.
"

This is where the direction changed from "dpcm_xxx" being override of
DAI capabilities to "dpcm_xxx" being required to match DAI capabilities,
because some machine drivers incorrectly did an override that made no
sense...

(C) is essentially (A) && (B)

Clearly there's a contradiction. If (C) was the correct solution, then
it would revert the direction in (A) and report an error for dummy dais.

It's been my question from the beginning of this thread, when the
direction information can come from 2 sources, which one do we trust?

> (D) b73287f0b0745961b14e5ebcce92cc8ed24d4d52
> ("ASoC: soc-pcm: dpcm: fix playback/capture checks") expanded it to
> multi connection.

You really want to look at

(E) 4f8721542f7b75954bfad98c51aa59d683d35b50
("ASoC: core: use less strict tests for dailink capabilities")

"
This patch modifies the snd_soc_dai_link_set_capabilities() helper so
that the dpcm_playback (resp. dpcm_capture) dailink capabilities are set
if at least one dai supports playback (resp. capture).

Likewise the checks are modified so that an error is reported only
when dpcm_playback (resp. dpcm_capture) is set but none of the CPU
DAIs support playback (resp. capture).
"

This one has not fundamentally changed the reasons why (C) was
introduced, the requirement is that dpcm_xxx be aligned with at least
ONE DAI capability. It's still not clear how dummy-dais would be handled
since the number of channels may or may not be set for those dummy-dais.

> So, I would say nothing has changed, but become more complicated.

It's not really become more complicated, the open is which of (B) or (C)
are correct.

> Or if (B) added dpcm_xxx as "option flag", it was understandable for me.
> like this
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		playback = (cpu_dai->driver->playback.channels_min > 0) ||
> 			   rtd->dai_link->dpcm_playback;
> 		capture  = (cpu_dai->driver->capture.channels_min  > 0) ||
> 			   rtd->dai_link->dpcm_capture;

That would essentially revert (C), since the direction would ignore the
actual capabilities of the DAI.

IMHO, we should really try with this revert and go back to the initial
definition of (A), where dpcm_xxx is an optional escape mechanism to
allow machine drivers to set the direction. I would guess that would
impact mostly DT/simple-card users and Qualcomm, if the commit message
of (C) is still relevant.

Then we can discuss about merging code and removing flags. For now we
have different directions/opinions on something that's 10 years old,
last modified 4 years ago. We will break lots of eggs if we don't first
agree on what works and what doesn't.

This 23-patch code merge/simplification is premature at this point IMHO,
we should first land in a state where everyone is happy with how
dpcm_xxx flags need to be handled. We can merge dpcm_xxx and xxx_only
flags later when we understand what they are supposed to do...

And now I need a coffee refill :-)

Regards,
-Pierre