Re: [PATCH v3 kernel 1/3] snd_pcm_start_at and friends.

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 2/10/15 2:21 PM, Nick Stoughton wrote:
> The most important clock is MONOTONIC_RAW, from which the audio clocks
> are ultimately derived. Any system clock that is adjusted by NTP has the
> problem that Pierre-Louis describes. My implementation allowed for other
> clocks, but my actual use-case only uses MONOTONIC_RAW, and we are
> achieving very high accuracy using it.
>
> The primary driver (at least for me) for snd_pcm_start_at() is to be
> able to have two separate devices (which are in the background
> exchanging information about their respective CLOCK_MONOTONIC_RAW
> counters) start playing the same audio at the same time. Therefore, we
> need a clock that runs at a constant frequency and from which the I2S
> bit clock is derived.

Sounds like a very specific case. You *may* derive your MONOTONIC_RAW 
counters and bitclocks from the same osc in your hardware, but that's 
not typically how things work. There are quite a few systems where the 
bitclock or wallclock isn't aligned with the system time reported with 
MONOTONIC_RAW, and you really need to track sample/bitclock/wallclock 
counters to track the drift which by nature differs between devices.


>
> *______________________________*
> *Nick Stoughton*
> *Aether Things Inc *
> **San Francisco**
> +1 (510) 388 1413
>
> On Fri, Feb 6, 2015 at 1:02 PM, Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com
> <mailto:pierre-louis.bossart@linux.intel.com>> wrote:
>
>     On 02/06/2015 11:08 AM, Tim Cussins wrote:
>     > Hi Pierre,
>     >
>     > You got here quick!
>
>     That's what 6am flights do to you...
>
>      >
>      > On 06/02/15 16:32, Pierre-Louis Bossart wrote:
>      >> On 02/06/2015 10:16 AM, Tim Cussins wrote:
>      >>> We introduce the kernel-side of the START_AT ioctl.
>      >>>
>      >>> struct runtime is updated to hold information about the currently
>      >>> active start_at timer, if any. This facilitates cancellation via
>      >>> snd_pcm_start_at_abort(), and querying via snd_pcm_status().
>      >>>
>      >>> struct snd_start_at holds a startat operation and its arguments.
>      >>>
>      >>> Signed-off-by: Tim Cussins <timcussins@eml.cc>
>      >>>
>      >>> diff --git a/include/uapi/sound/asound.h
>     b/include/uapi/sound/asound.h
>      >>> index 0e88e7a..2943e1a 100644
>      >>> --- a/include/uapi/sound/asound.h
>      >>> +++ b/include/uapi/sound/asound.h
>      >>> @@ -421,7 +421,10 @@ struct snd_pcm_status {
>      >>>       snd_pcm_state_t suspended_state; /* suspended stream state */
>      >>>       __u32 reserved_alignment;    /* must be filled with zero */
>      >>>       struct timespec audio_tstamp;    /* from sample counter
>     or wall clock */
>      >>> -    unsigned char reserved[56-sizeof(struct timespec)]; /*
>     must be filled with zero */
>      >>> +    int startat_pending;        /* 1 if a start_at timer is
>     pending, 0 otherwise */
>      >>> +    int startat_clock_type;        /* start_at clock type, if
>     pending */
>      >>> +    struct timespec startat_start_time;    /* start_at start
>     time, if pending */
>      >>> +    unsigned char reserved[48-(2*sizeof(struct timespec))]; /*
>     must be filled with zero */
>      >>>   };
>      >>>
>      >>>   struct snd_pcm_mmap_status {
>      >>> @@ -473,6 +476,34 @@ enum {
>      >>>       SNDRV_PCM_TSTAMP_TYPE_LAST =
>     SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW,
>      >>>   };
>      >>>
>      >>> +enum {
>      >>> +    SNDRV_PCM_STARTAT_OP_SET = 0,
>      >>> +    SNDRV_PCM_STARTAT_OP_CANCEL,
>      >>> +    SNDRV_PCM_STARTAT_OP_STATUS,
>      >>> +    SNDRV_PCM_STARTAT_OP_LAST = SNDRV_PCM_STARTAT_OP_STATUS,
>      >>> +};
>      >>> +
>      >>> +enum {
>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY = 0,
>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC,
>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LAST =
>     SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>      >>> +};
>      >>
>      >> Looks like you went back to the original design with all clocks
>     mixed. I think it's a better idea to split system timers and audio
>     clocks.
>      >> And you are missing MONOTONIC_RAW (no NTP corrections).
>      >
>      > It's not /quite/ the original design. The start_at API now has
>     its own enum of clock types, which are separate from, but
>     semantically the same as, timestamping clock types.
>      >
>      > For the implementation of snd_pcm_start_at, I genuinely prefer
>     the unified approach. From the point of view of a /client/ of
>     snd_pcm_start_at, system and audio clocks are merely different
>     points in time with no other distinguishing characteristics.
>     Directing users to 2 different tables of clocks could be considered
>     obtuse, if not Kafkaesque :P
>      >
>      > Ok, so maybe that's putting it a bit too strong :D Let see if I
>     can see it your way: I can see two reasons for having 2 categories
>     of clocks.
>      >
>      > (1) Because the underlying implementation of snd_pcm_start_at (or
>     some other API) is different.
>      > (2) Because you want to gather pairs of timestamps
>     (a_system_timestamp, an_audio_timestamp) in order to estimate clock
>     drift.
>      >
>      > I think we'd agree that (1) is not a strong justification: We
>     shouldn't allow implementation details to propagate into the API
>     without good cause.
>      >
>      > The second point is interesting, but doesn't seem to preclude
>     having an dedicated clock type enum for use with snd_pcm_start_at.
>      >
>      > I guess what I'm arguing here is that the timestamping evolutions
>     and start_at aren't /required/ to use the same clock type enums, and
>     it seems more sensible to me that start_at has it's own, unified,
>     clock type enum.
>      >
>      > I hope I haven't mischaracterised your view on split enums. Hit
>     me back with some corrections.
>      >
>      > More importantly, I hope I haven't stepped on your toes too much
>     - I really value your efforts and feedback on this stuff!
>
>     I understand the logic of making things unified to specify the
>     start, but the audio or link clocks are really different in nature
>     from the system ones. I guess my reaction really comes from the
>     number of times i've had to explain that a system timer can't
>     control audio playback unless the same oscillator is used. Pretty
>     basic but not everyone gets it. It'd also be weird to define a clock
>     type to specify the start time and use a second one to know the time
>     once the stream has started.
>
>      >
>      >>
>      >>> + woul
>      >>> +struct snd_start_at {
>      >>> +    int op;                /* startat operation to be performed */
>      >>> +    union {                /* fields for setting a startat
>     timer */
>      >>> +        struct {
>      >>> +            int clock_type;            /* clock type e.g.
>     SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY */
>      >>> +            struct timespec start_time;    /* start time */
>      >>> +        } set;
>      >>> +        struct {
>      >>> +            int clock_type;
>      >>> +            struct timespec current_time;
>      >>> +        } status;
>      >>> +    } args;
>      >>> +};
>      >>> +
>      >>>   /* channel positions */
>      >>>   enum {
>      >>>       SNDRV_CHMAP_UNKNOWN = 0,
>      >>> @@ -551,6 +582,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)
>      >>> +
>      >>>
>      >>>
>       /*****************************************************************************
>      >>>    *
>                     *
>      >>>
>      >>
>      >
>
>     _______________________________________________
>     Alsa-devel mailing list
>     Alsa-devel@alsa-project.org <mailto:Alsa-devel@alsa-project.org>
>     http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
>