[PATCH v3 0/3] ASoC: grace time for DPCM cleanup

[PATCH v3 0/3] ASoC: grace time for DPCM cleanup

[Kuninori Morimoto]

Hi Mark, Pierre-Louis, Jerome
Cc each ASoC driver maintainer

This is v3 of DPCM cleanup

As we discussed in [1], we don't need to use dpcm_playback/capture flag,
so we remove it. But we have been using it for 10 years, some driver might
get damage. The most likely case is that the device/driver can use both
playback/capture, but have only one flag, and not using xxx_only flag.
[1/3] patch indicates warning in such case.

And because of its history, DPCM has been checking CPU side only. But it should
check Codec side too same as non-DPCM. Some device/driver has been bypassed
this check. It should be error. [2/3] patch indicates warning in such case.

Because dpcm_xxx flag is no longer used by [1/3] patch, 
snd_soc_dai_link_set_capabilities() is no longer needed. [3/3] patch remove it.

These adds grace time for DPCM cleanup.
I'm not sure when dpcm_xxx will be removed, and Codec check bypass will be error,
but maybe v6.11 or v6.12 ?
Please check each driver by that time.

[1] https://lore.kernel.org/r/87edaym2cg.wl-kuninori.morimoto.gx@renesas.com

Link: https://lore.kernel.org/r/87edaym2cg.wl-kuninori.morimoto.gx@renesas.com
Link: https://lore.kernel.org/r/87wmo6dyxg.wl-kuninori.morimoto.gx@renesas.com

v2 -> v3
	- tidyup typo (reuqsts -> requests)
	- add Tested-by on git-log

v1 -> v2
	- tidyup Codec check warning output condition

Kuninori Morimoto (3):
  ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition
  ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch
  ASoC: remove snd_soc_dai_link_set_capabilities()

 include/sound/soc-dai.h               |   1 -
 include/sound/soc.h                   |   1 +
 sound/soc/fsl/imx-card.c              |   3 -
 sound/soc/generic/audio-graph-card.c  |   2 -
 sound/soc/generic/audio-graph-card2.c |   2 -
 sound/soc/generic/simple-card.c       |   2 -
 sound/soc/meson/axg-card.c            |   1 -
 sound/soc/meson/gx-card.c             |   1 -
 sound/soc/qcom/common.c               |   1 -
 sound/soc/soc-dai.c                   |  38 ---------
 sound/soc/soc-pcm.c                   | 110 +++++++++++++++++++-------
 11 files changed, 81 insertions(+), 81 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition

[Kuninori Morimoto]

I have been wondering why DPCM needs special flag (= dpcm_playback/capture)
to use it. Below is the history why it was added to ASoC.

(A) In beginning, there was no dpcm_xxx flag on ASoC.
    It checks channels_min for DPCM, same as current non-DPCM.
    Let's name it as "validation check" here.

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		if (cpu_dai->driver->playback.channels_min)
			playback = 1;
		if (cpu_dai->driver->capture.channels_min)
			capture = 1;

