[PATCH 0/4] ASoC: Fix dailink checks for DPCM
We've had a couple of changes that introduce regressions with the multi-cpu DAI solutions, and while trying to fix them we found additional inconsistencies that should also go to stable branches.
Bard Liao (1): ASoC: core: only convert non DPCM link to DPCM link
Pierre-Louis Bossart (3): ASoC: soc-pcm: dpcm: fix playback/capture checks ASoC: Intel: boards: replace capture_only by dpcm_capture ASoC: SOF: nocodec: conditionally set dpcm_capture/dpcm_playback flags
sound/soc/intel/boards/glk_rt5682_max98357a.c | 2 +- sound/soc/intel/boards/kbl_da7219_max98927.c | 4 +- sound/soc/intel/boards/kbl_rt5663_max98927.c | 2 +- .../intel/boards/kbl_rt5663_rt5514_max98927.c | 2 +- sound/soc/soc-core.c | 22 ++++++++-- sound/soc/soc-pcm.c | 44 ++++++++++++++----- sound/soc/sof/nocodec.c | 6 ++- 7 files changed, 62 insertions(+), 20 deletions(-)
base-commit: 8a9144c1cf523221b37dd3393827253c91fcbf54
Recent changes in the ASoC core prevent multi-cpu BE dailinks from being used. DPCM does support multi-cpu DAIs for BE Dailinks, but not for FE.
Handle the FE checks first, and make sure all DAIs support the same capabilities within the same dailink.
BugLink: https://github.com/thesofproject/linux/issues/2031 Fixes: 9b5db059366ae2 ("ASoC: soc-pcm: dpcm: Only allow playback/capture if supported") Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Daniel Baluta daniel.baluta@gmail.com --- sound/soc/soc-pcm.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 276505fb9d50..2c114b4542ce 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2789,20 +2789,44 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) struct snd_pcm *pcm; char new_name[64]; int ret = 0, playback = 0, capture = 0; + int stream; int i;
+ if (rtd->dai_link->dynamic && rtd->num_cpus > 1) { + dev_err(rtd->dev, + "DPCM doesn't support Multi CPU for Front-Ends yet\n"); + return -EINVAL; + } + if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) { - cpu_dai = asoc_rtd_to_cpu(rtd, 0); - if (rtd->num_cpus > 1) { - dev_err(rtd->dev, - "DPCM doesn't support Multi CPU yet\n"); - return -EINVAL; + if (rtd->dai_link->dpcm_playback) { + stream = SNDRV_PCM_STREAM_PLAYBACK; + + for_each_rtd_cpu_dais(rtd, i, cpu_dai) + if (!snd_soc_dai_stream_valid(cpu_dai, + stream)) { + dev_err(rtd->card->dev, + "CPU DAI %s for rtd %s does not support playback\n", + cpu_dai->name, + rtd->dai_link->stream_name); + return -EINVAL; + } + playback = 1; + } + if (rtd->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)) { + dev_err(rtd->card->dev, + "CPU DAI %s for rtd %s does not support capture\n", + cpu_dai->name, + rtd->dai_link->stream_name); + return -EINVAL; + } + capture = 1; } - - playback = rtd->dai_link->dpcm_playback && - snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK); - capture = rtd->dai_link->dpcm_capture && - snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE); } else { /* Adapt stream for codec2codec links */ int cpu_capture = rtd->dai_link->params ?
From: Bard Liao yung-chuan.liao@linux.intel.com
Additional checks for valid DAIs expose a corner case, where existing BE dailinks get modified, e.g. HDMI links are tagged with dpcm_capture=1 even if the DAIs are for playback.
This patch makes those changes conditional and flags configuration issues when a BE dailink is has no_pcm=0 but dpcm_playback or dpcm_capture=1 (which makes no sense).
As discussed on the alsa-devel mailing list, there are redundant flags for dpcm_playback, dpcm_capture, playback_only, capture_only. This will have to be cleaned-up in a future update. For now only correct and flag problematic configurations.
Fixes: 218fe9b7ec7f3 ("ASoC: soc-core: Set dpcm_playback / dpcm_capture") Suggested-by: Daniel Baluta daniel.baluta@nxp.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Daniel Baluta daniel.baluta@gmail.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/soc-core.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index b07eca2c6ccc..7b387202c5db 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1648,9 +1648,25 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) dai_link->platforms->name = component->name;
/* convert non BE into BE */ - dai_link->no_pcm = 1; - dai_link->dpcm_playback = 1; - dai_link->dpcm_capture = 1; + if (!dai_link->no_pcm) { + dai_link->no_pcm = 1; + + if (dai_link->dpcm_playback) + dev_warn(card->dev, + "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_playback=1\n", + dai_link->name); + if (dai_link->dpcm_capture) + dev_warn(card->dev, + "invalid configuration, dailink %s has flags no_pcm=0 and dpcm_capture=1\n", + dai_link->name); + + /* convert normal link into DPCM one */ + if (!(dai_link->dpcm_playback || + dai_link->dpcm_capture)) { + dai_link->dpcm_playback = !dai_link->capture_only; + dai_link->dpcm_capture = !dai_link->playback_only; + } + }
/* * override any BE fixups
It's not clear why specific FE dailinks use capture_only flags, likely blind copy/paste from Chromebook driver to the other. Replace by dpcm_capture, this will make future alignment and removal of flags easier.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Daniel Baluta daniel.baluta@gmail.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/intel/boards/glk_rt5682_max98357a.c | 2 +- sound/soc/intel/boards/kbl_da7219_max98927.c | 4 ++-- sound/soc/intel/boards/kbl_rt5663_max98927.c | 2 +- sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/sound/soc/intel/boards/glk_rt5682_max98357a.c b/sound/soc/intel/boards/glk_rt5682_max98357a.c index 48eda1a8aa6c..954ab01f695b 100644 --- a/sound/soc/intel/boards/glk_rt5682_max98357a.c +++ b/sound/soc/intel/boards/glk_rt5682_max98357a.c @@ -407,7 +407,7 @@ static struct snd_soc_dai_link geminilake_dais[] = { .name = "Glk Audio Echo Reference cap", .stream_name = "Echoreference Capture", .init = NULL, - .capture_only = 1, + .dpcm_capture = 1, .nonatomic = 1, .dynamic = 1, SND_SOC_DAILINK_REG(echoref, dummy, platform), diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c index cc9b5eab8b4a..e29c31ffd241 100644 --- a/sound/soc/intel/boards/kbl_da7219_max98927.c +++ b/sound/soc/intel/boards/kbl_da7219_max98927.c @@ -692,7 +692,7 @@ static struct snd_soc_dai_link kabylake_dais[] = { .name = "Kbl Audio Echo Reference cap", .stream_name = "Echoreference Capture", .init = NULL, - .capture_only = 1, + .dpcm_capture = 1, .nonatomic = 1, SND_SOC_DAILINK_REG(echoref, dummy, platform), }, @@ -858,7 +858,7 @@ static struct snd_soc_dai_link kabylake_max98_927_373_dais[] = { .name = "Kbl Audio Echo Reference cap", .stream_name = "Echoreference Capture", .init = NULL, - .capture_only = 1, + .dpcm_capture = 1, .nonatomic = 1, SND_SOC_DAILINK_REG(echoref, dummy, platform), }, diff --git a/sound/soc/intel/boards/kbl_rt5663_max98927.c b/sound/soc/intel/boards/kbl_rt5663_max98927.c index 658a9da3a40f..09ba55fc36d5 100644 --- a/sound/soc/intel/boards/kbl_rt5663_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_max98927.c @@ -672,7 +672,7 @@ static struct snd_soc_dai_link kabylake_dais[] = { .name = "Kbl Audio Echo Reference cap", .stream_name = "Echoreference Capture", .init = NULL, - .capture_only = 1, + .dpcm_capture = 1, .nonatomic = 1, SND_SOC_DAILINK_REG(echoref, dummy, platform), }, diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index 1b1f8d7a4ea3..b34cf6cf1139 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -566,7 +566,7 @@ static struct snd_soc_dai_link kabylake_dais[] = { .name = "Kbl Audio Echo Reference cap", .stream_name = "Echoreference Capture", .init = NULL, - .capture_only = 1, + .dpcm_capture = 1, .nonatomic = 1, SND_SOC_DAILINK_REG(echoref, dummy, platform), },
With additional checks on dailinks, we see errors such as
[ 3.000418] sof-nocodec sof-nocodec: CPU DAI DMIC01 Pin for rtd NoCodec-6 does not support playback
It's not clear why we set the dpcm_playback and dpcm_capture flags unconditionally, add a check on number of channels for each direction to avoid invalid configurations.
Fixes: 8017b8fd37bf5e ('ASoC: SOF: Add Nocodec machine driver support') Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Daniel Baluta daniel.baluta@gmail.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com --- sound/soc/sof/nocodec.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/nocodec.c b/sound/soc/sof/nocodec.c index ce053ba8f2e8..d03b5be31255 100644 --- a/sound/soc/sof/nocodec.c +++ b/sound/soc/sof/nocodec.c @@ -52,8 +52,10 @@ static int sof_nocodec_bes_setup(struct device *dev, links[i].platforms->name = dev_name(dev); links[i].codecs->dai_name = "snd-soc-dummy-dai"; links[i].codecs->name = "snd-soc-dummy"; - links[i].dpcm_playback = 1; - links[i].dpcm_capture = 1; + if (ops->drv[i].playback.channels_min) + links[i].dpcm_playback = 1; + if (ops->drv[i].capture.channels_min) + links[i].dpcm_capture = 1; }
card->dai_link = links;
On Mon, 8 Jun 2020 14:44:11 -0500, Pierre-Louis Bossart wrote:
We've had a couple of changes that introduce regressions with the multi-cpu DAI solutions, and while trying to fix them we found additional inconsistencies that should also go to stable branches.
Bard Liao (1): ASoC: core: only convert non DPCM link to DPCM link
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks commit: b73287f0b0745961b14e5ebcce92cc8ed24d4d52 [2/4] ASoC: core: only convert non DPCM link to DPCM link commit: 607fa205a7e4dfad28b8a67ab1c985756ddbccb0 [3/4] ASoC: Intel: boards: replace capture_only by dpcm_capture commit: dc261875865539ca91bff9bc44d3e62f811dec1d [4/4] ASoC: SOF: nocodec: conditionally set dpcm_capture/dpcm_playback flags commit: ba4e5abc6c4e173af7c941c03c067263b686665d
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
Hi Pierre,
On Mon, Jun 08, 2020 at 02:44:12PM -0500, Pierre-Louis Bossart wrote:
Recent changes in the ASoC core prevent multi-cpu BE dailinks from being used. DPCM does support multi-cpu DAIs for BE Dailinks, but not for FE.
First I want to apologize for introducing this regression. Actually when I made the "Only allow playback/capture if supported" patch I did not realize it would also be used for BE DAIs. :)
Handle the FE checks first, and make sure all DAIs support the same capabilities within the same dailink.
BugLink: https://github.com/thesofproject/linux/issues/2031 Fixes: 9b5db059366ae2 ("ASoC: soc-pcm: dpcm: Only allow playback/capture if supported") Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Daniel Baluta daniel.baluta@gmail.com
sound/soc/soc-pcm.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 276505fb9d50..2c114b4542ce 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2789,20 +2789,44 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) struct snd_pcm *pcm; char new_name[64]; int ret = 0, playback = 0, capture = 0;
int stream; int i;
if (rtd->dai_link->dynamic && rtd->num_cpus > 1) {
dev_err(rtd->dev,"DPCM doesn't support Multi CPU for Front-Ends yet\n");return -EINVAL;}
if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
cpu_dai = asoc_rtd_to_cpu(rtd, 0);if (rtd->num_cpus > 1) {dev_err(rtd->dev,"DPCM doesn't support Multi CPU yet\n");return -EINVAL;
if (rtd->dai_link->dpcm_playback) {stream = SNDRV_PCM_STREAM_PLAYBACK;for_each_rtd_cpu_dais(rtd, i, cpu_dai)if (!snd_soc_dai_stream_valid(cpu_dai,stream)) {dev_err(rtd->card->dev,"CPU DAI %s for rtd %s does not support playback\n",cpu_dai->name,rtd->dai_link->stream_name);return -EINVAL;
Unfortunately the "return -EINVAL" here and below break the case where dpcm_playback/dpcm_capture does not exactly match the capabilities of the referenced CPU DAIs. To quote from my commit message:
At the moment, PCM devices for DPCM are only created based on the dpcm_playback/capture parameters of the DAI link, without considering if the CPU/FE DAI is actually capable of playback/capture.
Normally the dpcm_playback/capture parameter should match the capabilities of the CPU DAI. However, there is no way to set that parameter from the device tree (e.g. with simple-audio-card or qcom sound cards). dpcm_playback/capture are always both set to 1.
The basic idea for my commit was to basically stop using dpcm_playback/capture for the device tree case and infer the capabilities solely based on referenced DAIs. The DAIs expose if they are capable of playback/capture, so I see no reason to be required to duplicate that into the DAI link setup (unless you want to specifically restrict a DAI link to one direction for some reason...)
With your patch probe now fails with:
7702000.sound: CPU DAI MultiMedia1 for rtd MultiMedia1 does not support capture
because sound/soc/qcom/common.c sets dpcm_playback = dpcm_capture = 1 even though that FE DAI is currently configured to be playback-only.
I believe Srinivas fixed that problem for the BE DAIs in commit a2120089251f ("ASoC: qcom: common: set correct directions for dailinks") (https://lore.kernel.org/alsa-devel/20200612123711.29130-2-srinivas.kandagatl...)
For the QCOM case it may be feasible to set dpcm_playback/dpcm_capture appropriately because it is basically only used with one particular DAI driver. But simple-audio-card is generic and used with many different drivers so hard-coding a call into some other driver like Srinivas did above won't work in that case.
I wonder if we should downgrade your dev_err(...) to a dev_dbg(...), and then simply avoid setting playback/capture = 0. This should fix the case I'm talking about.
What do you think?
Thanks, Stephan
}playback = 1;}if (rtd->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)) {dev_err(rtd->card->dev,"CPU DAI %s for rtd %s does not support capture\n",cpu_dai->name,rtd->dai_link->stream_name);return -EINVAL;}capture = 1;
playback = rtd->dai_link->dpcm_playback &&snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK);capture = rtd->dai_link->dpcm_capture &&snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE);-- 2.20.1
On Tue, Jun 16, 2020 at 10:54:17AM +0200, Stephan Gerhold wrote:
Hi Pierre,
On Mon, Jun 08, 2020 at 02:44:12PM -0500, Pierre-Louis Bossart wrote:
Recent changes in the ASoC core prevent multi-cpu BE dailinks from being used. DPCM does support multi-cpu DAIs for BE Dailinks, but not for FE.
First I want to apologize for introducing this regression. Actually when I made the "Only allow playback/capture if supported" patch I did not realize it would also be used for BE DAIs. :)
Handle the FE checks first, and make sure all DAIs support the same capabilities within the same dailink.
BugLink: https://github.com/thesofproject/linux/issues/2031 Fixes: 9b5db059366ae2 ("ASoC: soc-pcm: dpcm: Only allow playback/capture if supported") Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Daniel Baluta daniel.baluta@gmail.com
sound/soc/soc-pcm.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 276505fb9d50..2c114b4542ce 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2789,20 +2789,44 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) struct snd_pcm *pcm; char new_name[64]; int ret = 0, playback = 0, capture = 0;
int stream; int i;
if (rtd->dai_link->dynamic && rtd->num_cpus > 1) {
dev_err(rtd->dev,"DPCM doesn't support Multi CPU for Front-Ends yet\n");return -EINVAL;}
if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
cpu_dai = asoc_rtd_to_cpu(rtd, 0);if (rtd->num_cpus > 1) {dev_err(rtd->dev,"DPCM doesn't support Multi CPU yet\n");return -EINVAL;
if (rtd->dai_link->dpcm_playback) {stream = SNDRV_PCM_STREAM_PLAYBACK;for_each_rtd_cpu_dais(rtd, i, cpu_dai)if (!snd_soc_dai_stream_valid(cpu_dai,stream)) {dev_err(rtd->card->dev,"CPU DAI %s for rtd %s does not support playback\n",cpu_dai->name,rtd->dai_link->stream_name);return -EINVAL;Unfortunately the "return -EINVAL" here and below break the case where dpcm_playback/dpcm_capture does not exactly match the capabilities of the referenced CPU DAIs. To quote from my commit message:
At the moment, PCM devices for DPCM are only created based on the dpcm_playback/capture parameters of the DAI link, without considering if the CPU/FE DAI is actually capable of playback/capture. Normally the dpcm_playback/capture parameter should match the capabilities of the CPU DAI. However, there is no way to set that parameter from the device tree (e.g. with simple-audio-card or qcom sound cards). dpcm_playback/capture are always both set to 1.The basic idea for my commit was to basically stop using dpcm_playback/capture for the device tree case and infer the capabilities solely based on referenced DAIs. The DAIs expose if they are capable of playback/capture, so I see no reason to be required to duplicate that into the DAI link setup (unless you want to specifically restrict a DAI link to one direction for some reason...)
With your patch probe now fails with:
7702000.sound: CPU DAI MultiMedia1 for rtd MultiMedia1 does not support capture
because sound/soc/qcom/common.c sets dpcm_playback = dpcm_capture = 1 even though that FE DAI is currently configured to be playback-only.
I believe Srinivas fixed that problem for the BE DAIs in commit a2120089251f ("ASoC: qcom: common: set correct directions for dailinks") (https://lore.kernel.org/alsa-devel/20200612123711.29130-2-srinivas.kandagatl...)
For the QCOM case it may be feasible to set dpcm_playback/dpcm_capture appropriately because it is basically only used with one particular DAI driver. But simple-audio-card is generic and used with many different drivers so hard-coding a call into some other driver like Srinivas did above won't work in that case.
I wonder if we should downgrade your dev_err(...) to a dev_dbg(...), and then simply avoid setting playback/capture = 0.
Hmm, I wanted to write "avoid setting playback/capture = 1" here of course. If dpcm_playback/capture is set but not actually supported don't error out but just ignore it. That would essentially make dpcm_playback/capture just a restriction of the CPU DAI capabilities.
Not sure if there is even a usecase for such a restriction, maybe dpcm_playback/capture could even be removed entirely...
This should fix the case I'm talking about.
What do you think?
Thanks, Stephan
}playback = 1;}if (rtd->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)) {dev_err(rtd->card->dev,"CPU DAI %s for rtd %s does not support capture\n",cpu_dai->name,rtd->dai_link->stream_name);return -EINVAL;}capture = 1;
playback = rtd->dai_link->dpcm_playback &&snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK);capture = rtd->dai_link->dpcm_capture &&snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE);-- 2.20.1
On 6/16/20 4:02 AM, Stephan Gerhold wrote:
On Tue, Jun 16, 2020 at 10:54:17AM +0200, Stephan Gerhold wrote:
Hi Pierre,
On Mon, Jun 08, 2020 at 02:44:12PM -0500, Pierre-Louis Bossart wrote:
Recent changes in the ASoC core prevent multi-cpu BE dailinks from being used. DPCM does support multi-cpu DAIs for BE Dailinks, but not for FE.
First I want to apologize for introducing this regression. Actually when I made the "Only allow playback/capture if supported" patch I did not realize it would also be used for BE DAIs. :)
No need to apologize, it helped identify configuration issues none of us identified. That's progress for me.
Handle the FE checks first, and make sure all DAIs support the same capabilities within the same dailink.
BugLink: https://github.com/thesofproject/linux/issues/2031 Fixes: 9b5db059366ae2 ("ASoC: soc-pcm: dpcm: Only allow playback/capture if supported") Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Daniel Baluta daniel.baluta@gmail.com
sound/soc/soc-pcm.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 276505fb9d50..2c114b4542ce 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2789,20 +2789,44 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) struct snd_pcm *pcm; char new_name[64]; int ret = 0, playback = 0, capture = 0;
int stream; int i;
if (rtd->dai_link->dynamic && rtd->num_cpus > 1) {
dev_err(rtd->dev,"DPCM doesn't support Multi CPU for Front-Ends yet\n");return -EINVAL;}
if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
cpu_dai = asoc_rtd_to_cpu(rtd, 0);if (rtd->num_cpus > 1) {dev_err(rtd->dev,"DPCM doesn't support Multi CPU yet\n");return -EINVAL;
if (rtd->dai_link->dpcm_playback) {stream = SNDRV_PCM_STREAM_PLAYBACK;for_each_rtd_cpu_dais(rtd, i, cpu_dai)if (!snd_soc_dai_stream_valid(cpu_dai,stream)) {dev_err(rtd->card->dev,"CPU DAI %s for rtd %s does not support playback\n",cpu_dai->name,rtd->dai_link->stream_name);return -EINVAL;Unfortunately the "return -EINVAL" here and below break the case where dpcm_playback/dpcm_capture does not exactly match the capabilities of the referenced CPU DAIs. To quote from my commit message:
At the moment, PCM devices for DPCM are only created based on the dpcm_playback/capture parameters of the DAI link, without considering if the CPU/FE DAI is actually capable of playback/capture. Normally the dpcm_playback/capture parameter should match the capabilities of the CPU DAI. However, there is no way to set that parameter from the device tree (e.g. with simple-audio-card or qcom sound cards). dpcm_playback/capture are always both set to 1.The basic idea for my commit was to basically stop using dpcm_playback/capture for the device tree case and infer the capabilities solely based on referenced DAIs. The DAIs expose if they are capable of playback/capture, so I see no reason to be required to duplicate that into the DAI link setup (unless you want to specifically restrict a DAI link to one direction for some reason...)
With your patch probe now fails with:
7702000.sound: CPU DAI MultiMedia1 for rtd MultiMedia1 does not support capturebecause sound/soc/qcom/common.c sets dpcm_playback = dpcm_capture = 1 even though that FE DAI is currently configured to be playback-only.
I believe Srinivas fixed that problem for the BE DAIs in commit a2120089251f ("ASoC: qcom: common: set correct directions for dailinks") (https://lore.kernel.org/alsa-devel/20200612123711.29130-2-srinivas.kandagatl...)
For the QCOM case it may be feasible to set dpcm_playback/dpcm_capture appropriately because it is basically only used with one particular DAI driver. But simple-audio-card is generic and used with many different drivers so hard-coding a call into some other driver like Srinivas did above won't work in that case.
Doesn't simple-card rely on DT blobs that can also be updated?
I wonder if we should downgrade your dev_err(...) to a dev_dbg(...), and then simply avoid setting playback/capture = 0.
Hmm, I wanted to write "avoid setting playback/capture = 1" here of course. If dpcm_playback/capture is set but not actually supported don't error out but just ignore it. That would essentially make dpcm_playback/capture just a restriction of the CPU DAI capabilities.
Not sure if there is even a usecase for such a restriction, maybe dpcm_playback/capture could even be removed entirely...
I'd rather keep the error and fix those bad configurations, and in a second step unify dpcm_capture/dpcm_playback/playback_only/capture_only. We have too many flags to identify directions and when given too many choices it's likely we are going to have corner cases for a while.
On Tue, Jun 16, 2020 at 09:23:25AM -0500, Pierre-Louis Bossart wrote:
On 6/16/20 4:02 AM, Stephan Gerhold wrote:
On Tue, Jun 16, 2020 at 10:54:17AM +0200, Stephan Gerhold wrote:
For the QCOM case it may be feasible to set dpcm_playback/dpcm_capture appropriately because it is basically only used with one particular DAI driver. But simple-audio-card is generic and used with many different drivers so hard-coding a call into some other driver like Srinivas did above won't work in that case.
Doesn't simple-card rely on DT blobs that can also be updated?
DT is an ABI just like ACPI - it's just more featureful. Many systems can easily update their DTs but not all of them and users don't always want to try to keep it in lock step with the kernel. Stuff like this is why I've been dubious about putting DPCM things in there, it's too much of a hard coding of internal APIs.
On 6/16/20 9:52 AM, Mark Brown wrote:
On Tue, Jun 16, 2020 at 09:23:25AM -0500, Pierre-Louis Bossart wrote:
On 6/16/20 4:02 AM, Stephan Gerhold wrote:
On Tue, Jun 16, 2020 at 10:54:17AM +0200, Stephan Gerhold wrote:
For the QCOM case it may be feasible to set dpcm_playback/dpcm_capture appropriately because it is basically only used with one particular DAI driver. But simple-audio-card is generic and used with many different drivers so hard-coding a call into some other driver like Srinivas did above won't work in that case.
Doesn't simple-card rely on DT blobs that can also be updated?
DT is an ABI just like ACPI - it's just more featureful. Many systems can easily update their DTs but not all of them and users don't always want to try to keep it in lock step with the kernel. Stuff like this is why I've been dubious about putting DPCM things in there, it's too much of a hard coding of internal APIs.
ok, but is there any actual use of dpcm_playback/capture outside of C code?
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep, NULL, &dai_link->dai_fmt); if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1; dai_link->dpcm_capture = 1;
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
On Tue, Jun 16, 2020 at 10:05:39AM -0500, Pierre-Louis Bossart wrote:
On 6/16/20 9:52 AM, Mark Brown wrote:
On Tue, Jun 16, 2020 at 09:23:25AM -0500, Pierre-Louis Bossart wrote:
On 6/16/20 4:02 AM, Stephan Gerhold wrote:
On Tue, Jun 16, 2020 at 10:54:17AM +0200, Stephan Gerhold wrote:
For the QCOM case it may be feasible to set dpcm_playback/dpcm_capture appropriately because it is basically only used with one particular DAI driver. But simple-audio-card is generic and used with many different drivers so hard-coding a call into some other driver like Srinivas did above won't work in that case.
Doesn't simple-card rely on DT blobs that can also be updated?
DT is an ABI just like ACPI - it's just more featureful. Many systems can easily update their DTs but not all of them and users don't always want to try to keep it in lock step with the kernel. Stuff like this is why I've been dubious about putting DPCM things in there, it's too much of a hard coding of internal APIs.
ok, but is there any actual use of dpcm_playback/capture outside of C code?
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep, NULL, &dai_link->dai_fmt); if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1; dai_link->dpcm_capture = 1;
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
But how do you know which capabilities to set? The device tree doesn't tells us that. We could add some code to look up the snd_soc_dai_driver early, based on the references in the device tree (basically something like snd_soc_of_get_dai_name(), see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...)
At least to me that function doesn't exactly look trivial though, and that's just to properly fill in the dpcm_playback/capture parameters. Essentially those parameters only complicate the current device tree use case, where you want the DAI link to be for both playback/capture, but restricted to the capabilities of the DAI.
Just wondering if setting up dpcm_playback/capture properly is worth it at all in this case. This isn't necessary for the non-DPCM case either, there we automatically set it based on the DAI capabilities.
Thanks, Stephan
On 6/16/20 10:55 AM, Stephan Gerhold wrote:
On Tue, Jun 16, 2020 at 10:05:39AM -0500, Pierre-Louis Bossart wrote:
On 6/16/20 9:52 AM, Mark Brown wrote:
On Tue, Jun 16, 2020 at 09:23:25AM -0500, Pierre-Louis Bossart wrote:
On 6/16/20 4:02 AM, Stephan Gerhold wrote:
On Tue, Jun 16, 2020 at 10:54:17AM +0200, Stephan Gerhold wrote:
For the QCOM case it may be feasible to set dpcm_playback/dpcm_capture appropriately because it is basically only used with one particular DAI driver. But simple-audio-card is generic and used with many different drivers so hard-coding a call into some other driver like Srinivas did above won't work in that case.
Doesn't simple-card rely on DT blobs that can also be updated?
DT is an ABI just like ACPI - it's just more featureful. Many systems can easily update their DTs but not all of them and users don't always want to try to keep it in lock step with the kernel. Stuff like this is why I've been dubious about putting DPCM things in there, it's too much of a hard coding of internal APIs.
ok, but is there any actual use of dpcm_playback/capture outside of C code?
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep, NULL, &dai_link->dai_fmt); if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1; dai_link->dpcm_capture = 1;
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
But how do you know which capabilities to set? The device tree doesn't tells us that. We could add some code to look up the snd_soc_dai_driver early, based on the references in the device tree (basically something like snd_soc_of_get_dai_name(), see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...)
At least to me that function doesn't exactly look trivial though, and that's just to properly fill in the dpcm_playback/capture parameters. Essentially those parameters only complicate the current device tree use case, where you want the DAI link to be for both playback/capture, but restricted to the capabilities of the DAI.
Just wondering if setting up dpcm_playback/capture properly is worth it at all in this case. This isn't necessary for the non-DPCM case either, there we automatically set it based on the DAI capabilities.
We can add a simple loop for each direction that relies on snd_soc_dai_stream_valid() to identify if each DAI is capable of doing playback/capture.
On Tue, Jun 16, 2020 at 10:05:39AM -0500, Pierre-Louis Bossart wrote:
On 6/16/20 9:52 AM, Mark Brown wrote:
On Tue, Jun 16, 2020 at 09:23:25AM -0500, Pierre-Louis Bossart wrote:
Doesn't simple-card rely on DT blobs that can also be updated?
DT is an ABI just like ACPI - it's just more featureful. Many systems can easily update their DTs but not all of them and users don't always want to try to keep it in lock step with the kernel. Stuff like this is why I've been dubious about putting DPCM things in there, it's too much of a hard coding of internal APIs.
ok, but is there any actual use of dpcm_playback/capture outside of C code?
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
...
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
Right, just heading off the idea that we can fix things by updating DTs.
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep, NULL, &dai_link->dai_fmt); if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1; dai_link->dpcm_capture = 1;
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
But how do you know which capabilities to set? The device tree doesn't tells us that. We could add some code to look up the snd_soc_dai_driver early, based on the references in the device tree (basically something like snd_soc_of_get_dai_name(), see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...)
At least to me that function doesn't exactly look trivial though, and that's just to properly fill in the dpcm_playback/capture parameters. Essentially those parameters only complicate the current device tree use case, where you want the DAI link to be for both playback/capture, but restricted to the capabilities of the DAI.
Just wondering if setting up dpcm_playback/capture properly is worth it at all in this case. This isn't necessary for the non-DPCM case either, there we automatically set it based on the DAI capabilities.
We can add a simple loop for each direction that relies on snd_soc_dai_stream_valid() to identify if each DAI is capable of doing playback/capture.
see below completely untested diff to show what I had in mind: we already make use of snd_soc_dai_stream_valid() in other parts of the core so we should be able to determine dpcm_playback/capture based on the same information already used.
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 9ad35d9940fe..4c67f1f65eb4 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -215,7 +215,9 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, struct asoc_simple_dai *dai; struct snd_soc_dai_link_component *cpus = dai_link->cpus; struct snd_soc_dai_link_component *codecs = dai_link->codecs; + int stream; int ret; + int i;
/* Do it all CPU endpoint, and 1st Codec endpoint */ if (!li->cpu && dup_codec) @@ -317,8 +319,34 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, if (ret < 0) goto out_put_node;
- dai_link->dpcm_playback = 1; - dai_link->dpcm_capture = 1; + for_each_pcm_streams(stream) { + struct snd_soc_dai_link_component *cpu; + struct snd_soc_dai_link_component *codec; + struct snd_soc_dai *d; + bool dpcm_direction = true; + + for_each_link_cpus(dai_link, i, cpu) { + d = snd_soc_find_dai(cpu); + if (!d || !snd_soc_dai_stream_valid(d, stream)) { + dpcm_direction = false; + break; + } + } + for_each_link_codecs(dai_link, i, codec) { + d = snd_soc_find_dai(codec); + if (!d || !snd_soc_dai_stream_valid(d, stream)) { + dpcm_direction = false; + break; + } + } + if (dpcm_direction) { + if (stream == SNDRV_PCM_STREAM_PLAYBACK) + dai_link->dpcm_playback = 1; + if (stream == SNDRV_PCM_STREAM_CAPTURE) + dai_link->dpcm_capture = 1; + } + } + dai_link->ops = &graph_ops; dai_link->init = asoc_simple_dai_init;
On Tue, Jun 16, 2020 at 12:05:49PM -0500, Pierre-Louis Bossart wrote:
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep, NULL, &dai_link->dai_fmt); if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1; dai_link->dpcm_capture = 1;
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
But how do you know which capabilities to set? The device tree doesn't tells us that. We could add some code to look up the snd_soc_dai_driver early, based on the references in the device tree (basically something like snd_soc_of_get_dai_name(), see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...)
At least to me that function doesn't exactly look trivial though, and that's just to properly fill in the dpcm_playback/capture parameters. Essentially those parameters only complicate the current device tree use case, where you want the DAI link to be for both playback/capture, but restricted to the capabilities of the DAI.
Just wondering if setting up dpcm_playback/capture properly is worth it at all in this case. This isn't necessary for the non-DPCM case either, there we automatically set it based on the DAI capabilities.
We can add a simple loop for each direction that relies on snd_soc_dai_stream_valid() to identify if each DAI is capable of doing playback/capture.
see below completely untested diff to show what I had in mind: we already make use of snd_soc_dai_stream_valid() in other parts of the core so we should be able to determine dpcm_playback/capture based on the same information already used.
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 9ad35d9940fe..4c67f1f65eb4 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -215,7 +215,9 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, struct asoc_simple_dai *dai; struct snd_soc_dai_link_component *cpus = dai_link->cpus; struct snd_soc_dai_link_component *codecs = dai_link->codecs;
int stream; int ret;int i; /* Do it all CPU endpoint, and 1st Codec endpoint */ if (!li->cpu && dup_codec)@@ -317,8 +319,34 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1;dai_link->dpcm_capture = 1;
for_each_pcm_streams(stream) {struct snd_soc_dai_link_component *cpu;struct snd_soc_dai_link_component *codec;struct snd_soc_dai *d;bool dpcm_direction = true;for_each_link_cpus(dai_link, i, cpu) {d = snd_soc_find_dai(cpu);if (!d || !snd_soc_dai_stream_valid(d, stream)) {dpcm_direction = false;break;}}for_each_link_codecs(dai_link, i, codec) {d = snd_soc_find_dai(codec);if (!d || !snd_soc_dai_stream_valid(d, stream)) {dpcm_direction = false;break;}}if (dpcm_direction) {if (stream == SNDRV_PCM_STREAM_PLAYBACK)dai_link->dpcm_playback = 1;if (stream == SNDRV_PCM_STREAM_CAPTURE)dai_link->dpcm_capture = 1;}}dai_link->ops = &graph_ops; dai_link->init = asoc_simple_dai_init;
Thanks for the diff! I tested it for my case and it seems to work fine so far. I'm fine with this solution given that it fixes the problem I mentioned. We would need to patch it into at least simple-audio-card.c, audio-graph-card.c and soc/qcom/common.c (probably best to create a shared function in soc-core.c then).
However, personally I still think that dpcm_playback/capture essentially just duplicate the capabilities that are already exposed as part of the DAI drivers. We don't need that duplication in the non-DPCM case, so I wonder if we really need it for DPCM.
With your diff we go over all the DAIs to set dpcm_playback/capture correctly so that soc_new_pcm() can then verify that they were set correctly. IMO it would be much simpler to restore the previous behavior and just make soc_new_pcm() rely on the DAI capabilities to decide if playback/capture is supported, without producing the error.
Thanks, Stephan
On 6/17/20 4:01 AM, Stephan Gerhold wrote:
On Tue, Jun 16, 2020 at 12:05:49PM -0500, Pierre-Louis Bossart wrote:
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep, NULL, &dai_link->dai_fmt); if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1; dai_link->dpcm_capture = 1;
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
But how do you know which capabilities to set? The device tree doesn't tells us that. We could add some code to look up the snd_soc_dai_driver early, based on the references in the device tree (basically something like snd_soc_of_get_dai_name(), see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...)
At least to me that function doesn't exactly look trivial though, and that's just to properly fill in the dpcm_playback/capture parameters. Essentially those parameters only complicate the current device tree use case, where you want the DAI link to be for both playback/capture, but restricted to the capabilities of the DAI.
Just wondering if setting up dpcm_playback/capture properly is worth it at all in this case. This isn't necessary for the non-DPCM case either, there we automatically set it based on the DAI capabilities.
We can add a simple loop for each direction that relies on snd_soc_dai_stream_valid() to identify if each DAI is capable of doing playback/capture.
see below completely untested diff to show what I had in mind: we already make use of snd_soc_dai_stream_valid() in other parts of the core so we should be able to determine dpcm_playback/capture based on the same information already used.
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 9ad35d9940fe..4c67f1f65eb4 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -215,7 +215,9 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, struct asoc_simple_dai *dai; struct snd_soc_dai_link_component *cpus = dai_link->cpus; struct snd_soc_dai_link_component *codecs = dai_link->codecs;
int stream; int ret;int i; /* Do it all CPU endpoint, and 1st Codec endpoint */ if (!li->cpu && dup_codec)@@ -317,8 +319,34 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1;dai_link->dpcm_capture = 1;
for_each_pcm_streams(stream) {struct snd_soc_dai_link_component *cpu;struct snd_soc_dai_link_component *codec;struct snd_soc_dai *d;bool dpcm_direction = true;for_each_link_cpus(dai_link, i, cpu) {d = snd_soc_find_dai(cpu);if (!d || !snd_soc_dai_stream_valid(d, stream)) {dpcm_direction = false;break;}}for_each_link_codecs(dai_link, i, codec) {d = snd_soc_find_dai(codec);if (!d || !snd_soc_dai_stream_valid(d, stream)) {dpcm_direction = false;break;}}if (dpcm_direction) {if (stream == SNDRV_PCM_STREAM_PLAYBACK)dai_link->dpcm_playback = 1;if (stream == SNDRV_PCM_STREAM_CAPTURE)dai_link->dpcm_capture = 1;}}dai_link->ops = &graph_ops; dai_link->init = asoc_simple_dai_init;Thanks for the diff! I tested it for my case and it seems to work fine so far. I'm fine with this solution given that it fixes the problem I mentioned. We would need to patch it into at least simple-audio-card.c, audio-graph-card.c and soc/qcom/common.c (probably best to create a shared function in soc-core.c then).
Yes, I worked on a helper in soc-dai.c and have a tentative proposal in https://github.com/thesofproject/linux/pull/2203
However, personally I still think that dpcm_playback/capture essentially just duplicate the capabilities that are already exposed as part of the DAI drivers. We don't need that duplication in the non-DPCM case, so I wonder if we really need it for DPCM.
Fully agree, but removing dpcm_playback/capture/playback_only/capture_only should be done in a follow-up patchset. It's just too complicated to both fix the current DPCM configurations and clean-up at the same time, it'd prefer to do this cleanup in two steps.
With your diff we go over all the DAIs to set dpcm_playback/capture correctly so that soc_new_pcm() can then verify that they were set correctly. IMO it would be much simpler to restore the previous behavior and just make soc_new_pcm() rely on the DAI capabilities to decide if playback/capture is supported, without producing the error.
I don't understand what previous behavior you are referring to (it's not something I personally changed), and these flags are also hard-coded in static dailink descriptors for machine drivers.
On Wed, Jun 17, 2020 at 09:33:40AM -0500, Pierre-Louis Bossart wrote:
On 6/17/20 4:01 AM, Stephan Gerhold wrote:
On Tue, Jun 16, 2020 at 12:05:49PM -0500, Pierre-Louis Bossart wrote:
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep, NULL, &dai_link->dai_fmt); if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1; dai_link->dpcm_capture = 1;
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
But how do you know which capabilities to set? The device tree doesn't tells us that. We could add some code to look up the snd_soc_dai_driver early, based on the references in the device tree (basically something like snd_soc_of_get_dai_name(), see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...)
At least to me that function doesn't exactly look trivial though, and that's just to properly fill in the dpcm_playback/capture parameters. Essentially those parameters only complicate the current device tree use case, where you want the DAI link to be for both playback/capture, but restricted to the capabilities of the DAI.
Just wondering if setting up dpcm_playback/capture properly is worth it at all in this case. This isn't necessary for the non-DPCM case either, there we automatically set it based on the DAI capabilities.
We can add a simple loop for each direction that relies on snd_soc_dai_stream_valid() to identify if each DAI is capable of doing playback/capture.
see below completely untested diff to show what I had in mind: we already make use of snd_soc_dai_stream_valid() in other parts of the core so we should be able to determine dpcm_playback/capture based on the same information already used.
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 9ad35d9940fe..4c67f1f65eb4 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -215,7 +215,9 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, struct asoc_simple_dai *dai; struct snd_soc_dai_link_component *cpus = dai_link->cpus; struct snd_soc_dai_link_component *codecs = dai_link->codecs;
int stream; int ret;int i; /* Do it all CPU endpoint, and 1st Codec endpoint */ if (!li->cpu && dup_codec)@@ -317,8 +319,34 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1;dai_link->dpcm_capture = 1;
for_each_pcm_streams(stream) {struct snd_soc_dai_link_component *cpu;struct snd_soc_dai_link_component *codec;struct snd_soc_dai *d;bool dpcm_direction = true;for_each_link_cpus(dai_link, i, cpu) {d = snd_soc_find_dai(cpu);if (!d || !snd_soc_dai_stream_valid(d, stream)) {dpcm_direction = false;break;}}for_each_link_codecs(dai_link, i, codec) {d = snd_soc_find_dai(codec);if (!d || !snd_soc_dai_stream_valid(d, stream)) {dpcm_direction = false;break;}}if (dpcm_direction) {if (stream == SNDRV_PCM_STREAM_PLAYBACK)dai_link->dpcm_playback = 1;if (stream == SNDRV_PCM_STREAM_CAPTURE)dai_link->dpcm_capture = 1;}}dai_link->ops = &graph_ops; dai_link->init = asoc_simple_dai_init;Thanks for the diff! I tested it for my case and it seems to work fine so far. I'm fine with this solution given that it fixes the problem I mentioned. We would need to patch it into at least simple-audio-card.c, audio-graph-card.c and soc/qcom/common.c (probably best to create a shared function in soc-core.c then).
Yes, I worked on a helper in soc-dai.c and have a tentative proposal in https://github.com/thesofproject/linux/pull/2203
Thanks!
However, personally I still think that dpcm_playback/capture essentially just duplicate the capabilities that are already exposed as part of the DAI drivers. We don't need that duplication in the non-DPCM case, so I wonder if we really need it for DPCM.
Fully agree, but removing dpcm_playback/capture/playback_only/capture_only should be done in a follow-up patchset. It's just too complicated to both fix the current DPCM configurations and clean-up at the same time, it'd prefer to do this cleanup in two steps.
With your diff we go over all the DAIs to set dpcm_playback/capture correctly so that soc_new_pcm() can then verify that they were set correctly. IMO it would be much simpler to restore the previous behavior and just make soc_new_pcm() rely on the DAI capabilities to decide if playback/capture is supported, without producing the error.
I don't understand what previous behavior you are referring to (it's not something I personally changed), and these flags are also hard-coded in static dailink descriptors for machine drivers.
At the end the question is if those machine drivers that have dpcm_playback/capture hardcoded just set it because it was required to make DPCM work, or if they actually use it to restrict the direction of a DAI link.
If they just did it to make DPCM work, then dpcm_playback/capture will most likely be equal to the capabilities of the DAIs. In that case there is little reason to duplicate that information on the DAI link. (We can just get the capabilities by always iterating over the DAIs, just like your helpers do for the device tree case).
IMO dpcm_playback/capture would be only needed at all if you want to restrict the direction of a DAI link, instead of using everything possible using the configured DAI links.
But I agree, removing dpcm_playback/capture is way too much refactoring to fix the problem I mentioned.
The previous behavior I'm referring to is that until your patch (the one I replied to), it was not required to set dpcm_playback/capture appropriately:
Before your changes there was just
playback = rtd->dai_link->dpcm_playback && snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK); capture = rtd->dai_link->dpcm_capture && snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE);
so even if both dpcm_playback/capture were set to 1 (= true), it would just set playback/capture = false if the CPU DAI does not actually support those.
Your patch added error conditions if dpcm_playback/capture do not match the actual capabilities of the DAI, so now I get e.g. "CPU DAI MultiMedia1 for rtd MultiMedia1 does not support capture".
Something like this would restore the previous behavior:
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 39ce61c5b874..2b32c2a1fad7 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2920,31 +2920,31 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) { if (rtd->dai_link->dpcm_playback) { stream = SNDRV_PCM_STREAM_PLAYBACK; + playback = 1;
for_each_rtd_cpu_dais(rtd, i, cpu_dai) if (!snd_soc_dai_stream_valid(cpu_dai, stream)) { - dev_err(rtd->card->dev, + dev_dbg(rtd->card->dev, "CPU DAI %s for rtd %s does not support playback\n", cpu_dai->name, rtd->dai_link->stream_name); - return -EINVAL; + playback = 0; } - playback = 1; } if (rtd->dai_link->dpcm_capture) { stream = SNDRV_PCM_STREAM_CAPTURE; + capture = 1;
for_each_rtd_cpu_dais(rtd, i, cpu_dai) if (!snd_soc_dai_stream_valid(cpu_dai, stream)) { - dev_err(rtd->card->dev, + dev_dbg(rtd->card->dev, "CPU DAI %s for rtd %s does not support capture\n", cpu_dai->name, rtd->dai_link->stream_name); - return -EINVAL; + capture = 0; } - capture = 1; } } else { /* Adapt stream for codec2codec links */
(I just removed the error case you introduced in your patch...)
I understand why you added those error checks, but as a temporary fix it might be easier to relax those error checks again for now. I'm not sure if any other drivers have dpcm_playback/capture set "incorrectly" (better: not matching the DAI capabilities) as well.
Thanks, Stephan
On Wed, Jun 17, 2020 at 07:46:35PM +0200, Stephan Gerhold wrote:
At the end the question is if those machine drivers that have dpcm_playback/capture hardcoded just set it because it was required to make DPCM work, or if they actually use it to restrict the direction of a DAI link.
The other question would be if they are restricting it to limit the direction of a DAI link beyond the limits that the hardware has why are they doing that? I'm not sure that'd be a sensible thing to do.
On 6/18/20 10:01 AM, Mark Brown wrote:
On Wed, Jun 17, 2020 at 07:46:35PM +0200, Stephan Gerhold wrote:
At the end the question is if those machine drivers that have dpcm_playback/capture hardcoded just set it because it was required to make DPCM work, or if they actually use it to restrict the direction of a DAI link.
I think those flags are absolutely not DPCM specific, the only use I see for the flags is to set:
if (rtd->dai_link->no_pcm || rtd->dai_link->params) { if (playback) pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; if (capture) pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; goto out; }
and that's why I highlighted some time back that they are probably redundant with capture_only and playback_only. We don't need 4 flags to specify 2 directions.
In all cases the use for those flags seems to be to restrict the direction of a DAI link.
Note that people can screw-up the configurations without DPCM, e.g. by not setting capture_only for a microphone, I found last week a WoV DAI link on Broadwell where the capture_only flag was not set... DPCM does not have a monopoly on brokenness...
The other question would be if they are restricting it to limit the direction of a DAI link beyond the limits that the hardware has why are they doing that? I'm not sure that'd be a sensible thing to do.
I don't see any such case. When both directions are not set, it's only because the hardware is only capable of one, e.g. dmic, HDMI or SoundWire.
There's one set of cases where we have amplifiers on an SSP link (which is bidirectional), but the amplifier itself does not provide any capture/feedback. That part is probably borderline incorrect, but harmless since the topology does not try to use those links for capture.
On Thu, Jun 18, 2020 at 10:45:45AM -0500, Pierre-Louis Bossart wrote:
On 6/18/20 10:01 AM, Mark Brown wrote:
On Wed, Jun 17, 2020 at 07:46:35PM +0200, Stephan Gerhold wrote:
At the end the question is if those machine drivers that have dpcm_playback/capture hardcoded just set it because it was required to make DPCM work, or if they actually use it to restrict the direction of a DAI link.
I think those flags are absolutely not DPCM specific, the only use I see for the flags is to set:
if (rtd->dai_link->no_pcm || rtd->dai_link->params) { if (playback) pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; if (capture) pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; goto out; }
and that's why I highlighted some time back that they are probably redundant with capture_only and playback_only. We don't need 4 flags to specify 2 directions.
In all cases the use for those flags seems to be to restrict the direction of a DAI link.
Note that people can screw-up the configurations without DPCM, e.g. by not setting capture_only for a microphone, I found last week a WoV DAI link on Broadwell where the capture_only flag was not set... DPCM does not have a monopoly on brokenness...
The other question would be if they are restricting it to limit the direction of a DAI link beyond the limits that the hardware has why are they doing that? I'm not sure that'd be a sensible thing to do.
I don't see any such case. When both directions are not set, it's only because the hardware is only capable of one, e.g. dmic, HDMI or SoundWire.
If we end up simplifying those flags, and eventually removing dpcm_playback/capture entirely, wouldn't it be the easiest solution to just relax the error checks for 5.8? (Like the diff I suggested?)
Pierre's diff to set dpcm_playback/capture correctly for simple-audio-card etc would work too, but I'm not sure if this is worth it if we end up removing those anyway.
Thanks, Stephan
On Mon, Jun 22, 2020 at 07:54:23PM +0200, Stephan Gerhold wrote:
If we end up simplifying those flags, and eventually removing dpcm_playback/capture entirely, wouldn't it be the easiest solution to just relax the error checks for 5.8? (Like the diff I suggested?)
Pierre's diff to set dpcm_playback/capture correctly for simple-audio-card etc would work too, but I'm not sure if this is worth it if we end up removing those anyway.
Yeah, if we're removing them anyway then taking something that just ignores them as a fix seems reasonable.
On Tue 16 Jun 2020 at 18:42, Mark Brown broonie@kernel.org wrote:
On Tue, Jun 16, 2020 at 10:05:39AM -0500, Pierre-Louis Bossart wrote:
On 6/16/20 9:52 AM, Mark Brown wrote:
On Tue, Jun 16, 2020 at 09:23:25AM -0500, Pierre-Louis Bossart wrote:
Doesn't simple-card rely on DT blobs that can also be updated?
DT is an ABI just like ACPI - it's just more featureful. Many systems can easily update their DTs but not all of them and users don't always want to try to keep it in lock step with the kernel. Stuff like this is why I've been dubious about putting DPCM things in there, it's too much of a hard coding of internal APIs.
ok, but is there any actual use of dpcm_playback/capture outside of C code?
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
...
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
Right, just heading off the idea that we can fix things by updating DTs.
Hi everyone,
Getting to this a bit late in the game but ...
This patch breaks things for me as well. The Amlogic S400 (axg-card) and Libretech S905x card (gx-card) are not probing anymore after this change. Both have some BEs handling only one direction.
Like for Stephan (and simple-card), the related cards used to set dpcm_capture/dpcm_playback to 1.
Before this patch, these flags just applied an additionnal restiction on the link. So the card was setting it to 1 and let soc-pcm.c figure out what was actually needed. Whether it is usefull to have such flags is certainly up for debate ...
However, with patch, the meaning of the flags changed from "retrict" to "require" which is something else entirely.
Commit 25612477d20b ("ASoC: soc-dai: set dai_link dpcm_ flags with a helper") makes things worse (for me) by requiring all the elements on the link to support the stream direction for the direction to be enabled.
This breaks platforms where there is multiple codecs with different capabilities on a link. For example, we may have 2 codecs on a link, one doing playback, the other capture. This is the case on several Amlogic platforms.
With the new meaning of those flag, the card driver has to set something that ASoC core will also compute on its own, and verify it agrees with the card. This is redundant. What is the point of this ? Can't we just cut the middle man then ?
The old meaning at least allowed to pass the additional information that a direction was to be forcefully disabled. This is also redundant with capture_only/playback_only though ...
Can we revert this change until we figure out to do with those flags ?
I could propose a patch to move on but I'm not entirely clear what it kind of restriction we were trying to put on Multi-CPU links
IMO, on a Multi-CPU/Multi-Codec link, we should allow the direction as long as at least one CPU and one Codec on the link are capable of handling the direction (not all of them, as it seems to be set now)
Cheers Jerome
Commit 25612477d20b ("ASoC: soc-dai: set dai_link dpcm_ flags with a helper") makes things worse (for me) by requiring all the elements on the link to support the stream direction for the direction to be enabled.
This breaks platforms where there is multiple codecs with different capabilities on a link. For example, we may have 2 codecs on a link, one doing playback, the other capture. This is the case on several Amlogic platforms.
With the new meaning of those flag, the card driver has to set something that ASoC core will also compute on its own, and verify it agrees with the card. This is redundant. What is the point of this ? Can't we just cut the middle man then ?
The old meaning at least allowed to pass the additional information that a direction was to be forcefully disabled. This is also redundant with capture_only/playback_only though ...
My plan was to remove two of the 4 flags after all configurations were checked.
Can we revert this change until we figure out to do with those flags ?
I could propose a patch to move on but I'm not entirely clear what it kind of restriction we were trying to put on Multi-CPU links
IMO, on a Multi-CPU/Multi-Codec link, we should allow the direction as long as at least one CPU and one Codec on the link are capable of handling the direction (not all of them, as it seems to be set now)
You have a valid point Jerome. I must admit I was looking at TDM configurations, where we do want all DAIs in the same dailink to be consistent.
But when I checked older code there is indeed a precedent for some KabyLake platforms with an amplifier on playback and a microphone codec on capture for the same dailink.
I think your proposal of checking that at least one DAI matches the dailink capabilities, and conversely changing the helper to set the properties if one or more dais support a direction smay indeed be required.
If this was a feature and not a bug to have different capabilities on the same link so be it. Mark, do you concur?
On Fri, Jul 17, 2020 at 12:13:14PM -0500, Pierre-Louis Bossart wrote:
IMO, on a Multi-CPU/Multi-Codec link, we should allow the direction as long as at least one CPU and one Codec on the link are capable of handling the direction (not all of them, as it seems to be set now)
...
If this was a feature and not a bug to have different capabilities on the same link so be it. Mark, do you concur?
Yes.
On 7/17/20 1:18 PM, Mark Brown wrote:
On Fri, Jul 17, 2020 at 12:13:14PM -0500, Pierre-Louis Bossart wrote:
IMO, on a Multi-CPU/Multi-Codec link, we should allow the direction as long as at least one CPU and one Codec on the link are capable of handling the direction (not all of them, as it seems to be set now)
...
If this was a feature and not a bug to have different capabilities on the same link so be it. Mark, do you concur?
Yes.
ok, I cooked a quick patch to do this, testing on-going: https://github.com/thesofproject/linux/pull/2293
participants (4)
-
Jerome Brunet -
Mark Brown -
Pierre-Louis Bossart -
Stephan Gerhold