[PATCH v8 0/4] ASoC: grace time for DPCM cleanup
Hi Mark, Pierre-Louis, Jerome Cc each ASoC driver maintainer
This is v8 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.
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.12 or v6.13 ? Please check each driver by that time.
Previous patch-set try to check both CPU and Codec in DPCM, but we noticed that there are some special DAI which we can't handle today [2]. So I will escape it in this patch-set.
[1] https://lore.kernel.org/r/87edaym2cg.wl-kuninori.morimoto.gx@renesas.com [2] https://lore.kernel.org/all/3e67d62d-fe08-4f55-ab5b-ece8a57154f9@linux.intel...
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 Link: https://lore.kernel.org/r/87msole5wc.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/871q5tnuok.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87bk4oqerx.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/8734pctmte.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87r0ctwzr4.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87cymvlmki.wl-kuninori.morimoto.gx@renesas.com
v8 -> v7 - rebase to latest ASoC branch (for-6.12)
v6 -> v7 - based on latest ASoC branch (for-6.12)
v5 -> v6 - based on latest ASoC branch
v4 -> v5 - typo fix: limition -> limitation - typo fix: catpure -> capture - include Intel patch
v3 -> v4 - don't check Codec on DPCM - include Jerome's dpcm_xxx update patch
v2 -> v3 - tidyup typo (reuqsts -> requests) - add Tested-by on git-log
v1 -> v2 - tidyup Codec check warning output condition
Jerome Brunet (1): ASoC: amlogic: do not use dpcm_playback/capture flags
Kuninori Morimoto (3): ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition ASoC: remove snd_soc_dai_link_set_capabilities() ASoC: Intel: sof_sdw: use playback/capture_only flags
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 | 11 +++-- sound/soc/meson/gx-card.c | 1 - sound/soc/meson/meson-card-utils.c | 4 +- sound/soc/qcom/common.c | 1 - sound/soc/sdw_utils/soc_sdw_utils.c | 4 +- sound/soc/soc-dai.c | 38 ---------------- sound/soc/soc-pcm.c | 65 +++++++++++++++------------ 13 files changed, 47 insertions(+), 88 deletions(-)
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 e844f6afc5b5..81ef380b2e06 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 f79abcf36105..f4a012e633b3 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2745,6 +2745,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; @@ -2755,43 +2756,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 limitation + */ + 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 */
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/capture 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 ab4e109fe71d..0d1b215f24f4 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(const 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 0e18ccabe28c..98b37dd2b901 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 3425fbbcbd7e..1bdcfc4d4222 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -279,8 +279,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 56f7f946882e..051adb567397 100644 --- a/sound/soc/generic/audio-graph-card2.c +++ b/sound/soc/generic/audio-graph-card2.c @@ -966,8 +966,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, lnk, cpu_port, codec_port, 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 d2588f1ea54e..42c60b92cca5 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -291,8 +291,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, np, 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 56b4a3654aec..928cf5cb5999 100644 --- a/sound/soc/qcom/common.c +++ b/sound/soc/qcom/common.c @@ -155,7 +155,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 302ca753a1f3..4e08892d24c6 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -479,44 +479,6 @@ bool snd_soc_dai_stream_valid(const 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) {
From: Jerome Brunet jbrunet@baylibre.com
dpcm_playback/capture flags are being deprecated in ASoC. Use playback/capture_only flags instead
Suggested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Jerome Brunet jbrunet@baylibre.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/meson/axg-card.c | 10 +++++----- sound/soc/meson/meson-card-utils.c | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/meson/axg-card.c b/sound/soc/meson/axg-card.c index 09aa36e94c85..646ab87afac2 100644 --- a/sound/soc/meson/axg-card.c +++ b/sound/soc/meson/axg-card.c @@ -132,7 +132,7 @@ static int axg_card_add_tdm_loopback(struct snd_soc_card *card, lb->stream_name = lb->name; lb->cpus->of_node = pad->cpus->of_node; lb->cpus->dai_name = "TDM Loopback"; - lb->dpcm_capture = 1; + lb->capture_only = 1; lb->no_pcm = 1; lb->ops = &axg_card_tdm_be_ops; lb->init = axg_card_tdm_dai_lb_init; @@ -176,7 +176,7 @@ static int axg_card_parse_cpu_tdm_slots(struct snd_soc_card *card,
/* Disable playback is the interface has no tx slots */ if (!tx) - link->dpcm_playback = 0; + link->capture_only = 1;
for (i = 0, rx = 0; i < AXG_TDM_NUM_LANES; i++) { snprintf(propname, 32, "dai-tdm-slot-rx-mask-%d", i); @@ -186,9 +186,9 @@ static int axg_card_parse_cpu_tdm_slots(struct snd_soc_card *card,
/* Disable capture is the interface has no rx slots */ if (!rx) - link->dpcm_capture = 0; + link->playback_only = 1;
- /* ... but the interface should at least have one of them */ + /* ... but the interface should at least have one direction */ if (!tx && !rx) { dev_err(card->dev, "tdm link has no cpu slots\n"); return -EINVAL; @@ -275,7 +275,7 @@ static int axg_card_parse_tdm(struct snd_soc_card *card, return ret;
/* Add loopback if the pad dai has playback */ - if (link->dpcm_playback) { + if (!link->capture_only) { ret = axg_card_add_tdm_loopback(card, index); if (ret) return ret; diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c index ed6c7e2f609c..1a4ef124e4e2 100644 --- a/sound/soc/meson/meson-card-utils.c +++ b/sound/soc/meson/meson-card-utils.c @@ -186,9 +186,9 @@ int meson_card_set_fe_link(struct snd_soc_card *card, link->dpcm_merged_rate = 1;
if (is_playback) - link->dpcm_playback = 1; + link->playback_only = 1; else - link->dpcm_capture = 1; + link->capture_only = 1;
return meson_card_set_link_name(card, link, node, "fe"); }
Prepare for removal of dpcm_playback and dpcm_capture flags in dailinks.
[Kuninori adjusted Pierre-Louis's patch]
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sdw_utils/soc_sdw_utils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sdw_utils/soc_sdw_utils.c b/sound/soc/sdw_utils/soc_sdw_utils.c index 6183629d1754..e8d0f199155d 100644 --- a/sound/soc/sdw_utils/soc_sdw_utils.c +++ b/sound/soc/sdw_utils/soc_sdw_utils.c @@ -950,8 +950,8 @@ void asoc_sdw_init_dai_link(struct device *dev, struct snd_soc_dai_link *dai_lin dai_links->num_cpus = cpus_num; dai_links->codecs = codecs; dai_links->num_codecs = codecs_num; - dai_links->dpcm_playback = playback; - dai_links->dpcm_capture = capture; + dai_links->playback_only = playback && !capture; + dai_links->capture_only = !playback && capture; dai_links->init = init; dai_links->ops = ops; }
On Wed, 21 Aug 2024 02:13:29 +0000, Kuninori Morimoto wrote:
Cc each ASoC driver maintainer
This is v8 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.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/4] ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition commit: fd69dfe6789f4ed46d1fdb52e223cff83946d997 [2/4] ASoC: remove snd_soc_dai_link_set_capabilities() commit: 12806510481497a01d01edd64d7bb53a4d9ec28d [3/4] ASoC: amlogic: do not use dpcm_playback/capture flags commit: 46fb727a28d8c7195f915150a669d927d463069b [4/4] ASoC: Intel: sof_sdw: use playback/capture_only flags commit: 61e1f74f739546415570ccc1ac14e1b26afe4705
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Kuninori Morimoto
-
Mark Brown