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@xxxxxxxxx>
---
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
