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

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

> Because Japanese will dive into long vacation since next week,
> I want to post mail before that. I will back at 7th May.

Enjoy!

>>>> (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).
>>> "
>>
>> I'm still not yet 100% understand around this "dummy" DAI, but is it
>> *not* soc-utils.c :: dummy_dai, but some original dummy DAI is used
>> somewhere ?
>>
>> I know ${LINUX}/sound/soc/codecs/hda.c :: card_binder_dai is one of
>> the DAI which is used but doesn't have channels_min.
>> I think it is used as BE "Codec", but code is checking "CPU" side.
>>
>> Do you know what does this "BE dummy DAI" specifically means here?
> 
> 	(A) : checked CPU capabilities
> 	(B) : uses dpcm_xxx flag
> 	(C) : checks both dpcm_xxx and capabilities
> 	...
> 
> In my understanding, in summary, this dpcm_xxx flag was added to rescue
> dummy DAI which is used on DCPM BE as CPU at (B), because some of them
> might not have channels_min (This "dummy DAI" is not same as soc-utils's
> dummy DAI). Let's name it as "no_chan_DAI" here.
> In this patch, it was added as "mandatory flag", not "option flag",
> thus all DPCM needed to use this dpcm_xxx flag.
> 
> After that (C) was added, but it was contradiction, because
> it checks both dpcm_xxx and channels_min.
> If my understanding was correct, original "no_chan_DAI" was supposed to
> stop working, because it doesn't have channels_min. But there is no
> such report after (C), during this 4 years.
> We don't know which DAI is the "no_chan_DAI" (?)
> 
> Possibilities are as follows
> 	- No one is using "no_chan_DAI"
> 	- "no_chan_DAI" is no longer exist : it was removed ?
> 	- "no_chan_DAI" is no longer exist : it has channels_min ?
> 
> If my expectation was correct, we don't need dpcm_xxx anymore.

I agree with your analysis. We don't have a clear memory/understanding
of which "no_chan_DAI" platforms (B) was supposed to handle, and why no
one reported them as broken by (C).

> But because we have been used dpcm_xxx for 10 years since (B),
> I understand to feel anxious to suddenly remove dpcm_xxx.

Indeed we err on the side of paranoia with such changes!

> I think it should be removed anyway, but want to have grace time ?
> If so, the idea is that we can use it as "option flag" instead of
> "mandatory flag" for a while, like below
> 
> 	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;
> 
> * maybe we want to indicate message like "place re-check the flag and
>   remove it" via dev_info() if dpcm_xxx flag was used ?
> 
> I think +2 kernel version or so is enough for grace time ?
> After that, we can remove dpcm_xxx flag.

I am good with that plan, but I'll need to investigate first why we had
a failure with one of the Chromebooks on this v3 patchset. That may give
us some insights on "special" uses of those flags.

> When we consider it very detail, above code can't 100% keep compatibility
> if the user have been used this dpcm_xxx flag to limit availability,
> for example in case of DAI can use both playback/capture, but it had
> dpcm_playback flag only. But it can use playback_only flag, instead.
> But it is very difficult to find such DAI. Each user need to check.
> 
> I personally think that remove dpcm_xxx directly is no ploblem, but what
> do you think ? I'm happy to hear any opinion, and happy to create
> grace time code if someone want, but if there was no comment during
> Japanese long vacation, I will create patch to remove dpcm_xxx directly.
> 
> 
> BTW, I would like to know detail things around this topic. I'm happy if
> someone knows it.
> 
> * Why dummy DAI doesn't/can't have channels_min ?
> 
> * Why it checks CPU side channels_min only when DPCM ?
>   I think it should check both CPU and Codec.
>   I could understand if it checks FE:CPU and BE:Codec (it is assuming
>   other side was dummy), but both (FE/BE) check CPU side only...