Re: [PATCH] ASoC: soc-pcm: dpcm: Only allow playback/capture if supported

From: Stephan Gerhold <stephan@gerhold.net>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org, Mark Brown <broonie@kernel.org>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH] ASoC: soc-pcm: dpcm: Only allow playback/capture if supported
Date: Thu, 16 Apr 2020 23:41:11 +0200	[thread overview]
Message-ID: <20200416214111.GA898@gerhold.net> (raw)
In-Reply-To: <61ccf27d-a511-b3a4-2da7-56edb01b24c6@linux.intel.com>

On Thu, Apr 16, 2020 at 03:51:23PM -0500, Pierre-Louis Bossart wrote:
> 
> > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> > index 454735f8fa92..77a680da366f 100644
> > --- a/sound/soc/soc-pcm.c
> > +++ b/sound/soc/soc-pcm.c
> > @@ -2911,8 +2911,17 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> >   	int i;
> >   	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> > -		playback = rtd->dai_link->dpcm_playback;
> > -		capture = rtd->dai_link->dpcm_capture;
> > +		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;
> > +		}
> > +
> > +		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);
> 
> This commit introduces major regressions with SOF on CherryTrail and
> Broadwell:
> 
>     [   25.705750]  SSP2-Codec: ASoC: no backend playback stream
>     [   27.923378]  SSP2-Codec: ASoC: no users playback at close - state
> 
> it's likely due to the check for min_channels > 0 in
> snd_soc_dai_stream_valid(), which wasn't a requirement before.
> 
> We are testing a fix [1] but other users of DPCM might be impacted.
> 

Indeed, I actually ran into a similar problem myself for q6afe-dai:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=for-5.7&id=0c824ec094b5cda766c80d88c2036e28c24a4cb1

As mentioned in that commit message it was already broken on 5.7-rc1
for me, because of commit 0e9cf4c452ad ("ASoC: pcm: check if cpu-dai
supports a given stream"). [2]

With this commit it's more visible at least, you get a proper error
instead of silently not calling hw_params() for example. :)

[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0e9cf4c452ad7e2776441cbac0b9983abaf17ff0

> Mark, this commit is on your for-5.7 branch but not on for-next? Not sure
> which SHA1 to use for the Fixes: tag
> 
> [1] https://github.com/thesofproject/linux/pull/2018/commits/4fa10638dca8aad7a320e85cc3e00b179b8de410