On Fri, 10 May 2019 04:32:11 +0200, Yang, Libin wrote:
-----Original Message----- From: Ranjani Sridharan [mailto:ranjani.sridharan@linux.intel.com] Sent: Friday, May 10, 2019 10:03 AM To: Yang, Libin libin.yang@intel.com; Takashi Iwai tiwai@suse.de Cc: alsa-devel@alsa-project.org; Sridharan, Ranjani ranjani.sridharan@intel.com; pierre-louis.bossart@linux.intel.com; Wang, Rander rander.wang@intel.com; broonie@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:
- Pause-release after S3 without RESUME_INFO
Prepare() -> trigger start 2. pause-release without S3 without/with RESUME_INFO Trigger pause-release
- 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 */