(B) commit 1e9de42f4324 ("ASoC: dpcm: Explicitly set BE DAI link supported
    stream directions") force to use dpcm_xxx flag on DPCM. According to
    this commit log, this is because "Some BE dummy DAI doesn't set
    channels_min for playback/capture". But we don't know which DAI is it,
    and not know why it can't/don't have channels_min. Let's name it as
    "no_chan_DAI" here. According to the code and git-log, it is used as
    DCPM-BE and is CPU DAI. I think the correct solution was set
    channels_min on "no_chan_DAI" side, not update ASoC framework side. But
    everything is under smoke today.

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		playback = rtd->dai_link->dpcm_playback;
		capture  = rtd->dai_link->dpcm_capture;

(C) commit 9b5db059366a ("ASoC: soc-pcm: dpcm: Only allow playback/capture
    if supported") checks channels_min (= validation check) again. Because
    DPCM availability was handled by dpcm_xxx flag at that time, but some
    Sound Card set it even though it wasn't available. Clearly there's
    a contradiction here. I think correct solution was update Sound Card
    side instead of ASoC framework. Sound Card side will be updated to
    handle this issue later (commit 25612477d20b ("ASoC: soc-dai: set
    dai_link dpcm_ flags with a helper"))

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		...
		playback = rtd->dai_link->dpcm_playback &&
			   snd_soc_dai_stream_valid(cpu_dai, ...);
		capture = rtd->dai_link->dpcm_capture &&
			   snd_soc_dai_stream_valid(cpu_dai, ...);

This (C) patch should have broken "no_chan_DAI" which doesn't have
channels_min, but there was no such report during this 4 years.
Possibilities case are as follows
	- No one is using "no_chan_DAI"
	- "no_chan_DAI" is no longer exist : was removed ?
	- "no_chan_DAI" is no longer exist : has channels_min ?

Because of these history, this dpcm_xxx is unneeded flag today. But because
we have been used it for 10 years since (B), it may have been used
differently. For example some DAI available both playback/capture, but it
set dpcm_playback flag only, in this case dpcm_xxx flag is used as
availability limitation. We can use playback_only flag instead in this
case, but it is very difficult to find such DAI today.

Let's add grace time to remove dpcm_playback/capture flag.

This patch don't use dpcm_xxx flag anymore, and indicates warning to use
xxx_only flag if both playback/capture were available but using only
one of dpcm_xxx flag, and not using xxx_only flag.

Link: https://lore.kernel.org/r/87edaym2cg.wl-kuninori.morimoto.gx@renesas.com
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Jerome Brunet <jbrunet@baylibre.com>
---
 include/sound/soc.h |  1 +
 sound/soc/soc-pcm.c | 65 ++++++++++++++++++++++++++-------------------
 2 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 33671437ee896..2a3da1d913776 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -815,6 +815,7 @@ struct snd_soc_dai_link {
 	/* This DAI link can route to other DAI links at runtime (Frontend)*/
 	unsigned int dynamic:1;
 
+	/* REMOVE ME */
 	/* DPCM capture and Playback support */
 	unsigned int dpcm_capture:1;
 	unsigned int dpcm_playback:1;
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 711b2f49ed88d..c4d80cad59829 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2795,6 +2795,7 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 {
 	struct snd_soc_dai_link *dai_link = rtd->dai_link;
 	struct snd_soc_dai *cpu_dai;
+	struct snd_soc_dai_link_ch_map *ch_maps;
 	int has_playback = 0;
 	int has_capture  = 0;
 	int i;
@@ -2805,43 +2806,51 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 	}
 
 	if (dai_link->dynamic || dai_link->no_pcm) {
-		int stream;
 
-		if (dai_link->dpcm_playback) {
-			stream = SNDRV_PCM_STREAM_PLAYBACK;
+		for_each_rtd_ch_maps(rtd, i, ch_maps) {
+			cpu_dai	  = snd_soc_rtd_to_cpu(rtd,   ch_maps->cpu);
 
-			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
-				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
-					has_playback = 1;
-					break;
-				}
-			}
-			if (!has_playback) {
-				dev_err(rtd->card->dev,
-					"No CPU DAIs support playback for stream %s\n",
-					dai_link->stream_name);
-				return -EINVAL;
-			}
+			if (snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK))
+				has_playback = 1;
+
+			if (snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE))
+				has_capture = 1;
 		}
-		if (dai_link->dpcm_capture) {
-			stream = SNDRV_PCM_STREAM_CAPTURE;
 
-			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
-				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
-					has_capture = 1;
-					break;
-				}
+		/*
+		 * REMOVE ME
+		 *
+		 * dpcm_xxx flag will be removed soon, Indicates warning if dpcm_xxx flag was used
+		 * as availability limition
+		 */
+		if (has_playback && has_capture) {
+			if ( dai_link->dpcm_playback &&
+			    !dai_link->dpcm_capture  &&
+			    !dai_link->playback_only) {
+				dev_warn(rtd->card->dev,
+					 "both playback/capture are available,"
+					 " but not using playback_only flag (%s)\n",
+					 dai_link->stream_name);
+				dev_warn(rtd->card->dev,
+					 "dpcm_playback/capture are no longer needed,"
+					 " please use playback/capture_only instead\n");
+				has_capture = 0;
 			}
 
-			if (!has_capture) {
-				dev_err(rtd->card->dev,
-					"No CPU DAIs support capture for stream %s\n",
-					dai_link->stream_name);
-				return -EINVAL;
+			if (!dai_link->dpcm_playback &&
+			     dai_link->dpcm_capture  &&
+			    !dai_link->capture_only) {
+				dev_warn(rtd->card->dev,
+					 "both playback/capture are available,"
+					 " but not using capture_only flag (%s)\n",
+					 dai_link->stream_name);
+				dev_warn(rtd->card->dev,
+					 "dpcm_playback/capture are no longer needed,"
+					 " please use playback/capture_only instead\n");
+				has_playback = 0;
 			}
 		}
 	} else {
-		struct snd_soc_dai_link_ch_map *ch_maps;
 		struct snd_soc_dai *codec_dai;
 
 		/* Adapt stream for codec2codec links */
-- 
2.25.1

[PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Kuninori Morimoto]

Current DCPM is checking CPU side availability only, but it should also
check Codec availability. But because of long DPCM operation history,
it is possible that the some Codec driver check have been bypassed.

It should be error, but let's add grace time to update driver.

This patch indicates warning in above case. Each applicable driver need
to update during this grace time.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/soc-pcm.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index c4d80cad59829..71db7379e08aa 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2795,6 +2795,7 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 {
 	struct snd_soc_dai_link *dai_link = rtd->dai_link;
 	struct snd_soc_dai *cpu_dai;
+	struct snd_soc_dai *codec_dai;
 	struct snd_soc_dai_link_ch_map *ch_maps;
 	int has_playback = 0;
 	int has_capture  = 0;
@@ -2806,15 +2807,25 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 	}
 
 	if (dai_link->dynamic || dai_link->no_pcm) {
+		int has_playback_both = 0;
+		int has_capture_both = 0;
 
 		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);
 
 			if (snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK))
 				has_playback = 1;
 
 			if (snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE))
 				has_capture = 1;
+
+			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
+			    snd_soc_dai_stream_valid(cpu_dai,   SNDRV_PCM_STREAM_PLAYBACK))
+				has_playback_both = 1;
+			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
+			    snd_soc_dai_stream_valid(cpu_dai,   SNDRV_PCM_STREAM_CAPTURE))
+				has_capture_both = 1;
 		}
 
 		/*
@@ -2850,9 +2861,39 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 				has_playback = 0;
 			}
 		}
-	} else {
-		struct snd_soc_dai *codec_dai;
 
+		/*
+		 * REMOVE ME
+		 *
+		 * Current DPCM is checking CPU side only, but both CPU and Codec should be
+		 * checked. Indicate warning if there was CPU / Codec mismatch.
+		 * To keep compatibility, warning only for now.
+		 */
+		if ((dai_link->dpcm_playback || dai_link->playback_only) &&
+		    !has_playback_both)
+			dev_warn(rtd->card->dev,
+				 "System requests playback, but not available (%s)."
+				 " Please update Codec driver\n",
+				 dai_link->stream_name);
+		if ((dai_link->dpcm_capture || dai_link->capture_only) &&
+		     !has_capture_both)
+			dev_warn(rtd->card->dev,
+				 "System requests capture, but not available (%s)."
+				 " Please update Codec driver\n",
+				 dai_link->stream_name);
+
+		/*
+		 * REMOVE ME
+		 *
+		 * In case of there was no dpcm_xxx flag, and CPU / Codec mismatch,
+		 * follow new style
+		 */
+		if (!dai_link->dpcm_playback && has_playback)
+			has_playback = has_playback_both;
+		if (!dai_link->dpcm_capture && has_capture)
+			has_capture = has_capture_both;
+
+	} else {
 		/* Adapt stream for codec2codec links */
 		int cpu_capture  = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE);
 		int cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK);
