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@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);elseruntime->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