[alsa-devel] [PATCH RFC 2/9] ALSA: core: allow for trigger_tstamp snapshot in .trigger

[Takashi Iwai]

At Wed, 10 Dec 2014 11:22:19 -0600,
Pierre-Louis Bossart wrote:
> 
> Thanks for the review.
> 
> On 12/10/14, 10:31 AM, Takashi Iwai wrote:
> > At Mon,  8 Dec 2014 16:23:39 -0600,
> > Pierre-Louis Bossart wrote:
> >>
> >> Don't use generic snapshot of trigger_tstamp if low-level driver or
> >> hardware can get a more precise value for better audio/system time
> >> synchronization.
> >>
> >> Also add definitions for delayed updates if actual trigger tstamp
> >> can be only be provided after a delay due to hardware constraints.
> >>
> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> >> ---
> >>   include/sound/pcm.h     | 2 ++
> >>   sound/core/pcm_native.c | 6 +++++-
> >>   2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> >> index 1e7f74a..83c669f 100644
> >> --- a/include/sound/pcm.h
> >> +++ b/include/sound/pcm.h
> >> @@ -281,6 +281,8 @@ struct snd_pcm_runtime {
> >>   	/* -- Status -- */
> >>   	struct snd_pcm_substream *trigger_master;
> >>   	struct timespec trigger_tstamp;	/* trigger timestamp */
> >> +	int trigger_tstamp_latched;     /* trigger timestamp latched in low-level driver/hardware */
> >
> > Better to use bool nowadays.
> 
> ok
> 
> >
> >> +	int trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */
> >
> > This isn't used at all in this patch.  I found it being used in the
> > later usb-audio patch.  If it's the only place, can't it be rather put
> > locally to usb-audio object instead of the common pcm runtime?
> 
> It's not limited to USB. We have upcoming hardware where the 
> trigger_tstamp will only be determined with a delay due to IPC. USB is 
> just an example of a common pattern where the trigger_tstamp will be 
> known for sure after a couple of ms.

Well, the question is whether this *must* be put in the common place.
In other words, will this field be referred in the common PCM code?
If not, it should be recorded rather locally.

> 
> >
> >>   	int overrange;
> >>   	snd_pcm_uframes_t avail_max;
> >>   	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> >> index 5dc83fb..37a7137 100644
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> @@ -806,10 +806,14 @@ static int snd_pcm_channel_info_user(struct snd_pcm_substream *substream,
> >>   static void snd_pcm_trigger_tstamp(struct snd_pcm_substream *substream)
> >>   {
> >>   	struct snd_pcm_runtime *runtime = substream->runtime;
> >> +
> >>   	if (runtime->trigger_master == NULL)
> >>   		return;
> >>   	if (runtime->trigger_master == substream) {
> >> -		snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> >> +		if (runtime->trigger_tstamp_latched == 0)
> >> +			snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> >> +		else
> >> +			runtime->trigger_tstamp_latched = 0;
> >
> > IMO, it's better to clear the flat at the beginning of PCM trigger
> > commonly.  Looking at the later patch, you clear in each driver's
> > callback.  This should be moved into the common place.
> 
> I must admit I don't really understand the logic of all the _pre and 
> _post operations. Did you mean clearing this field in snd_pcm_do_start()?

Yes, either snd_pcm_do_start() and snd_pcm_pre_start().  There aren't
much difference in this case.


Takashi