[alsa-devel] [PATCH 3/3] ALSA: pcm: Add timestamp type to sw_params

[Takashi Iwai]

At Thu, 10 Jul 2014 10:23:37 -0500,
Pierre-Louis Bossart wrote:
> 
> On 7/10/14, 8:04 AM, Takashi Iwai wrote:
> > For allowing adjusting the timestamp type on the fly, add it to
> > sw_params.  The existing ioctl is still kept for compatibility.
> 
> I have strong objections to this extension. It will result in a 
> discontinuity in the timestamps reported in pcm_status without a clear 
> indication of what timestamp is reported (when does this change occur?), 
> and it's completely unclear how userspace would handle a step (positive 
> or negative) between ntp-adjusted and non-ntp-adjusted times.

Well, I don't understand the logic; it's app itself who changes the
timestamp type.  It must know when it's changed (because app is
changing it), and how to handle (because app chooses the timestamp
type).  And the current type can be queried via sw_params_*_get()
thing.

Of course, as default, there is no change from the current behavior --
it takes CLOCK_MONOTONIC.  So, there should be no breakage by this
change unless you change the application side to use the new call.

> For apps 
> that make use of the audio_time reported with a wall clock this could 
> lead to complete nonsense.
> I would have no problems if this was a fixed parameter defined once 
> before audio streaming starts.

You don't have to change the setup after the stream starts.  It's
usually set before the stream starting.  The point of using sw_params
is that it's independent from the hardware driver, thus it fits better
than hw_params.  The switching after stream isn't the purpose.  It's
just a gratis bonus.


Takashi

> > Along with this, increment the PCM protocol version.
> >
> > The extension was suggested by Clemens Ladisch.
> >
> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > ---
> >   include/uapi/sound/asound.h | 6 ++++--
> >   sound/core/pcm_native.c     | 3 +++
> >   2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> > index cbf7dc850a46..a7e062f91f39 100644
> > --- a/include/uapi/sound/asound.h
> > +++ b/include/uapi/sound/asound.h
> > @@ -139,7 +139,7 @@ struct snd_hwdep_dsp_image {
> >    *                                                                           *
> >    *****************************************************************************/
> >
> > -#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 11)
> > +#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 12)
> >
> >   typedef unsigned long snd_pcm_uframes_t;
> >   typedef signed long snd_pcm_sframes_t;
> > @@ -391,7 +391,9 @@ struct snd_pcm_sw_params {
> >   	snd_pcm_uframes_t silence_threshold;	/* min distance from noise for silence filling */
> >   	snd_pcm_uframes_t silence_size;		/* silence block size */
> >   	snd_pcm_uframes_t boundary;		/* pointers wrap point */
> > -	unsigned char reserved[64];		/* reserved for future */
> > +	unsigned int tstamp_type;		/* timestamp type */
> > +	int pads;				/* alignment, reserved */
> > +	unsigned char reserved[56];		/* reserved for future */
> >   };
> >
> >   struct snd_pcm_channel_info {
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > index 2372c49a8e84..81dedc381efd 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -543,6 +543,8 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream,
> >
> >   	if (params->tstamp_mode > SNDRV_PCM_TSTAMP_LAST)
> >   		return -EINVAL;
> > +	if (params->tstamp_type > SNDRV_PCM_TSTAMP_TYPE_LAST)
> > +		return -EINVAL;
> >   	if (params->avail_min == 0)
> >   		return -EINVAL;
> >   	if (params->silence_size >= runtime->boundary) {
> > @@ -557,6 +559,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream,
> >   	err = 0;
> >   	snd_pcm_stream_lock_irq(substream);
> >   	runtime->tstamp_mode = params->tstamp_mode;
> > +	runtime->tstamp_type = params->tstamp_type;
> >   	runtime->period_step = params->period_step;
> >   	runtime->control->avail_min = params->avail_min;
> >   	runtime->start_threshold = params->start_threshold;
> >
>