-- 
2.25.1

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Pierre-Louis Bossart]

On 5/19/24 18:31, Kuninori Morimoto wrote:
> Current DCPM is checking CPU side availability only, but it should also
> check Codec availability. But because of long DPCM operation history,
> it is possible that the some Codec driver check have been bypassed.
> 
> It should be error, but let's add grace time to update driver.
> 
> This patch indicates warning in above case. Each applicable driver need
> to update during this grace time.

...

> +		/*
> +		 * REMOVE ME
> +		 *
> +		 * Current DPCM is checking CPU side only, but both CPU and Codec should be
> +		 * checked. Indicate warning if there was CPU / Codec mismatch.
> +		 * To keep compatibility, warning only for now.
> +		 */
> +		if ((dai_link->dpcm_playback || dai_link->playback_only) &&
> +		    !has_playback_both)
> +			dev_warn(rtd->card->dev,
> +				 "System requests playback, but not available (%s)."
> +				 " Please update Codec driver\n",
> +				 dai_link->stream_name);
> +		if ((dai_link->dpcm_capture || dai_link->capture_only) &&
> +		     !has_capture_both)
> +			dev_warn(rtd->card->dev,
> +				 "System requests capture, but not available (%s)."
> +				 " Please update Codec driver\n",
> +				 dai_link->stream_name);
> +

