diff mbox series

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

Message ID 87jzjpe5vh.wl-kuninori.morimoto.gx@renesas.com
State New
Headers show
Series ASoC: grace time for DPCM cleanup | expand

Commit Message

Kuninori Morimoto May 19, 2024, 11:31 p.m. UTC 
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(-)

Comments

Pierre-Louis Bossart May 20, 2024, 3:55 p.m. UTC | #1 
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.
Kuninori Morimoto May 20, 2024, 11:27 p.m. UTC | #2 
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
Kuninori Morimoto May 21, 2024, 1:15 a.m. UTC | #3 
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
Mark Brown May 21, 2024, 3:12 p.m. UTC | #4 
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.
Pierre-Louis Bossart May 21, 2024, 4:03 p.m. UTC | #5 
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.
Kuninori Morimoto May 21, 2024, 11:52 p.m. UTC | #6 
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
Pierre-Louis Bossart May 22, 2024, 1:35 p.m. UTC | #7 
> 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).
diff mbox series

Patch

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