[Intel-gfx] [PATCH 1/2] drm: use monotonic time in drm_calc_vbltimestamp_from_scanoutpos

[Mario Kleiner]

On 23.10.12 20:53, Imre Deak wrote:
> For measuring duration we want to avoid that our start/end timestamps
> jump, so use monotonic instead of real time for that.
>
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
>   drivers/gpu/drm/drm_irq.c |   18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 89b830d..7dc203d 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -576,7 +576,8 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>   					  unsigned flags,
>   					  struct drm_crtc *refcrtc)
>   {
> -	struct timeval stime, raw_time;
> +	ktime_t stime, etime, mono_time_offset;
> +	struct timeval tv_etime;
>   	struct drm_display_mode *mode;
>   	int vbl_status, vtotal, vdisplay;
>   	int vpos, hpos, i;
> @@ -625,13 +626,14 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>   		preempt_disable();
>
>   		/* Get system timestamp before query. */
> -		do_gettimeofday(&stime);
> +		stime = ktime_get();
>
>   		/* 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);
> +		etime = ktime_get();

Here is possibly a tiny race: The wall_to_monotonic offset value could 
change between the ktime_get() - which uses it internally for wallclock 
-> monotonic clock conversion, and the ktime_get_monotonic_offset() 
query below, so the later subtraction of mono_time_offset from etime 
would not cancel out the addition to etime inside ktime_get() and you 
wouldn't get correct walltime back. There seem to be multiple sources of 
change to the value, e.g., do_settimeofday(), do_adjtimex() - the admin 
or ntp changing the system clock. The internal code, e.g., ktime_get() 
use a seqlock to protect against this race.

There's a function ktime_get_real(void) which directly gives you the 
wall time you want as ktime_t, but then you'd still need to do the 
ktime_get() query in the !drm_timestamp_monotonic case to calculate 
duration_ns below.

Same problem in the 2nd patch for get_drm_timestamp(). Otoh, the time 
window for the race is small and it can only happen in the non-default 
case of !drm_timestamp_monotonic, so i don't know if it is worth fixing it?

Other than that:

Reviewed-by: mario.kleiner

> +		mono_time_offset = ktime_get_monotonic_offset();
>
>   		preempt_enable();
>
> @@ -642,7 +644,7 @@ 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 = ktime_to_ns(etime) - ktime_to_ns(stime);
>
>   		/* Accept result with <  max_error nsecs timing uncertainty. */
>   		if (duration_ns <= (s64) *max_error)
> @@ -689,14 +691,18 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>   		vbl_status |= 0x8;
>   	}
>
> +	etime = ktime_sub(etime, mono_time_offset);
> +	/* save this only for debugging purposes */
> +	tv_etime = ktime_to_timeval(etime);
>   	/* 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);
> +	etime = ktime_sub_ns(etime, delta_ns);
> +	*vblank_time = ktime_to_timeval(etime);
>
>   	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_time.tv_sec, (long)raw_time.tv_usec,
> +		  (long)tv_etime.tv_sec, (long)tv_etime.tv_usec,
>   		  (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
>   		  (int)duration_ns/1000, i);
>
>