mbox series

[v2,00/21] ASoC: replace dpcm_playback/capture to playback/capture_only

Message ID 87ttw1gqgn.wl-kuninori.morimoto.gx@renesas.com
Headers show
Series ASoC: replace dpcm_playback/capture to playback/capture_only | expand

Message

Kuninori Morimoto May 25, 2023, 1:16 a.m. UTC
Hi Mark

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.

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
  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(-)

Comments

Pierre-Louis Bossart May 25, 2023, 11:47 p.m. UTC | #1
> 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(-)
>
Kuninori Morimoto May 26, 2023, 1:11 a.m. UTC | #2
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
Kuninori Morimoto June 1, 2023, 11:45 p.m. UTC | #3
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
Pierre-Louis Bossart June 2, 2023, 2:53 p.m. UTC | #4
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'.
Mark Brown June 2, 2023, 3:31 p.m. UTC | #5
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).
Kuninori Morimoto June 4, 2023, 11:31 p.m. UTC | #6
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
Kuninori Morimoto June 4, 2023, 11:49 p.m. UTC | #7
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