| Message ID | 8734sf53kv.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: replace dpcm_playback/capture to playback/capture_only | expand |
On Mon 25 Mar 2024 at 04:37, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> 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@linux.intel.com > Cc: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > 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; > }
Hi Jerome Thank you for your feedback > 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. Current ASoC have validation for ^^^ part DPCM [CPU/xxxx]-[xxxx/Codec] ^^^^ (A) Normal [CPU/Codec] ^^^^^^^^^^^ (In many case, this "xxxx" is "dummy") By this patch-set, It will check all cases DPCM [CPU/xxxx]-[xxxx/Codec] ^^^^^^^^^ ^^^^^^^^^^ (B) Normal [CPU/Codec] ^^^^^^^^^^^ At first, in [CPU/xxxx] case, "xxxx" part should be also checked (in many case, this "xxxx" is "dummy"). And, because it didn't check (A) part before, (B) part might be error on some board (at least Intel board). To avoid such case, temporally it uses "dummy" instead of "Codec" before [15/15]. This means (B) part checked as like below. [xxxx/Codec] -> [xxxx/dummy] Because "dummy" will pass all cases, (B) part is almost same as no check. Yes, it is no meaning, but the code will be simple. > Where you have a CPU supporting both direction and 2 codecs, each > supporting 1 stream direction ? This is a valid i2s configuration. (snip) > > /* > > - * 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 ? Oops, indeed it will indicate warning in your case. How about this ? if ((dai_link->no_pcm) && (!codec_play_t && !codec_capture_t)) { dev_warn_once(...) ... } Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On Wed 27 Mar 2024 at 01:06, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > Hi Jerome > > Thank you for your feedback > >> 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. > > Current ASoC have validation for ^^^ part > > DPCM > [CPU/xxxx]-[xxxx/Codec] > ^^^^ (A) > Normal > [CPU/Codec] > ^^^^^^^^^^^ > > (In many case, this "xxxx" is "dummy") Yes for many DPCM user, you have: DPCM [CPU/dummy]-[dummy/Codec] FYI: on Amlogic it is mostly the following (only considering DCPM, omitting C2C ...) DPCM [CPU-FE/dummy]-[CPU-BE/Codec] With possibly several BE instances per FE, and several codecs per BE. And there is even this for loopbacks: DPCM [CPU-FE/dummy]-[CPU-BE/dummy] > By this patch-set, It will check all cases > > DPCM > [CPU/xxxx]-[xxxx/Codec] > ^^^^^^^^^ ^^^^^^^^^^ (B) > Normal > [CPU/Codec] > ^^^^^^^^^^^ > > At first, in [CPU/xxxx] case, "xxxx" part should be also checked > (in many case, this "xxxx" is "dummy"). > > And, because it didn't check (A) part before, > (B) part might be error on some board (at least Intel board). > To avoid such case, temporally it uses "dummy" instead of "Codec" > before [15/15]. This means (B) part checked as like below. > > [xxxx/Codec] -> [xxxx/dummy] > > Because "dummy" will pass all cases, (B) part is almost same as no check. > Yes, it is no meaning, but the code will be simple. > >> Where you have a CPU supporting both direction and 2 codecs, each >> supporting 1 stream direction ? This is a valid i2s configuration. > (snip) >> > /* >> > - * 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 ? > > Oops, indeed it will indicate warning in your case. > How about this ? > > if ((dai_link->no_pcm) && ^ Actually my comment applies to all links, DPCM backend or not > (!codec_play_t && !codec_capture_t)) { A codec that does not support playback and does not support capture does not support much, does it ? ;) I suppose you meant something like: > (!cpu_play_t && !codec_capture_t)) { Then at first glance, maybe ... CPU and codec seem to exclude each other but that will only work as long as DCPM is limited to a single CPU per link. > dev_warn_once(...) > ... > } > > Thank you for your help !! > > Best regards > --- > Renesas Electronics > Ph.D. Kuninori Morimoto
Hi Jerome Thank you for your feedback > DPCM > [CPU/xxxx]-[xxxx/Codec] > ^^^^ (A) (snip) > DPCM > [CPU/xxxx]-[xxxx/Codec] > ^^^^^^^^^ ^^^^^^^^^^ (B) (snip) > > if ((dai_link->no_pcm) && > ^ Actually my comment applies to all links, DPCM backend or not (snip) > A codec that does not support playback and does not support capture does > not support much, does it ? ;) Unfortunately, some codec which is used on DPCM BE doesn't have .channels_min = 1 settings which is used on snd_soc_dai_stream_valid(). At least Intel is using it. For both Playback/Capture. But it *was* no problem, because (A) part was not checked before. Because of this background, [01/15] patch is using dummy Codec to avoid (B) check. [15/15] patch will indicate WARNING for such codec driver instead of just avoiding. At least, below are the drivers. It is mentioned in git-log. sound/soc/codecs/hda.c sound/pci/hda/patch_hdmi.c dai_link->no_pcm only is OK I think, because it needs to keep compatibility and try to indicates warning for (A) and/or (B) part. If such things happen on FE side, it is just error, not warning. > (!cpu_play_t && !codec_capture_t)) { > > Then at first glance, maybe ... CPU and codec seem to exclude each other but > that will only work as long as DCPM is limited to a single CPU per link. Hmm ?? I'm confusing Did you copy-and-paste miss ?? I have never indicate this pair + I have indicated before - I have not + ( cpu_capture_t && !codec_capture_t) // in [15/15] patch + (!codec_play_t && !codec_capture_t) // in previous mail - (!cpu_play_t && !codec_capture_t) and I'm sorry but I can't understand what you want to tell me. Do you mean Multi-CPU case ? If you can indicate Image or code, it can more help me. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
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); + 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; }
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. 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@linux.intel.com Cc: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- sound/soc/soc-pcm.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)