RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource
> -Original Message- > From: Peter Zijlstra [mailto:pet...@infradead.org] > Sent: Friday, September 04, 2015 7:28 AM > To: Richard Cochran > Cc: Hall, Christopher S; Thomas Gleixner; Kirsher, Jeffrey T; > h...@zytor.com; mi...@redhat.com; john.stu...@linaro.org; x...@kernel.org; > linux-ker...@vger.kernel.org; netdev@vger.kernel.org; intel-wired- > l...@lists.osuosl.org > Subject: Re: [PATCH v3 1/4] Add correlated clocksource deriving system time > from an auxiliary clocksource > > > > (several milliseconds) and the result will be out of date by some > fraction of that > > > amount. > > > > Why does it take milliseconds to read one audio time stamp? > > So what I suspect, but please correct me if I'm wrong Chris, is that a > DSP will buffer and process audio signals, and only later wake up the > main CPU. > > So by the time the CPU is made aware of the data, it's 'old'. That's about right. The DSP runs on a 1 ms cadence. Any access to registers controlled by the DSP will take 1-2 DSP ticks to access. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource
> -Original Message- > From: Thomas Gleixner [mailto:t...@linutronix.de] > Sent: Friday, September 04, 2015 9:35 AM > To: Peter Zijlstra > Cc: Richard Cochran; Hall, Christopher S; Kirsher, Jeffrey T; > h...@zytor.com; mi...@redhat.com; john.stu...@linaro.org; x...@kernel.org; > linux-ker...@vger.kernel.org; netdev@vger.kernel.org; intel-wired- > l...@lists.osuosl.org > Subject: Re: [PATCH v3 1/4] Add correlated clocksource deriving system time > from an auxiliary clocksource > > On Fri, 4 Sep 2015, Peter Zijlstra wrote: > > On Fri, Sep 04, 2015 at 05:17:43PM +0200, Richard Cochran wrote: > > > On Fri, Sep 04, 2015 at 05:10:21PM +0200, Peter Zijlstra wrote: > > > > I think what they're getting at is asking if there's a rate limit to > > > > time adjustments, without that, saving the last n transition points > will > > > > still not cover any given length of history. > > > > > > As if the ntp code isn't complex enough already - now we're adding > > > sample histories and adjustment rating limiting? > > > > > > And all for some unknown DSP in a mythical sound card?? > > > > Hehe, I'm just a 'translator' here. But going by you answer I'm taking > > it there isn't in fact a rate-limit to adjustments. Which, even if you > > were not opposed to that direction, makes it an unfeasible proposition. > > > > Also, I'm not thinking its too mythical, sound/soc/intel/ is full of > > audio DSP stuff, I think a newer version will just gain ART support. > > Right, but we still do not know how that is going to be used. And > that's the key question. As long as that is not answered all can do is > wild guessing. It's not wild guessing. We do have it working on other OSs and have a pretty good idea of how it will work. The DSP firmware will be largely identical for Linux. I think now, we have a chicken and egg problem. We can't post audio drivers that break, or are broken by, the current ART interface. How do I move this forward? Should I minimally (I don't know exactly what that means just yet) rewrite the ART interface so that the audio driver is mostly not broken and post that along with the audio driver code? Is this an acceptable approach? > > Thanks, > > tglx -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource
> -Original Message-
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Saturday, August 22, 2015 1:17 PM
> To: Hall, Christopher S
> Cc: Kirsher, Jeffrey T; h...@zytor.com; mi...@redhat.com;
> john.stu...@linaro.org; richardcoch...@gmail.com; x...@kernel.org; linux-
> ker...@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> l...@lists.osuosl.org; pet...@infradead.org
> Subject: Re: [PATCH v3 1/4] Add correlated clocksource deriving system time
> from an auxiliary clocksource
>
> > +/**
> > + * get_correlated_timestamp - Get a correlated timestamp
> > + *
> > + * Reads a timestamp from a device and correlates it to system time
> > + */
> > +int get_correlated_timestamp(struct correlated_ts *crt,
> > +struct correlated_cs *crs)
> > +{
> > + struct timekeeper *tk = &tk_core.timekeeper;
> > + unsigned long seq;
> > + cycles_t cycles;
> > + ktime_t base;
> > + s64 nsecs;
> > + int ret;
> > +
> > + do {
> > + seq = read_seqcount_begin(&tk_core.seq);
> > + /*
> > +* Verify that the correlated clocksoure is related to
> > +* the currently installed timekeeper clocksoure
> > +*/
> > + if (tk->tkr_mono.clock != crs->related_cs)
> > + return -ENODEV;
> > +
> > + /*
> > +* Try to get a timestamp from the device.
> > +*/
> > + ret = crt->get_ts(crt);
> > + if (ret)
> > + return ret;
> > +
[Re-added code for context]
In addition to the network interface, ART will be used in the audio interface
as well.
We need to support the case where an audio co-processor will control the audio
device.
In this case, the get_ts() function supplied by the audio driver will be very
slow
(several milliseconds) and the result will be out of date by some fraction of
that
amount.
This loop makes strict requirements on the latency and recency. Is it possible
to relax
that requirement in some way?
For example, supply the ART value as an argument and, in the case of the
realtime
clock, keep a short history of clock changes. It would fail in cases where
there
are a lot of calls to adjtimex(), but it will would work most of the time.
What can you suggest? Thanks
Chris
> > + } while (read_seqcount_retry(&tk_core.seq, seq));
> > + return 0;
> > +}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 3/4] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl
> -Original Message-
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: Sunday, August 23, 2015 4:26 AM
> To: Thomas Gleixner
> Cc: Hall, Christopher S; Kirsher, Jeffrey T; h...@zytor.com;
> mi...@redhat.com; john.stu...@linaro.org; x...@kernel.org; linux-
> ker...@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> l...@lists.osuosl.org; pet...@infradead.org
> Subject: Re: [PATCH v3 3/4] Add support for driver cross-timestamp to
> PTP_SYS_OFFSET ioctl
>
> On Sun, Aug 23, 2015 at 10:15:00AM +0200, Thomas Gleixner wrote:
> > So why can't you take N samples from the synced hardware? It does not
> > make any sense to me to switch to the imprecise mode if nsamples > 1.
>
> Ok, then I prefer to leave this "imprecise" method in place and ...
>
> > You can also provide a new IOCTL PTP_SYS_OFFSET_PRECISE which returns
> > -ENOSYS if hardware timestamping is not available and avoid the whole
> > nsamples dance for the case where we can get precise timestamps.
>
> have this for the new way.
>
> By keeping the imprecise method, we will be able to run both methods
> on the new hardware. That will help to quantify how imprecise the old
> method is.
This means: remove code changes from the PTP_SYS_OFFSET ioctl and call
getsynctime64() from a new ioctl PTP_SYS_OFFSET_PRECISE. Right?
And use the same type (struct ptp_sys_offset) for the new ioctl? Or should a
new simplified struct be used? Such as:
struct precise_ptp_sys_offset {
struct ptp_clock_time device;
struct ptp_clock_time system;
};
Does it make sense to keep the "cross-timestamp" capabilities flag as-is?
>
> Thanks,
> Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 4/4] Added getsynctime64() callback
> -Original Message-
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: Monday, August 10, 2015 1:50 AM
> To: Hall, Christopher S
> Cc: john.stu...@linaro.org; t...@linutronix.de; mi...@redhat.com; Kirsher,
> Jeffrey T; Ronciak, John; h...@zytor.com; x...@kernel.org; linux-
> ker...@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] Added getsynctime64() callback
>
> On Fri, Aug 07, 2015 at 04:01:35PM -0700, Christopher Hall wrote:
> > --- a/drivers/net/ethernet/intel/e1000e/defines.h
> > +++ b/drivers/net/ethernet/intel/e1000e/defines.h
> > @@ -527,6 +527,13 @@
> > #define E1000_RXCW_C 0x2000/* Receive config */
> > #define E1000_RXCW_SYNCH 0x4000/* Receive config synch
> */
> >
> > +/* HH Time Sync */
> > +#define E1000_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK 0xF000 /* max
> delay */
> > +#define E1000_TSYNCTXCTL_SYNC_COMP 0x4000 /* sync
> complete
> > + */
> > +#define E1000_TSYNCTXCTL_START_SYNC 0x8000 /*
> initiate sync
> > + */
>
> Split comment looks bad. Trim this leading space instead. ^^
OK.
>
> > @@ -98,6 +100,91 @@ static int e1000e_phc_adjtime(struct ptp_clock_info
> *ptp, s64 delta)
> > return 0;
> > }
> >
> > +#define HW_WAIT_COUNT (2)
> > +#define HW_RETRY_COUNT (2)
>
> A busy wait, plus a retry, ...
>
> > +#define SYNCTIME_RETRY_COUNT (2)
>
> plus another retry!
>
> Seems a bit heavy handed to me. Is the HW really that flakey?
>
> I would expect that a reasonably long polling loop should be
> sufficient. If not, then the HW ignores certain requests, and that is
> worth a comment.
>
> In any case, I don't understand why you have two nested retry loops.
The retry in get_synctime() is a left over from the previous patch. It's not
necessary,
the current timekeeping code won't fail in a way necessitating a retry. It's
removed.
The inner retry loop is due to huge inaccuracies in udelay(). I've done some
testing
and it appears udelay(2) actually results in about an 8 microsecond delay. On
Skylake the time for completion of the cross timestamp should be about 2
microseconds.
If we eliminate the inner most loop we either spin for too long or possibly
risk not
waiting long enough. Are there any guarantees for udelay()?
As for HW_RETRY_LOOP, I will confirm whether this is necessary. It was in
reference
code I was given, but, I agree, it seems odd.
>
> > +static int e1000e_phc_getsynctime(struct ptp_clock_info *ptp,
> > + struct timespec64 *devts,
> > + struct timespec64 *systs)
> > +{
> > + struct e1000_adapter *adapter = container_of(ptp, struct
> e1000_adapter,
> > +ptp_clock_info);
> > + unsigned long flags;
> > + u32 remainder;
> > + struct correlated_ts art_correlated_ts;
> > + u64 device_time;
> > + int i, ret;
> > +
> > + if (!cpu_has_art)
> > + return -EOPNOTSUPP;
>
> Perform this check before registration, setting .getsynctime64
> accordingly.
The problem here is that ART initialization doesn't happen until we install TSC
as a clocksource. This design is per Thomas' suggestion. That occurs after
the driver is loaded (as a module).
In my somewhat limited testing, it's about 400 ms later. The problem is the
several seconds of TSC frequency refinement. I, in principle, agree, but we
either need to move the ART initialization earlier (probably split it) or defer
PTP clock initialization in the driver.
>
> Thanks,
> Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] Add calls to translate Always Running Timer (ART) to system time
> -Original Message-
> From: John Stultz [mailto:john.stu...@linaro.org]
> Sent: Monday, July 27, 2015 9:11 PM
> To: Hall, Christopher S
> Cc: Thomas Gleixner; Richard Cochran; Ingo Molnar; Kirsher, Jeffrey T;
> Ronciak, John; H. Peter Anvin; x...@kernel.org; lkml;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 3/5] Add calls to translate Always Running Timer
> (ART) to system time
>
> On Mon, Jul 27, 2015 at 5:46 PM, Christopher Hall
> wrote:
> > +static bool checked_art_to_tsc(cycle_t *tsc)
> > +{
> > + if (!has_art())
> > + return false;
> > + *tsc = art_to_tsc(*tsc);
> > + return true;
> > +}
> > +
> > +static int art_to_rawmono64(struct timespec64 *rawmono, cycle_t art)
> > +{
> > + if (!checked_art_to_tsc(&art))
> > + return -ENXIO;
> > + return tsc_to_rawmono64(rawmono, art);
> > +}
> > +EXPORT_SYMBOL(art_to_rawmono64);
>
> This all seems to assume the TSC is the current clocksource, which it
> may not be if the user has overridden it.
I don't make that assumption. The counter_to_* functions take a
pointer to a clocksource struct. They return -ENXIO if that clocksource
doesn’t match the current clocksource.
The tsc_to_* functions pass the tsc clocksource pointer to the counter_to_*
functions. These tsc conversion functions are called by the art_to_*
functions.
>
> If instead there were a counter_to_rawmono64() which took the counter
> value and maybe the name of the clocksource (if the strncmp is
> affordable for your use), it might be easier for the core to provide
> an error if the current timekeeping clocksource isn't the one the
> counter value is based on. This would also allow the tsc_to_*()
> midlayers to be dropped (since they don't seem to do much).
>
> thanks
> -john
Again, thanks for your input.
Chris
RE: [PATCH 1/5] Add functions producing system time given a backing counter value
> -Original Message-
> From: John Stultz [mailto:john.stu...@linaro.org]
> Sent: Monday, July 27, 2015 8:44 PM
> To: Hall, Christopher S
> Cc: Thomas Gleixner; Richard Cochran; Ingo Molnar; Kirsher, Jeffrey T;
> Ronciak, John; H. Peter Anvin; x...@kernel.org; lkml;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 1/5] Add functions producing system time given a
> backing counter value
>
> On Mon, Jul 27, 2015 at 5:46 PM, Christopher Hall
> wrote:
> > * counter_to_rawmono64
> > * counter_to_mono64
> > * counter_to_realtime64
> >
> > Enables drivers to translate a captured system clock counter to system
> > time. This is useful for network and audio devices that capture
> timestamps
> > in terms of both the system clock and device clock.
>
> Huh. So for counter_to_realtime64 & mono64, this seems to ignore the
> fact that the multiplier is constantly adjusted and corrected. So that
> calling the function twice with the same counter value may result in
> different returned values.
>
> I've not yet groked the whole patchset, but it seems like there needs
> to be some mechanism that ensures the counter value is captured and
> used in the same (or at least close) interval that the timekeeper data
> is valid for.
The ART (and derived TSC) values are always in the past. There's no
chance that we could exceed the interval. I don't think any similar
usage would be a problem either.
Are you suggesting that, for completeness, this be enforced by the
conversion function?
I do a check here to make sure that the current counter value isn't before
the beginning of the current interval:
timekeeping_get_delta()
...
if (cycle_now < tkr->cycle_last &&
tkr->cycle_last - cycle_now < ROLLOVER_THRESHOLD)
return -EAGAIN;
If tkr->cycle_last - cycle_now is large, the assumption is that
rollover occurred. Otherwise, the caller should re-read the counter
so that it falls within the current interval. In my "normal use"
testing, re-read never occurred.
Thanks for your input.
Chris
>
> thanks
> -john
N�r��yb�X��ǧv�^�){.n�+���z�^�)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
RE: [PATCH 3/5] Add calls to translate Always Running Timer (ART) to system time
> -Original Message-
> From: Andy Lutomirski [mailto:l...@kernel.org]
> Sent: Monday, July 27, 2015 6:32 PM
> To: Hall, Christopher S; john.stu...@linaro.org; t...@linutronix.de;
> richardcoch...@gmail.com; mi...@redhat.com; Kirsher, Jeffrey T; Ronciak,
> John; h...@zytor.com; x...@kernel.org
> Cc: linux-ker...@vger.kernel.org; netdev@vger.kernel.org; Borislav
> Petkov
> Subject: Re: [PATCH 3/5] Add calls to translate Always Running Timer
> (ART) to system time
>
> On 07/27/2015 05:46 PM, Christopher Hall wrote:
> > * art_to_mono64
> > * art_to_rawmono64
> > * art_to_realtime64
> >
> > Intel audio and PCH ethernet devices use the Always Running Timer
> (ART) to
> > relate their device clock to system time
> >
> > Signed-off-by: Christopher Hall
> > ---
> > arch/x86/Kconfig | 12
> > arch/x86/include/asm/art.h | 42 ++
> > arch/x86/kernel/Makefile | 1 +
> > arch/x86/kernel/art.c | 134
> +
> > arch/x86/kernel/tsc.c | 4 ++
> > 5 files changed, 193 insertions(+)
> > create mode 100644 arch/x86/include/asm/art.h
> > create mode 100644 arch/x86/kernel/art.c
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index b3a1a5d..1ef9985 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1175,6 +1175,18 @@ config X86_CPUID
> > with major 203 and minors 0 to 31 for /dev/cpu/0/cpuid to
> > /dev/cpu/31/cpuid.
> >
> > +config X86_ART
> > + bool "Always Running Timer"
> > + default y
> > + depends on X86_TSC
> > + ---help---
> > + This option provides functionality to drivers and devices that
> use
> > + the always-running-timer (ART) to correlate their device clock
> > + counter with the system clock counter. The TSC is *exactly*
> related
> > + to the ART by a ratio m/n specified by CPUID leaf 0x15
> > + (n=EAX,m=EBX). If ART is unused or unavailable there isn't any
> > + performance impact. It's safe to say Y.
> > +
>
> Is there a good reason to make this optional?
If there aren't any objections, it sound OK to me. So no, I don't know
of any good reasons.
>
> Also, is there *still* no way to ask the thing for its nominal
> frequnency? Or can we expect CPUID leaf 16H to work on CPUs that
> support this and can we expect it to actually work?
There isn't any way to query nominal frequency. CPUID leaf 0x15 only
exposes the relationship between ART and TSC. CPUID leaf 0x16 stays
the more or less the same and isn't related to ART.
The SDM says "The
> returned information should not be used for any other purpose as the
> returned information does not accurately correlate to information /
> counters returned by other processor interfaces."
>
> Also, does this thing let us learn the real time base? SDM 17.14.4
> suggests that the ART value isn't affected by "privileged software" (aka
> buggy/malicious firmware). Or, alternatively, how do we learn the
> offset K between ART and scaled TSC?
ART isn't affected by software. The determination of K used to convert ART to
TSC is in a footnote (2) in that section of the SDM. I'm not going to risk
repeating it here and possibly altering its meaning.
>
> > choice
> > prompt "High Memory Support"
> > default HIGHMEM4G
> > diff --git a/arch/x86/include/asm/art.h b/arch/x86/include/asm/art.h
> > new file mode 100644
> > index 000..da58ce4
> > --- /dev/null
> > +++ b/arch/x86/include/asm/art.h
> > @@ -0,0 +1,42 @@
> > +/*
> > + * x86 ART related functions
> > + */
> > +#ifndef _ASM_X86_ART_H
> > +#define _ASM_X86_ART_H
> > +
> > +#ifndef CONFIG_X86_ART
> > +
> > +static inline int setup_art(void)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline bool has_art(void)
> > +{
> > + return false;
> > +}
> > +
> > +static inline int art_to_rawmono64(struct timespec64 *rawmono,
> cycle_t art)
> > +{
> > + return -ENXIO;
> > +}
> > +static inline int art_to_realtime64(struct timespec64 *realtime,
> cycle_t art)
> > +{
> > + return -ENXIO;
> > +}
> > +static inline int art_to_mono64(struct timespec64 *mono, cycle_t art)
> > +{
> > + return -ENXIO;
> > +}
> > +
> > +#else
> > +
> > +extern int setup_art(void);
> > +extern bool has_art(void);
> > +extern int art_to_rawmono64(struct timespec64 *rawmono, c
RE: [PATCH v3] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl
I am assuming the patch is rejected at this point. I will re-submit later as
soon as I am able to post a full end to end solution.
Chris
> -Original Message-
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: Thursday, July 09, 2015 7:58 AM
> To: Hall, Christopher S
> Cc: t...@linutronix.de; john.stu...@linaro.org; Ronciak, John; linux-
> ker...@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH v3] Add support for driver cross-timestamp to
> PTP_SYS_OFFSET ioctl
>
> On Wed, Jul 08, 2015 at 01:46:41PM -0700, Christopher Hall wrote:
> > This patch allows system and device time ("cross-timestamp") to be
> > performed by the driver. Currently, the cross-timestamping is performed
> > in the PTP_SYS_OFFSET ioctl. The PTP clock driver reads gettimeofday()
> > and the gettime64() callback provided by the driver. The cross-timestamp
> > is best effort where the latency between the capture of system time
> > (getnstimeofday()) and the device time (driver callback) may be
> > significant.
>
> The interface looks okay to me. Now all we need is a user of it...
>
> Acked-by: Richard Cochran
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
