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

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

[Mario Kleiner]

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,
-mario

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

[Imre Deak]

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