Re: [RFC 1/4] time: export getnstime_raw_and_real for DRM

Re: [RFC 1/4] time: export getnstime_raw_and_real for DRM

[Mario Kleiner]

> Subject: Re: [RFC 4/4] drm: add support for raw monotonic vblank
> 	timestamps
> Message-ID: <1349446447.17758.73.camel@thor.local>
> Content-Type: text/plain; charset="ISO-8859-1"
>
> On Fre, 2012-10-05 at 16:59 +0300, Imre Deak wrote:
>> On Fri, 2012-10-05 at 15:55 +0200, Michel D?nzer wrote:
>>> On Fre, 2012-10-05 at 16:37 +0300, Imre Deak wrote:
>>>> In practice we never want the timestamps for vblank and page flip events
>>>> to be affected by time adjustments, so in addition to the gettimeofday
>>>> timestamps we used so far add support for raw monotonic timestamps.
>>>>
>>>> For backward compatibility use flags to select between the old and new
>>>> timestamp format.
>>>>
>>>> Note that with this change we will save the timestamp in both formats,
>>>> for cases where multiple clients are expecting an event notification in
>>>> different time formats.
>>>
>>> I wonder if all this trouble is really necessary. I honestly can't
>>> imagine any user of this API requiring non-monotonic timestamps and
>>> breaking with monotonic ones. I think it was simply a mistake that we
>>> didn't make them monotonic in the first place (or maybe it wasn't even
>>> possible when this API was first introduced).

Hi

The OML_sync_control spec requires monotonic timestamps, so yes, it was 
either a mistake or it wasn't possible at that time.

If an app developer follows the spec, he would consider the current 
behaviour broken -- or should be able to handle monotonic timestamps.

>>
>> Yea, I'd rather simply switch over to monotonic timestamps too. But that
>> would break apps that already compare against the wall time for whatever
>> purpose (for example A/V sync).
>
> Are there actually any such apps in the real world? Do they work when
> the wall time jumps?
>

Psychtoolbox as my example of a very timing sensitive app: Currently 
uses wall time as a reference for historical reasons. We tell/expect 
users not to do anything in the middle of a work session that could 
cause a sudden jump in wall time, but then ntp time adjustments are 
usually in very small steps, so not a problem, and big adjustments 
should only happen if users do something they'll certainly not do during 
a timing-sensitve session, like manually set the system clock, 
reboot/suspend/resume the machine etc. In practice, this works well 
enough over the typical duration of such sessions (minutes to a few 
hours) and is sometimes needed if multiple machines need to be 
time-synced over the local network.

But then Psychtoolbox checks each timestamp it gets from somewhere 
"outside" (OML_sync_control / INTEL_swap_events / ALSA audio timestamps, 
network receive timestamps, evdev, x11, ...) if it is in gettimeofday() 
aka CLOCK_REALTIME aka wall time or in CLOCK_MONOTONIC time and just 
remaps to whatever its reference clock is.

There's no way around this than to have no fixed expectations, but to 
remap stuff on the fly, because different parts of the Linux universe 
have decided on different time bases, or even switched somewhere from 
one kernel version to the next in the last years, e.g., ALSA, which at 
some time switched from wall clock to CLOCK_MONOTONIC. Sometimes 
clock_gettime() wasn't available at all in older setups, so there only 
was gettimeofday(). Or toolkits like GStreamer use different timebases 
dependent on OS and sometimes even on plugins.

I would expect that other timing sensitive apps have to have ways to 
handle this in similar ways.

Wrt. to the drm vblank/pageflip timestamps, the userspace extensions 
which expose these (INTEL_swap_events, OML_sync_control) don't allow 
apps to select which timebase to use, they define monotonic time as what 
is returned, so i don't know how a userspace app could actually ask the 
DRM for one or the other format? So i guess just switching to 
CLOCK_MONOTONIC shouldn't be that bad.

Kristian, i assume Wayland will also return presentation timestamps in 
the format and microsecond precision of the DRM, right?

On 05.10.12 18:22, intel-gfx-request@lists.freedesktop.org wrote:
> Message: 7 Date: Fri, 5 Oct 2012 12:14:29 -0400 From: Kristian H?gsberg

...
> I just had a quick look at driver/input/evdev.c, since evdev devices
> did a similar change recently to allow evdev timestamp from the
> monotonic clock.  They're using a different time API:
>
> 	time_mono = ktime_get();
>          time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());
>
> and
>
>          event->time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
> 			                mono : real);
>
> I'm not really up-to-date on kernel time APIs, but I wonder if that
> may be better?  At least, I suspect we wouldn't need changes outside
> drm if we use this API.
>
> Kristian

