From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pierre-Louis Bossart Subject: Re: [PATCH RFC 2/9] ALSA: core: allow for trigger_tstamp snapshot in .trigger Date: Wed, 10 Dec 2014 11:22:19 -0600 Message-ID: <5488814B.5030406@linux.intel.com> References: <1418077426-8309-1-git-send-email-pierre-louis.bossart@linux.intel.com> <1418077426-8309-3-git-send-email-pierre-louis.bossart@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by alsa0.perex.cz (Postfix) with ESMTP id A7663260568 for ; Wed, 10 Dec 2014 18:22:58 +0100 (CET) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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 >> --- >> 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. > >> 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()? > > thanks, > > Takashi > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >