Re: [RFC 0/4] drm: add raw monotonic timestamp support (Imre Deak)

[Imre Deak <imre.deak@intel.com>]

On Sat, 2012-10-06 at 04:46 +0200, Mario Kleiner wrote:
> On 05.10.12 15:37, intel-gfx-request@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@intel.com>
> > To: Daniel Vetter<daniel.vetter@ffwll.ch>,	Chris Wilson
> > 	<chris@chris-wilson.co.uk>, Kristian H?gsberg<krh@bitplanet.net>
> > Cc:intel-gfx@lists.freedesktop.org,dri-devel@lists.freedesktop.org
> > Subject: [Intel-gfx] [RFC 0/4] drm: add raw monotonic timestamp
> > 	support
> > Message-ID:<1349444222-22274-1-git-send-email-imre.deak@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: [Intel-gfx] [RFC 3/4] drm: use raw time in
> > 	drm_calc_vbltimestamp_from_scanoutpos
> > Message-ID: <1349444222-22274-4-git-send-email-imre.deak@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@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