I mentioned in my previous feedback that this isn't quite right. There
are cases where the CPU dai reports capabilities that the codec DAI does
not support - e.g. when the AEC reference is generated in firmware on
the host DSP.

And sure enough we get that warning in the first test:

https://sof-ci.01.org/linuxpr/PR5005/build3040/devicetest/index.html?model=GLK_BOB_DA7219-ipc3&testcase=verify-kernel-boot-log

May 20 13:35:38 jf-glk-bob-da7219-1 kernel: sof_da7219 glk_da7219_def:
System requests capture, but not available (SSP1-Codec). Please update
Codec driver

For those systems, trying to match CPU and codec dais is not going to
work. Either we skip this verification or we have an escape mechanism to
avoid triggering errors.

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Kuninori Morimoto]

Hi Pierre-Louis

Thank you for your feedback

> I mentioned in my previous feedback that this isn't quite right. There
> are cases where the CPU dai reports capabilities that the codec DAI does
> not support - e.g. when the AEC reference is generated in firmware on
> the host DSP.

Hmm... I thought all issue was solved...

> For those systems, trying to match CPU and codec dais is not going to
> work. Either we skip this verification or we have an escape mechanism to
> avoid triggering errors.

Sorry, but I'm not 100% understand about your situation.
Why Codec can't have channels_min ?
If the Codec can flexibly adjust to paired CPU, Codec can have full channels
support, like dummy DAI ? This means verification is based on CPU only.
Is it not enough ?  and/or Can you show me the driver ?

	static struct snd_soc_dai_driver dummy_dai = {
		...
		.playback = {
=>			.channels_min	= 1,
=>			.channels_max	= 384,
			...
		},
		.capture = {
			...
=>			.channels_min	= 1,
=>			.channels_max	= 384,
			...
		 },
		...
	};

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Pierre-Louis Bossart]

>> I mentioned in my previous feedback that this isn't quite right. There
>> are cases where the CPU dai reports capabilities that the codec DAI does
>> not support - e.g. when the AEC reference is generated in firmware on
>> the host DSP.
> 
> Hmm... I thought all issue was solved...
> 
>> For those systems, trying to match CPU and codec dais is not going to
>> work. Either we skip this verification or we have an escape mechanism to
>> avoid triggering errors.
> 
> Sorry, but I'm not 100% understand about your situation.
> Why Codec can't have channels_min ?
> If the Codec can flexibly adjust to paired CPU, Codec can have full channels
> support, like dummy DAI ? This means verification is based on CPU only.
> Is it not enough ?  and/or Can you show me the driver ?
> 
> 	static struct snd_soc_dai_driver dummy_dai = {
> 		...
> 		.playback = {
> =>			.channels_min	= 1,
> =>			.channels_max	= 384,
> 			...
> 		},
> 		.capture = {
> 			...
> =>			.channels_min	= 1,
> =>			.channels_max	= 384,
> 			...
> 		 },
> 		...
> 	};

We cannot change the Maxim amplifier driver, it's used in a variety of
usages and platforms, and there's no reason to create a fake capture dai
just to reflect the use of a capture stream on the CPU side on some
Chromebooks.

The dailinks used for amplifiers in
sound/soc/intel/boards/sof_boards_helpers.c set dpcm_capture always

	link->dpcm_capture = 1; /* feedback stream or firmware-generated echo
reference */

which means that this test will fail:

if ((dai_link->dpcm_capture || dai_link->capture_only) &&
		     !has_capture_both)

I don't disagree that the unconditional use of dpcm_capture isn't very
elegant, but it is what it is. This platform has been around since 2019
and still has about 6 or 7 years of support, so we can't break it with
stricter criteria.

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Kuninori Morimoto]

Hi Pierre-Louis, Mark

> We cannot change the Maxim amplifier driver, it's used in a variety of
> usages and platforms, and there's no reason to create a fake capture dai
> just to reflect the use of a capture stream on the CPU side on some
> Chromebooks.

Why cannot ??
There is no effect to user if Maxim driver has full channel setting same as
dammy DAI. It will be handled together with CPU, and system gets CPU
channels as-is.

> I don't disagree that the unconditional use of dpcm_capture isn't very
> elegant, but it is what it is. This platform has been around since 2019
> and still has about 6 or 7 years of support, so we can't break it with
> stricter criteria.

My opinion is that working without channels settings is wrong.
I can understand that it was working in long years, but it is working with
wrong settings. So justify a wrong-settings is not good idea for me.
And I don't think it is stricter criteria, it becomes *sane* criteria, IMO.

