| Message ID | 1351018407-7009-2-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > >
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); > > > >
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 --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);
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(-)