Userspace apps only have access to what gettimeofday() and 
clock_gettime() for CLOCK_REALTIME (== gettimeofday() afaik) and 
CLOCK_MONOTONIC return, so whatever is returned should be in 
CLOCK_MONOTONIC format, otherwise there will be lots of tears and dead 
kittens. I think what evdev does makes a lot of sense, but i'm also not 
up-to-date about the various layers of timing apis.

-mario

Re: [RFC 1/4] time: export getnstime_raw_and_real for DRM

[Imre Deak]

On Sat, 2012-10-06 at 03:41 +0200, Mario Kleiner wrote:
> [...]
> 
> But then Psychtoolbox checks each timestamp it gets from somewhere 
> "outside" (OML_sync_control / INTEL_swap_events / ALSA audio timestamps, 
> network receive timestamps, evdev, x11, ...) if it is in gettimeofday() 
> aka CLOCK_REALTIME aka wall time or in CLOCK_MONOTONIC time and just 
> remaps to whatever its reference clock is.
> 
> There's no way around this than to have no fixed expectations, but to 
> remap stuff on the fly, because different parts of the Linux universe 
> have decided on different time bases, or even switched somewhere from 
> one kernel version to the next in the last years, e.g., ALSA, which at 
> some time switched from wall clock to CLOCK_MONOTONIC. Sometimes 
> clock_gettime() wasn't available at all in older setups, so there only 
> was gettimeofday(). Or toolkits like GStreamer use different timebases 
> dependent on OS and sometimes even on plugins.
> 
> I would expect that other timing sensitive apps have to have ways to 
> handle this in similar ways.

I think the question is, can we be sure? I don't think there is any
guarantee that random application X will not be confused by an
unconditional switch to monotonic timestamps.

> Wrt. to the drm vblank/pageflip timestamps, the userspace extensions 
> which expose these (INTEL_swap_events, OML_sync_control) don't allow 
> apps to select which timebase to use, they define monotonic time as what 
> is returned, so i don't know how a userspace app could actually ask the 
> DRM for one or the other format? So i guess just switching to 
> CLOCK_MONOTONIC shouldn't be that bad.

An application could just use the kernel DRM interface directly. I admit
this is not very likely but this is what should determine the rules by
which we change the ABI. 

> Kristian, i assume Wayland will also return presentation timestamps in 
> the format and microsecond precision of the DRM, right?
> 
> On 05.10.12 18:22, intel-gfx-request@lists.freedesktop.org wrote:
> > Message: 7 Date: Fri, 5 Oct 2012 12:14:29 -0400 From: Kristian H?gsberg
> 
> ...
> > I just had a quick look at driver/input/evdev.c, since evdev devices
> > did a similar change recently to allow evdev timestamp from the
> > monotonic clock.  They're using a different time API:
> >
> > 	time_mono = ktime_get();
> >          time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());
> >
> > and
> >
> >          event->time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
> > 			                mono : real);
> >
> > I'm not really up-to-date on kernel time APIs, but I wonder if that
> > may be better?  At least, I suspect we wouldn't need changes outside
> > drm if we use this API.
> >
> > Kristian
> 
> Userspace apps only have access to what gettimeofday() and 
> clock_gettime() for CLOCK_REALTIME (== gettimeofday() afaik) and 
> CLOCK_MONOTONIC return, so whatever is returned should be in 
> CLOCK_MONOTONIC format, otherwise there will be lots of tears and dead 
> kittens. I think what evdev does makes a lot of sense, but i'm also not 
> up-to-date about the various layers of timing apis.

Yes, this should be the case, regardless of which kernel interface we
decide to use.

--Imre

Re: [RFC 1/4] time: export getnstime_raw_and_real for DRM

[Mario Kleiner]