Because it was working with wrong-settings, we need to makes it sane.
This is the reason why it has grace time.

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Pierre-Louis Bossart]

On 5/20/24 20:15, Kuninori Morimoto wrote:
> 
> Hi Pierre-Louis, Mark
> 
>> We cannot change the Maxim amplifier driver, it's used in a variety of
>> usages and platforms, and there's no reason to create a fake capture dai
>> just to reflect the use of a capture stream on the CPU side on some
>> Chromebooks.
> 
> Why cannot ??
> There is no effect to user if Maxim driver has full channel setting same as
> dammy DAI. It will be handled together with CPU, and system gets CPU
> channels as-is.

That would be changing the meaning and purpose of a 'dummy dai'

A 'dummy dai' has historically been used when data was
transmitted/received but the control of that DAI was done externally
with a sideband interface.

Here there is just no hardware for capture in the Maxim amp.

Adding a pretend DAI for the sake of adding a stricter 'sanity check'
does not sound good to me.

>> I don't disagree that the unconditional use of dpcm_capture isn't very
>> elegant, but it is what it is. This platform has been around since 2019
>> and still has about 6 or 7 years of support, so we can't break it with
>> stricter criteria.
> 
> My opinion is that working without channels settings is wrong.
> I can understand that it was working in long years, but it is working with
> wrong settings. So justify a wrong-settings is not good idea for me.
> And I don't think it is stricter criteria, it becomes *sane* criteria, IMO.
> 
> Because it was working with wrong-settings, we need to makes it sane.
> This is the reason why it has grace time.

allow me to give you another counter example, beyond the AEC reference I
mentioned earlier. It's not uncommon for CPU DAIs to have loopback
capabilities, which are used for tests on boards where the codec has no
capture capabilities. I think it's a feature that needs to be allowed,
not a 'wrong setting'.

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

On Tue, May 21, 2024 at 08:43:09AM -0500, Pierre-Louis Bossart wrote:

> allow me to give you another counter example, beyond the AEC reference I
> mentioned earlier. It's not uncommon for CPU DAIs to have loopback
> capabilities, which are used for tests on boards where the codec has no
> capture capabilities. I think it's a feature that needs to be allowed,
> not a 'wrong setting'.

This is something we could do properly if we had full digital routing
rather than bolting things on the side of the CPU<->CODEC model - having
these things where we have to take a CODEC into account even though
we're not actually using it is one of the big issues with DPCM.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Pierre-Louis Bossart]

On 5/21/24 10:12, Mark Brown wrote:
> On Tue, May 21, 2024 at 08:43:09AM -0500, Pierre-Louis Bossart wrote:
> 
>> allow me to give you another counter example, beyond the AEC reference I
>> mentioned earlier. It's not uncommon for CPU DAIs to have loopback
>> capabilities, which are used for tests on boards where the codec has no
>> capture capabilities. I think it's a feature that needs to be allowed,
>> not a 'wrong setting'.
> 
> This is something we could do properly if we had full digital routing
> rather than bolting things on the side of the CPU<->CODEC model - having
> these things where we have to take a CODEC into account even though
> we're not actually using it is one of the big issues with DPCM.

No disagreement here, these echo references and loopbacks are ugly to
support with the dependency between playback and capture directions that
isn't well handled, e.g. if someone starts to capture before playback
started.

For now we're kind of stuck, what I would suggest is just to remove the
extra check that both CPU and codec dai can support a direction, and
move on with the other cleanups suggested by Morimoto-san.

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

On Tue, May 21, 2024 at 11:03:41AM -0500, Pierre-Louis Bossart wrote:
> On 5/21/24 10:12, Mark Brown wrote:
> > On Tue, May 21, 2024 at 08:43:09AM -0500, Pierre-Louis Bossart wrote:

> > This is something we could do properly if we had full digital routing
> > rather than bolting things on the side of the CPU<->CODEC model - having
> > these things where we have to take a CODEC into account even though
> > we're not actually using it is one of the big issues with DPCM.

> No disagreement here, these echo references and loopbacks are ugly to
> support with the dependency between playback and capture directions that
> isn't well handled, e.g. if someone starts to capture before playback
> started.

> For now we're kind of stuck, what I would suggest is just to remove the
> extra check that both CPU and codec dai can support a direction, and
> move on with the other cleanups suggested by Morimoto-san.

Oh, I agree - my point is that as things stand the framework really
can't cope with what needs expressing so we need these things that don't
look quite right.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Kuninori Morimoto]

Hi Pierre-Louis, Mark

