On Sat, 2012-10-06 at 04:46 +0200, Mario Kleiner wrote:
> On 05.10.12 15:37, intel-gfx-request at lists.freedesktop.org wrote:
> >
> > Today's Topics:
> >
> > 1. [RFC 0/4] drm: add raw monotonic timestamp support (Imre Deak)
> > 2. [RFC 1/4] time: export getnstime_raw_and_real for DRM (Imre Deak)
> > 3. [RFC 2/4] drm: make memset/calloc for _vblank_time more
> > robust (Imre Deak)
> > 4. [RFC 3/4] drm: use raw time in
> > drm_calc_vbltimestamp_from_scanoutpos (Imre Deak)
> > 5. [RFC 4/4] drm: add support for raw monotonic vblank
> > timestamps (Imre Deak)
> >
> >
> > ----------------------------------------------------------------------
> >
> > Message: 1
> > Date: Fri, 5 Oct 2012 16:36:58 +0300
> > From: Imre Deak<imre.deak at intel.com>
> > To: Daniel Vetter<daniel.vetter at ffwll.ch>, Chris Wilson
> > <chris at chris-wilson.co.uk>, Kristian H?gsberg<krh at bitplanet.net>
> > Cc:intel-gfx at lists.freedesktop.org,dri-devel at lists.freedesktop.org
> > Subject: [RFC 0/4] drm: add raw monotonic timestamp
> > support
> > Message-ID:<1349444222-22274-1-git-send-email-imre.deak at intel.com>
> >
> > 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.
> >
>
> I'm mostly fine with this, although the wall time compatibility stuff
> may not be useful given that userspace apps, e.g., OpenGL clients, have
> no way to actually ask for wall vs. monotonic, and the spec actually
> expects them to expect monotonic timestamps.
>
> I also see that an update to nouveau-kms is missing? Afaik the vblank
> timestamping on nouveau-kms is still handled by the fallback in the drm,
> but pageflip completion uses do_gettimeofday() for the timestamping and
> returns a hard-coded zero vblank count all time for pageflip events
> (yay!). Lucas Stach and me have written and tested some patches to fix
> this over a year ago, but somehow they never made it into the kms
> driver, mostly due to bad timing in when stuff was submitted, reviewed
> and revised, not for serious technical objections iirc.
>
> Ideally we could give them another try, or at least update nouveaus
> pageflip timestamping to avoid really bad breakage for OpenGL clients or
> the nouveau-ddx due to inconsistent vblank vs. pageflip timestamps.
>
> I quickly looked over the patches, i think they look mostly good, see
> some small comments below.
>
> > Subject: [RFC 3/4] drm: use raw time in
> > drm_calc_vbltimestamp_from_scanoutpos
> > Message-ID: <1349444222-22274-4-git-send-email-imre.deak at intel.com>
> >
> > The timestamp is used here for handling the timeout case, so we don't
> > want it to be affected by time adjustments.
> >
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> > drivers/gpu/drm/drm_irq.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 77f6577..5e42981 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -576,7 +576,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
> > unsigned flags,
> > struct drm_crtc *refcrtc)
> > {
> > - struct timeval stime, raw_time;
> > + struct timespec raw_stime, raw_etime, real_etime;
> > struct drm_display_mode *mode;
> > int vbl_status, vtotal, vdisplay;
> > int vpos, hpos, i;
> > @@ -625,13 +625,13 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
> > preempt_disable();
> >
> > /* Get system timestamp before query. */
> > - do_gettimeofday(&stime);
> > + getrawmonotonic(&raw_stime);
> >
> > /* Get vertical and horizontal scanout pos. vpos, hpos. */
> > vbl_status = dev->driver->get_scanout_position(dev, crtc, &vpos, &hpos);
> >
> > /* Get system timestamp after query. */
> > - do_gettimeofday(&raw_time);
> > + getnstime_raw_and_real(&raw_etime, &real_etime);
> >
> > preempt_enable();
> >
> > @@ -642,7 +642,8 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
> > return -EIO;
> > }
> >
> > - duration_ns = timeval_to_ns(&raw_time) - timeval_to_ns(&stime);
> > + duration_ns = timespec_to_ns(&raw_etime) -
> > + timespec_to_ns(&raw_stime);
> >
> > /* Accept result with < max_error nsecs timing uncertainty. */
> > if (duration_ns <= (s64) *max_error)
> > @@ -692,11 +693,11 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
> > /* Subtract time delta from raw timestamp to get final
> > * vblank_time timestamp for end of vblank.
> > */
> > - *vblank_time = ns_to_timeval(timeval_to_ns(&raw_time) - delta_ns);
> > + *vblank_time = ns_to_timeval(timeval_to_ns(&real_time) - delta_ns);
>
> This commit without the followup commit wouldn't compile, because you
> changed real_time into real_etime. Your followup commit fixes this, so
> squash them into one to avoid compilation problems for bisection?
>
> Then, for the followup patch "[RFC 4/4] drm: add support for raw
> monotonic vblank timestamps"
>
> > @@ -693,12 +694,13 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
> > /* Subtract time delta from raw timestamp to get final
> > * vblank_time timestamp for end of vblank.
> > */
> > - *vblank_time = ns_to_timeval(timeval_to_ns(&real_time) - delta_ns);
> > + vblank_time->real = ns_to_timeval(timespec_to_ns(&real_etime) - delta_ns);
> > + vblank_time->raw = ns_to_timeval(timespec_to_ns(&raw_etime) - delta_ns);
> >
> > DRM_DEBUG("crtc %d : v %d p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
> > crtc, (int)vbl_status, hpos, vpos,
> > (long)raw_stime.tv_sec, (long)raw_stime.tv_nsec / 1000,
>
> This shouldn't be raw_stime, but real_etime, so the debug output of
> these vars can be compared to the values of vblank_time->real.tv_sec etc.
>
> > - (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
> > + (long)vblank_time->real.tv_sec, (long)vblank_time->real.tv_usec,
> > (int)duration_ns/1000, i);
Thanks for the review, I'll fix both issues.
--Imre
