Message ID | 87ttw1gqgn.wl-kuninori.morimoto.gx@renesas.com |
---|---|
Headers | show |
Series | ASoC: replace dpcm_playback/capture to playback/capture_only | expand |
> This is v2 patch-set of dpcm_playback/capture flag cleanup. > > Current ASoC DPCM need dpcm_playback/capture flags to use it. > But we are using playback/capture_only flag on Normal/Codec2Codec case. > I think these are duplicated, we can share same flags for all cases. > > On v1 patch-set, we noticed that some DPCM BE Codec valid check > breaks compatibility. > > static int soc_get_playback_capture(...) > { > ... > (A) if (dai_link->dynamic || dai_link->no_pcm) { > ... > if (dai_link->dpcm_playback) { > ... > (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > (C) if (snd_soc_dai_stream_valid(cpu_dai, stream)) { > has_playback = 1; > break; > } > } > ... > } > > (A) is for DPCM case, and "dai_link->no_pcm" means BE, > (B)/(C) means CPU validation check. > In many case, DPCM is like this. > > FE (dynamic) BE (no_pcm) > [CPU/dummy-Codec] - [dummy-CPU/Codec] > > DPCM FE (dynamic) Codec no check is no problem, because it is dummy DAI. > DPCM BE (no_pcm) Codec no check is not good, > but checking it might breaks compatibility, because some Codec doesn't have > necessary settings (= channels_min). Solving this issue seems not easy, > because it is using very complex setting timing. > > v2 ignores DPCM BE Codec check, same as before, but added comment for it. > I hope we can valid check for all cases in some day. Our CI tests show a rather large regression on a CometLake ChromeBook, see https://sof-ci.01.org/linuxpr/PR4379/build5131/devicetest/index.html?model=CML_HEL_RT5682&testcase=verify-kernel-boot-log [ 12.883662] kernel: SSP1-Codec: SSP1-Codec: 1 cpus to 4 codecs link is not supported yet [ 12.883674] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: can't create pcm SSP1-Codec :-22 This is problematic, 1:4 connections have been handled for a very long time, this is basic TDM. git blame tells me this was added by " ASoC: soc-pcm.c: cleanup normal connection loop at soc_get_playback_capture() part1 " below... > > v1 -> v2 > - Add Reviewed-by > - Separate Intel patch > - tidyup playback/capture_only flags conversion > > Link: https://lore.kernel.org/r/87353uqjiu.wl-kuninori.morimoto.gx@renesas.com > Link: https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@linux.intel.com > > Kuninori Morimoto (21): > ASoC: soc-pcm.c: indicate error if stream has no playback no capture > ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture() > ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error > ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture() > ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture() > ASoC: soc-pcm.c: cleanup normal connection loop at soc_get_playback_capture() part1 ...here... > ASoC: soc-pcm.c: cleanup normal connection loop at soc_get_playback_capture() part2 > ASoC: soc-pcm.c: cleanup soc_get_playback_capture() > ASoC: amd: replace dpcm_playback/capture to playback/capture_only > ASoC: fsl: replace dpcm_playback/capture to playback/capture_only > ASoC: sof: replace dpcm_playback/capture to playback/capture_only > ASoC: meson: replace dpcm_playback/capture to playback/capture_only > ASoC: Intel: replace dpcm_playback/capture to playback/capture_only > ASoC: samsung: replace dpcm_playback/capture to playback/capture_only > ASoC: mediatek: replace dpcm_playback/capture to playback/capture_only > ASoC: soc-dai.c: replace dpcm_playback/capture to playback/capture_only > ASoC: Intel/avs: replace dpcm_playback/capture to playback/capture_only > ASoC: soc-core.c: replace dpcm_playback/capture to playback/capture_only > ASoC: soc-topology.c: replace dpcm_playback/capture to playback/capture_only > ASoC: soc-compress.c: replace dpcm_playback/capture to playback/capture_only > ASoC: soc-pcm.c: remove dpcm_playback/capture > > include/sound/soc.h | 4 - > sound/soc/amd/acp-da7219-max98357a.c | 20 +-- > sound/soc/amd/acp-es8336.c | 2 - > sound/soc/amd/acp/acp-mach-common.c | 20 +-- > sound/soc/amd/acp3x-rt5682-max9836.c | 6 +- > sound/soc/amd/vangogh/acp5x-mach.c | 3 - > sound/soc/fsl/fsl-asoc-card.c | 16 +-- > sound/soc/fsl/imx-audmix.c | 6 +- > sound/soc/fsl/imx-card.c | 4 +- > sound/soc/intel/avs/boards/da7219.c | 2 - > sound/soc/intel/avs/boards/dmic.c | 4 +- > sound/soc/intel/avs/boards/hdaudio.c | 4 - > sound/soc/intel/avs/boards/i2s_test.c | 2 - > sound/soc/intel/avs/boards/max98357a.c | 2 +- > sound/soc/intel/avs/boards/max98373.c | 2 - > sound/soc/intel/avs/boards/max98927.c | 2 - > sound/soc/intel/avs/boards/nau8825.c | 2 - > sound/soc/intel/avs/boards/rt274.c | 2 - > sound/soc/intel/avs/boards/rt286.c | 2 - > sound/soc/intel/avs/boards/rt298.c | 2 - > sound/soc/intel/avs/boards/rt5682.c | 2 - > sound/soc/intel/avs/boards/ssm4567.c | 2 - > sound/soc/intel/boards/bdw-rt5650.c | 4 - > sound/soc/intel/boards/bdw-rt5677.c | 4 - > sound/soc/intel/boards/bdw_rt286.c | 10 +- > sound/soc/intel/boards/bxt_da7219_max98357a.c | 32 +++-- > sound/soc/intel/boards/bxt_rt298.c | 26 ++-- > sound/soc/intel/boards/bytcht_cx2072x.c | 6 +- > sound/soc/intel/boards/bytcht_da7213.c | 6 +- > sound/soc/intel/boards/bytcht_es8316.c | 6 +- > sound/soc/intel/boards/bytcht_nocodec.c | 6 +- > sound/soc/intel/boards/bytcr_rt5640.c | 6 +- > sound/soc/intel/boards/bytcr_rt5651.c | 6 +- > sound/soc/intel/boards/bytcr_wm5102.c | 6 +- > sound/soc/intel/boards/cht_bsw_max98090_ti.c | 6 +- > sound/soc/intel/boards/cht_bsw_nau8824.c | 6 +- > sound/soc/intel/boards/cht_bsw_rt5645.c | 6 +- > sound/soc/intel/boards/cht_bsw_rt5672.c | 6 +- > sound/soc/intel/boards/cml_rt1011_rt5682.c | 14 +-- > sound/soc/intel/boards/ehl_rt5660.c | 14 +-- > sound/soc/intel/boards/glk_rt5682_max98357a.c | 30 +++-- > sound/soc/intel/boards/hsw_rt5640.c | 10 +- > sound/soc/intel/boards/kbl_da7219_max98357a.c | 26 ++-- > sound/soc/intel/boards/kbl_da7219_max98927.c | 54 ++++----- > sound/soc/intel/boards/kbl_rt5660.c | 18 ++- > sound/soc/intel/boards/kbl_rt5663_max98927.c | 44 +++---- > .../intel/boards/kbl_rt5663_rt5514_max98927.c | 22 ++-- > sound/soc/intel/boards/skl_hda_dsp_common.c | 14 +-- > .../soc/intel/boards/skl_nau88l25_max98357a.c | 26 ++-- > sound/soc/intel/boards/skl_nau88l25_ssm4567.c | 26 ++-- > sound/soc/intel/boards/skl_rt286.c | 26 ++-- > sound/soc/intel/boards/sof_cs42l42.c | 12 +- > sound/soc/intel/boards/sof_da7219_max98373.c | 16 +-- > sound/soc/intel/boards/sof_es8336.c | 8 +- > sound/soc/intel/boards/sof_nau8825.c | 12 +- > sound/soc/intel/boards/sof_pcm512x.c | 8 +- > sound/soc/intel/boards/sof_rt5682.c | 12 +- > sound/soc/intel/boards/sof_sdw.c | 4 +- > sound/soc/intel/boards/sof_ssp_amp.c | 11 +- > sound/soc/intel/boards/sof_wm8804.c | 2 - > sound/soc/mediatek/mt2701/mt2701-cs42448.c | 20 +-- > sound/soc/mediatek/mt2701/mt2701-wm8960.c | 6 +- > sound/soc/mediatek/mt6797/mt6797-mt6351.c | 24 ++-- > sound/soc/mediatek/mt8173/mt8173-max98090.c | 6 +- > .../mediatek/mt8173/mt8173-rt5650-rt5514.c | 6 +- > .../mediatek/mt8173/mt8173-rt5650-rt5676.c | 10 +- > sound/soc/mediatek/mt8173/mt8173-rt5650.c | 10 +- > .../mediatek/mt8183/mt8183-da7219-max98357.c | 34 +++--- > .../mt8183/mt8183-mt6358-ts3a227-max98357.c | 34 +++--- > .../mt8186/mt8186-mt6366-da7219-max98357.c | 86 +++++-------- > .../mt8186/mt8186-mt6366-rt1019-rt5682s.c | 86 +++++-------- > sound/soc/mediatek/mt8188/mt8188-mt6359.c | 48 ++++---- > .../mt8192/mt8192-mt6359-rt1015-rt5682.c | 78 ++++++------ > sound/soc/mediatek/mt8195/mt8195-mt6359.c | 60 +++++---- > sound/soc/meson/axg-card.c | 8 +- > sound/soc/meson/meson-card-utils.c | 4 +- > sound/soc/samsung/odroid.c | 10 +- > sound/soc/soc-compress.c | 11 +- > sound/soc/soc-core.c | 20 +-- > sound/soc/soc-dai.c | 6 +- > sound/soc/soc-pcm.c | 114 +++++++----------- > sound/soc/soc-topology-test.c | 2 - > sound/soc/soc-topology.c | 4 +- > sound/soc/sof/nocodec.c | 4 - > 84 files changed, 511 insertions(+), 842 deletions(-) >
Hi Pierre-Louis Thank you for reporting > [ 12.883662] kernel: SSP1-Codec: SSP1-Codec: 1 cpus to 4 codecs link is not supported yet > [ 12.883674] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: can't create pcm SSP1-Codec :-22 > > This is problematic, 1:4 connections have been handled for a very long > time, this is basic TDM. > > git blame tells me this was added by > " > ASoC: soc-pcm.c: cleanup normal connection loop at > soc_get_playback_capture() part1 > " Oh, I see. Indeed it doesn't allow 1:4 connection. Thank you for reporting, I will fix it in v3. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Pierre-Louis Cc Mark Can I ask you about your opinion ? > > This is problematic, 1:4 connections have been handled for a very long > > time, this is basic TDM. static int soc_get_playback_capture(...) { ... if (dai_link->dynamic || dai_link->no_pcm) { ... } else { ... for_each_rtd_codec_dais(rtd, i, codec_dai) { if (dai_link->num_cpus == 1) { cpu_dai = asoc_rtd_to_cpu(rtd, 0); } else if (dai_link->num_cpus == dai_link->num_codecs) { cpu_dai = asoc_rtd_to_cpu(rtd, i); } else { dev_err(rtd->card->dev, "N cpus to M codecs link is not supported yet\n"); return -EINVAL; } 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; } ... } In case of CPU:Codec = 1:N, and if its validation were CPU : OK Codec : OK Codec : NG ... Current soc_get_playback_capture() will have has_playback/capture = 1 evan though one of Codec was NG. I think it should be error, but am I right ? Thank you for your help !! Best regards --- Kuninori Morimoto
On 6/1/23 18:45, Kuninori Morimoto wrote: > > Hi Pierre-Louis > Cc Mark > > Can I ask you about your opinion ? > >>> This is problematic, 1:4 connections have been handled for a very long >>> time, this is basic TDM. > > static int soc_get_playback_capture(...) > { > ... > if (dai_link->dynamic || dai_link->no_pcm) { > ... > } else { > ... > for_each_rtd_codec_dais(rtd, i, codec_dai) { > if (dai_link->num_cpus == 1) { > cpu_dai = asoc_rtd_to_cpu(rtd, 0); > } else if (dai_link->num_cpus == dai_link->num_codecs) { > cpu_dai = asoc_rtd_to_cpu(rtd, i); > } else { > dev_err(rtd->card->dev, > "N cpus to M codecs link is not supported yet\n"); > return -EINVAL; > } > > 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; > } > ... > } > > In case of CPU:Codec = 1:N, and if its validation were > > CPU : OK > > Codec : OK > Codec : NG > ... > > Current soc_get_playback_capture() will have has_playback/capture = 1 > evan though one of Codec was NG. > I think it should be error, but am I right ? Indeed, we should only enable playback (resp. capture) when all codec dais have the same settings. We should revert the logic here IMHO to go from 'at least one' to 'all'.
On Thu, Jun 01, 2023 at 11:45:31PM +0000, Kuninori Morimoto wrote: > In case of CPU:Codec = 1:N, and if its validation were > CPU : OK > > Codec : OK > Codec : NG > ... > Current soc_get_playback_capture() will have has_playback/capture = 1 > evan though one of Codec was NG. > I think it should be error, but am I right ? I guess the question here is if anything is relying on being able to play/capture to the other CODECs when one of them is bad for some reason. I'd need to spend some time digging into it to refresh my memory, I do recall some systems where the TDM has a mix of things on it (eg, HDMI and analog outputs).
Hi Mark Thank you for the feedback > > In case of CPU:Codec = 1:N, and if its validation were > > > CPU : OK > > > > Codec : OK > > Codec : NG > > ... > > > Current soc_get_playback_capture() will have has_playback/capture = 1 > > evan though one of Codec was NG. > > I think it should be error, but am I right ? > > I guess the question here is if anything is relying on being able to > play/capture to the other CODECs when one of them is bad for some > reason. I'd need to spend some time digging into it to refresh my > memory, I do recall some systems where the TDM has a mix of things on it > (eg, HDMI and analog outputs). Hmm.. in such case, we want to know whether it was acceptable settings or not, otherwise it is impossible to handle error. But in general, apart from whether actually use it or not, I think it should have available settings, but I'm not 100% sure... Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Pierre-Louis Cc Amadeusz Thank you for your feedback > > In case of CPU:Codec = 1:N, and if its validation were > > > > CPU : OK > > > > Codec : OK > > Codec : NG > > ... > > > > Current soc_get_playback_capture() will have has_playback/capture = 1 > > evan though one of Codec was NG. > > I think it should be error, but am I right ? > > Indeed, we should only enable playback (resp. capture) when all codec > dais have the same settings. We should revert the logic here IMHO to go > from 'at least one' to 'all'. Thank you ! The code can be more cleanup if my understanding was correct. As my v1 patch-set and Amadeusz revealed, DPCM BE Codec has been not checked, and Intel drivers rely on it [1]. But it seems it is using complex driver style, and also I can't test it, because I can't access to it. [1] https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@linux.intel.com I'm happy if Intel drivers are updated around it. (Add missing .channels_min or update snd_soc_dai_stream_valid() (?)) I will post remaining patch-set first, thus it is not rush, and of course not mandatory though Thank you for your help !! Best regards --- Kuninori Morimoto