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

[Takashi Iwai]

At Tue, 30 Dec 2014 08:42:28 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> Regards,
> Libin
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Thursday, December 25, 2014 6:11 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 Mon, 22 Dec 2014 07:42:38 +0000,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > > > > > 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?
> > 
> > The place would be OK, but you can't call it unconditionally there.
> > In some cases (e.g. XRUN), the trigger won't be called at all.
> > The trigger state change is a bit tricky, so be careful to track the state change
> > there.
> > 
> 
> Right. What do you think the following code?
> 
> @@ -1518,6 +1519,8 @@ static int snd_pcm_pre_drain_init(struct snd_pcm_substream *substream, int state
>  static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
> +	int trigger = 0;
> +
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>  		switch (runtime->status->state) {
>  		case SNDRV_PCM_STATE_PREPARED:
> @@ -1525,10 +1528,12 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
>  			if (! snd_pcm_playback_empty(substream)) {
>  				snd_pcm_do_start(substream, SNDRV_PCM_STATE_DRAINING);
>  				snd_pcm_post_start(substream, SNDRV_PCM_STATE_DRAINING);
> +				trigger = 1;
>  			}
>  			break;
>  		case SNDRV_PCM_STATE_RUNNING:
>  			runtime->status->state = SNDRV_PCM_STATE_DRAINING;
> +			trigger = 1;
>  			break;
>  		case SNDRV_PCM_STATE_XRUN:
>  			runtime->status->state = SNDRV_PCM_STATE_SETUP;
> @@ -1545,6 +1550,12 @@ 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 && trigger &&
> +		(substream->runtime->hw.info & SNDRV_PCM_INFO_DRAIN_TRIGGER))
> +		return substream->ops->trigger(substream,
> +				SNDRV_PCM_TRIGGER_DRAIN);
> +

Better to use bool in such a case.
In anyway, an easier way would be to check like

	if (runtime->status->state == SNDRV_PCM_STATE_DRAINING &&
	    runtime->trigger_master == substream &&
	    (runtime->hw.info & SNDRV_PCM_INFO_DRAIN_TRIGGER))


Takashi