[alsa-devel] [PATCH V2] ASoC: soc-pcm: BE dai needs prepare when pause release after resume

[Takashi Iwai]

On Fri, 10 May 2019 04:32:11 +0200,
Yang, Libin wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Ranjani Sridharan [mailto:ranjani.sridharan at linux.intel.com]
> >Sent: Friday, May 10, 2019 10:03 AM
> >To: Yang, Libin <libin.yang at intel.com>; Takashi Iwai <tiwai at suse.de>
> >Cc: alsa-devel at alsa-project.org; Sridharan, Ranjani
> ><ranjani.sridharan at intel.com>; pierre-louis.bossart at linux.intel.com; Wang,
> >Rander <rander.wang at intel.com>; broonie at kernel.org
> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: soc-pcm: BE dai needs prepare
> >when pause release after resume
> >
> >> > >
> >> > > So in the current scenario what we see is that after resuming from
> >> > > S3, a pause-release action from the user results in a FE prepare()
> >> > > followed by the START trigger (and not a PAUSE-RELEASE trigger).
> >> > >
> >> > > Libin's patch proposes to do a prepare() for the BE even in the
> >> > > case of a regular pause-release. But this might not be ideal since
> >> > > other drivers might have logic in the prepare() ioctl that might
> >> > > end up with errors.
> >> >
> >> > Right.
> >> >
> >> > > So I am thinking maybe we can have some internal logic in the SOF
> >> > > prepare() callback that will also call the BE prepare() when the
> >> > > be->dpcm[stream].state is SND_SOC_DPCM_STATE_PAUSED? Would that
> >> >
> >> > make
> >> > > sense?
> >> >
> >> > Yes, that would work, I guess.  Eventually this might be needed to
> >> > be addressed in ALSA core side, too, but it's good to have some fix
> >> > beforehand in DPCM.
> >>
> >> Ranjani, with "regular pause-release", do you mean pause-release
> >> without S3? The prepare() is called from alsa core (pcm_native.c) in
> >> S3 case.
> >> Prepare() being called in pause-release after S3 is because of S3, not
> >> because of pause-release. Actually, if you pause-release without S3
> >> (not sure in pm-runtime case), ASoC's prepare() will not be called. So
> >> dpcm_be_dai_prepare() will not be called. So you assumption of
> >> "regular pause-release" calling prepare() is wrong.
> >Oh yes. That's right. Thanks for pointing it out.
> >In this case, the patch sounds like a good fix. Basically, you're saying that if the
> >FE prepare() gets called (which happens in the case of pause-release without
> >INFO_RESUME) it should also call the BE prepare(), right?
> 
> I mean as there is a S3, we need prepare() for both FE and BE.
> And logically, if ASoC calls FE prepare(), it should also call BE prepare().
> Otherwise, FE and BE are not synced. The behavior is unknown unless
> we really know what's happening in ASoC.
> 
> >
> >Takashi, what do you think?
> >
> >>
> >> Please let me describe the flow below:
> >> 1. Pause-release after S3 without RESUME_INFO
> >> Prepare() -> trigger start
> >> 2. pause-release without S3 without/with RESUME_INFO Trigger
> >> pause-release
> >
> >> 3. Pause-release after S3 with RESUME_INFO Trigger resume
> >Are you sure about this? A paused stream will not be suspended. So it would
> >still be trigger PAUSE-RELEASE in this case?
> 
> Hum, maybe you are right. I didn't test such case. If we don't need call 
> "trigger resume" even after
> S3? If it triggers PAUSE-RELEASE, how can we know it is after S3 or not?
> Driver may do different operations for pause release for with S3 or without S3.

Yes, some change in ALSA PCM core side is needed for that.  It's what
I mentioned in my previous post.

My rough idea is a patch like below.  It'll make trigger(SUSPEND) call
for all cases already in PREPARE or later state.
You'd need to implement the corresponding trigger stuff properly in
the driver side, of course.


thanks,

Takashi

---
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -298,6 +298,7 @@ typedef int __bitwise snd_pcm_subformat_t;
 #define SNDRV_PCM_INFO_HAS_LINK_ESTIMATED_ATIME    0x04000000  /* report estimated link audio time */
 #define SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME 0x08000000  /* report synchronized audio/system time */
 
+#define SNDRV_PCM_INFO_SUSPEND_TRIGGER	0x20000000		/* internal kernel flag - always trigger at suspend */
 #define SNDRV_PCM_INFO_DRAIN_TRIGGER	0x40000000		/* internal kernel flag - trigger in drain */
 #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
 
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1463,7 +1463,8 @@ static int snd_pcm_do_suspend(struct snd_pcm_substream *substream, int state)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	if (runtime->trigger_master != substream)
 		return 0;
-	if (! snd_pcm_running(substream))
+	if (!(runtime->info & SNDRV_PCM_INFO_TRIGGER_SUSPEND) &&
+	    !snd_pcm_running(substream))
 		return 0;
 	substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
 	return 0; /* suspend unconditionally */