Re: [RFC] ALSA: provide a simple mechanism to start playback at a given time

[Takashi Iwai <tiwai@suse.de>]

At Thu, 16 Oct 2014 11:53:07 +0100,
Tim Cussins wrote:
> 
> Hi again Nick!
> 
> On 14/10/14 09:29, Nick Stoughton wrote:
> > Initial implementation / Request For Comment.
> >
> > Given an absolute time based on a given clock (CLOCK_MONOTONIC,
> > CLOCK_MONOTONIC_RAW, CLOCK_REALTIME etc), setup a high resolution timer
> > to cause playback to be triggered at that time.
> > ---
> >   include/linux/timekeeping.h |  3 ++
> >   include/uapi/sound/asound.h |  6 +++
> >   kernel/time/timekeeping.c   | 16 ++++++++
> >   sound/core/pcm_native.c     | 99 +++++++++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 124 insertions(+)
> >
> > diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> > index 1caa6b0..74eb6b2 100644
> > --- a/include/linux/timekeeping.h
> > +++ b/include/linux/timekeeping.h
> > @@ -32,6 +32,9 @@ extern void ktime_get_ts64(struct timespec64 *ts);
> >   extern int __getnstimeofday64(struct timespec64 *tv);
> >   extern void getnstimeofday64(struct timespec64 *tv);
> >
> > +/*  Get the offset between monotonic and monotonic raw clocks */
> > +extern ktime_t ktime_get_raw_offset(void);
> > +
> >   #if BITS_PER_LONG == 64
> >   static inline int __getnstimeofday(struct timespec *ts)
> >   {
> > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> > index 6ee5867..c7aa88c 100644
> > --- a/include/uapi/sound/asound.h
> > +++ b/include/uapi/sound/asound.h
> > @@ -462,6 +462,11 @@ struct snd_xfern {
> >   	snd_pcm_uframes_t frames;
> >   };
> >
> > +struct snd_clock_time {
> > +        clockid_t       clock;
> > +        struct timespec time;
> > +};
> > +
> 
> Takashi added snd_pcm_tstamp_type_t in July - If the final start_at 
> patch includes some "struct snd_clock_time", then it'd probably use 
> Takashi's type in favour of a raw clockid_t.

Yeah, that would make things more consistent.  But...

> I like the idea of a single struct that includes the context for the 
> timespec btw. Cool.

... thinking of this again, can we drop specifying the type here but
assumes to be same as the pre-given timestamp type?  Then we'd just
need to pass timespec to ioctl.

And, if we introduce a new PTP or DEVICE_SPECIFIC timestamp type as
we've discussed, these need to provide the wakeup / callback at the
programmed time.  In the latter case, the device driver should provide
it (hmm, so the start_at() would end up calling the callback?)  This
needs to be sorted out a bit better...

> >   enum {
> >   	SNDRV_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0,	/* gettimeofday equivalent */
> >   	SNDRV_PCM_TSTAMP_TYPE_MONOTONIC,	/* posix_clock_monotonic equivalent */
> > @@ -547,6 +552,7 @@ 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	_IOR('A', 0x62, struct snd_clock_time)
> >
> >   /*****************************************************************************
> >    *                                                                           *
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index ec1791f..3d5d0bc 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -789,6 +789,22 @@ error: /* even if we error out, we forwarded the time, so call update */
> >   }
> >   EXPORT_SYMBOL(timekeeping_inject_offset);
> >
> > +/**
> > + * ktime_get_raw_offset - get the offset in ktime format between
> > + * the monotonic_raw clock and the monotonic clock
> > + */
> > +ktime_t ktime_get_raw_offset(void)
> > +{
> > +        struct timespec rawtime = {0,0};
> > +        struct timespec now = {0,0};
> > +        struct timespec delta;
> > +
> > +        ktime_get_ts(&now);
> > +        getrawmonotonic(&rawtime);
> > +        delta = timespec_sub(now, rawtime);
> > +        return timespec_to_ktime(delta);
> > +}
> > +EXPORT_SYMBOL(ktime_get_raw_offset);

This part needs a review and comment from Thomas and others.
We can have this code snippet even in the PCM code without disturbing
the core side.


> >    * timekeeping_get_tai_offset - Returns current TAI offset from UTC
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > index 85fe1a2..1121127 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -27,6 +27,7 @@
> >   #include <linux/pm_qos.h>
> >   #include <linux/aio.h>
> >   #include <linux/dma-mapping.h>
> > +#include <linux/time.h>
> >   #include <sound/core.h>
> >   #include <sound/control.h>
> >   #include <sound/info.h>
> > @@ -38,6 +39,9 @@
> >   #if defined(CONFIG_MIPS) && defined(CONFIG_DMA_NONCOHERENT)
> >   #include <dma-coherence.h>
> >   #endif
> > +#if defined(CONFIG_HIGH_RES_TIMERS)
> > +#include <linux/hrtimer.h>
> > +#endif
> >
> >   /*
> >    *  Compatibility
> > @@ -1016,6 +1020,99 @@ static struct action_ops snd_pcm_action_start = {
> >   	.post_action = snd_pcm_post_start
> >   };
> >
> > +#ifdef CONFIG_HIGH_RES_TIMERS
> > +/*
> > + * hrtimer interface
> > + */
> > +
> > +struct hrtimer_pcm {
> > +	struct hrtimer timer;
> > +	struct snd_pcm_substream *substream;
> > +};
> > +
> > +/*
> > + * called from a hard irq context - no need for locks.
> > + * only problem is that the caller might have gone away and closed the substream
> > + * before the timer expires.
> > + */
> > +enum hrtimer_restart snd_pcm_do_start_time(struct hrtimer *timer)
> > +{
> > +        struct hrtimer_pcm *pcm_timer;
> > +	struct snd_pcm_substream *substream;
> > +        int ret;
> > +
> > +        pcm_timer = container_of(timer, struct hrtimer_pcm, timer);
> > +        substream = pcm_timer->substream;
> > +
> > +        if (substream->runtime) {
> > +                ret = snd_pcm_do_start(substream, SNDRV_PCM_STATE_RUNNING);
> > +                if (ret == 0) {
> > +                        snd_pcm_post_start(substream, SNDRV_PCM_STATE_RUNNING);
> > +                }
> > +        }
> > +	kfree(pcm_timer);
> > +
> > +        return HRTIMER_NORESTART;
> > +}
> > +
> > +int snd_pcm_start_at(struct snd_pcm_substream *substream,
> > +        struct snd_clock_time __user *_start_time)
> > +{
> > +        struct hrtimer_pcm *pcm_timer;
> > +        struct snd_clock_time start_time;
> > +        int clock;
> > +        ktime_t start_kt;
> > +        ktime_t raw_offset;
> > +        int ret;
> > +
> > +	if (copy_from_user(&start_time, _start_time, sizeof(start_time)))
> > +		return -EFAULT;
> > +
> > +        if (!timespec_valid(&start_time.time))
> > +		return -EINVAL;
> > +
> > +        start_kt = timespec_to_ktime(start_time.time);
> > +        clock = start_time.clock;
> > +        if (start_time.clock == CLOCK_MONOTONIC_RAW) {
> > +                raw_offset = ktime_get_raw_offset();
> > +
> > +                /* convert to clock monotonic */
> > +                start_kt = ktime_add(raw_offset, start_kt);
> > +                clock = CLOCK_MONOTONIC;
> > +        }
> > +
> > +        /*  if not playback substream or less than 100 ms give up */

Is there any technical reason to give up for over longer schedule?


> > +	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK ||
> > +           (ktime_compare(start_kt, ktime_add_ns(ktime_get(), 100000000)) <= 0)) {
> > +		return snd_pcm_action_lock_irq(&snd_pcm_action_start,
> > +                        substream, SNDRV_PCM_STATE_RUNNING);
> > +        }
> > +
> > +        ret = snd_pcm_pre_start(substream, SNDRV_PCM_STATE_PREPARED);
> > +        if (ret != 0) {
> > +                return ret;
> > +        }
> > +
> > +        pcm_timer = kmalloc(sizeof(*pcm_timer), GFP_KERNEL);
> > +        hrtimer_init(&pcm_timer->timer, clock, HRTIMER_MODE_ABS);
> > +
> > +        pcm_timer->timer.function = snd_pcm_do_start_time;
> > +        pcm_timer->substream = substream;
> > +
> > +	hrtimer_start(&pcm_timer->timer, start_kt, HRTIMER_MODE_ABS);
> > +
> > +	return 0;
> > +}
> > +#else
> > +/* without high res time, interface is identical to snd_pcm_start() */
> > +int snd_pcm_start_at(struct snd_pcm_substream *substream,
> > +        struct snd_clock_time __user *_start_time)
> > +{
> > +        return snd_pcm_action_lock_irq(&snd_pcm_action_start,
> > +                substream, SNDRV_PCM_STATE_RUNNING);
> > +}
> > +#endif
> > +
> 
> I'd prefer to return an error code when snd_pcm_start_at cannot be 
> supported, rather than lying to userspace that it will be satisfied :)
> 
> There might be a number of reasons that snd_pcm_start_at cannot be 
> satisfied: unsupported clock-type, timespec in the past, incompatible 
> kernel config are just three.
> 
> While one might argue that start_at is a "best effort" mechanism, on 
> principle the function should make every effort to return an error when 
> it would be meaningful (read: handleable) by userspace. Not sure how you 
> feel about that ... :)

Yeah, I second that, too.  (BTW, the function should be static.)


thanks,

Takashi