diff mbox

[1/2] drm: use monotonic time in drm_calc_vbltimestamp_from_scanoutpos

Message ID 1351018407-7009-2-git-send-email-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Imre Deak Oct. 23, 2012, 6:53 p.m. UTC
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@intel.com>
---
 drivers/gpu/drm/drm_irq.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Mario Kleiner Oct. 24, 2012, 11:05 p.m. UTC | #1
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@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);
>
>
Imre Deak Oct. 25, 2012, 10:28 a.m. UTC | #2
On Thu, 2012-10-25 at 01:05 +0200, Mario Kleiner wrote:
> 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@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?

I was also hold up by this for a while, since there is no function to
get both clocks atomically. But it isn't really a problem if you think
about it: etime - mono_time_offset is a correct wall time value
regardless whether mono_time_offset has changed or not after
ktime_get(). The only difference is whether the user sees the time value
before or after the adjustment, but you can't guard against that anyway
(except using monotonic time values always).

It would be a problem if as ktime_get() we would do the reverse and
calculate the monotonic time from the wall time. There not getting the
wall time and the wall_to_monotonic offset atomically could result in a
incorrect monotonic time value, for example one that jumps backwards.

--Imre

> 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);
> >
> >
Mario Kleiner Oct. 25, 2012, 7:45 p.m. UTC | #3
On 25.10.12 12:28, Imre Deak wrote:
> On Thu, 2012-10-25 at 01:05 +0200, Mario Kleiner wrote:
>> 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@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?
>
> I was also hold up by this for a while, since there is no function to
> get both clocks atomically. But it isn't really a problem if you think
> about it: etime - mono_time_offset is a correct wall time value
> regardless whether mono_time_offset has changed or not after
> ktime_get(). The only difference is whether the user sees the time value
> before or after the adjustment, but you can't guard against that anyway
> (except using monotonic time values always).
>
> It would be a problem if as ktime_get() we would do the reverse and
> calculate the monotonic time from the wall time. There not getting the
> wall time and the wall_to_monotonic offset atomically could result in a
> incorrect monotonic time value, for example one that jumps backwards.
>

Yes, agreed. Your patches should be good to go as they are - i like them :)

-mario


> --Imre
>
>> Other than that:
>>
>> Reviewed-by: mario.kleiner
>>
diff mbox

Patch

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();
+		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);