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

[Takashi Iwai]

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.

> 
> 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?

Yes.


thanks,

Takashi