| Message ID | 1418837267-10896-1-git-send-email-timcussins@eml.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > Add notion of TSTAMP_CLASS (SYSTEM/AUDIO) as per Pierre's suggestion. > Add snd_pcm_start_at() > Do your implementation need to set specific start threshold to prevent the driver automatically start when you fill the buffer ? Do the driver allow to start when there is no data ?
Hi Raymond, Thanks for the feedback :) On 18/12/14 01:05, Raymond Yau wrote: > > > > Add notion of TSTAMP_CLASS (SYSTEM/AUDIO) as per Pierre's suggestion. > > Add snd_pcm_start_at() > > > > Do your implementation need to set specific start threshold to prevent > the driver automatically start when you fill the buffer ? > Do the driver allow to start when there is no data ? > It's the responsibility of the client to set the start threshold to a safe and responsible value. It might suit some applications to allow both threshold start _and_ start_at: My implementation doesn't preclude this. Cheers, Tim
Hi all, I posted this V2 patch a few weeks ago, it provoked a little chat but not much else :) It probably looked like dumped it on the list just before the holiday - that's what happened, and it was a pretty crap move. There was some pressure at this end, and it was all a bit awkward ;) Anyways, I know I didn't handle that the right way. So, in the spirit of a brighter 2015, I'm back to get start_at functionality merged! For fun and profit, etc. The second patch was much better than the first (naturally :P), but didn't feature too much in the way of documentation and explanation, which was a big misstep. If no-one has any major objections, I'll amend the V2 patch to include a clearer explanation of the implementation, how to use start_at, and why it should be merged with open arms! ;) If you have any further suggestions about how to get code like this merged, I'm all ears. Thanks, Tim On 17/12/14 17:27, Tim Cussins wrote: > Add notion of TSTAMP_CLASS (SYSTEM/AUDIO) as per Pierre's suggestion. > Add snd_pcm_start_at() > > Signed-off-by: Tim Cussins <timcussins@eml.cc> > > diff --git a/include/pcm.h b/include/pcm.h > index 0655e7f..c57edca 100644 > --- a/include/pcm.h > +++ b/include/pcm.h > @@ -323,13 +323,25 @@ typedef enum _snd_pcm_tstamp { > SND_PCM_TSTAMP_LAST = SND_PCM_TSTAMP_ENABLE > } snd_pcm_tstamp_t; > > +typedef enum _snd_pcm_tstamp_class { > + SND_PCM_TSTAMP_CLASS_SYSTEM = 0, > + SND_PCM_TSTAMP_CLASS_AUDIO, > + SND_PCM_TSTAMP_CLASS_LAST = SND_PCM_TSTAMP_CLASS_AUDIO, > +} snd_pcm_tstamp_class_t; > + > typedef enum _snd_pcm_tstamp_type { > SND_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0, /** gettimeofday equivalent */ > - SND_PCM_TSTAMP_TYPE_MONOTONIC, /** posix_clock_monotonic equivalent */ > + SND_PCM_TSTAMP_TYPE_MONOTONIC, /** posix_clock_monotonic equivalent */ > SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /** monotonic_raw (no NTP) */ > SND_PCM_TSTAMP_TYPE_LAST = SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW, > } snd_pcm_tstamp_type_t; > > +typedef enum _snd_pcm_audio_tstamp_type { > + SND_PCM_AUDIO_TSTAMP_TYPE_DEFAULT = 0, > + SND_PCM_AUDIO_TSTAMP_TYPE_LINK, > + SND_PCM_AUDIO_TSTAMP_TYPE_LAST = SND_PCM_AUDIO_TSTAMP_TYPE_LINK, > +} snd_pcm_audio_tstamp_t; > + > /** Unsigned frames quantity */ > typedef unsigned long snd_pcm_uframes_t; > /** Signed frames quantity */ > @@ -478,6 +490,7 @@ int snd_pcm_prepare(snd_pcm_t *pcm); > int snd_pcm_reset(snd_pcm_t *pcm); > int snd_pcm_status(snd_pcm_t *pcm, snd_pcm_status_t *status); > int snd_pcm_start(snd_pcm_t *pcm); > +int snd_pcm_start_at(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t* start_time); > int snd_pcm_drop(snd_pcm_t *pcm); > int snd_pcm_drain(snd_pcm_t *pcm); > int snd_pcm_pause(snd_pcm_t *pcm, int enable); > diff --git a/include/sound/asound.h b/include/sound/asound.h > index 1f23cd6..1592160 100644 > --- a/include/sound/asound.h > +++ b/include/sound/asound.h > @@ -466,12 +466,30 @@ struct snd_xfern { > }; > > enum { > + SNDRV_PCM_TSTAMP_CLASS_SYSTEM = 0, > + SNDRV_PCM_TSTAMP_CLASS_AUDIO, > + SNDRV_PCM_TSTAMP_CLASS_LAST = SNDRV_PCM_TSTAMP_CLASS_AUDIO, > +}; > + > +enum { > SNDRV_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0, /* gettimeofday equivalent */ > SNDRV_PCM_TSTAMP_TYPE_MONOTONIC, /* posix_clock_monotonic equivalent */ > - SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /* monotonic_raw (no NTP) */ > + SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /* monotonic_raw (no NTP) */ > SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, > }; > > +enum { > + SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT = 0, > + SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK, > + SNDRV_PCM_AUDIO_TSTAMP_TYPE_LAST = SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK, > +}; > + > +struct snd_start_at { > + int tstamp_class; > + int tstamp_type; > + struct timespec start_time; > +}; > + > /* channel positions */ > enum { > SNDRV_CHMAP_UNKNOWN = 0, > @@ -550,6 +568,8 @@ enum { > #define SNDRV_PCM_IOCTL_READN_FRAMES _IOR('A', 0x53, struct snd_xfern) > #define SNDRV_PCM_IOCTL_LINK _IOW('A', 0x60, int) > #define SNDRV_PCM_IOCTL_UNLINK _IO('A', 0x61) > +#define SNDRV_PCM_IOCTL_START_AT _IOW('A', 0x62, struct snd_start_at) > + > > /***************************************************************************** > * * > diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c > index baa47c7..40a4689 100644 > --- a/src/pcm/pcm.c > +++ b/src/pcm/pcm.c > @@ -1085,6 +1085,37 @@ int snd_pcm_start(snd_pcm_t *pcm) > } > > /** > + * \brief Start a PCM at a specified point in the future > + * \param pcm PCM handle > + * \param tstamp_class specifies the class of tstamp_type > + * \param tstamp_type specifies the clock with which to interpret \p start_time > + * \param start_time Absolute time at which to start the stream > + * \return 0 on success otherwise a negative error code > + * \retval -ENOSYS operation not supported for the current timestamp type > + * \retval -EINVAL timespec, tstamp_class or tstamp_type is invalid > + * \retval -ETIME requested start_time cannot be satisfied > + * > + * This method is non-blocking: It establishes an appropriate timer in the kernel > + * that will start the stream on expiry. > + * > + * The timer is unconditionally cancelled upon any \a attempt to change the stream > + * state e.g. drop, drain, start, start_at. > + */ > +int snd_pcm_start_at(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t *start_time) > +{ > + assert(pcm); > + assert(start_time); > + if (CHECK_SANITY(! pcm->setup)) { > + SNDMSG("PCM not set up"); > + return -EIO; > + } > + if (pcm->fast_ops->start_at) { > + return pcm->fast_ops->start_at(pcm->fast_op_arg, tstamp_class, tstamp_type, start_time); > + } > + return -EINVAL; > +} > + > +/** > * \brief Stop a PCM dropping pending frames > * \param pcm PCM handle > * \return 0 on success otherwise a negative error code > diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c > index c34b766..0190787 100644 > --- a/src/pcm/pcm_hw.c > +++ b/src/pcm/pcm_hw.c > @@ -620,6 +620,26 @@ static int snd_pcm_hw_start(snd_pcm_t *pcm) > return 0; > } > > +static int snd_pcm_hw_start_at(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t *start_time) > +{ > + snd_pcm_hw_t *hw = pcm->private_data; > + int err; > + > + struct snd_start_at start_at = { > + .tstamp_class = tstamp_class, > + .tstamp_type = tstamp_type, > + .start_time = *start_time > + }; > + > + sync_ptr(hw, 0); > + if (ioctl(hw->fd, SNDRV_PCM_IOCTL_START_AT, &start_at) < 0) { > + err = -errno; > + SYSMSG("SNDRV_PCM_IOCTL_START_AT failed (%i)", err); > + return err; > + } > + return 0; > +} > + > static int snd_pcm_hw_drop(snd_pcm_t *pcm) > { > snd_pcm_hw_t *hw = pcm->private_data; > @@ -1336,6 +1356,7 @@ static const snd_pcm_fast_ops_t snd_pcm_hw_fast_ops = { > .prepare = snd_pcm_hw_prepare, > .reset = snd_pcm_hw_reset, > .start = snd_pcm_hw_start, > + .start_at = snd_pcm_hw_start_at, > .drop = snd_pcm_hw_drop, > .drain = snd_pcm_hw_drain, > .pause = snd_pcm_hw_pause, > diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h > index 394505f..5cdfd3a 100644 > --- a/src/pcm/pcm_local.h > +++ b/src/pcm/pcm_local.h > @@ -154,6 +154,7 @@ typedef struct { > int (*prepare)(snd_pcm_t *pcm); > int (*reset)(snd_pcm_t *pcm); > int (*start)(snd_pcm_t *pcm); > + int (*start_at)(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t *start_time); > int (*drop)(snd_pcm_t *pcm); > int (*drain)(snd_pcm_t *pcm); > int (*pause)(snd_pcm_t *pcm, int enable); >
Takashi, Pierre, I didn't make it clear that I reworked the patch so that it was compatible with, but not dependent on, Pierre's work (viz: Timestamping Evolutions). In particular you'll see that I lifted snd_pcm_audio_tstamp_t from his patch: This means that our patches can be discussed/merged independently. Was this a reasonable approach? Thanks, Tim On 17/12/14 17:27, Tim Cussins wrote: > Add notion of TSTAMP_CLASS (SYSTEM/AUDIO) as per Pierre's suggestion. > Add snd_pcm_start_at() > > Signed-off-by: Tim Cussins <timcussins@eml.cc> > > diff --git a/include/pcm.h b/include/pcm.h > index 0655e7f..c57edca 100644 > --- a/include/pcm.h > +++ b/include/pcm.h > @@ -323,13 +323,25 @@ typedef enum _snd_pcm_tstamp { > SND_PCM_TSTAMP_LAST = SND_PCM_TSTAMP_ENABLE > } snd_pcm_tstamp_t; > > +typedef enum _snd_pcm_tstamp_class { > + SND_PCM_TSTAMP_CLASS_SYSTEM = 0, > + SND_PCM_TSTAMP_CLASS_AUDIO, > + SND_PCM_TSTAMP_CLASS_LAST = SND_PCM_TSTAMP_CLASS_AUDIO, > +} snd_pcm_tstamp_class_t; > + > typedef enum _snd_pcm_tstamp_type { > SND_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0, /** gettimeofday equivalent */ > - SND_PCM_TSTAMP_TYPE_MONOTONIC, /** posix_clock_monotonic equivalent */ > + SND_PCM_TSTAMP_TYPE_MONOTONIC, /** posix_clock_monotonic equivalent */ > SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /** monotonic_raw (no NTP) */ > SND_PCM_TSTAMP_TYPE_LAST = SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW, > } snd_pcm_tstamp_type_t; > > +typedef enum _snd_pcm_audio_tstamp_type { > + SND_PCM_AUDIO_TSTAMP_TYPE_DEFAULT = 0, > + SND_PCM_AUDIO_TSTAMP_TYPE_LINK, > + SND_PCM_AUDIO_TSTAMP_TYPE_LAST = SND_PCM_AUDIO_TSTAMP_TYPE_LINK, > +} snd_pcm_audio_tstamp_t; > + > /** Unsigned frames quantity */ > typedef unsigned long snd_pcm_uframes_t; > /** Signed frames quantity */ > @@ -478,6 +490,7 @@ int snd_pcm_prepare(snd_pcm_t *pcm); > int snd_pcm_reset(snd_pcm_t *pcm); > int snd_pcm_status(snd_pcm_t *pcm, snd_pcm_status_t *status); > int snd_pcm_start(snd_pcm_t *pcm); > +int snd_pcm_start_at(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t* start_time); > int snd_pcm_drop(snd_pcm_t *pcm); > int snd_pcm_drain(snd_pcm_t *pcm); > int snd_pcm_pause(snd_pcm_t *pcm, int enable); > diff --git a/include/sound/asound.h b/include/sound/asound.h > index 1f23cd6..1592160 100644 > --- a/include/sound/asound.h > +++ b/include/sound/asound.h > @@ -466,12 +466,30 @@ struct snd_xfern { > }; > > enum { > + SNDRV_PCM_TSTAMP_CLASS_SYSTEM = 0, > + SNDRV_PCM_TSTAMP_CLASS_AUDIO, > + SNDRV_PCM_TSTAMP_CLASS_LAST = SNDRV_PCM_TSTAMP_CLASS_AUDIO, > +}; > + > +enum { > SNDRV_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0, /* gettimeofday equivalent */ > SNDRV_PCM_TSTAMP_TYPE_MONOTONIC, /* posix_clock_monotonic equivalent */ > - SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /* monotonic_raw (no NTP) */ > + SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /* monotonic_raw (no NTP) */ > SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, > }; > > +enum { > + SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT = 0, > + SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK, > + SNDRV_PCM_AUDIO_TSTAMP_TYPE_LAST = SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK, > +}; > + > +struct snd_start_at { > + int tstamp_class; > + int tstamp_type; > + struct timespec start_time; > +}; > + > /* channel positions */ > enum { > SNDRV_CHMAP_UNKNOWN = 0, > @@ -550,6 +568,8 @@ enum { > #define SNDRV_PCM_IOCTL_READN_FRAMES _IOR('A', 0x53, struct snd_xfern) > #define SNDRV_PCM_IOCTL_LINK _IOW('A', 0x60, int) > #define SNDRV_PCM_IOCTL_UNLINK _IO('A', 0x61) > +#define SNDRV_PCM_IOCTL_START_AT _IOW('A', 0x62, struct snd_start_at) > + > > /***************************************************************************** > * * > diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c > index baa47c7..40a4689 100644 > --- a/src/pcm/pcm.c > +++ b/src/pcm/pcm.c > @@ -1085,6 +1085,37 @@ int snd_pcm_start(snd_pcm_t *pcm) > } > > /** > + * \brief Start a PCM at a specified point in the future > + * \param pcm PCM handle > + * \param tstamp_class specifies the class of tstamp_type > + * \param tstamp_type specifies the clock with which to interpret \p start_time > + * \param start_time Absolute time at which to start the stream > + * \return 0 on success otherwise a negative error code > + * \retval -ENOSYS operation not supported for the current timestamp type > + * \retval -EINVAL timespec, tstamp_class or tstamp_type is invalid > + * \retval -ETIME requested start_time cannot be satisfied > + * > + * This method is non-blocking: It establishes an appropriate timer in the kernel > + * that will start the stream on expiry. > + * > + * The timer is unconditionally cancelled upon any \a attempt to change the stream > + * state e.g. drop, drain, start, start_at. > + */ > +int snd_pcm_start_at(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t *start_time) > +{ > + assert(pcm); > + assert(start_time); > + if (CHECK_SANITY(! pcm->setup)) { > + SNDMSG("PCM not set up"); > + return -EIO; > + } > + if (pcm->fast_ops->start_at) { > + return pcm->fast_ops->start_at(pcm->fast_op_arg, tstamp_class, tstamp_type, start_time); > + } > + return -EINVAL; > +} > + > +/** > * \brief Stop a PCM dropping pending frames > * \param pcm PCM handle > * \return 0 on success otherwise a negative error code > diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c > index c34b766..0190787 100644 > --- a/src/pcm/pcm_hw.c > +++ b/src/pcm/pcm_hw.c > @@ -620,6 +620,26 @@ static int snd_pcm_hw_start(snd_pcm_t *pcm) > return 0; > } > > +static int snd_pcm_hw_start_at(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t *start_time) > +{ > + snd_pcm_hw_t *hw = pcm->private_data; > + int err; > + > + struct snd_start_at start_at = { > + .tstamp_class = tstamp_class, > + .tstamp_type = tstamp_type, > + .start_time = *start_time > + }; > + > + sync_ptr(hw, 0); > + if (ioctl(hw->fd, SNDRV_PCM_IOCTL_START_AT, &start_at) < 0) { > + err = -errno; > + SYSMSG("SNDRV_PCM_IOCTL_START_AT failed (%i)", err); > + return err; > + } > + return 0; > +} > + > static int snd_pcm_hw_drop(snd_pcm_t *pcm) > { > snd_pcm_hw_t *hw = pcm->private_data; > @@ -1336,6 +1356,7 @@ static const snd_pcm_fast_ops_t snd_pcm_hw_fast_ops = { > .prepare = snd_pcm_hw_prepare, > .reset = snd_pcm_hw_reset, > .start = snd_pcm_hw_start, > + .start_at = snd_pcm_hw_start_at, > .drop = snd_pcm_hw_drop, > .drain = snd_pcm_hw_drain, > .pause = snd_pcm_hw_pause, > diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h > index 394505f..5cdfd3a 100644 > --- a/src/pcm/pcm_local.h > +++ b/src/pcm/pcm_local.h > @@ -154,6 +154,7 @@ typedef struct { > int (*prepare)(snd_pcm_t *pcm); > int (*reset)(snd_pcm_t *pcm); > int (*start)(snd_pcm_t *pcm); > + int (*start_at)(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t *start_time); > int (*drop)(snd_pcm_t *pcm); > int (*drain)(snd_pcm_t *pcm); > int (*pause)(snd_pcm_t *pcm, int enable); >
>> Do your implementation need to set specific start threshold to prevent >> the driver automatically start when you fill the buffer ? > > >> Do the driver allow to start when there is no data ? >> > > It's the responsibility of the client to set the start threshold to a > safe and responsible value. > > It might suit some applications to allow both threshold start _and_ > start_at: My implementation doesn't preclude this. Now I am confused... My understanding was that this feature is similar to SSYNC in HDAudio, where everything is ready, buffers filled, DMAs armed, FIFOs filled and the start condition only opens the last gate at a specific time - possibly with multiple streams starting at the same time. If you add a condition on the start_threshold you really don't need any hardware-driven start, do you? Thanks, -Pierre
> I didn't make it clear that I reworked the patch so that it was > compatible with, but not dependent on, Pierre's work (viz: Timestamping > Evolutions). In particular you'll see that I lifted > snd_pcm_audio_tstamp_t from his patch: This means that our patches can > be discussed/merged independently. Was this a reasonable approach? Fine with me. There may be another dependency though, you probably need to provide INFO fields and the ability to query whether the hardware supports this functionality (which brings us back to the discussion on where to store those fields..)
On 06/01/15 14:42, Pierre-Louis Bossart wrote: > >>> Do your implementation need to set specific start threshold to prevent >>> the driver automatically start when you fill the buffer ? >> >> >>> Do the driver allow to start when there is no data ? >>> >> >> It's the responsibility of the client to set the start threshold to a >> safe and responsible value. >> >> It might suit some applications to allow both threshold start _and_ >> start_at: My implementation doesn't preclude this. > > Now I am confused... My understanding was that this feature is similar > to SSYNC in HDAudio, where everything is ready, buffers filled, DMAs > armed, FIFOs filled and the start condition only opens the last gate at > a specific time - possibly with multiple streams starting at the same > time. If you add a condition on the start_threshold you really don't > need any hardware-driven start, do you? What you've described is exactly what I had in mind, so we're still on the same page. I wanted to make it clear that my implementation of start_at doesn't *prevent* client code starting on a threshold *and* using start_at, even if it seems to us like a strange idea. Preventing the use of both requires us to show why it's never a useful idea, decide on policy (what do happens when client code tries to use both), and implement that policy. I'd rather just leave it as 'possible' :) > Thanks, > -Pierre > > >
On 06/01/15 14:46, Pierre-Louis Bossart wrote: > >> I didn't make it clear that I reworked the patch so that it was >> compatible with, but not dependent on, Pierre's work (viz: Timestamping >> Evolutions). In particular you'll see that I lifted >> snd_pcm_audio_tstamp_t from his patch: This means that our patches can >> be discussed/merged independently. Was this a reasonable approach? > > Fine with me. There may be another dependency though, you probably need > to provide INFO fields and the ability to query whether the hardware > supports this functionality (which brings us back to the discussion on > where to store those fields..) > Fair point :) But the patch, as it stands, doesn't depend on those fields.
>> >> >>>> Do your implementation need to set specific start threshold to prevent >>>> the driver automatically start when you fill the buffer ? >>> >>> >>> >>>> Do the driver allow to start when there is no data ? >>>> >>> >>> It's the responsibility of the client to set the start threshold to a >>> safe and responsible value. >>> >>> It might suit some applications to allow both threshold start _and_ >>> start_at: My implementation doesn't preclude this. >> >> >> Now I am confused... My understanding was that this feature is similar >> to SSYNC in HDAudio, where everything is ready, buffers filled, DMAs >> armed, FIFOs filled and the start condition only opens the last gate at >> a specific time - possibly with multiple streams starting at the same >> time. If you add a condition on the start_threshold you really don't >> need any hardware-driven start, do you? > > > What you've described is exactly what I had in mind, so we're still on the same page. > > I wanted to make it clear that my implementation of start_at doesn't *prevent* client code starting on a threshold *and* using start_at, even if it seems to us like a strange idea. http://www.alsa-project.org/alsa-doc/alsa-lib/pcm.html Handshake between application and library Do alsa lib assume all read/write must only call after calling snd_pcm_sw_params() ? It seem that proposed start_at() can only be call in SND_PCM_STATE_PREPARED and should fail when stream is already running Are there any mean to cancel the scheduled snd_pcm_start_at ? Seem there is no check when the application call snd_pcm_start_at() multiple times > > Preventing the use of both requires us to show why it's never a useful idea, decide on policy (what do happens when client code tries to use both), and implement that policy. I'd rather just leave it as 'possible' :at()
On 09/01/15 12:03, Raymond Yau wrote: > > >> > >> > >>>> Do your implementation need to set specific start threshold to prevent > >>>> the driver automatically start when you fill the buffer ? > >>> > >>> > >>> > >>>> Do the driver allow to start when there is no data ? > >>>> > >>> > >>> It's the responsibility of the client to set the start threshold to a > >>> safe and responsible value. > >>> > >>> It might suit some applications to allow both threshold start _and_ > >>> start_at: My implementation doesn't preclude this. > >> > >> > >> Now I am confused... My understanding was that this feature is similar > >> to SSYNC in HDAudio, where everything is ready, buffers filled, DMAs > >> armed, FIFOs filled and the start condition only opens the last gate at > >> a specific time - possibly with multiple streams starting at the same > >> time. If you add a condition on the start_threshold you really don't > >> need any hardware-driven start, do you? > > > > > > What you've described is exactly what I had in mind, so we're still > on the same page. > > > > I wanted to make it clear that my implementation of start_at doesn't > *prevent* client code starting on a threshold *and* using start_at, even > if it seems to us like a strange idea. > > http://www.alsa-project.org/alsa-doc/alsa-lib/pcm.html > > Handshake between application and library > > Do alsa lib assume all read/write must only call after calling > snd_pcm_sw_params() ? > > It seem that proposed start_at() can only be call in > SND_PCM_STATE_PREPARED and should fail when stream is already running This sounds ok to me - I followed Nick Stoughton's lead on this. > Are there any mean to cancel the scheduled snd_pcm_start_at ? There is no explicit mechanism in the proposed patch: The pending timer is cancelled if client code attempts to change the stream state. Would you like to see an explicit cancellation API? > Seem there is no check when the application call snd_pcm_start_at() > multiple times The code only allows _one_ start_at timer to be pending. When snd_pcm_start_at() is called, the pending timer (if any) is cancelled (atomically) before being replaced by the new timer. > > > > Preventing the use of both requires us to show why it's never a > useful idea, decide on policy (what do happens when client code tries to > use both), and implement that policy. I'd rather just leave it as > 'possible' :at() >
diff --git a/include/pcm.h b/include/pcm.h index 0655e7f..c57edca 100644 --- a/include/pcm.h +++ b/include/pcm.h @@ -323,13 +323,25 @@ typedef enum _snd_pcm_tstamp { SND_PCM_TSTAMP_LAST = SND_PCM_TSTAMP_ENABLE } snd_pcm_tstamp_t; +typedef enum _snd_pcm_tstamp_class { + SND_PCM_TSTAMP_CLASS_SYSTEM = 0, + SND_PCM_TSTAMP_CLASS_AUDIO, + SND_PCM_TSTAMP_CLASS_LAST = SND_PCM_TSTAMP_CLASS_AUDIO, +} snd_pcm_tstamp_class_t; + typedef enum _snd_pcm_tstamp_type { SND_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0, /** gettimeofday equivalent */ - SND_PCM_TSTAMP_TYPE_MONOTONIC, /** posix_clock_monotonic equivalent */ + SND_PCM_TSTAMP_TYPE_MONOTONIC, /** posix_clock_monotonic equivalent */ SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /** monotonic_raw (no NTP) */ SND_PCM_TSTAMP_TYPE_LAST = SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW, } snd_pcm_tstamp_type_t; +typedef enum _snd_pcm_audio_tstamp_type { + SND_PCM_AUDIO_TSTAMP_TYPE_DEFAULT = 0, + SND_PCM_AUDIO_TSTAMP_TYPE_LINK, + SND_PCM_AUDIO_TSTAMP_TYPE_LAST = SND_PCM_AUDIO_TSTAMP_TYPE_LINK, +} snd_pcm_audio_tstamp_t; + /** Unsigned frames quantity */ typedef unsigned long snd_pcm_uframes_t; /** Signed frames quantity */ @@ -478,6 +490,7 @@ int snd_pcm_prepare(snd_pcm_t *pcm); int snd_pcm_reset(snd_pcm_t *pcm); int snd_pcm_status(snd_pcm_t *pcm, snd_pcm_status_t *status); int snd_pcm_start(snd_pcm_t *pcm); +int snd_pcm_start_at(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t* start_time); int snd_pcm_drop(snd_pcm_t *pcm); int snd_pcm_drain(snd_pcm_t *pcm); int snd_pcm_pause(snd_pcm_t *pcm, int enable); diff --git a/include/sound/asound.h b/include/sound/asound.h index 1f23cd6..1592160 100644 --- a/include/sound/asound.h +++ b/include/sound/asound.h @@ -466,12 +466,30 @@ struct snd_xfern { }; enum { + SNDRV_PCM_TSTAMP_CLASS_SYSTEM = 0, + SNDRV_PCM_TSTAMP_CLASS_AUDIO, + SNDRV_PCM_TSTAMP_CLASS_LAST = SNDRV_PCM_TSTAMP_CLASS_AUDIO, +}; + +enum { SNDRV_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0, /* gettimeofday equivalent */ SNDRV_PCM_TSTAMP_TYPE_MONOTONIC, /* posix_clock_monotonic equivalent */ - SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /* monotonic_raw (no NTP) */ + SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /* monotonic_raw (no NTP) */ SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, }; +enum { + SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT = 0, + SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK, + SNDRV_PCM_AUDIO_TSTAMP_TYPE_LAST = SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK, +}; + +struct snd_start_at { + int tstamp_class; + int tstamp_type; + struct timespec start_time; +}; + /* channel positions */ enum { SNDRV_CHMAP_UNKNOWN = 0, @@ -550,6 +568,8 @@ enum { #define SNDRV_PCM_IOCTL_READN_FRAMES _IOR('A', 0x53, struct snd_xfern) #define SNDRV_PCM_IOCTL_LINK _IOW('A', 0x60, int) #define SNDRV_PCM_IOCTL_UNLINK _IO('A', 0x61) +#define SNDRV_PCM_IOCTL_START_AT _IOW('A', 0x62, struct snd_start_at) + /***************************************************************************** * * diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index baa47c7..40a4689 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -1085,6 +1085,37 @@ int snd_pcm_start(snd_pcm_t *pcm) } /** + * \brief Start a PCM at a specified point in the future + * \param pcm PCM handle + * \param tstamp_class specifies the class of tstamp_type + * \param tstamp_type specifies the clock with which to interpret \p start_time + * \param start_time Absolute time at which to start the stream + * \return 0 on success otherwise a negative error code + * \retval -ENOSYS operation not supported for the current timestamp type + * \retval -EINVAL timespec, tstamp_class or tstamp_type is invalid + * \retval -ETIME requested start_time cannot be satisfied + * + * This method is non-blocking: It establishes an appropriate timer in the kernel + * that will start the stream on expiry. + * + * The timer is unconditionally cancelled upon any \a attempt to change the stream + * state e.g. drop, drain, start, start_at. + */ +int snd_pcm_start_at(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t *start_time) +{ + assert(pcm); + assert(start_time); + if (CHECK_SANITY(! pcm->setup)) { + SNDMSG("PCM not set up"); + return -EIO; + } + if (pcm->fast_ops->start_at) { + return pcm->fast_ops->start_at(pcm->fast_op_arg, tstamp_class, tstamp_type, start_time); + } + return -EINVAL; +} + +/** * \brief Stop a PCM dropping pending frames * \param pcm PCM handle * \return 0 on success otherwise a negative error code diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c index c34b766..0190787 100644 --- a/src/pcm/pcm_hw.c +++ b/src/pcm/pcm_hw.c @@ -620,6 +620,26 @@ static int snd_pcm_hw_start(snd_pcm_t *pcm) return 0; } +static int snd_pcm_hw_start_at(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t *start_time) +{ + snd_pcm_hw_t *hw = pcm->private_data; + int err; + + struct snd_start_at start_at = { + .tstamp_class = tstamp_class, + .tstamp_type = tstamp_type, + .start_time = *start_time + }; + + sync_ptr(hw, 0); + if (ioctl(hw->fd, SNDRV_PCM_IOCTL_START_AT, &start_at) < 0) { + err = -errno; + SYSMSG("SNDRV_PCM_IOCTL_START_AT failed (%i)", err); + return err; + } + return 0; +} + static int snd_pcm_hw_drop(snd_pcm_t *pcm) { snd_pcm_hw_t *hw = pcm->private_data; @@ -1336,6 +1356,7 @@ static const snd_pcm_fast_ops_t snd_pcm_hw_fast_ops = { .prepare = snd_pcm_hw_prepare, .reset = snd_pcm_hw_reset, .start = snd_pcm_hw_start, + .start_at = snd_pcm_hw_start_at, .drop = snd_pcm_hw_drop, .drain = snd_pcm_hw_drain, .pause = snd_pcm_hw_pause, diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 394505f..5cdfd3a 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -154,6 +154,7 @@ typedef struct { int (*prepare)(snd_pcm_t *pcm); int (*reset)(snd_pcm_t *pcm); int (*start)(snd_pcm_t *pcm); + int (*start_at)(snd_pcm_t *pcm, snd_pcm_tstamp_class_t tstamp_class, int tstamp_type, const snd_htimestamp_t *start_time); int (*drop)(snd_pcm_t *pcm); int (*drain)(snd_pcm_t *pcm); int (*pause)(snd_pcm_t *pcm, int enable);
Add notion of TSTAMP_CLASS (SYSTEM/AUDIO) as per Pierre's suggestion. Add snd_pcm_start_at() Signed-off-by: Tim Cussins <timcussins@eml.cc>