[alsa-devel] [PATCH v3 kernel 1/3] snd_pcm_start_at and friends.
Tim Cussins
timcussins at eml.cc
Wed Feb 11 10:34:39 CET 2015
Hi Nick, Pierre,
On 10/02/15 20:37, Pierre-Louis Bossart wrote:
> 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.
First off, sorry for leaving MONOTONIC_RAW out of the picture :) I did
this because Nick's original patch required changes to kernel code
outside ALSA, and I wanted to keep the discussion simple to begin with.
So, ok, I'll add support for this :)
Pierre seems right though: while MONOTONIC_RAW represents the source of
audio clocks for *some* systems, but it's probably not generally true.
Although embedded systems might use the a single oscillator for the SoC
and audio hardware, I imagine most sound cards have their own
oscillators. And, for example, our embedded system will have a separate
audio oscillator.
>
>>
>> *______________________________*
>> *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 at linux.intel.com
>> <mailto:pierre-louis.bossart at 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 at 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 at alsa-project.org <mailto:Alsa-devel at alsa-project.org>
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>
>>
>
More information about the Alsa-devel
mailing list