[PATCH 0/4] ASoC: Fix dailink checks for DPCM

[PATCH 0/4] ASoC: Fix dailink checks for DPCM

[Pierre-Louis Bossart]

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
-- 
2.20.1

[PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

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 ?
-- 
2.20.1

[PATCH 2/4] ASoC: core: only convert non DPCM link to DPCM link

[Pierre-Louis Bossart]

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
-- 
2.20.1

[PATCH 3/4] ASoC: Intel: boards: replace capture_only by dpcm_capture

[Pierre-Louis Bossart]

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),
 	},
-- 
2.20.1

[PATCH 4/4] ASoC: SOF: nocodec: conditionally set dpcm_capture/dpcm_playback flags

[Pierre-Louis Bossart]

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

Re: [PATCH 0/4] ASoC: Fix dailink checks for DPCM

[Mark Brown]

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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Stephan Gerhold]

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.kandagatla@linaro.org/)

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);
>  	} else {
>  		/* Adapt stream for codec2codec links */
>  		int cpu_capture = rtd->dai_link->params ?
> -- 
> 2.20.1
> 

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Stephan Gerhold]

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.kandagatla@linaro.org/)
> 
> 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);
> >  	} else {
> >  		/* Adapt stream for codec2codec links */
> >  		int cpu_capture = rtd->dai_link->params ?
> > -- 
> > 2.20.1
> > 

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

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 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.kandagatla@linaro.org/)
>>
>> 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.

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Mark Brown]

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

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.

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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

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.

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Stephan Gerhold]

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/sound/soc/soc-core.c#n2988)

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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

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/sound/soc/soc-core.c#n2988)
> 
> 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.

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Mark Brown]

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

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.

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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

>>> 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/sound/soc/soc-core.c#n2988) 
>>
>>
>> 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;

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Stephan Gerhold]

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/sound/soc/soc-core.c#n2988)
> > > 
> > > 
> > > 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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

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/sound/soc/soc-core.c#n2988)
>>>>
>>>>
>>>> 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.

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Stephan Gerhold]

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/sound/soc/soc-core.c#n2988)
> > > > > 
> > > > > 
> > > > > 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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Mark Brown]

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

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.

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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

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.

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Stephan Gerhold]

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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Mark Brown]

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

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.

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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Jerome Brunet]

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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

> 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?

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Mark Brown]

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

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.

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

Re: [PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

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