* [PATCH 0/3] ASoC: grace time for DPCM cleanup
@ 2024-05-07 4:32 Kuninori Morimoto
2024-05-07 4:33 ` [PATCH 1/3] ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition Kuninori Morimoto
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Kuninori Morimoto @ 2024-05-07 4:32 UTC (permalink / raw
To: "Amadeusz Sławiński", Alexandre Belloni,
Alper Nebi Yasak, AngeloGioacchino Del Regno, Banajit Goswami,
Bard Liao, Brent Lu, Cezary Rojewski, Charles Keepax,
Claudiu Beznea, Cristian Ciocaltea, Daniel Baluta, Hans de Goede,
Jaroslav Kysela, Jerome Brunet, Jiawei Wang, Jonathan Corbet,
Kai Vehmanen, Kevin Hilman, Liam Girdwood, Mark Brown, Maso Huang,
Matthias Brugger, Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
Hi Mark, Pierre-Louis
Cc each ASoC driver maintainer
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
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 | 96 ++++++++++++++++++---------
11 files changed, 67 insertions(+), 81 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition
2024-05-07 4:32 [PATCH 0/3] ASoC: grace time for DPCM cleanup Kuninori Morimoto
@ 2024-05-07 4:33 ` Kuninori Morimoto
2024-05-07 4:33 ` [PATCH 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch Kuninori Morimoto
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Kuninori Morimoto @ 2024-05-07 4:33 UTC (permalink / raw
To: "Amadeusz Sławiński", Alexandre Belloni,
Alper Nebi Yasak, AngeloGioacchino Del Regno, Banajit Goswami,
Bard Liao, Brent Lu, Cezary Rojewski, Charles Keepax,
Claudiu Beznea, Cristian Ciocaltea, Daniel Baluta, Hans de Goede,
Jaroslav Kysela, Jerome Brunet, Jiawei Wang, Jonathan Corbet,
Kai Vehmanen, Kevin Hilman, Liam Girdwood, Mark Brown, Maso Huang,
Matthias Brugger, Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-sound
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>
---
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 33671437ee89..2a3da1d91377 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 711b2f49ed88..c4d80cad5982 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch
2024-05-07 4:32 [PATCH 0/3] ASoC: grace time for DPCM cleanup Kuninori Morimoto
2024-05-07 4:33 ` [PATCH 1/3] ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition Kuninori Morimoto
@ 2024-05-07 4:33 ` Kuninori Morimoto
2024-05-07 4:33 ` [PATCH 3/3] ASoC: remove snd_soc_dai_link_set_capabilities() Kuninori Morimoto
2024-05-07 8:47 ` [PATCH 0/3] ASoC: grace time for DPCM cleanup Jerome Brunet
3 siblings, 0 replies; 14+ messages in thread
From: Kuninori Morimoto @ 2024-05-07 4:33 UTC (permalink / raw
To: "Amadeusz Sławiński", Alexandre Belloni,
Alper Nebi Yasak, AngeloGioacchino Del Regno, Banajit Goswami,
Bard Liao, Brent Lu, Cezary Rojewski, Charles Keepax,
Claudiu Beznea, Cristian Ciocaltea, Daniel Baluta, Hans de Goede,
Jaroslav Kysela, Jerome Brunet, Jiawei Wang, Jonathan Corbet,
Kai Vehmanen, Kevin Hilman, Liam Girdwood, Mark Brown, Maso Huang,
Matthias Brugger, Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-sound
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>
---
sound/soc/soc-pcm.c | 31 +++++++++++++++++++++++++++++--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index c4d80cad5982..8652057fce55 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,25 @@ 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 (has_playback && !has_playback_both)
+ dev_warn(rtd->card->dev,
+ "CPU playback is available but Codec playback is not (%s)"
+ " Please update Codec driver\n",
+ dai_link->stream_name);
+ if (has_capture && !has_capture_both)
+ dev_warn(rtd->card->dev,
+ "CPU capture is available but Codec capture is not (%s)"
+ " Please update Codec driver\n",
+ dai_link->stream_name);
+ } 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] ASoC: remove snd_soc_dai_link_set_capabilities()
2024-05-07 4:32 [PATCH 0/3] ASoC: grace time for DPCM cleanup Kuninori Morimoto
2024-05-07 4:33 ` [PATCH 1/3] ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition Kuninori Morimoto
2024-05-07 4:33 ` [PATCH 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch Kuninori Morimoto
@ 2024-05-07 4:33 ` Kuninori Morimoto
2024-05-07 8:47 ` [PATCH 0/3] ASoC: grace time for DPCM cleanup Jerome Brunet
3 siblings, 0 replies; 14+ messages in thread
From: Kuninori Morimoto @ 2024-05-07 4:33 UTC (permalink / raw
To: "Amadeusz Sławiński", Alexandre Belloni,
Alper Nebi Yasak, AngeloGioacchino Del Regno, Banajit Goswami,
Bard Liao, Brent Lu, Cezary Rojewski, Charles Keepax,
Claudiu Beznea, Cristian Ciocaltea, Daniel Baluta, Hans de Goede,
Jaroslav Kysela, Jerome Brunet, Jiawei Wang, Jonathan Corbet,
Kai Vehmanen, Kevin Hilman, Liam Girdwood, Mark Brown, Maso Huang,
Matthias Brugger, Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-sound
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>
---
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 adcd8719d343..69ba1a628eab 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 cb8723965f2f..9c7e24cebd7b 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 83e3ba773fbd..714ce1f4a061 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 62606e20be9a..0d2ac4c9ba3d 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 9c79ff6a568f..5e66812ffadf 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 8c5605c1e34e..09aa36e94c85 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 f1539e542638..7edca3e49c8f 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 747041fa7866..24862002e82b 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 fefe394dce72..f8e46bec6f80 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-07 4:32 [PATCH 0/3] ASoC: grace time for DPCM cleanup Kuninori Morimoto
` (2 preceding siblings ...)
2024-05-07 4:33 ` [PATCH 3/3] ASoC: remove snd_soc_dai_link_set_capabilities() Kuninori Morimoto
@ 2024-05-07 8:47 ` Jerome Brunet
2024-05-08 0:06 ` Kuninori Morimoto
3 siblings, 1 reply; 14+ messages in thread
From: Jerome Brunet @ 2024-05-07 8:47 UTC (permalink / raw
To: Kuninori Morimoto
Cc: Amadeusz Sławiński, Alexandre Belloni, Alper Nebi Yasak,
AngeloGioacchino Del Regno, Banajit Goswami, Bard Liao, Brent Lu,
Cezary Rojewski, Charles Keepax, Claudiu Beznea,
Cristian Ciocaltea, Daniel Baluta, Hans de Goede, Jaroslav Kysela,
Jerome Brunet, Jiawei Wang, Jonathan Corbet, Kai Vehmanen,
Kevin Hilman, Liam Girdwood, Mark Brown, Maso Huang,
Matthias Brugger, Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
On Tue 07 May 2024 at 04:32, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:
> Hi Mark, Pierre-Louis
> Cc each ASoC driver maintainer
>
> 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.
Hi Kuninori-san,
I have tested this series on an Amlogic device (vim3l)
This brings warnings for cases which are perfectly fine.
For example, one of the DPCM backends is the TDM interface. This
interface is capable of both playback and capture. It can be associated
with any i2s/TDM codec.
The codec may do playback and capture too, but it
may also do a single direction. Then usual example is the hdmi codec
which does playback only.
In this case I get:
axg-sound-card sound: CPU capture is available but Codec capture is not (be.dai-link-6) Please update Codec driver
I don't think this is right.
>
> [1] https://lore.kernel.org/r/87edaym2cg.wl-kuninori.morimoto.gx@renesas.com
>
> 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 | 96 ++++++++++++++++++---------
> 11 files changed, 67 insertions(+), 81 deletions(-)
--
Jerome
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-07 8:47 ` [PATCH 0/3] ASoC: grace time for DPCM cleanup Jerome Brunet
@ 2024-05-08 0:06 ` Kuninori Morimoto
2024-05-09 5:50 ` Kuninori Morimoto
0 siblings, 1 reply; 14+ messages in thread
From: Kuninori Morimoto @ 2024-05-08 0:06 UTC (permalink / raw
To: Jerome Brunet
Cc: Amadeusz Sławiński, Alexandre Belloni, Alper Nebi Yasak,
AngeloGioacchino Del Regno, Banajit Goswami, Bard Liao, Brent Lu,
Cezary Rojewski, Charles Keepax, Claudiu Beznea,
Cristian Ciocaltea, Daniel Baluta, Hans de Goede, Jaroslav Kysela,
Jiawei Wang, Jonathan Corbet, Kai Vehmanen, Kevin Hilman,
Liam Girdwood, Mark Brown, Maso Huang, Matthias Brugger,
Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
Hi Jerome
Thank you for testing
> I have tested this series on an Amlogic device (vim3l)
> This brings warnings for cases which are perfectly fine.
>
> For example, one of the DPCM backends is the TDM interface. This
> interface is capable of both playback and capture. It can be associated
> with any i2s/TDM codec.
>
> The codec may do playback and capture too, but it
> may also do a single direction. Then usual example is the hdmi codec
> which does playback only.
>
> In this case I get:
> axg-sound-card sound: CPU capture is available but Codec capture is not (be.dai-link-6) Please update Codec driver
>
> I don't think this is right.
Hmm..., I'm confusing
Does it mean you want to use "playback only" on it ?
If so, did you get below warning too ?
"both playback/capture are available, but not using playback_only flag (%s)\n",
If not, can you please fill below ?
Card
dpcm_playback = (0 or 1)
dpcm_capture = (0 or 1)
playback_only = (0 or 1)
capture_only = (0 or 1)
BE.CPU
playback = (available, not available)
capture = (available, not available)
BE.Codec
playback = (available, not available)
capture = (available, not available)
Expect
playback = (available, not available)
capture = (available, not available)
Thank you for your help !!
Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-08 0:06 ` Kuninori Morimoto
@ 2024-05-09 5:50 ` Kuninori Morimoto
2024-05-09 8:51 ` Jerome Brunet
0 siblings, 1 reply; 14+ messages in thread
From: Kuninori Morimoto @ 2024-05-09 5:50 UTC (permalink / raw
To: Jerome Brunet
Cc: Amadeusz Sławiński, Alexandre Belloni, Alper Nebi Yasak,
AngeloGioacchino Del Regno, Banajit Goswami, Bard Liao, Brent Lu,
Cezary Rojewski, Charles Keepax, Claudiu Beznea,
Cristian Ciocaltea, Daniel Baluta, Hans de Goede, Jaroslav Kysela,
Jiawei Wang, Jonathan Corbet, Kai Vehmanen, Kevin Hilman,
Liam Girdwood, Mark Brown, Maso Huang, Matthias Brugger,
Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
Hi Jerome
I need your help
> > I have tested this series on an Amlogic device (vim3l)
> > This brings warnings for cases which are perfectly fine.
> >
> > For example, one of the DPCM backends is the TDM interface. This
> > interface is capable of both playback and capture. It can be associated
> > with any i2s/TDM codec.
> >
> > The codec may do playback and capture too, but it
> > may also do a single direction. Then usual example is the hdmi codec
> > which does playback only.
> >
> > In this case I get:
> > axg-sound-card sound: CPU capture is available but Codec capture is not (be.dai-link-6) Please update Codec driver
> >
> > I don't think this is right.
>
> Hmm..., I'm confusing
> Does it mean you want to use "playback only" on it ?
> If so, did you get below warning too ?
> "both playback/capture are available, but not using playback_only flag (%s)\n",
>
> If not, can you please fill below ?
>
> Card
> dpcm_playback = (0 or 1)
> dpcm_capture = (0 or 1)
> playback_only = (0 or 1)
> capture_only = (0 or 1)
> BE.CPU
> playback = (available, not available)
> capture = (available, not available)
> BE.Codec
> playback = (available, not available)
> capture = (available, not available)
> Expect
> playback = (available, not available)
> capture = (available, not available)
I need feedback from you, it is still not clear for me.
But I noticed that we want to update below. I'm happy if it can solve your
issue.
- if (has_playback && !has_playback_both)
+ if (has_playback && !has_playback_both && !dai_link->capture_only)
dev_warn(rtd->card->dev, ...)
- if (has_capture && !has_capture_both)
+ if (has_capture && !has_capture_both && !dai_link->playback_only)
dev_warn(rtd->card->dev, ...)
Thank you for your help !!
Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-09 5:50 ` Kuninori Morimoto
@ 2024-05-09 8:51 ` Jerome Brunet
2024-05-09 23:42 ` Kuninori Morimoto
0 siblings, 1 reply; 14+ messages in thread
From: Jerome Brunet @ 2024-05-09 8:51 UTC (permalink / raw
To: Kuninori Morimoto
Cc: Jerome Brunet, Amadeusz Sławiński, Alexandre Belloni,
Alper Nebi Yasak, AngeloGioacchino Del Regno, Banajit Goswami,
Bard Liao, Brent Lu, Cezary Rojewski, Charles Keepax,
Claudiu Beznea, Cristian Ciocaltea, Daniel Baluta, Hans de Goede,
Jaroslav Kysela, Jiawei Wang, Jonathan Corbet, Kai Vehmanen,
Kevin Hilman, Liam Girdwood, Mark Brown, Maso Huang,
Matthias Brugger, Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
On Thu 09 May 2024 at 05:50, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:
> Hi Jerome
>
> I need your help
>
>> > I have tested this series on an Amlogic device (vim3l)
>> > This brings warnings for cases which are perfectly fine.
>> >
>> > For example, one of the DPCM backends is the TDM interface. This
>> > interface is capable of both playback and capture. It can be associated
>> > with any i2s/TDM codec.
>> >
>> > The codec may do playback and capture too, but it
>> > may also do a single direction. Then usual example is the hdmi codec
>> > which does playback only.
>> >
>> > In this case I get:
>> > axg-sound-card sound: CPU capture is available but Codec capture is not (be.dai-link-6) Please update Codec driver
>> >
>> > I don't think this is right.
>>
>> Hmm..., I'm confusing
>> Does it mean you want to use "playback only" on it ?
>> If so, did you get below warning too ?
>> "both playback/capture are available, but not using playback_only flag (%s)\n",
>>
I've checked. No such trace, no.
>> If not, can you please fill below ?
>>
>> Card
>> dpcm_playback = (0 or 1)
>> dpcm_capture = (0 or 1)
>> playback_only = (0 or 1)
>> capture_only = (0 or 1)
>> BE.CPU
>> playback = (available, not available)
>> capture = (available, not available)
>> BE.Codec
>> playback = (available, not available)
>> capture = (available, not available)
>> Expect
>> playback = (available, not available)
>> capture = (available, not available)
I'm not too sure I undestand this. I'll try to illustrate the case
raising the warning as precisely as possible bellow
>
> I need feedback from you, it is still not clear for me.
Sorry. There are some national holidays in France. I'm not spending much
time near the keyboard ATM
> But I noticed that we want to update below. I'm happy if it can solve your
> issue.
>
> - if (has_playback && !has_playback_both)
> + if (has_playback && !has_playback_both && !dai_link->capture_only)
> dev_warn(rtd->card->dev, ...)
>
> - if (has_capture && !has_capture_both)
> + if (has_capture && !has_capture_both && !dai_link->playback_only)
> dev_warn(rtd->card->dev, ...)
>
Honestly I'm a bit lost in all these flag :/
Some for BE error reported here is the full picture
PCM
=====|=====
V
--------
|CPU FE| This CPU (FIFO) does Playback only.
--------
| Because of the CPU, link a playback only one
V
----------
|Codec FE| Using Dummy here
----------
|
V
~~~~~~~~~~
|Routing |
~~~~~~~~~~
^
|
V
--------
|CPU BE| This is the TDM interface. Capable of both Playback and
-------- Capture. Through routing it can be connected to Playback
^ and/or Capture FE CPUs.
|
|
V
-------------
|BE Codec(s)| Possibly N codecs, supporting both direction, or a
------------- Single one, or one direction each. In this particular case
| it is Playback only C2C.
|
---- V ------------------ From Here, it is specific to HDMI -------------
~~~~~~~~~~~~~
|C2C Routing| SoC has routing has mux between the different TDM
~~~~~~~~~~~~~ interfaces and the HDMI controller
|
|
V
---------
|C2C CPU| This is a playback only CPU for HDMI
---------
|
V
-----------
|HDMI Codec| The usual HDMI codec, playback only
-----------
Better picture are available in the SoC doc.
There is publicly available datasheet here [1]
Audio paths are displayed in Section 9, page 807, Figure 9-1.
A TDM interface in this figure is combination of a TDMOUT and a TDMIN
(axg-tdm-formatter.c). The axg-tdm-interface.c joins them because they
use the same pads and clocks.
[1]: https://dl.khadas.com/products/vim3l/datasheet/s905d3_datasheet_0.2_wesion.pdf
>
>
> Thank you for your help !!
>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto
--
Jerome
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-09 8:51 ` Jerome Brunet
@ 2024-05-09 23:42 ` Kuninori Morimoto
2024-05-12 9:33 ` Jerome Brunet
0 siblings, 1 reply; 14+ messages in thread
From: Kuninori Morimoto @ 2024-05-09 23:42 UTC (permalink / raw
To: Jerome Brunet
Cc: Amadeusz Sławiński, Alexandre Belloni, Alper Nebi Yasak,
AngeloGioacchino Del Regno, Banajit Goswami, Bard Liao, Brent Lu,
Cezary Rojewski, Charles Keepax, Claudiu Beznea,
Cristian Ciocaltea, Daniel Baluta, Hans de Goede, Jaroslav Kysela,
Jiawei Wang, Jonathan Corbet, Kai Vehmanen, Kevin Hilman,
Liam Girdwood, Mark Brown, Maso Huang, Matthias Brugger,
Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
Hi Jerome
Thank you for your reply
> >> If so, did you get below warning too ?
> >> "both playback/capture are available, but not using playback_only flag (%s)\n",
> >>
>
> I've checked. No such trace, no.
OK, thanks
> >> Card
> >> dpcm_playback = (0 or 1)
> >> dpcm_capture = (0 or 1)
> >> playback_only = (0 or 1)
> >> capture_only = (0 or 1)
> >> BE.CPU
> >> playback = (available, not available)
> >> capture = (available, not available)
> >> BE.Codec
> >> playback = (available, not available)
> >> capture = (available, not available)
> >> Expect
> >> playback = (available, not available)
> >> capture = (available, not available)
>
> I'm not too sure I undestand this. I'll try to illustrate the case
> raising the warning as precisely as possible bellow
Thanks
Because you got was
(A) axg-sound-card sound: CPU capture is available but Codec capture is
not (be.dai-link-6) Please update Codec driver
It is for BE. And validation check is for each rtd only, this means it checks
BE only, relationship with other rtd is not related here.
> --------
> |CPU BE| This is the TDM interface. Capable of both Playback and
> -------- Capture. Through routing it can be connected to Playback
> ^ and/or Capture FE CPUs.
> |
> V
> -------------
> |BE Codec(s)| Possibly N codecs, supporting both direction, or a
> ------------- Single one, or one direction each. In this particular case
> | it is Playback only C2C.
So, I think the warning happen here.
The validation check is checking this BE only.
As I mentioned above, you use this BE through playback only FE and/or C2C,
but that relationship is not related to here.
According to above explanation, this BE itself is available for both
playback and capture. And you didn't get below warning, I guess this BE
has both dpcm_playback/capture flag, and no xxx_only flag.
"both playback/capture are available, but not using playback_only
flag (%s)\n",
Before my patch, the validation check is checks CPU-BE only, but it also
checks BE-Codec after my patch, and you got the warning (A).
So, I guess your BE-Codec simply missing capture channels_min settings.
Please double check it, or please tell me which codec driver this BE is
using.
static struct snd_soc_dai_driver xxx_dai = {
...
.playback = {
...
.channels_min = x,
...
},
.capture = {
...
=> .channels_min = x,
...
},
},
> But I noticed that we want to update below. I'm happy if it can solve your
> issue.
>
> - if (has_playback && !has_playback_both)
> + if (has_playback && !has_playback_both && !dai_link->capture_only)
> dev_warn(rtd->card->dev, ...)
>
> - if (has_capture && !has_capture_both)
> + if (has_capture && !has_capture_both && !dai_link->playback_only)
> dev_warn(rtd->card->dev, ...)
>
> Honestly I'm a bit lost in all these flag :/
Thanks, no problem, me too :9
Unfortunately and confusingly, there are many combination exist around here.
But because of your feedback, I noticed one missing pattern. Thanks
Thank you for your help !!
Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-09 23:42 ` Kuninori Morimoto
@ 2024-05-12 9:33 ` Jerome Brunet
2024-05-13 0:11 ` Kuninori Morimoto
0 siblings, 1 reply; 14+ messages in thread
From: Jerome Brunet @ 2024-05-12 9:33 UTC (permalink / raw
To: Kuninori Morimoto
Cc: Jerome Brunet, Amadeusz Sławiński, Alexandre Belloni,
Alper Nebi Yasak, AngeloGioacchino Del Regno, Banajit Goswami,
Bard Liao, Brent Lu, Cezary Rojewski, Charles Keepax,
Claudiu Beznea, Cristian Ciocaltea, Daniel Baluta, Hans de Goede,
Jaroslav Kysela, Jiawei Wang, Jonathan Corbet, Kai Vehmanen,
Kevin Hilman, Liam Girdwood, Mark Brown, Maso Huang,
Matthias Brugger, Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
On Thu 09 May 2024 at 23:42, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:
> Hi Jerome
>
> Thank you for your reply
>
>> >> If so, did you get below warning too ?
>> >> "both playback/capture are available, but not using playback_only flag (%s)\n",
>> >>
>>
>> I've checked. No such trace, no.
>
> OK, thanks
>
>> >> Card
>> >> dpcm_playback = (0 or 1)
>> >> dpcm_capture = (0 or 1)
>> >> playback_only = (0 or 1)
>> >> capture_only = (0 or 1)
>> >> BE.CPU
>> >> playback = (available, not available)
>> >> capture = (available, not available)
>> >> BE.Codec
>> >> playback = (available, not available)
>> >> capture = (available, not available)
>> >> Expect
>> >> playback = (available, not available)
>> >> capture = (available, not available)
>>
>> I'm not too sure I undestand this. I'll try to illustrate the case
>> raising the warning as precisely as possible bellow
>
> Thanks
>
> Because you got was
>
> (A) axg-sound-card sound: CPU capture is available but Codec capture is
> not (be.dai-link-6) Please update Codec driver
>
> It is for BE. And validation check is for each rtd only, this means it checks
> BE only, relationship with other rtd is not related here.
>
>> --------
>> |CPU BE| This is the TDM interface. Capable of both Playback and
>> -------- Capture. Through routing it can be connected to Playback
>> ^ and/or Capture FE CPUs.
>> |
>> V
>> -------------
>> |BE Codec(s)| Possibly N codecs, supporting both direction, or a
>> ------------- Single one, or one direction each. In this particular case
>> | it is Playback only C2C.
>
> So, I think the warning happen here.
> The validation check is checking this BE only.
>
> As I mentioned above, you use this BE through playback only FE and/or C2C,
> but that relationship is not related to here.
>
> According to above explanation, this BE itself is available for both
> playback and capture. And you didn't get below warning, I guess this BE
> has both dpcm_playback/capture flag, and no xxx_only flag.
>
> "both playback/capture are available, but not using playback_only
> flag (%s)\n",
>
Apparently so, but it should not.
Before your patchset, axg-card.c (and gx-card.c) relied on
snd_soc_dai_link_set_capabilities() to init dpcm_playback/capture flags.
That was done following another semantic change on those flags:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc/meson?h=v6.9-rc7&id=da3f23fde9d7b4a7e0ca9a9a096cec3104df1b82
- 1st problem: I see that following your removal of
snd_soc_dai_link_set_capabilities(), the dpcm_playback/capture flags
are no longer properly initialised in the amlogic card drivers.
That will need fixing.
Then, for TDM backends (like here), the tdm slots are checked and the
direction is disabled if it has no slots:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/meson/axg-card.c?h=v6.9-rc7#n171
In theory, dpcm_capture should be 0 for this link which has no Rx
slot:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi?h=v6.9-rc7#n218
... Sooo, that is 2nd problem
> Before my patch, the validation check is checks CPU-BE only, but it also
> checks BE-Codec after my patch, and you got the warning (A).
Yes I've got that. I was not expecting a failure on this case TBH.
I was more looking for the infamous 2 codecs case, each with a single
direction (which I did not check yet ...)
>
> So, I guess your BE-Codec simply missing capture channels_min settings.
> Please double check it, or please tell me which codec driver this BE is
> using.
Here the codec:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/meson/g12a-tohdmitx.c?h=v6.9-rc7
>
> static struct snd_soc_dai_driver xxx_dai = {
> ...
> .playback = {
> ...
> .channels_min = x,
> ...
> },
> .capture = {
> ...
> => .channels_min = x,
> ...
> },
This codec is not meant to have capture channels.
I think DT description and the card driver settings (before the removal of
snd_soc_dai_link_set_capabilities()) are correct.
IMO, those check become too restrictive. We are adding a lot of code I'm
not sure I understand what we stand by going so far in the
consistency checks.
Initially those dpcm_playback/capture flag could be used to just
forcefully disable a link direction, regardless of the CPUs or codecs present
on the link. It was just another setting and it was not required to be consistent
with anything. It just declared whether the direction was allowed on the
link, or not.
It changed this commit, and the flags suddenly needed to be consistent
with whatever was on link:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc?h=v6.9-rc7&id=b73287f0b074
I complained back then and I still don't think this change was good.
If the flags needs to consistent with what is on the link, and we have
the ability to check it, why let card drivers set it and then complain
about it in ASoC checks ? Why not deal with it in the framework directly ?
I think the simplest solution would to just go back to the initial
meaning of these flags which was just the card driver declaring the
direction allowed/disallowed on a link. Then ASoC can mix that
information with whatever it finds with DAI drivers and figure out what
is actually possible.
It would give a clear and separate semantic meaning to those flags
instead something redundant with the DAI driver info.
What we have currently is not straight forward to set and check.
It makes the code unnecessarily complicated and difficult to maintain. I
think the difficulties with this patchset are a good illustration of
that, unfortunately.
> },
>
>> But I noticed that we want to update below. I'm happy if it can solve your
>> issue.
>>
>> - if (has_playback && !has_playback_both)
>> + if (has_playback && !has_playback_both && !dai_link->capture_only)
>> dev_warn(rtd->card->dev, ...)
>>
>> - if (has_capture && !has_capture_both)
>> + if (has_capture && !has_capture_both && !dai_link->playback_only)
>> dev_warn(rtd->card->dev, ...)
>>
>> Honestly I'm a bit lost in all these flag :/
>
> Thanks, no problem, me too :9
>
> Unfortunately and confusingly, there are many combination exist around here.
My point exactly ;)
> But because of your feedback, I noticed one missing pattern. Thanks
>
>
> Thank you for your help !!
Thanks a lot for all those reworks !
>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto
--
Jerome
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-12 9:33 ` Jerome Brunet
@ 2024-05-13 0:11 ` Kuninori Morimoto
2024-05-13 7:36 ` Jerome Brunet
0 siblings, 1 reply; 14+ messages in thread
From: Kuninori Morimoto @ 2024-05-13 0:11 UTC (permalink / raw
To: Jerome Brunet
Cc: Amadeusz Sławiński, Alexandre Belloni, Alper Nebi Yasak,
AngeloGioacchino Del Regno, Banajit Goswami, Bard Liao, Brent Lu,
Cezary Rojewski, Charles Keepax, Claudiu Beznea,
Cristian Ciocaltea, Daniel Baluta, Hans de Goede, Jaroslav Kysela,
Jiawei Wang, Jonathan Corbet, Kai Vehmanen, Kevin Hilman,
Liam Girdwood, Mark Brown, Maso Huang, Matthias Brugger,
Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
Hi Jerome
Thank you for your feedback and analysis !
> - 1st problem: I see that following your removal of
> snd_soc_dai_link_set_capabilities(), the dpcm_playback/capture flags
> are no longer properly initialised in the amlogic card drivers.
> That will need fixing.
(snip)
> This codec is not meant to have capture channels.
> I think DT description and the card driver settings (before the removal of
> snd_soc_dai_link_set_capabilities()) are correct.
OK, I see. Thank you for your analysis.
The problem was my patch checks CPU direction vs Codec direction only,
thus, it will indicates unexpected warnings, like this case.
Thank you for finding it, I hope v2 patch should be OK for you.
> IMO, those check become too restrictive. We are adding a lot of code I'm
> not sure I understand what we stand by going so far in the
> consistency checks.
>
> Initially those dpcm_playback/capture flag could be used to just
> forcefully disable a link direction, regardless of the CPUs or codecs present
> on the link. It was just another setting and it was not required to be consistent
> with anything. It just declared whether the direction was allowed on the
> link, or not.
>
> It changed this commit, and the flags suddenly needed to be consistent
> with whatever was on link:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc?h=v6.9-rc7&id=b73287f0b074
>
> I complained back then and I still don't think this change was good.
>
> If the flags needs to consistent with what is on the link, and we have
> the ability to check it, why let card drivers set it and then complain
> about it in ASoC checks ? Why not deal with it in the framework directly ?
>
> I think the simplest solution would to just go back to the initial
> meaning of these flags which was just the card driver declaring the
> direction allowed/disallowed on a link. Then ASoC can mix that
> information with whatever it finds with DAI drivers and figure out what
> is actually possible.
>
> It would give a clear and separate semantic meaning to those flags
> instead something redundant with the DAI driver info.
>
> What we have currently is not straight forward to set and check.
> It makes the code unnecessarily complicated and difficult to maintain. I
> think the difficulties with this patchset are a good illustration of
> that, unfortunately.
By this patch-set, it will be handled automatically via CPU and Codec
driver settings, Card driver will no longer need to consider about
direction (like non-DPCM), and I hope people (including you) will be
happy about that.
Thank you for your help !!
Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-13 0:11 ` Kuninori Morimoto
@ 2024-05-13 7:36 ` Jerome Brunet
2024-05-13 23:42 ` Kuninori Morimoto
0 siblings, 1 reply; 14+ messages in thread
From: Jerome Brunet @ 2024-05-13 7:36 UTC (permalink / raw
To: Kuninori Morimoto
Cc: Jerome Brunet, Amadeusz Sławiński, Alexandre Belloni,
Alper Nebi Yasak, AngeloGioacchino Del Regno, Banajit Goswami,
Bard Liao, Brent Lu, Cezary Rojewski, Charles Keepax,
Claudiu Beznea, Cristian Ciocaltea, Daniel Baluta, Hans de Goede,
Jaroslav Kysela, Jiawei Wang, Jonathan Corbet, Kai Vehmanen,
Kevin Hilman, Liam Girdwood, Mark Brown, Maso Huang,
Matthias Brugger, Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
On Mon 13 May 2024 at 00:11, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:
> Hi Jerome
>
> Thank you for your feedback and analysis !
>
>> - 1st problem: I see that following your removal of
>> snd_soc_dai_link_set_capabilities(), the dpcm_playback/capture flags
>> are no longer properly initialised in the amlogic card drivers.
>> That will need fixing.
> (snip)
>> This codec is not meant to have capture channels.
>> I think DT description and the card driver settings (before the removal of
>> snd_soc_dai_link_set_capabilities()) are correct.
>
> OK, I see. Thank you for your analysis.
>
> The problem was my patch checks CPU direction vs Codec direction only,
> thus, it will indicates unexpected warnings, like this case.
>
> Thank you for finding it, I hope v2 patch should be OK for you.
>
I'll check
>> IMO, those check become too restrictive. We are adding a lot of code I'm
>> not sure I understand what we stand by going so far in the
>> consistency checks.
>>
>> Initially those dpcm_playback/capture flag could be used to just
>> forcefully disable a link direction, regardless of the CPUs or codecs present
>> on the link. It was just another setting and it was not required to be consistent
>> with anything. It just declared whether the direction was allowed on the
>> link, or not.
>>
>> It changed this commit, and the flags suddenly needed to be consistent
>> with whatever was on link:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc?h=v6.9-rc7&id=b73287f0b074
>>
>> I complained back then and I still don't think this change was good.
>>
>> If the flags needs to consistent with what is on the link, and we have
>> the ability to check it, why let card drivers set it and then complain
>> about it in ASoC checks ? Why not deal with it in the framework directly ?
>>
>> I think the simplest solution would to just go back to the initial
>> meaning of these flags which was just the card driver declaring the
>> direction allowed/disallowed on a link. Then ASoC can mix that
>> information with whatever it finds with DAI drivers and figure out what
>> is actually possible.
>>
>> It would give a clear and separate semantic meaning to those flags
>> instead something redundant with the DAI driver info.
>>
>> What we have currently is not straight forward to set and check.
>> It makes the code unnecessarily complicated and difficult to maintain. I
>> think the difficulties with this patchset are a good illustration of
>> that, unfortunately.
>
> By this patch-set, it will be handled automatically via CPU and Codec
> driver settings, Card driver will no longer need to consider about
> direction (like non-DPCM), and I hope people (including you) will be
> happy about that.
>
If it makes things simpler, I am happy for sure :)
However, I'm a bit confused. If it is handled automatically by the CPUs
and Codecs settings, does it mean dpcm_playback/capture flags are
no-ops from now on ?
Should I update my card drivers to ditch those flags completely ?
May I still disable a direction on a link from the card driver, like in
the case I described above, when a TDM link has no slots for a direction ?
> Thank you for your help !!
>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto
--
Jerome
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-13 7:36 ` Jerome Brunet
@ 2024-05-13 23:42 ` Kuninori Morimoto
2024-05-14 9:04 ` Jerome Brunet
0 siblings, 1 reply; 14+ messages in thread
From: Kuninori Morimoto @ 2024-05-13 23:42 UTC (permalink / raw
To: Jerome Brunet
Cc: Amadeusz Sławiński, Alexandre Belloni, Alper Nebi Yasak,
AngeloGioacchino Del Regno, Banajit Goswami, Bard Liao, Brent Lu,
Cezary Rojewski, Charles Keepax, Claudiu Beznea,
Cristian Ciocaltea, Daniel Baluta, Hans de Goede, Jaroslav Kysela,
Jiawei Wang, Jonathan Corbet, Kai Vehmanen, Kevin Hilman,
Liam Girdwood, Mark Brown, Maso Huang, Matthias Brugger,
Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
Hi Jerome
Thank you for your reply
> However, I'm a bit confused. If it is handled automatically by the CPUs
> and Codecs settings, does it mean dpcm_playback/capture flags are
> no-ops from now on ?
Yes.
dpcm_playback/capture flag itself will be exist for a while, but it will be
removed soon (v6.11 ? v6.12 ? not yet fixed).
Some driver might is using dpcm_xxx flag as limitation of direction. For
example HW can use both playback/capture, but driver want to use playback
only, in this case, driver might have dpcm_playback flag only.
In this case, driver authoer need to update to use playback_only flag
instead. [1/3] patch will indicate warning about it, for a while.
> Should I update my card drivers to ditch those flags completely ?
If the driver is using dpcm_xxx flag as limitation of direction,
driver author need to update to use xxx_only flag.
If the driver have no such flag miss, I will remove all dpcm_xxx flags
when end of its support.
Of course we can avoid extra problem if each driver author remove/update
it by themself, instead of me ;P
> May I still disable a direction on a link from the card driver, like in
> the case I described above, when a TDM link has no slots for a direction ?
For example, in case of CPU can handle both playback/capture, and Codec
handles playback only, it will be playback only automatically, no Card
settings is needed.
If both CPU/Codec can handle playback/capture, but you want to enable
playback only, you can use playback_only flag.
Is it help you ?
Thank you for your help !!
Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] ASoC: grace time for DPCM cleanup
2024-05-13 23:42 ` Kuninori Morimoto
@ 2024-05-14 9:04 ` Jerome Brunet
0 siblings, 0 replies; 14+ messages in thread
From: Jerome Brunet @ 2024-05-14 9:04 UTC (permalink / raw
To: Kuninori Morimoto
Cc: Jerome Brunet, Amadeusz Sławiński, Alexandre Belloni,
Alper Nebi Yasak, AngeloGioacchino Del Regno, Banajit Goswami,
Bard Liao, Brent Lu, Cezary Rojewski, Charles Keepax,
Claudiu Beznea, Cristian Ciocaltea, Daniel Baluta, Hans de Goede,
Jaroslav Kysela, Jiawei Wang, Jonathan Corbet, Kai Vehmanen,
Kevin Hilman, Liam Girdwood, Mark Brown, Maso Huang,
Matthias Brugger, Neil Armstrong, Nicolas Ferre, Peter Ujfalusi,
Pierre-Louis Bossart, Ranjani Sridharan, Sascha Hauer, Shawn Guo,
Shengjiu Wang, Srinivas Kandagatla, Sylwester Nawrocki,
Takashi Iwai, Vinod Koul, Xiubo Li, alsa-devel, imx, linux-doc,
linux-sound
On Mon 13 May 2024 at 23:42, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:
> Hi Jerome
>
> Thank you for your reply
>
>> However, I'm a bit confused. If it is handled automatically by the CPUs
>> and Codecs settings, does it mean dpcm_playback/capture flags are
>> no-ops from now on ?
>
> Yes.
> dpcm_playback/capture flag itself will be exist for a while, but it will be
> removed soon (v6.11 ? v6.12 ? not yet fixed).
>
> Some driver might is using dpcm_xxx flag as limitation of direction. For
> example HW can use both playback/capture, but driver want to use playback
> only, in this case, driver might have dpcm_playback flag only.
>
> In this case, driver authoer need to update to use playback_only flag
> instead. [1/3] patch will indicate warning about it, for a while.
>
>
>> Should I update my card drivers to ditch those flags completely ?
>
> If the driver is using dpcm_xxx flag as limitation of direction,
> driver author need to update to use xxx_only flag.
> If the driver have no such flag miss, I will remove all dpcm_xxx flags
> when end of its support.
> Of course we can avoid extra problem if each driver author remove/update
> it by themself, instead of me ;P
Makes sense. I'll check your v2 and prepare the update of the related
card driver
>
>> May I still disable a direction on a link from the card driver, like in
>> the case I described above, when a TDM link has no slots for a direction ?
>
> For example, in case of CPU can handle both playback/capture, and Codec
> handles playback only, it will be playback only automatically, no Card
> settings is needed.
>
> If both CPU/Codec can handle playback/capture, but you want to enable
> playback only, you can use playback_only flag.
>
> Is it help you ?
>
Path forward is clear. Thanks a lot.
>
>
> Thank you for your help !!
>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto
--
Jerome
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-05-14 9:06 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-07 4:32 [PATCH 0/3] ASoC: grace time for DPCM cleanup Kuninori Morimoto
2024-05-07 4:33 ` [PATCH 1/3] ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition Kuninori Morimoto
2024-05-07 4:33 ` [PATCH 2/3] ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch Kuninori Morimoto
2024-05-07 4:33 ` [PATCH 3/3] ASoC: remove snd_soc_dai_link_set_capabilities() Kuninori Morimoto
2024-05-07 8:47 ` [PATCH 0/3] ASoC: grace time for DPCM cleanup Jerome Brunet
2024-05-08 0:06 ` Kuninori Morimoto
2024-05-09 5:50 ` Kuninori Morimoto
2024-05-09 8:51 ` Jerome Brunet
2024-05-09 23:42 ` Kuninori Morimoto
2024-05-12 9:33 ` Jerome Brunet
2024-05-13 0:11 ` Kuninori Morimoto
2024-05-13 7:36 ` Jerome Brunet
2024-05-13 23:42 ` Kuninori Morimoto
2024-05-14 9:04 ` Jerome Brunet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).