Re: [PATCH 15/15] ASoC: soc-pcm: indicate warning if DPCM BE Codec has no settings

[Jerome Brunet <jbrunet@xxxxxxxxxxxx>]

On Mon 25 Mar 2024 at 04:37, Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx> wrote:

> Historically, ASoC doesn't have validation check for DPCM BE Codec,
> but it should have. Current ASoC is ignoring it same as before,
> but let's indicate the warning about that.
>
> This warning and code should be removed and cleanuped if DPCM BE Codec
> has necessary settings.

Hello Kuninori-san,

I'm not quite sure what you mean by "should have validation" and what
setting exactly we should validate ?

I know I should be able to able to understand that
from the code below but, somehow I have trouble deciphering it.

Could you explain it a bit more ?

I wonder if this going to trip over the same corner case as last time:

Where you have a CPU supporting both direction and 2 codecs, each
supporting 1 stream direction ? This is a valid i2s configuration.

Maybe I'm not understanding the patchset correctly.

> One of the big user which doesn't have it is Intel.
>
> 	--- sound/soc/codecs/hda.c ---
>
> 	static struct snd_soc_dai_driver card_binder_dai = {
> 		.id = -1,
> 		.name = "codec-probing-DAI",
> +		.capture.channels_min = 1,
> +		.playback.channels_min = 1,
> 	};
>
> 	--- sound/pci/hda/patch_hdmi.c ---
>
> 	static int generic_hdmi_build_pcms(...)
> 	{
> 		...
> 		for (...) {
> 			...
> +			pstr->channels_min = 1;
> 		}
>
> 		return 0;
> 	}
>
> Link: https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@xxxxxxxxxxxxxxx
> Cc: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
>  sound/soc/soc-pcm.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index ac42c089815b..9a54d5d49b65 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2796,7 +2796,6 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
>  	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;
> @@ -2817,24 +2816,37 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
>  	 *	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);
> +		int cpu_play_t, cpu_capture_t;
> +		int codec_play_t, codec_capture_t;
> +
> +		cpu_dai		= snd_soc_rtd_to_cpu(rtd,   ch_maps->cpu);
> +		codec_dai	= snd_soc_rtd_to_codec(rtd, ch_maps->codec);
> +
> +		cpu_play_t	= snd_soc_dai_stream_valid(cpu_dai,   cpu_playback);
> +		codec_play_t	= snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK);
> +
> +		cpu_capture_t	= snd_soc_dai_stream_valid(cpu_dai,   cpu_capture);
> +		codec_capture_t	= snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE);
>  
>  		/*
> -		 * FIXME
> +		 * FIXME / CLEAN-UP-ME
>  		 *
>  		 * DPCM BE Codec has been no checked before.
>  		 * It should be checked, but it breaks compatibility.
>  		 * It ignores BE Codec here, so far.
>  		 */
> -		if (dai_link->no_pcm)
> -			codec_dai = dummy_dai;
> +		if ((dai_link->no_pcm) &&
> +		    ((cpu_play_t	&& !codec_play_t) ||
> +		     (cpu_capture_t	&& !codec_capture_t))) {
> +			dev_warn_once(rtd->dev, "DCPM BE Codec has no stream settings (%s)\n",
> +				      codec_dai->name);

Taking one codec at a time, would you trigger a warning for the use case I
described above ?

> +			codec_play_t	= 1;
> +			codec_capture_t	= 1;
> +		}
>  
> -		if (snd_soc_dai_stream_valid(cpu_dai,   cpu_playback) &&
> -		    snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK))
> +		if (cpu_play_t && codec_play_t)
>  			has_playback = 1;
> -		if (snd_soc_dai_stream_valid(cpu_dai,   cpu_capture) &&
> -		    snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE))
> +		if (cpu_capture_t && codec_capture_t)
>  			has_capture = 1;
>  	}


-- 
Jerome