Re: Asoc: Intel: SST (CHT) regression in asoc/for-5.11

[Takashi Iwai <tiwai@suse.de>]

On Tue, 01 Dec 2020 17:15:15 +0100,
Ranjani Sridharan wrote:
> 
> > > 
> > > I was able to reproduce this error with Mark's for-next branch on a
> > > CHT device w/ rt5640, and git bisect points to this commit:
> > > 
> > > a27b421f1d04b201c474a15ee1591919c81fb413 is the first bad commit
> > > commit a27b421f1d04b201c474a15ee1591919c81fb413
> > > Author: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> > > Date:   Tue Nov 17 13:50:01 2020 -0800
> > > 
> > >     ASoC: pcm: call snd_soc_dapm_stream_stop() in soc_pcm_hw_clean
> > > 
> > >     Currently, the SND_SOC_DAPM_STREAM_START event is sent during
> > >     pcm_prepare() but the SND_SOC_DAPM_STREAM_STOP event is
> > >     sent only in dpcm_fe_dai_shutdown() after soc_pcm_close().
> > >     This results in an imbalance between when the DAPM widgets
> > >     receive the PRE/POST_PMU/PMD events. So call
> > >     snd_soc_dapm_stream_stop() in soc_pcm_hw_clean() before the
> > >     snd_soc_pcm_component_hw_free() to keep the stream_stop DAPM
> > >     event balanced with the stream_start event in soc_pm_prepare().
> > > 
> > >     Also, in order to prevent duplicate DAPM stream events,
> > >     remove the call for DAPM STREAM_START event in
> > > dpcm_fe_dai_prepare()
> > >     and the call for DAPM STREAM_STOP event in
> > > dpcm_fe_dai_shutdown().
> > > 
> > >     Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > >     Reviewed-by: Pierre-Louis Bossart
> > > <pierre-louis.bossart@linux.intel.com>
> > >     Signed-off-by: Ranjani Sridharan <
> > > ranjani.sridharan@linux.intel.com>
> > >     Link:
> > > https://lore.kernel.org/r/20201117215001.163107-1-ranjani.sridharan@linux.intel.com
> > >     Signed-off-by: Mark Brown <broonie@kernel.org>
> > > 
> > >  sound/soc/soc-pcm.c | 10 +++-------
> > >  1 file changed, 3 insertions(+), 7 deletions(-)
> > > 
> > > 
> > > I am not sure why this break the Atom/SST driver, this was reviewed
> > > and seemed legit - even required IIRC to deal with topology
> > > pipelines
> > > initialized on-demand. Reverting this patch restores functionality.
> > > I
> > > would guess it's the DAPM_STREAM_START that's now missing (or in
> > > the
> > > 'wrong' location) and causing issues?
> > 
> > Indeed the DAPM_START_STREAM call completely disappeared after the
> > patch, which looks very wrong.  This has to be revisited before 5.11
> > merge.
> 
> Hi Pierre/Takashi,
> 
> The DAPM_STREAM_START event is still there in soc_pcm_prepare() and
> this patch only removed the duplicate call in dpcm_fe_dai_prepare().

Ah, thanks, I see now.

But note that the PCM prepare callback may be called multiple times in
row; i.e. it's not always paired with hw_clean (that is via either
hw_params error path or hw_free).  So if the balance really matters,
we need another type of checks, not relying on the call pattern.


Takashi