Re: [PATCH 3/3] ALSA: pcm: Add timestamp type to sw_params

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

On 7/10/14, 10:42 AM, Takashi Iwai wrote:
> 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.

So we agree that the dynamic switch isn't the intended usage but we 
allow for it anyway... I guess given that we can enable/disable 
timestamps dynamically this follows the same logic, it's just weird to 
have a sw_param whose intended use is really a hw_param, something you 
set once and don't modify later. If we want user-space to do the right 
thing we should at least document the consequences of a dynamic switch.
No further objections.

>
> Takashi
>
>>> Along with this, increment the PCM protocol version.
>>>
>>> The extension was suggested by Clemens Ladisch.
>>>
>>> Signed-off-by: Takashi Iwai <tiwai@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;
>>>
>>