On 08.10.12 13:35, Imre Deak wrote:
> On Sat, 2012-10-06 at 03:41 +0200, Mario Kleiner wrote:
>> [...]
>>
>> But then Psychtoolbox checks each timestamp it gets from somewhere
>> "outside" (OML_sync_control / INTEL_swap_events / ALSA audio timestamps,
>> network receive timestamps, evdev, x11, ...) if it is in gettimeofday()
>> aka CLOCK_REALTIME aka wall time or in CLOCK_MONOTONIC time and just
>> remaps to whatever its reference clock is.
>>
>> There's no way around this than to have no fixed expectations, but to
>> remap stuff on the fly, because different parts of the Linux universe
>> have decided on different time bases, or even switched somewhere from
>> one kernel version to the next in the last years, e.g., ALSA, which at
>> some time switched from wall clock to CLOCK_MONOTONIC. Sometimes
>> clock_gettime() wasn't available at all in older setups, so there only
>> was gettimeofday(). Or toolkits like GStreamer use different timebases
>> dependent on OS and sometimes even on plugins.
>>
>> I would expect that other timing sensitive apps have to have ways to
>> handle this in similar ways.
>
> I think the question is, can we be sure? I don't think there is any
> guarantee that random application X will not be confused by an
> unconditional switch to monotonic timestamps.
>
>> Wrt. to the drm vblank/pageflip timestamps, the userspace extensions
>> which expose these (INTEL_swap_events, OML_sync_control) don't allow
>> apps to select which timebase to use, they define monotonic time as what
>> is returned, so i don't know how a userspace app could actually ask the
>> DRM for one or the other format? So i guess just switching to
>> CLOCK_MONOTONIC shouldn't be that bad.
>
> An application could just use the kernel DRM interface directly. I admit
> this is not very likely but this is what should determine the rules by
> which we change the ABI.
>

Ok, that's a good point, agreed.
-mario

Re: [RFC 1/4] time: export getnstime_raw_and_real for DRM

[Imre Deak]

On Tue, 2012-10-09 at 01:00 +0200, Mario Kleiner wrote:
> On 08.10.12 13:35, Imre Deak wrote:
> > On Sat, 2012-10-06 at 03:41 +0200, Mario Kleiner wrote:
> >> [...]
> >>
> >> But then Psychtoolbox checks each timestamp it gets from somewhere
> >> "outside" (OML_sync_control / INTEL_swap_events / ALSA audio timestamps,
> >> network receive timestamps, evdev, x11, ...) if it is in gettimeofday()
> >> aka CLOCK_REALTIME aka wall time or in CLOCK_MONOTONIC time and just
> >> remaps to whatever its reference clock is.
> >>
> >> There's no way around this than to have no fixed expectations, but to
> >> remap stuff on the fly, because different parts of the Linux universe
> >> have decided on different time bases, or even switched somewhere from
> >> one kernel version to the next in the last years, e.g., ALSA, which at
> >> some time switched from wall clock to CLOCK_MONOTONIC. Sometimes
> >> clock_gettime() wasn't available at all in older setups, so there only
> >> was gettimeofday(). Or toolkits like GStreamer use different timebases
> >> dependent on OS and sometimes even on plugins.
> >>
> >> I would expect that other timing sensitive apps have to have ways to
> >> handle this in similar ways.
> >
> > I think the question is, can we be sure? I don't think there is any
> > guarantee that random application X will not be confused by an
> > unconditional switch to monotonic timestamps.
> >
> >> Wrt. to the drm vblank/pageflip timestamps, the userspace extensions
> >> which expose these (INTEL_swap_events, OML_sync_control) don't allow
> >> apps to select which timebase to use, they define monotonic time as what
> >> is returned, so i don't know how a userspace app could actually ask the
> >> DRM for one or the other format? So i guess just switching to
> >> CLOCK_MONOTONIC shouldn't be that bad.
> >
> > An application could just use the kernel DRM interface directly. I admit
> > this is not very likely but this is what should determine the rules by
> > which we change the ABI.
> >
> Ok, that's a good point, agreed.

Still pondering about this, since I wouldn't want to complicate things
unnecessarily by storing both timestamps.

Based on the feedback you guys gave, I now also think we could simply
switch over to monotonic timestamps and have a module flag to fall back
to the old behavior. This is what ALSA does and what V4L2 people are
planning to do as well. As you pointed out the mainstream users of this
interface already expect a monotonic timestamp and I couldn't find
either a good reason to support both formats at the same time.

--Imre

[RFC 0/4] drm: add raw monotonic timestamp support

[Imre Deak]

This is needed to make applications depending on vblank/page flip
timestamps independent of time ajdustments.

I've tested these with an updated intel-gpu-test/flip_test and will send
the update for that once there's no objection about this patchset.

The patchset is based on danvet's dinq branch with the following
additional patches from the intel-gfx ML applied: 
    drm/i915: paper over a pipe-enable vs pageflip race
    drm/i915: don't frob the vblank ts in finish_page_flip
    drm/i915: call drm_handle_vblank before finish_page_flip

