| Message ID | 1418077426-8309-3-git-send-email-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + 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? > 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. thanks, Takashi
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. > >> 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 >
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); > >> + 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
>>>> + 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. well i wasn't sure of what to do here: 1. we can leave the low-lever driver update the trigger_tstamp on its own, and then this field is not needed at the core level, but the core or the application will not be notified that an update is pending 2. or we use this field to let the core know, and possibly let userspace know that the trigger will be updated shortly. However, I didn't find an existing mechanism to notify userspace that the new trigger_tstamp is dirty and has changed so the use of this field is indeed incomplete for now. I could go either way. >>> >>> 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. ok, will cleanup.
At Wed, 10 Dec 2014 12:43:38 -0600, Pierre-Louis Bossart wrote: > > > >>>> + 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. > > well i wasn't sure of what to do here: > 1. we can leave the low-lever driver update the trigger_tstamp on its > own, and then this field is not needed at the core level, but the core > or the application will not be notified that an update is pending > 2. or we use this field to let the core know, and possibly let userspace > know that the trigger will be updated shortly. However, I didn't find an > existing mechanism to notify userspace that the new trigger_tstamp is > dirty and has changed so the use of this field is indeed incomplete for now. We're extending the status struct in anyway, so can we put a (bit) flag somewhere indicating the behavior? Takashi
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 */ + int trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */ 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; } else { snd_pcm_trigger_tstamp(runtime->trigger_master); runtime->trigger_tstamp = runtime->trigger_master->runtime->trigger_tstamp;
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(-)