Thank you for clarifying the issue.

> allow me to give you another counter example, beyond the AEC reference I
> mentioned earlier. It's not uncommon for CPU DAIs to have loopback
> capabilities, which are used for tests on boards where the codec has no
> capture capabilities. I think it's a feature that needs to be allowed,
> not a 'wrong setting'.

This helped me to understand the situation.

> > For now we're kind of stuck, what I would suggest is just to remove the
> > extra check that both CPU and codec dai can support a direction, and
> > move on with the other cleanups suggested by Morimoto-san.
> 
> Oh, I agree - my point is that as things stand the framework really
> can't cope with what needs expressing so we need these things that don't
> look quite right.

I think same situation can be happen not only DPCM, but normal connection,
too ? And my personally want to have validation check as much as possible,
and automatically validation without Card/rtd flags.
 
In case of ignoring Codec check on DPCM, it allows unexpected direction,
I think. For example if it is not using dummy DAI, and in case of CPU can
playback/capture, Codec can playback, and user expect playback only,
in this case unexpected "capture" will be available. He need to add
playback_only flag, but it is not good for me.

Can we avoid validation check if DAI has some kind of new flag, like
ignore_pair ?

pseudo code is like this

	if (valid(cpu, PLAYBACK))
		has_cpu_playback = 1;

	if (valid(cpu, CAPTURE))
		has_cpu_capture = 1;

	if (valid(codec, PLAYBACK))
		has_codec_playback = 1;

	if (valid(codec, CAPTURE))
		has_codec_capture = 1;

	if (cpu->ignore_pair) {
		has_codec_playback = 1;
		has_codec_capture  = 1;
	}

	if (codec->ignore_pair) {
		has_cpu_playback = 1;
		has_cpu_capture  = 1;
	}

Or more detail ignore_pair_playback, ignore_pair_capture

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Pierre-Louis Bossart]

> In case of ignoring Codec check on DPCM, it allows unexpected direction,
> I think. For example if it is not using dummy DAI, and in case of CPU can
> playback/capture, Codec can playback, and user expect playback only,
> in this case unexpected "capture" will be available. He need to add
> playback_only flag, but it is not good for me.
> 
> Can we avoid validation check if DAI has some kind of new flag, like
> ignore_pair ?
> 
> pseudo code is like this
> 
> 	if (valid(cpu, PLAYBACK))
> 		has_cpu_playback = 1;
> 
> 	if (valid(cpu, CAPTURE))
> 		has_cpu_capture = 1;
> 
> 	if (valid(codec, PLAYBACK))
> 		has_codec_playback = 1;
> 
> 	if (valid(codec, CAPTURE))
> 		has_codec_capture = 1;
> 
> 	if (cpu->ignore_pair) {
> 		has_codec_playback = 1;
> 		has_codec_capture  = 1;
> 	}
> 
> 	if (codec->ignore_pair) {
> 		has_cpu_playback = 1;
> 		has_cpu_capture  = 1;
> 	}
> 
> Or more detail ignore_pair_playback, ignore_pair_capture

There are two options
a) we don't even try to test the codec-cpu match in terms of
capabilities. That means the same behavior as today.
b) we add a chicken bit for platforms to disable the codec-cpu match if
it breaks specific platforms.

The problem with b) is that we don't know what platforms will break. I
reported one example that was caught by our CI, but there could be
additional Chromebooks impacted, who knows.

My vote is a).

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Kuninori Morimoto]

Hi Pierre-Louis

Thank you for your feedback

> There are two options
> a) we don't even try to test the codec-cpu match in terms of
> capabilities. That means the same behavior as today.
> b) we add a chicken bit for platforms to disable the codec-cpu match if
> it breaks specific platforms.
> 
> The problem with b) is that we don't know what platforms will break. I
> reported one example that was caught by our CI, but there could be
> additional Chromebooks impacted, who knows.
> 
> My vote is a).

Yeah, it is a little big problem for us.
Let's keep current style for now.

I will mark it on source code when we remove dpcm_xxx flag
to be possible to resolve someday.

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v3 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch

[Kuninori Morimoto]

Hi Pierre-Louis, Mark again

> I think same situation can be happen not only DPCM, but normal connection,
> too ? And my personally want to have validation check as much as possible,
> and automatically validation without Card/rtd flags.
>  
> In case of ignoring Codec check on DPCM, it allows unexpected direction,
> I think. For example if it is not using dummy DAI, and in case of CPU can
> playback/capture, Codec can playback, and user expect playback only,
> in this case unexpected "capture" will be available. He need to add
> playback_only flag, but it is not good for me.