Imre Deak (4):
  time: export getnstime_raw_and_real for DRM
  drm: make memset/calloc for _vblank_time more robust
  drm: use raw time in drm_calc_vbltimestamp_from_scanoutpos
  drm: add support for raw monotonic vblank timestamps

 drivers/gpu/drm/drm_crtc.c                |    2 +
 drivers/gpu/drm/drm_ioctl.c               |    3 ++
 drivers/gpu/drm/drm_irq.c                 |   83 ++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_irq.c           |    2 +-
 drivers/gpu/drm/i915/intel_display.c      |   12 ++---
 drivers/gpu/drm/radeon/radeon_display.c   |   10 ++--
 drivers/gpu/drm/radeon/radeon_drv.c       |    2 +-
 drivers/gpu/drm/radeon/radeon_kms.c       |    2 +-
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c |    9 ++--
 include/drm/drm.h                         |    5 +-
 include/drm/drmP.h                        |   38 +++++++++++--
 include/drm/drm_mode.h                    |    4 +-
 kernel/time/timekeeping.c                 |    2 +-
 13 files changed, 113 insertions(+), 61 deletions(-)

-- 
1.7.9.5

[RFC 1/4] time: export getnstime_raw_and_real for DRM

[Imre Deak]

Needed by the upcoming DRM raw monotonic timestamp support.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 kernel/time/timekeeping.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d3b91e7..073d262 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -365,7 +365,7 @@ void ktime_get_ts(struct timespec *ts)
 }
 EXPORT_SYMBOL_GPL(ktime_get_ts);
 
-#ifdef CONFIG_NTP_PPS
+#if defined(CONFIG_NTP_PPS) || defined(CONFIG_DRM) || defined(CONFIG_DRM_MODULE)
 
 /**
  * getnstime_raw_and_real - get day and raw monotonic time in timespec format
-- 
1.7.9.5

Re: [RFC 1/4] time: export getnstime_raw_and_real for DRM

[Kristian Høgsberg]

On Fri, Oct 5, 2012 at 9:36 AM, Imre Deak <imre.deak@intel.com> wrote:
> Needed by the upcoming DRM raw monotonic timestamp support.

I just had a quick look at driver/input/evdev.c, since evdev devices
did a similar change recently to allow evdev timestamp from the
monotonic clock.  They're using a different time API:

	time_mono = ktime_get();
        time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());

and

        event->time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
			                mono : real);

I'm not really up-to-date on kernel time APIs, but I wonder if that
may be better?  At least, I suspect we wouldn't need changes outside
drm if we use this API.

Kristian


> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  kernel/time/timekeeping.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index d3b91e7..073d262 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -365,7 +365,7 @@ void ktime_get_ts(struct timespec *ts)
>  }
>  EXPORT_SYMBOL_GPL(ktime_get_ts);
>
> -#ifdef CONFIG_NTP_PPS
> +#if defined(CONFIG_NTP_PPS) || defined(CONFIG_DRM) || defined(CONFIG_DRM_MODULE)
>
>  /**
>   * getnstime_raw_and_real - get day and raw monotonic time in timespec format
> --
> 1.7.9.5
>

Re: [RFC 1/4] time: export getnstime_raw_and_real for DRM

[Imre Deak]

On Fri, 2012-10-05 at 12:14 -0400, Kristian Høgsberg wrote:
> On Fri, Oct 5, 2012 at 9:36 AM, Imre Deak <imre.deak@intel.com> wrote:
> > Needed by the upcoming DRM raw monotonic timestamp support.
> 
> I just had a quick look at driver/input/evdev.c, since evdev devices
> did a similar change recently to allow evdev timestamp from the
> monotonic clock.  They're using a different time API:
> 
> 	time_mono = ktime_get();
>         time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());
> 
> and
> 
>         event->time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
> 			                mono : real);
> 
> I'm not really up-to-date on kernel time APIs, but I wonder if that
> may be better?  At least, I suspect we wouldn't need changes outside
> drm if we use this API.


Thanks, I will use this instead of getnstime_raw_and_real.

The reason is as discussed on #intel-gfx that this provides a monotonic
vs. raw monotonic time and that as you say it won't require a change in
the time keeping code.

--Imre

> 
> Kristian
> 
> 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  kernel/time/timekeeping.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index d3b91e7..073d262 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -365,7 +365,7 @@ void ktime_get_ts(struct timespec *ts)
> >  }
> >  EXPORT_SYMBOL_GPL(ktime_get_ts);
> >
> > -#ifdef CONFIG_NTP_PPS
> > +#if defined(CONFIG_NTP_PPS) || defined(CONFIG_DRM) || defined(CONFIG_DRM_MODULE)
> >
> >  /**
> >   * getnstime_raw_and_real - get day and raw monotonic time in timespec format
> > --
> > 1.7.9.5
> >


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel