Re: [PATCH] ASoC: core: use less strict tests for dailink capabilities

[Jerome Brunet <jbrunet@baylibre.com>]

On Thu 23 Jul 2020 at 20:05, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:

> Previous updates to set dailink capabilities and check dailink
> capabilities were based on a flawed assumption that all dais support
> the same capabilities as the dailink. This is true for TDM
> configurations

Actually it is not true for TDM either, having codecs with different
playback/capture caps is valid with TDM as well

> but existing configurations use an amplifier and a
> capture device on the same dailink, and the tests would prevent the
> card from probing.
>
> This patch modifies the snd_soc_dai_link_set_capabilities()
> helper so that the dpcm_playback (resp. dpcm_capture) dailink
> capabilities are set if at least one dai supports playback (resp. capture).
>
> Likewise the checks are modified so that an error is reported only
> when dpcm_playback (resp. dpcm_capture) is set but none of the CPU
> DAIs support playback (resp. capture).

This was not is the original proposal you sent ...

>
> Fixes: 25612477d20b5 ('ASoC: soc-dai: set dai_link dpcm_ flags with a helper')
> Fixes: b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
> Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/soc-dai.c | 16 +++++++++-------
>  sound/soc/soc-pcm.c | 42 ++++++++++++++++++++++++------------------
>  2 files changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
> index 98f0c98b06bb..ef1700e5e1bf 100644
> --- a/sound/soc/soc-dai.c
> +++ b/sound/soc/soc-dai.c
> @@ -402,28 +402,30 @@ void snd_soc_dai_link_set_capabilities(struct snd_soc_dai_link *dai_link)
>  	struct snd_soc_dai_link_component *codec;
>  	struct snd_soc_dai *dai;
>  	bool supported[SNDRV_PCM_STREAM_LAST + 1];
> +	bool supported_cpu;
> +	bool supported_codec;
>  	int direction;
>  	int i;
>  
>  	for_each_pcm_streams(direction) {
> -		supported[direction] = true;
> +		supported_cpu = false;
> +		supported_codec = false;
>  
>  		for_each_link_cpus(dai_link, i, cpu) {
>  			dai = snd_soc_find_dai(cpu);
> -			if (!dai || !snd_soc_dai_stream_valid(dai, direction)) {
> -				supported[direction] = false;
> +			if (dai && snd_soc_dai_stream_valid(dai, direction)) {
> +				supported_cpu = true;
>  				break;
>  			}
>  		}
> -		if (!supported[direction])
> -			continue;
>  		for_each_link_codecs(dai_link, i, codec) {
>  			dai = snd_soc_find_dai(codec);
> -			if (!dai || !snd_soc_dai_stream_valid(dai, direction)) {
> -				supported[direction] = false;
> +			if (dai && snd_soc_dai_stream_valid(dai, direction)) {
> +				supported_codec = true;
>  				break;
>  			}
>  		}
> +		supported[direction] = supported_cpu && supported_codec;
>  	}
>  
>  	dai_link->dpcm_playback = supported[SNDRV_PCM_STREAM_PLAYBACK];
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index f2c7c85ad40c..aac61df0c50e 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2743,30 +2743,36 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>  		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;
> +			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
> +				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
> +					playback = 1;
> +					break;
>  				}
> -			playback = 1;
> +			}
> +
> +			if (!playback) {
> +				dev_err(rtd->card->dev,
> +					"No CPU DAIs support playback for stream %s\n",
> +					rtd->dai_link->stream_name);
> +				return -EINVAL;
> +			}

Again, this is changing the original meaning of the flag from "playback
allowed" to "playback required".

This patch (or the orignal) does not explain why this change of meaning
is necessary ? The point I was making here [0] still stands.

If your evil plan is to get rid of 2 of the 4 flags, why go through the
trouble of the changing the meaning and effect of one them ?

[0]: https://lore.kernel.org/r/1j1rla9s22.fsf@starbuckisacylon.baylibre.com

>  		}
>  		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;
> +			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
> +				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
> +					capture = 1;
> +					break;
>  				}
> -			capture = 1;
> +			}
> +
> +			if (!capture) {
> +				dev_err(rtd->card->dev,
> +					"No CPU DAIs support capture for stream %s\n",
> +					rtd->dai_link->stream_name);
> +				return -EINVAL;
> +			}
>  		}
>  	} else {
>  		/* Adapt stream for codec2codec links */