This patch-set has big effect to each vender, so let use step-by-step
approach for safety. I will remove Codec check from DPCM on next v4.
But it is our one of problems to be solved, IMO.
I will add mark there to indicate it when we remove dpcm_xxx flag.

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

[PATCH v3 3/3] ASoC: remove snd_soc_dai_link_set_capabilities()

[Kuninori Morimoto]

dpcm_xxx flags are no longer needed.

We need to use xxx_only flags instead if needed, but
snd_soc_dai_link_set_capabilities() user adds dpcm_xxx if playback/catpure
were available. Thus converting dpcm_xxx to xxx_only is not needed.
Just remove it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Tested-by: Jerome Brunet <jbrunet@baylibre.com>
---
 include/sound/soc-dai.h               |  1 -
 sound/soc/fsl/imx-card.c              |  3 ---
 sound/soc/generic/audio-graph-card.c  |  2 --
 sound/soc/generic/audio-graph-card2.c |  2 --
 sound/soc/generic/simple-card.c       |  2 --
 sound/soc/meson/axg-card.c            |  1 -
 sound/soc/meson/gx-card.c             |  1 -
 sound/soc/qcom/common.c               |  1 -
 sound/soc/soc-dai.c                   | 38 ---------------------------
 9 files changed, 51 deletions(-)

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index adcd8719d3435..69ba1a628eabc 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -219,7 +219,6 @@ void snd_soc_dai_resume(struct snd_soc_dai *dai);
 int snd_soc_dai_compress_new(struct snd_soc_dai *dai,
 			     struct snd_soc_pcm_runtime *rtd, int num);
 bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream);
-void snd_soc_dai_link_set_capabilities(struct snd_soc_dai_link *dai_link);
 void snd_soc_dai_action(struct snd_soc_dai *dai,
 			int stream, int action);
 static inline void snd_soc_dai_activate(struct snd_soc_dai *dai,
diff --git a/sound/soc/fsl/imx-card.c b/sound/soc/fsl/imx-card.c
index 0e18ccabe28c3..98b37dd2b9013 100644
--- a/sound/soc/fsl/imx-card.c
+++ b/sound/soc/fsl/imx-card.c
@@ -650,9 +650,6 @@ static int imx_card_parse_of(struct imx_card_data *data)
 			link->ops = &imx_aif_ops;
 		}
 
