On 3/31/24 19:30, Kuninori Morimoto wrote:
> Current soc_get_playback_capture() (A) is checking playback/capture
> availability for DPCM (X) / Normal (Y) / Codec2Codec (Z) connections.
>
> (A) static int soc_get_playback_capture(...)
> {
> ...
> ^ if (dai_link->dynamic || dai_link->no_pcm) {
> | ...
> |(a) if (dai_link->dpcm_playback) {
> | ...
> | ^ for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
> |(*) ...
> | v }
> | ...
> (X) }
> |(b) if (dai_link->dpcm_capture) {
> | ...
> | ^ for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
> |(*) ...
> | v }
> | ...
> v }
> } else {
> ^ ^ /* Adapt stream for codec2codec links */
> |(Z) int cpu_capture = ...
> | v int cpu_playback = ...
> (Y)
> | ^ for_each_rtd_ch_maps(rtd, i, ch_maps) {
> |(*) ...
> v v }
> }
> ...
> }
>
> (*) part is checking each DAI's availability.
>
> At first, (X) part is for DPCM, and it checks playback/capture
> availability if dai_link has dpcm_playback/capture flag (a)(b).
> But we are already using playback/capture_only flag for Normal (Y) and
> Codec2Codec (Z). We can use this flags for DPCM too.
>
> Before After
> dpcm_playback = 1; => /* no flags */
> dpcm_capture = 1;
>
> dpcm_playback = 1; => playback_only = 1;
>
> dpcm_capture = 1; => capture_only = 1;
>
> dpcm_playback = 0; => error
> dpcm_capture = 0;
>
> This patch convert dpcm_ flags to _only flag, and dpcm_ flag will be
> removed if all driver switched to _only flags.
>
> Here, CPU <-> Codec relationship is like this
>
> DPCM
> [CPU/dummy]-[dummy/Codec]
> ^^^^ ^^^^^
> Normal
> [CPU/Codec]
> ^^^^^^^^^^^
>
> DPCM part (X) is checking only CPU DAI, and
> Normal part (Y) is checking both CPU/Codec DAI
>
> Here, validation check on dummy DAI is always true,
> Therefor we want to expand validation check to all cases.
>
> One note here is that unfortunately DPCM BE Codec had been no validation
> check before, but all cases validation check breaks compatibility on
> some vender's devices. Thus this patch ignore it.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
> sound/soc/soc-pcm.c | 90 +++++++++++++++++++--------------------------
> 1 file changed, 38 insertions(+), 52 deletions(-)
>
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 77ee103b7cd1..8761ae8fc05f 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2793,7 +2793,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
> int *playback, int *capture)
> {
> struct snd_soc_dai_link *dai_link = rtd->dai_link;
> + struct snd_soc_dai_link_ch_map *ch_maps;
> struct snd_soc_dai *cpu_dai;
> + struct snd_soc_dai *codec_dai;
> + struct snd_soc_dai *dummy_dai = snd_soc_find_dai(&snd_soc_dummy_dlc);
> + int cpu_playback;
> + int cpu_capture;
> int has_playback = 0;
> int has_capture = 0;
> int i;
> @@ -2803,65 +2808,46 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
> return -EINVAL;
> }
>
> + /* REMOVE ME */
> if (dai_link->dynamic || dai_link->no_pcm) {
> - int stream;
> -
> - if (dai_link->dpcm_playback) {
> - stream = SNDRV_PCM_STREAM_PLAYBACK;
> -
> - for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
> - if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
> - has_playback = 1;
> - break;
> - }
> - }
> - if (!has_playback) {
> - dev_err(rtd->card->dev,
> - "No CPU DAIs support playback for stream %s\n",
> - dai_link->stream_name);
> - return -EINVAL;
> - }
> + if (dai_link->dpcm_playback && !dai_link->dpcm_capture)
> + dai_link->playback_only = 1;
> + if (!dai_link->dpcm_playback && dai_link->dpcm_capture)
> + dai_link->capture_only = 1;
> + if (!dai_link->dpcm_playback && !dai_link->dpcm_capture) {
> + dev_err(rtd->dev, "no dpcm_playback/capture are selected\n");
> + return -EINVAL;
> }
> - if (dai_link->dpcm_capture) {
> - stream = SNDRV_PCM_STREAM_CAPTURE;
> + }
>
> - for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
> - if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
> - has_capture = 1;
> - break;
> - }
> - }
> + /* Adapt stream for codec2codec links */
> + cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK);
> + cpu_capture = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE);
>
> - if (!has_capture) {
> - dev_err(rtd->card->dev,
> - "No CPU DAIs support capture for stream %s\n",
> - dai_link->stream_name);
> - return -EINVAL;
> - }
> - }
> - } else {
> - struct snd_soc_dai_link_ch_map *ch_maps;
> - struct snd_soc_dai *codec_dai;
> -
> - /* Adapt stream for codec2codec links */
> - int cpu_capture = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE);
> - int cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK);
> + /*
> + * see
> + * soc.h :: [dai_link->ch_maps Image sample]
> + */
> + for_each_rtd_ch_maps(rtd, i, ch_maps) {
> + cpu_dai = snd_soc_rtd_to_cpu(rtd, ch_maps->cpu);
> + codec_dai = snd_soc_rtd_to_codec(rtd, ch_maps->codec);
>
> /*
> - * see
> - * soc.h :: [dai_link->ch_maps Image sample]
> + * FIXME
> + *
> + * DPCM BE Codec has been no checked before.
> + * It should be checked, but it breaks compatibility.
> + * It ignores BE Codec here, so far.
> */
> - for_each_rtd_ch_maps(rtd, i, ch_maps) {
> - cpu_dai = snd_soc_rtd_to_cpu(rtd, ch_maps->cpu);
> - codec_dai = snd_soc_rtd_to_codec(rtd, ch_maps->codec);
> -
> - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
> - snd_soc_dai_stream_valid(cpu_dai, cpu_playback))
> - has_playback = 1;
> - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
> - snd_soc_dai_stream_valid(cpu_dai, cpu_capture))
> - has_capture = 1;
> - }
> + if (dai_link->no_pcm)
> + codec_dai = dummy_dai;
> +
> + if (snd_soc_dai_stream_valid(cpu_dai, cpu_playback) &&
> + snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK))
> + has_playback = 1;
> + if (snd_soc_dai_stream_valid(cpu_dai, cpu_capture) &&
> + snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE))
> + has_capture = 1;
> }
The problem I have is with the following code (not shown with diff)
if (dai_link->playback_only)
has_capture = 0;
if (dai_link->capture_only)
has_playback = 0;
So with this grand unification, all the loops above may make a decision
that could be overridden by these two branches.
This was not the case before for DPCM, all the 'has_capture' and
'has_playback' variables were used as a verification of the dai_link
settings with an error thrown e.g. if the dpcm_playback was set without
any DAIs supporting playback.
Now the dailink settings are used unconditionally. There is one warning
added if there are no settings for a dailink, but we've lost the
detection of a mismatch between dailink and the set of cpu/codec dais
that are part of this dailink.
