[PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Dylan Reid]

For playback add the codec-side delay to the timestamp, for capture
subtract it.  This brings the timestamps in line with the time that
was recently added to the delay reporting.

Signed-off-by: Dylan Reid <dgreid@chromium.org>
---
Changes since v1:
Calculate nsec from frames, subtract delay for playback as well as
capture.

 sound/pci/hda/hda_intel.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 735567e..3e6f2c6 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1889,6 +1889,23 @@ static void azx_timecounter_init(struct snd_pcm_substream *substream,
 		tc->cycle_last = last;
 }
 
+static u64 azx_subtract_codec_delay(struct snd_pcm_substream *substream,
+				u64 nsec)
+{
+	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
+	struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
+	u64 codec_frames, codec_nsecs;
+
+	if (!hinfo->ops.get_delay)
+		return nsec;
+
+	codec_frames = hinfo->ops.get_delay(hinfo, apcm->codec, substream);
+	codec_nsecs = div_u64(codec_frames * 1000000000LL,
+			      substream->runtime->rate);
+
+	return (nsec > codec_nsecs) ? nsec - codec_nsecs : 0;
+}
+
 static int azx_get_wallclock_tstamp(struct snd_pcm_substream *substream,
 				struct timespec *ts)
 {
@@ -1897,6 +1914,7 @@ static int azx_get_wallclock_tstamp(struct snd_pcm_substream *substream,
 
 	nsec = timecounter_read(&azx_dev->azx_tc);
 	nsec = div_u64(nsec, 3); /* can be optimized */
+	nsec = azx_subtract_codec_delay(substream, nsec);
 
 	*ts = ns_to_timespec(nsec);
 
-- 
1.8.1.3.605.g02339dd

Re: [PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Takashi Iwai]

At Mon,  8 Apr 2013 18:20:30 -0700,
Dylan Reid wrote:
> 
> For playback add the codec-side delay to the timestamp, for capture
> subtract it.  This brings the timestamps in line with the time that
> was recently added to the delay reporting.
> 
> Signed-off-by: Dylan Reid <dgreid@chromium.org>

Thanks, applied now.


Takashi
> ---
> Changes since v1:
> Calculate nsec from frames, subtract delay for playback as well as
> capture.
> 
>  sound/pci/hda/hda_intel.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 735567e..3e6f2c6 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1889,6 +1889,23 @@ static void azx_timecounter_init(struct snd_pcm_substream *substream,
>  		tc->cycle_last = last;
>  }
>  
> +static u64 azx_subtract_codec_delay(struct snd_pcm_substream *substream,
> +				u64 nsec)
> +{
> +	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
> +	struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
> +	u64 codec_frames, codec_nsecs;
> +
> +	if (!hinfo->ops.get_delay)
> +		return nsec;
> +
> +	codec_frames = hinfo->ops.get_delay(hinfo, apcm->codec, substream);
> +	codec_nsecs = div_u64(codec_frames * 1000000000LL,
> +			      substream->runtime->rate);
> +
> +	return (nsec > codec_nsecs) ? nsec - codec_nsecs : 0;
> +}
> +
>  static int azx_get_wallclock_tstamp(struct snd_pcm_substream *substream,
>  				struct timespec *ts)
>  {
> @@ -1897,6 +1914,7 @@ static int azx_get_wallclock_tstamp(struct snd_pcm_substream *substream,
>  
>  	nsec = timecounter_read(&azx_dev->azx_tc);
>  	nsec = div_u64(nsec, 3); /* can be optimized */
> +	nsec = azx_subtract_codec_delay(substream, nsec);
>  
>  	*ts = ns_to_timespec(nsec);
>  
> -- 
> 1.8.1.3.605.g02339dd
> 

Re: [PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Pierre-Louis Bossart]

On 4/8/13 6:20 PM, Dylan Reid wrote:
> For playback add the codec-side delay to the timestamp, for capture
> subtract it.  This brings the timestamps in line with the time that
> was recently added to the delay reporting.
>
> Signed-off-by: Dylan Reid <dgreid@chromium.org>
> ---
> Changes since v1:
> Calculate nsec from frames, subtract delay for playback as well as
> capture.

> +	return (nsec > codec_nsecs) ? nsec - codec_nsecs : 0;

Is this correct? I would think for capture you need to add the delay, 
and subtract it for playback?
-Pierre

Re: [PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Dylan Reid]

On Tue, Apr 9, 2013 at 8:55 PM, Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
> On 4/8/13 6:20 PM, Dylan Reid wrote:
>>
>> For playback add the codec-side delay to the timestamp, for capture
>> subtract it.  This brings the timestamps in line with the time that
>> was recently added to the delay reporting.
>>
>> Signed-off-by: Dylan Reid <dgreid@chromium.org>
>> ---
>> Changes since v1:
>> Calculate nsec from frames, subtract delay for playback as well as
>> capture.
>
>
>> +       return (nsec > codec_nsecs) ? nsec - codec_nsecs : 0;
>
>
> Is this correct? I would think for capture you need to add the delay, and
> subtract it for playback?

Should the capture timestamp represents the wall time when the sample
is converted by the A-to-D?  If so, would be the correct time be the
codec delay nsec before what is reported here?

> -Pierre
>

Re: [PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Pierre-Louis Bossart]

>>
>>> +       return (nsec > codec_nsecs) ? nsec - codec_nsecs : 0;
>>
>>
>> Is this correct? I would think for capture you need to add the delay, and
>> subtract it for playback?
>
> Should the capture timestamp represents the wall time when the sample
> is converted by the A-to-D?  If so, would be the correct time be the
> codec delay nsec before what is reported here?

For playback audio_timestamp = wallclk - codec_delay_in_nsec
for capture audio_timestamp = wallclk + codec_delay_in_nsec

Re: [PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Takashi Iwai]

At Wed, 10 Apr 2013 07:29:32 -0700,
Pierre-Louis Bossart wrote:
> 
> 
> >>
> >>> +       return (nsec > codec_nsecs) ? nsec - codec_nsecs : 0;
> >>
> >>
> >> Is this correct? I would think for capture you need to add the delay, and
> >> subtract it for playback?
> >
> > Should the capture timestamp represents the wall time when the sample
> > is converted by the A-to-D?  If so, would be the correct time be the
> > codec delay nsec before what is reported here?
> 
> For playback audio_timestamp = wallclk - codec_delay_in_nsec
> for capture audio_timestamp = wallclk + codec_delay_in_nsec

Hmm...  I'm confused, too.  From a pretty generic view,

- hwptr = the samples (frames) transferred on the (memory) buffer
- hwptr + PCM delay = the point being currently captured

Which position does the wall clock correspond?


Takashi

Re: [PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Pierre-Louis Bossart]

>> For playback audio_timestamp = wallclk - codec_delay_in_nsec
>> for capture audio_timestamp = wallclk + codec_delay_in_nsec
>
> Hmm...  I'm confused, too.  From a pretty generic view,
>
> - hwptr = the samples (frames) transferred on the (memory) buffer
> - hwptr + PCM delay = the point being currently captured
>
> Which position does the wall clock correspond?

you now have 3 positions:

For capture:
hwptr = sample in memory
hwptr+ delay = sample recorded on link (tracked by LPIB and wallclock)
hwptr+ delay + codec_delay = sample recorded by A/D

For playback
hwptr = next sample in memory to be DMA'ed
hwptr - delay = sample pushed on link (tracked by LPIB and wallclock)
hwptr - delay - codec_delay = sample played in A/D

the wallclock is only at the soc/chipset level, it doesn't know anything 
about the peripheral.
Makes sense?
-Pierre

Re: [PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Takashi Iwai]

At Thu, 11 Apr 2013 19:17:10 -0600,
Pierre-Louis Bossart wrote:
> 
> 
> >> For playback audio_timestamp = wallclk - codec_delay_in_nsec
> >> for capture audio_timestamp = wallclk + codec_delay_in_nsec
> >
> > Hmm...  I'm confused, too.  From a pretty generic view,
> >
> > - hwptr = the samples (frames) transferred on the (memory) buffer
> > - hwptr + PCM delay = the point being currently captured
> >
> > Which position does the wall clock correspond?
> 
> you now have 3 positions:
> 
> For capture:
> hwptr = sample in memory
> hwptr+ delay = sample recorded on link (tracked by LPIB and wallclock)
> hwptr+ delay + codec_delay = sample recorded by A/D
> 
> For playback
> hwptr = next sample in memory to be DMA'ed
> hwptr - delay = sample pushed on link (tracked by LPIB and wallclock)
> hwptr - delay - codec_delay = sample played in A/D
> 
> the wallclock is only at the soc/chipset level, it doesn't know anything 
> about the peripheral.
> Makes sense?

Thanks, now it's clearer.


Takashi

Re: [PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Takashi Iwai]

At Fri, 12 Apr 2013 10:55:27 +0200,
Takashi Iwai wrote:
> 
> At Thu, 11 Apr 2013 19:17:10 -0600,
> Pierre-Louis Bossart wrote:
> > 
> > 
> > >> For playback audio_timestamp = wallclk - codec_delay_in_nsec
> > >> for capture audio_timestamp = wallclk + codec_delay_in_nsec
> > >
> > > Hmm...  I'm confused, too.  From a pretty generic view,
> > >
> > > - hwptr = the samples (frames) transferred on the (memory) buffer
> > > - hwptr + PCM delay = the point being currently captured
> > >
> > > Which position does the wall clock correspond?
> > 
> > you now have 3 positions:
> > 
> > For capture:
> > hwptr = sample in memory
> > hwptr+ delay = sample recorded on link (tracked by LPIB and wallclock)
> > hwptr+ delay + codec_delay = sample recorded by A/D
> > 
> > For playback
> > hwptr = next sample in memory to be DMA'ed
> > hwptr - delay = sample pushed on link (tracked by LPIB and wallclock)
> > hwptr - delay - codec_delay = sample played in A/D
> > 
> > the wallclock is only at the soc/chipset level, it doesn't know anything 
> > about the peripheral.
> > Makes sense?
> 
> Thanks, now it's clearer.

Dylan, could you care to submit the fix patch?


thanks,

Takashi

Re: [PATCH v2] ALSA: hda - Apply codec delay to wallclock.

[Dylan Reid]

On Apr 13, 2013 1:55 AM, "Takashi Iwai" <tiwai@suse.de> wrote:
>
> At Fri, 12 Apr 2013 10:55:27 +0200,
> Takashi Iwai wrote:
> >
> > At Thu, 11 Apr 2013 19:17:10 -0600,
> > Pierre-Louis Bossart wrote:
> > >
> > >
> > > >> For playback audio_timestamp = wallclk - codec_delay_in_nsec
> > > >> for capture audio_timestamp = wallclk + codec_delay_in_nsec
> > > >
> > > > Hmm...  I'm confused, too.  From a pretty generic view,
> > > >
> > > > - hwptr = the samples (frames) transferred on the (memory) buffer
> > > > - hwptr + PCM delay = the point being currently captured
> > > >
> > > > Which position does the wall clock correspond?
> > >
> > > you now have 3 positions:
> > >
> > > For capture:
> > > hwptr = sample in memory
> > > hwptr+ delay = sample recorded on link (tracked by LPIB and wallclock)
> > > hwptr+ delay + codec_delay = sample recorded by A/D
> > >
> > > For playback
> > > hwptr = next sample in memory to be DMA'ed
> > > hwptr - delay = sample pushed on link (tracked by LPIB and wallclock)
> > > hwptr - delay - codec_delay = sample played in A/D
> > >
> > > the wallclock is only at the soc/chipset level, it doesn't know
anything
> > > about the peripheral.
> > > Makes sense?
> >
> > Thanks, now it's clearer.
>
> Dylan, could you care to submit the fix patch?
>

It will be the first thing I do when I get to the office tomorrow morning.

Thanks,

Dylan

>
> thanks,
>
> Takashi