-		if (link->no_pcm || link->dynamic)
-			snd_soc_dai_link_set_capabilities(link);
-
 		/* Get dai fmt */
 		ret = simple_util_parse_daifmt(dev, np, codec,
 					       NULL, &link->dai_fmt);
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c
index 83e3ba773fbd6..714ce1f4a061b 100644
--- a/sound/soc/generic/audio-graph-card.c
+++ b/sound/soc/generic/audio-graph-card.c
@@ -246,8 +246,6 @@ static int graph_dai_link_of_dpcm(struct simple_util_priv *priv,
 
 	graph_parse_convert(dev, ep, &dai_props->adata);
 
-	snd_soc_dai_link_set_capabilities(dai_link);
-
 	ret = graph_link_init(priv, cpu_ep, codec_ep, li, dai_name);
 
 	li->link++;
diff --git a/sound/soc/generic/audio-graph-card2.c b/sound/soc/generic/audio-graph-card2.c
index 81e84095107ed..6aeb578891f68 100644
--- a/sound/soc/generic/audio-graph-card2.c
+++ b/sound/soc/generic/audio-graph-card2.c
@@ -925,8 +925,6 @@ int audio_graph2_link_dpcm(struct simple_util_priv *priv,
 	graph_parse_convert(ep,  dai_props); /* at node of <dpcm> */
 	graph_parse_convert(rep, dai_props); /* at node of <CPU/Codec> */
 
-	snd_soc_dai_link_set_capabilities(dai_link);
-
 	graph_link_init(priv, rport, li, is_cpu);
 err:
 	of_node_put(ep);
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 9c79ff6a568f0..5e66812ffadfe 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -276,8 +276,6 @@ static int simple_dai_link_of_dpcm(struct simple_util_priv *priv,
 
 	simple_parse_convert(dev, np, &dai_props->adata);
 
-	snd_soc_dai_link_set_capabilities(dai_link);
-
 	ret = simple_link_init(priv, node, codec, li, prefix, dai_name);
 
 out_put_node:
diff --git a/sound/soc/meson/axg-card.c b/sound/soc/meson/axg-card.c
index 8c5605c1e34e8..09aa36e94c85b 100644
--- a/sound/soc/meson/axg-card.c
+++ b/sound/soc/meson/axg-card.c
@@ -339,7 +339,6 @@ static int axg_card_add_link(struct snd_soc_card *card, struct device_node *np,
 		dai_link->num_c2c_params = 1;
 	} else {
 		dai_link->no_pcm = 1;
-		snd_soc_dai_link_set_capabilities(dai_link);
 		if (axg_card_cpu_is_tdm_iface(dai_link->cpus->of_node))
 			ret = axg_card_parse_tdm(card, np, index);
 	}
diff --git a/sound/soc/meson/gx-card.c b/sound/soc/meson/gx-card.c
index f1539e542638d..7edca3e49c8f0 100644
--- a/sound/soc/meson/gx-card.c
+++ b/sound/soc/meson/gx-card.c
@@ -107,7 +107,6 @@ static int gx_card_add_link(struct snd_soc_card *card, struct device_node *np,
 		dai_link->num_c2c_params = 1;
 	} else {
 		dai_link->no_pcm = 1;
-		snd_soc_dai_link_set_capabilities(dai_link);
 		/* Check if the cpu is the i2s encoder and parse i2s data */
 		if (gx_card_cpu_identify(dai_link->cpus, "I2S Encoder"))
 			ret = gx_card_parse_i2s(card, np, index);
diff --git a/sound/soc/qcom/common.c b/sound/soc/qcom/common.c
index 3d02aa3844f29..11cbcb588336c 100644
--- a/sound/soc/qcom/common.c
+++ b/sound/soc/qcom/common.c
@@ -145,7 +145,6 @@ int qcom_snd_parse_of(struct snd_soc_card *card)
 
 		if (platform || !codec) {
 			/* DPCM */
-			snd_soc_dai_link_set_capabilities(link);
 			link->ignore_suspend = 1;
 			link->nonatomic = 1;
 		}
diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index fefe394dce72d..f8e46bec6f807 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -479,44 +479,6 @@ bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int dir)
 	return stream->channels_min;
 }
 
-/*
- * snd_soc_dai_link_set_capabilities() - set dai_link properties based on its DAIs
- */
-void snd_soc_dai_link_set_capabilities(struct snd_soc_dai_link *dai_link)
-{
-	bool supported[SNDRV_PCM_STREAM_LAST + 1];
-	int direction;
-
-	for_each_pcm_streams(direction) {
-		struct snd_soc_dai_link_component *cpu;
-		struct snd_soc_dai_link_component *codec;
-		struct snd_soc_dai *dai;
-		bool supported_cpu = false;
-		bool supported_codec = false;
-		int i;
-
-		for_each_link_cpus(dai_link, i, cpu) {
-			dai = snd_soc_find_dai_with_mutex(cpu);
-			if (dai && snd_soc_dai_stream_valid(dai, direction)) {
-				supported_cpu = true;
-				break;
-			}
-		}
-		for_each_link_codecs(dai_link, i, codec) {
-			dai = snd_soc_find_dai_with_mutex(codec);
-			if (dai && snd_soc_dai_stream_valid(dai, direction)) {
-				supported_codec = true;
-				break;
-			}
-		}
-		supported[direction] = supported_cpu && supported_codec;
-	}
-
-	dai_link->dpcm_playback = supported[SNDRV_PCM_STREAM_PLAYBACK];
-	dai_link->dpcm_capture  = supported[SNDRV_PCM_STREAM_CAPTURE];
-}
-EXPORT_SYMBOL_GPL(snd_soc_dai_link_set_capabilities);
-
 void snd_soc_dai_action(struct snd_soc_dai *dai,
 			int stream, int action)
 {
-- 
2.25.1

Re: [PATCH v3 0/3] ASoC: grace time for DPCM cleanup

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 223 bytes --]

On Sun, May 19, 2024 at 11:30:44PM +0000, Kuninori Morimoto wrote:

> v2 -> v3
> 	- tidyup typo (reuqsts -> requests)
> 	- add Tested-by on git-log

I was kind of expecting Jerome's patch to get integrated into the
series?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/3] ASoC: grace time for DPCM cleanup

[Kuninori Morimoto]

Hi Mark

> > v2 -> v3
> > 	- tidyup typo (reuqsts -> requests)
> > 	- add Tested-by on git-log
> 
> I was kind of expecting Jerome's patch to get integrated into the
> series?

Ah, OK
Will do in v4

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto