Re: [PATCH RFC 2/9] ALSA: core: allow for trigger_tstamp snapshot in .trigger

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

Thanks for the review.

On 12/10/14, 10:31 AM, Takashi Iwai wrote:
> At Mon,  8 Dec 2014 16:23:39 -0600,
> Pierre-Louis Bossart wrote:
>>
>> Don't use generic snapshot of trigger_tstamp if low-level driver or
>> hardware can get a more precise value for better audio/system time
>> synchronization.
>>
>> Also add definitions for delayed updates if actual trigger tstamp
>> can be only be provided after a delay due to hardware constraints.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   include/sound/pcm.h     | 2 ++
>>   sound/core/pcm_native.c | 6 +++++-
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
>> index 1e7f74a..83c669f 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -281,6 +281,8 @@ struct snd_pcm_runtime {
>>   	/* -- Status -- */
>>   	struct snd_pcm_substream *trigger_master;
>>   	struct timespec trigger_tstamp;	/* trigger timestamp */
>> +	int trigger_tstamp_latched;     /* trigger timestamp latched in low-level driver/hardware */
>
> Better to use bool nowadays.

ok

>
>> +	int trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */
>
> This isn't used at all in this patch.  I found it being used in the
> later usb-audio patch.  If it's the only place, can't it be rather put
> locally to usb-audio object instead of the common pcm runtime?

It's not limited to USB. We have upcoming hardware where the 
trigger_tstamp will only be determined with a delay due to IPC. USB is 
just an example of a common pattern where the trigger_tstamp will be 
known for sure after a couple of ms.

>
>>   	int overrange;
>>   	snd_pcm_uframes_t avail_max;
>>   	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index 5dc83fb..37a7137 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -806,10 +806,14 @@ static int snd_pcm_channel_info_user(struct snd_pcm_substream *substream,
>>   static void snd_pcm_trigger_tstamp(struct snd_pcm_substream *substream)
>>   {
>>   	struct snd_pcm_runtime *runtime = substream->runtime;
>> +
>>   	if (runtime->trigger_master == NULL)
>>   		return;
>>   	if (runtime->trigger_master == substream) {
>> -		snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
>> +		if (runtime->trigger_tstamp_latched == 0)
>> +			snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
>> +		else
>> +			runtime->trigger_tstamp_latched = 0;
>
> IMO, it's better to clear the flat at the beginning of PCM trigger
> commonly.  Looking at the later patch, you clear in each driver's
> callback.  This should be moved into the common place.

I must admit I don't really understand the logic of all the _pre and 
_post operations. Did you mean clearing this field in snd_pcm_do_start()?


>
> thanks,
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>