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
