Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Tim Cussins <timcussins@eml.cc>]

Hi guys,

Sorry for the (substantial) delay in responding to this :)

On 10/12/14 22:27, Takashi Iwai wrote:
> At Wed, 10 Dec 2014 15:48:06 -0600,
> Pierre-Louis Bossart wrote:
>>
>>> - Another concern is the compatibility with the current wallclock
>>>     implementation.  Judging from your patch, the audio_tstamp won't be
>>>     obtained from get_time_info callback in the default tstamp mode,
>>>     right?  This may result in a regression, as currently the driver
>>>     always gives the h/w audio_tstamp when the driver supports the
>>>     wallclock.
>>
>> Is this that big of a deal? To the best of my knowledge this wallclk
>> thing was implemented for HDaudio only when we were prototyping the new
>> hardware, and I don't think we ended-up contributing the corresponding
>> patches for PulseAudio. We've since realized that the wallclock can't be
>> available in all cases and that we need this selection capability in a
>> variety of cases.
>>
>> Also even if we kept the .wall_clock callback, the wallclock handling
>> could be relative (start at zero) or absolute. I implemented a reset to
>> zero on stream startup, since the counter is not maintained when the
>> hardware is idle, but there are implementations where the wallclock is
>> really absolute and not reset (see below).
>
> I'm not asking for keeping the wall_clock callback itself.  The
> requirement is the compatible kernel *behavior*.  This is essentially
> a MUST, especially when the backward compatibility isn't too
> difficult to achieve.
>
> For example, leave the type zero = TSTAMP_TYPE_COMPAT or such, and
> makes the PCM core and driver behaving as compatible as wall_clock.
> This should be relatively easy.
>
> BTW, what if the driver doesn't support the requested tstamp type?
> Isn't there any need to query the capability beforehand?
>
>
>>> - Last but not least: we're receiving multiple enhancement requests
>>>     regarding tstamp at the very same time.  This patchset conflicts
>>>     with Tim and Nick's start_at extention.
>>>
>>>     I believe this can be resolved later, but let's discuss the ground
>>>     line at first: the requirement and influence on both changes.
>>
>> I am aware of this and it's why I posted my patches earlier than planned
>> to avoid merging different concepts later, it's probably best to have
>> compatibility from day1.
>
> Yes, absolutely.

Agreed :)

>> My proposal was to have a start_at functionality based on the timestamp
>> definitions I suggested and keep audio and system timestamps separate
>> rather than add mixed typestamps such as
>> SND_PCM_TSTAMP_TYPE_AUDIO_WALLCLOCK. the code could be something like:
>>
>> start_at(typestamp type, timestamp subtype, timespec value ) {
>>
>> 	if (type == SYSTEM) {
>> 		_start_using_hrtimers(subtype, value) // would be CLOCK_REALTIME,
>> MONOTONIC, maybe RAW
>> 	} else if (type == AUDIO) {
>> 		if (subtype == ABSOLUTE_WALLCLOCK) // not reset on audio subsystem startup
>> 			_start_using_hardware(value)
>> 		else
>> 			// not sure what to do with regular counters, probably bail.
>> 			error;
>> 	}
>>
>> That way you can set what sort of system timestamp and what sort of
>> audio timestamp you want reported by snd_pcm_status, and you can
>> independently select the start timestamp of your choice.
>
> OK, let's see how other guys receive this idea.

This seems fine to me!

>
> thanks,
>
> Takashi
>