[alsa-devel] [PATCH] alsa-lib: Add htimestamp operation in plugin file

Fri Feb 3 11:57:53 CET 2017

Hi,

We had to re-look onto this change again for possible consideration.

The existing solution htimestamp (snd_pcm_generic_real_htimestamp)  ALWAYS reads the current time, 
while snd_pcm_generic_htimestamp reads correct timestamp from slave including evaluation of TSTAMP_MODE.

From documentation of /src/pcm/pcm.c, we can see:
"""" \par Timestamp mode

The timestamp mode specifies, if timestamps are activated. Currently, 
only #SND_PCM_TSTAMP_NONE and #SND_PCM_TSTAMP_MMAP modes are known.
The mmap mode means that timestamp is taken on every period time 
boundary. Corresponding position in the ring buffer assigned to 
timestamp can be obtained using #snd_pcm_htimestamp() function. """"

As snd_pcm_generic_htimestamp() internally calls snd_pcm_htimestamp() to read time, so accurate timestamp can be read from snd_pcm_generic_htimestamp().

Also, in case of pcm_file, if the underlying slave is hardware, then we would wish to read elapsed hardware time, 
as it will be the most accurate, as opposed to the elapsed wall time. 
This will provide pcm_file with the most accurate timestamps. 

Let us know, your opinion on this to re-consider this change.

Regards,
Mounesh

-----Original Message-----
From: alsa-devel-bounces at alsa-project.org [mailto:alsa-devel-bounces at alsa-project.org] On Behalf Of Sutar, Mounesh
Sent: 30 November 2016 12:57
To: Takashi Sakamoto <o-takashi at sakamocchi.jp>; Takashi Iwai <tiwai at suse.de>
Cc: sutar.mounesh at gmail.com; alsa-devel at alsa-project.org; Andreas Pape <apape at de.adit-jv.com>; Frkuska, Joshua <Joshua_Frkuska at mentor.com>
Subject: Re: [alsa-devel] [PATCH] alsa-lib: Add htimestamp operation in plugin file

Hi,
I did rebase my repo before applying this patch. It applied without any conflicts.

commit 0d1d25f166a0294bf596dd2e6f55579e5e04186e
Author: Andreas Pape <apape at de.adit-jv.com>
Date:   Wed Nov 30 12:06:14 2016 +0530

    alsa-lib: Add htimestamp operation in plugin file

    PCM operation htimestamp is not implemented in plugin file.
    Calling snd_pcm_htimestamp() on a plugin file crashes. This scenario
    is considered now.

    Signed-off-by: Andreas Pape <apape at de.adit-jv.com>
    Signed-off-by: Joshua Frkuska <joshua_frkuska at mentor.com>

M       src/pcm/pcm_file.c

commit 2ef7a53c31de47f5f33127a89054a661a31bd310
Author: Mengdong Lin <mengdong.lin at linux.intel.com>
Date:   Fri Nov 25 13:20:02 2016 +0800

    topology: Update physical link configurations in Broadwell text conf file

    To make this conf file a better example, add configurations for the
    physical link "Codec", same as that defined by Intel Broadwell upstream
    machine driver.

    Signed-off-by: Mengdong Lin <mengdong.lin at linux.intel.com>
    Signed-off-by: Takashi Iwai <tiwai at suse.de>

M       src/conf/topology/broadwell/broadwell.conf

It appears as, source changes are at different locations, it didn’t report any conflicts.
Sorry for not observing this source changes, before applying the patch and pushing it to mailing list.
And Thanks for pointing it to included fix commit.

Regards,
Mounesh

-----Original Message-----
From: Takashi Sakamoto [mailto:o-takashi at sakamocchi.jp]
Sent: 29 November 2016 21:04
To: Takashi Iwai <tiwai at suse.de>
Cc: sutar.mounesh at gmail.com; alsa-devel at alsa-project.org; Frkuska, Joshua <Joshua_Frkuska at mentor.com>; Andreas Pape <apape at de.adit-jv.com>; Sutar, Mounesh <Mounesh_Sutar at mentor.com>
Subject: Re: [PATCH] alsa-lib: Add htimestamp operation in plugin file

On 2016年11月30日 00:22, Takashi Iwai wrote:
> On Tue, 29 Nov 2016 16:15:57 +0100,
>>>> --- a/src/pcm/pcm_file.c	2013-07-08 14:31:36.000000000 +0200
>>>> +++ b/src/pcm/pcm_file.c	2015-05-04 16:26:10.413615403 +0200
>>>> @@ -698,6 +698,7 @@
>>>>  	.readi = snd_pcm_file_readi,
>>>>  	.readn = snd_pcm_file_readn,
>>>>  	.avail_update = snd_pcm_generic_avail_update,
>>>> +	.htimestamp = snd_pcm_generic_htimestamp,
>>>>  	.mmap_commit = snd_pcm_file_mmap_commit,
>>>>  	.poll_descriptors_count = snd_pcm_generic_poll_descriptors_count,
>>>>  	.poll_descriptors = snd_pcm_generic_poll_descriptors,
>>
>> I oppose this application, because designated initialization is 
>> already applied to the .htimestamp member.
>>
>> ...
>>         .poll_descriptors = snd_pcm_generic_poll_descriptors,
>>         .poll_revents = snd_pcm_generic_poll_revents,
>>         .htimestamp = snd_pcm_generic_htimestamp,

Oops, 'snd_pcm_generic_real_htimestamp' is proper here...

>> };
>>
>> Please see below commit:
>> pcm:file: add the missing htimestamp callback
>> http://git.alsa-project.org/?p=alsa-lib.git;a=commit;h=68ae0c72a53704
>> d416fe77d4c612d1f88d791e02
>>
>> This mistake causes below warning with -Woverride-init option.
>> pcm_file.c:714:16: warning: initialized field overwritten [-Woverride-init]
>>   .htimestamp = snd_pcm_generic_real_htimestamp,
>>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> I guess mentor/ADIT developers works for former snapshot, then missed 
>> to rebase to current mainline of alsa-lib. I suggest them to di 
>> re-evaluation with current mainline without this patch.
>
> I overlooked it, too.  Now the commit got reverted.

Thanks.

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel at alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel