[alsa-devel] [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Friday, December 19, 2014 4:48 PM
> To: Yang, Libin
> Cc: broonie at kernel.org; alsa-devel at alsa-project.org;
> liam.r.girdwood at linux.intel.com; Jie, Yang
> Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger
> 
> At Fri, 19 Dec 2014 06:29:33 +0000,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > Thanks for review, please see my comments.
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai at suse.de]
> > > Sent: Thursday, December 18, 2014 4:30 PM
> > > To: Yang, Libin
> > > Cc: broonie at kernel.org; alsa-devel at alsa-project.org;
> > > liam.r.girdwood at linux.intel.com; Jie, Yang
> > > Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN
> trigger
> > >
> > > At Thu, 18 Dec 2014 15:36:03 +0800,
> > > libin.yang at intel.com wrote:
> > > >
> > > > From: Libin Yang <libin.yang at intel.com>
> > > >
> > > > Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.
> > > >
> > > > Some audio devices require notification of drain events in order
> > > > to properly drain and shutdown an audio stream.
> > > >
> > > > Signed-off-by: Libin Yang <libin.yang at intel.com>
> > > > ---
> > > >  include/sound/pcm.h     | 1 +
> > > >  sound/core/pcm_native.c | 5 +++++
> > > >  2 files changed, 6 insertions(+)
> > > >
> > > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h index
> > > > 8bb00a2..bb77aa3 100644
> > > > --- a/include/sound/pcm.h
> > > > +++ b/include/sound/pcm.h
> > > > @@ -109,6 +109,7 @@ struct snd_pcm_ops {
> > > >  #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
> > > >  #define SNDRV_PCM_TRIGGER_SUSPEND	5
> > > >  #define SNDRV_PCM_TRIGGER_RESUME	6
> > > > +#define SNDRV_PCM_TRIGGER_DRAIN		7
> > > >
> > > >  #define SNDRV_PCM_POS_XRUN
> 	((snd_pcm_uframes_t)-1)
> > > >
> > > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > > > index
> > > > 166d59c..15fe682 100644
> > > > --- a/sound/core/pcm_native.c
> > > > +++ b/sound/core/pcm_native.c
> > > > @@ -1517,6 +1517,11 @@ static int snd_pcm_do_drain_init(struct
> > > > snd_pcm_substream *substream, int state)  {
> > > >  	struct snd_pcm_runtime *runtime = substream->runtime;
> > > >  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > > > +
> > > > +		if (substream->runtime->trigger_master == substream)
> > > > +			substream->ops->trigger(substream,
> > > > +					SNDRV_PCM_TRIGGER_DRAIN);
> > >
> > > The return value check is missing.
> >
> > Actually, the code doesn't care the return value and I don't want to
> > let the trigger return value impact the snd_pcm_do_drain_init() flow
> > just like snd_pcm_do_stop().
> 
> Well, the fundamental problem in this patch is that it was written *just for
> your hardware* although this is a public API extension.
> Consider again about it: if the return value of this trigger doesn't matter for
> your hardware, the driver can simply return zero always.
> Why do you need to drop the check in the common code?  Wouldn't any
> driver return any error there?

Right, I will add the return value check.

> 
> 
> > > Besides, this patch will break many drivers.  Many of them will spew
> > > errors if an unexpected trigger command is issued.  So, you have to
> > > limit this trigger only to some drivers.  A typical workaround is to
> > > add a new
> > > SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as
> > > done for resume and pause operations.
> >
> > Yes, that's a good suggestion. Thanks for reminding me.
> 
> OTOH, the info flag will be exposed to user-space while this is a kernel-space-
> only information.  So, in the code copying the info flag
> (snd_pcm_hw_refine()) would need to drop this bit, too.

Thanks, I will drop this bit in snd_pcm_hw_refine().

> 
> Alternatively, we may add just a new flag in struct snd_pcm or struct
> snd_pcm_substream and let the driver setting it manually.  This might need
> more changes in ASoC side, though, as the PCM creation isn't directly done
> by the driver.

Yes, it is a little complex for asoc if using this method. 
Not sure if it is OK we set the flag in snd_soc_ops->startup() each time 
when the function is called.

What do you think we just use the first method? 

> 
> 
> > > Also, do you want to see this trigger even if the stream isn't
> > > started before drain is issued?  Also, why no need for this trigger for
> capture streams?
> >
> > In this case, the trigger action will be started. For the stream not
> > started case, if I skip the trigger , I don't know where I can have
> > the chance to trigger later. And I didn't find the suitable place to trigger my
> code.
> > Put the trigger at the end of the function snd_pcm_do_drain_init() or
> > in the function snd_pcm_post_drain_init() will be OK?
> >
> > I didn't include the capture case because we found the capture
> > currently don't need the trigger.
> >
> > Our issue is the fw don't know when to stop fetching the data. For
> > playback we need fill silence data when draining, otherwise the fw will get
> the old data.
> > But for capture, after the driver has fetched the data, it will
> > stopped and no old data will be fetched.
> 
> A similar problem here.  If you extend the API, don't build up only from the
> bottom.  Rather define from a higher POV.
> 
> If calling the drain trigger for capture stream doesn't make *any* sense for all
> hardware, then we shouldn't add it.  This would be the only reason not to do
> it.  What about empty streams...?
> 
> That being said, we need to define the proper abstraction, what this trigger
> does for which purpose, not specific to a certain hardware.
> That is, the requirement goes up from your hardware level while the design
> comes down from the abstraction level.

Right. I will include the capture.

My patch will be like:

@@ -1566,6 +1567,11 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
 			snd_pcm_post_stop(substream, new_state);
 		}
 	}
+
+	if (substream->runtime->trigger_master == substream &&
+		(substream->runtime->hw.info & SNDRV_PCM_INFO_DRAIN_TRIGGER))
+		return substream->ops->trigger(substream,
+				SNDRV_PCM_TRIGGER_DRAIN);
+
 	return 0;
 }

I moved the trigger at the end of the function. Or do you think we need trigger 
before snd_pcm_do_stop() for capture case based on your knowledge?

And in the snd_pcm_hw_refine():

@@ -420,7 +420,8 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 
 	hw = &substream->runtime->hw;
 	if (!params->info) {
-		params->info = hw->info & ~SNDRV_PCM_INFO_FIFO_IN_FRAMES;
+		params->info = hw->info & ~SNDRV_PCM_INFO_FIFO_IN_FRAMES
+			& ~SNDRV_PCM_INFO_DRAIN_TRIGGER;
 		if (!hw_support_mmap(substream))
 			params->info &= ~(SNDRV_PCM_INFO_MMAP |
 					  SNDRV_PCM_INFO_MMAP_VALID);

Do you think whether it is OK?

> 
> So, in general I found it good to have this kind of API extension.
> But let's add it a bit more carefully.
> 
> 
> thanks,
> 
> Takashi

Regards,
Libin