[RFC 0/4] drm: add raw monotonic timestamp support

[RFC 0/4] drm: add raw monotonic timestamp support

[Imre Deak]

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.

The patchset is based on danvet's dinq branch with the following
additional patches from the intel-gfx ML applied: 
    drm/i915: paper over a pipe-enable vs pageflip race
    drm/i915: don't frob the vblank ts in finish_page_flip
    drm/i915: call drm_handle_vblank before finish_page_flip

Imre Deak (4):
  time: export getnstime_raw_and_real for DRM
  drm: make memset/calloc for _vblank_time more robust
  drm: use raw time in drm_calc_vbltimestamp_from_scanoutpos
  drm: add support for raw monotonic vblank timestamps

 drivers/gpu/drm/drm_crtc.c                |    2 +
 drivers/gpu/drm/drm_ioctl.c               |    3 ++
 drivers/gpu/drm/drm_irq.c                 |   83 ++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_irq.c           |    2 +-
 drivers/gpu/drm/i915/intel_display.c      |   12 ++---
 drivers/gpu/drm/radeon/radeon_display.c   |   10 ++--
 drivers/gpu/drm/radeon/radeon_drv.c       |    2 +-
 drivers/gpu/drm/radeon/radeon_kms.c       |    2 +-
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c |    9 ++--
 include/drm/drm.h                         |    5 +-
 include/drm/drmP.h                        |   38 +++++++++++--
 include/drm/drm_mode.h                    |    4 +-
 kernel/time/timekeeping.c                 |    2 +-
 13 files changed, 113 insertions(+), 61 deletions(-)

-- 
1.7.9.5

[RFC 1/4] time: export getnstime_raw_and_real for DRM

[Imre Deak]

Needed by the upcoming DRM raw monotonic timestamp support.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 kernel/time/timekeeping.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d3b91e7..073d262 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -365,7 +365,7 @@ void ktime_get_ts(struct timespec *ts)
 }
 EXPORT_SYMBOL_GPL(ktime_get_ts);
 
-#ifdef CONFIG_NTP_PPS
+#if defined(CONFIG_NTP_PPS) || defined(CONFIG_DRM) || defined(CONFIG_DRM_MODULE)
 
 /**
  * getnstime_raw_and_real - get day and raw monotonic time in timespec format
-- 
1.7.9.5

[RFC 2/4] drm: make memset/calloc for _vblank_time more robust

[Imre Deak]

Using sizeof(*var) instead of sizeof(var_type) allows changing var_type
w/o breaking callers that depend on the size.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_irq.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 076c4a8..77f6577 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -90,7 +90,7 @@ int drm_irq_by_busid(struct drm_device *dev, void *data,
 static void clear_vblank_timestamps(struct drm_device *dev, int crtc)
 {
 	memset(&dev->_vblank_time[crtc * DRM_VBLANKTIME_RBSIZE], 0,
-		DRM_VBLANKTIME_RBSIZE * sizeof(struct timeval));
+		DRM_VBLANKTIME_RBSIZE * sizeof(dev->_vblank_time[0]));
 }
 
 /*
@@ -248,7 +248,7 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
 		goto err;
 
 	dev->_vblank_time = kcalloc(num_crtcs * DRM_VBLANKTIME_RBSIZE,
-				    sizeof(struct timeval), GFP_KERNEL);
+				    sizeof(dev->_vblank_time[0]), GFP_KERNEL);
 	if (!dev->_vblank_time)
 		goto err;
 
-- 
1.7.9.5

[RFC 3/4] drm: use raw time in drm_calc_vbltimestamp_from_scanoutpos

[Imre Deak]

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);
 
 	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)raw_stime.tv_sec, (long)raw_stime.tv_nsec / 1000,
 		  (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
 		  (int)duration_ns/1000, i);
 
-- 
1.7.9.5

[RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Imre Deak]

In practice we never want the timestamps for vblank and page flip events
to be affected by time adjustments, so in addition to the gettimeofday
timestamps we used so far add support for raw monotonic timestamps.

For backward compatibility use flags to select between the old and new
timestamp format.

Note that with this change we will save the timestamp in both formats,
for cases where multiple clients are expecting an event notification in
different time formats. In theory we could track the clients and save
the timestamp only in one format if possible, but the overhead of this
tracking would be bigger than just saving both formats always.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_crtc.c                |    2 +
 drivers/gpu/drm/drm_ioctl.c               |    3 ++
 drivers/gpu/drm/drm_irq.c                 |   68 +++++++++++++++++------------
 drivers/gpu/drm/i915/i915_irq.c           |    2 +-
 drivers/gpu/drm/i915/intel_display.c      |   12 ++---
 drivers/gpu/drm/radeon/radeon_display.c   |   10 +++--
 drivers/gpu/drm/radeon/radeon_drv.c       |    2 +-
 drivers/gpu/drm/radeon/radeon_kms.c       |    2 +-
 drivers/gpu/drm/shmobile/shmob_drm_crtc.c |    9 ++--
 include/drm/drm.h                         |    5 ++-
 include/drm/drmP.h                        |   38 +++++++++++++---
 include/drm/drm_mode.h                    |    4 +-
 12 files changed, 104 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index c317f72..e492363 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3550,6 +3550,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			goto out;
 		}
 
+		e->timestamp_raw = page_flip->flags &
+				   DRM_MODE_PAGE_FLIP_TIMESTAMP_RAW;
 		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
 		e->event.base.length = sizeof e->event;
 		e->event.user_data = page_flip->user_data;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 64a62c6..0a30299 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -287,6 +287,9 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 		req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
 		req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0;
 		break;
+	case DRM_CAP_TIMESTAMP_RAW:
+		req->value = 1;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 5e42981..4879363 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -105,7 +105,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	u32 vblcount;
 	s64 diff_ns;
 	int vblrc;
-	struct timeval tvblank;
+	struct drm_vblank_time tvblank;
 
 	/* Prevent vblank irq processing while disabling vblank irqs,
 	 * so no updates of timestamps or count can happen after we've
@@ -137,8 +137,8 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	 * as updated by last invocation of drm_handle_vblank() in vblank irq.
 	 */
 	vblcount = atomic_read(&dev->_vblank_count[crtc]);
-	diff_ns = timeval_to_ns(&tvblank) -
-		  timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount));
+	diff_ns = timeval_to_ns(&tvblank.raw) -
+		  timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount).raw);
 
 	/* If there is at least 1 msec difference between the last stored
 	 * timestamp and tvblank, then we are currently executing our
@@ -550,7 +550,8 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  * @crtc: Which crtc's vblank timestamp to retrieve.
  * @max_error: Desired maximum allowable error in timestamps (nanosecs).
  *             On return contains true maximum error of timestamp.
- * @vblank_time: Pointer to struct timeval which should receive the timestamp.
+ * @vblank_time: Pointer to struct drm_vblank_time which should receive the
+ *		 timestamp both in raw monotonic and wall-time format.
  * @flags: Flags to pass to driver:
  *         0 = Default.
  *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler.
@@ -572,7 +573,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  */
 int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 					  int *max_error,
-					  struct timeval *vblank_time,
+					  struct drm_vblank_time *vblank_time,
 					  unsigned flags,
 					  struct drm_crtc *refcrtc)
 {
@@ -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,
-		  (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);
 
 	vbl_status = DRM_VBLANKTIME_SCANOUTPOS_METHOD;
@@ -730,8 +732,9 @@ EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
  * Returns non-zero if timestamp is considered to be very precise.
  */
 u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
-			      struct timeval *tvblank, unsigned flags)
+			      struct drm_vblank_time *tvblank, unsigned flags)
 {
+	struct timespec ts_raw, ts_real;
 	int ret;
 
 	/* Define requested maximum error on timestamps (nanoseconds). */
@@ -748,7 +751,13 @@ u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
 	/* GPU high precision timestamp query unsupported or failed.
 	 * Return gettimeofday timestamp as best estimate.
 	 */
-	do_gettimeofday(tvblank);
+	getnstime_raw_and_real(&ts_raw, &ts_real);
+
+	tvblank->raw.tv_sec = ts_raw.tv_sec;
+	tvblank->raw.tv_usec = ts_raw.tv_nsec / 1000;
+
+	tvblank->real.tv_sec = ts_real.tv_sec;
+	tvblank->real.tv_usec = ts_real.tv_nsec / 1000;
 
 	return 0;
 }
@@ -784,7 +793,7 @@ EXPORT_SYMBOL(drm_vblank_count);
  * value.
  */
 u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
-			      struct timeval *vblanktime)
+			      struct drm_vblank_time *vblanktime)
 {
 	u32 cur_vblank;
 
@@ -822,7 +831,7 @@ EXPORT_SYMBOL(drm_vblank_count_and_time);
 static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 {
 	u32 cur_vblank, diff, tslot, rc;
-	struct timeval t_vblank;
+	struct drm_vblank_time t_vblank;
 
 	/*
 	 * Interrupts were disabled prior to this call, so deal with counter
@@ -940,7 +949,7 @@ EXPORT_SYMBOL(drm_vblank_put);
 void drm_vblank_off(struct drm_device *dev, int crtc)
 {
 	struct drm_pending_vblank_event *e, *t;
-	struct timeval now;
+	struct drm_vblank_time now;
 	unsigned long irqflags;
 	unsigned int seq;
 
@@ -957,9 +966,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 			  wanted %d, current %d\n",
 			  e->event.sequence, seq);
 
-		e->event.sequence = seq;
-		e->event.tv_sec = now.tv_sec;
-		e->event.tv_usec = now.tv_usec;
+		drm_set_event_seq_and_time(&e->event, e->timestamp_raw, seq,
+					   &now);
 		drm_vblank_put(dev, e->pipe);
 		list_move_tail(&e->base.link, &e->base.file_priv->event_list);
 		wake_up_interruptible(&e->base.file_priv->event_wait);
@@ -1064,7 +1072,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 				  struct drm_file *file_priv)
 {
 	struct drm_pending_vblank_event *e;
-	struct timeval now;
+	struct drm_vblank_time now;
 	unsigned long flags;
 	unsigned int seq;
 	int ret;
@@ -1076,6 +1084,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 	}
 
 	e->pipe = pipe;
+	e->timestamp_raw = vblwait->request.type & _DRM_VBLANK_TIMESTAMP_RAW;
 	e->base.pid = current->pid;
 	e->event.base.type = DRM_EVENT_VBLANK;
 	e->event.base.length = sizeof e->event;
@@ -1108,9 +1117,8 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 
 	e->event.sequence = vblwait->request.sequence;
 	if ((seq - vblwait->request.sequence) <= (1 << 23)) {
-		e->event.sequence = seq;
-		e->event.tv_sec = now.tv_sec;
-		e->event.tv_usec = now.tv_usec;
+		drm_set_event_seq_and_time(&e->event, e->timestamp_raw, seq,
+					   &now);
 		drm_vblank_put(dev, pipe);
 		list_add_tail(&e->base.link, &e->base.file_priv->event_list);
 		wake_up_interruptible(&e->base.file_priv->event_wait);
@@ -1220,11 +1228,14 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 		     !dev->irq_enabled));
 
 	if (ret != -EINTR) {
-		struct timeval now;
+		struct drm_vblank_time now;
+		struct timeval *tv;
 
 		vblwait->reply.sequence = drm_vblank_count_and_time(dev, crtc, &now);
-		vblwait->reply.tval_sec = now.tv_sec;
-		vblwait->reply.tval_usec = now.tv_usec;
+		tv = vblwait->request.type & _DRM_VBLANK_TIMESTAMP_RAW ?
+			&now.raw : &now.real;
+		vblwait->reply.tval_sec = tv->tv_sec;
+		vblwait->reply.tval_usec = tv->tv_usec;
 
 		DRM_DEBUG("returning %d to client\n",
 			  vblwait->reply.sequence);
@@ -1240,7 +1251,7 @@ done:
 static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
 {
 	struct drm_pending_vblank_event *e, *t;
-	struct timeval now;
+	struct drm_vblank_time now;
 	unsigned long flags;
 	unsigned int seq;
 
@@ -1257,9 +1268,8 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
 		DRM_DEBUG("vblank event on %d, current %d\n",
 			  e->event.sequence, seq);
 
-		e->event.sequence = seq;
-		e->event.tv_sec = now.tv_sec;
-		e->event.tv_usec = now.tv_usec;
+		drm_set_event_seq_and_time(&e->event, e->timestamp_raw,
+					   seq, &now);
 		drm_vblank_put(dev, e->pipe);
 		list_move_tail(&e->base.link, &e->base.file_priv->event_list);
 		wake_up_interruptible(&e->base.file_priv->event_wait);
@@ -1284,7 +1294,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
 {
 	u32 vblcount;
 	s64 diff_ns;
-	struct timeval tvblank;
+	struct drm_vblank_time tvblank;
 	unsigned long irqflags;
 
 	if (!dev->num_crtcs)
@@ -1311,8 +1321,8 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
 	drm_get_last_vbltimestamp(dev, crtc, &tvblank, DRM_CALLED_FROM_VBLIRQ);
 
 	/* Compute time difference to timestamp of last vblank */
-	diff_ns = timeval_to_ns(&tvblank) -
-		  timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount));
+	diff_ns = timeval_to_ns(&tvblank.raw) -
+		  timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount).raw);
 
 	/* Update vblank timestamp and count if at least
 	 * DRM_REDUNDANT_VBLIRQ_THRESH_NS nanoseconds
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2dd5895..e3a75e3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -243,7 +243,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 
 static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 			      int *max_error,
-			      struct timeval *vblank_time,
+			      struct drm_vblank_time *vblank_time,
 			      unsigned flags)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ab1ef15..056e810 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6247,7 +6247,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
 	struct intel_unpin_work *work;
 	struct drm_i915_gem_object *obj;
 	struct drm_pending_vblank_event *e;
-	struct timeval tvbl;
 	unsigned long flags;
 
 	/* Ignore early vblank irqs */
@@ -6264,12 +6263,13 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
 	intel_crtc->unpin_work = NULL;
 
 	if (work->event) {
-		e = work->event;
-		e->event.sequence = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
-
-		e->event.tv_sec = tvbl.tv_sec;
-		e->event.tv_usec = tvbl.tv_usec;
+		struct drm_vblank_time tvbl;
+		u32 seq;
 
+		e = work->event;
+		seq = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
+		drm_set_event_seq_and_time(&e->event, e->timestamp_raw, seq,
+					   &tvbl);
 		list_add_tail(&e->base.link,
 			      &e->base.file_priv->event_list);
 		wake_up_interruptible(&e->base.file_priv->event_wait);
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 7ddef8f..55c014a 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -272,7 +272,6 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 	struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
 	struct radeon_unpin_work *work;
 	struct drm_pending_vblank_event *e;
-	struct timeval now;
 	unsigned long flags;
 	u32 update_pending;
 	int vpos, hpos;
@@ -329,10 +328,13 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 
 	/* wakeup userspace */
 	if (work->event) {
+		struct drm_vblank_time now;
+		u32 seq;
+
 		e = work->event;
-		e->event.sequence = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
-		e->event.tv_sec = now.tv_sec;
-		e->event.tv_usec = now.tv_usec;
+		seq = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
+		drm_set_event_seq_and_time(&e->event, e->timestamp_raw,
+					   seq, &now);
 		list_add_tail(&e->base.link, &e->base.file_priv->event_list);
 		wake_up_interruptible(&e->base.file_priv->event_wait);
 	}
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 8c593ea..f1825d2 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -84,7 +84,7 @@ int radeon_enable_vblank_kms(struct drm_device *dev, int crtc);
 void radeon_disable_vblank_kms(struct drm_device *dev, int crtc);
 int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
 				    int *max_error,
-				    struct timeval *vblank_time,
+				    struct drm_vblank_time *vblank_time,
 				    unsigned flags);
 void radeon_driver_irq_preinstall_kms(struct drm_device *dev);
 int radeon_driver_irq_postinstall_kms(struct drm_device *dev);
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index 414b4ac..3229989 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -564,7 +564,7 @@ void radeon_disable_vblank_kms(struct drm_device *dev, int crtc)
  */
 int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
 				    int *max_error,
-				    struct timeval *vblank_time,
+				    struct drm_vblank_time *vblank_time,
 				    unsigned flags)
 {
 	struct drm_crtc *drmcrtc;
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
index 0e7a930..9eb451f 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
@@ -451,7 +451,8 @@ void shmob_drm_crtc_finish_page_flip(struct shmob_drm_crtc *scrtc)
 {
 	struct drm_pending_vblank_event *event;
 	struct drm_device *dev = scrtc->crtc.dev;
-	struct timeval vblanktime;
+	struct drm_vblank_time vblanktime;
+	u32 seq;
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev->event_lock, flags);
@@ -462,9 +463,9 @@ void shmob_drm_crtc_finish_page_flip(struct shmob_drm_crtc *scrtc)
 	if (event == NULL)
 		return;
 
-	event->event.sequence = drm_vblank_count_and_time(dev, 0, &vblanktime);
-	event->event.tv_sec = vblanktime.tv_sec;
-	event->event.tv_usec = vblanktime.tv_usec;
+	seq = drm_vblank_count_and_time(dev, 0, &vblanktime);
+	drm_set_event_seq_and_time(&event->event, event->timestamp_raw,
+				   seq, &vblanktime);
 
 	spin_lock_irqsave(&dev->event_lock, flags);
 	list_add_tail(&event->base.link, &event->base.file_priv->event_list);
diff --git a/include/drm/drm.h b/include/drm/drm.h
index e51035a..3eb425c 100644
--- a/include/drm/drm.h
+++ b/include/drm/drm.h
@@ -465,6 +465,7 @@ enum drm_vblank_seq_type {
 	_DRM_VBLANK_RELATIVE = 0x1,	/**< Wait for given number of vblanks */
 	/* bits 1-6 are reserved for high crtcs */
 	_DRM_VBLANK_HIGH_CRTC_MASK = 0x0000003e,
+	_DRM_VBLANK_TIMESTAMP_RAW = 0x2000000,	/**< return raw monotonic time */
 	_DRM_VBLANK_EVENT = 0x4000000,   /**< Send event instead of blocking */
 	_DRM_VBLANK_FLIP = 0x8000000,   /**< Scheduled buffer swap should flip */
 	_DRM_VBLANK_NEXTONMISS = 0x10000000,	/**< If missed, wait for next vblank */
@@ -475,7 +476,8 @@ enum drm_vblank_seq_type {
 
 #define _DRM_VBLANK_TYPES_MASK (_DRM_VBLANK_ABSOLUTE | _DRM_VBLANK_RELATIVE)
 #define _DRM_VBLANK_FLAGS_MASK (_DRM_VBLANK_EVENT | _DRM_VBLANK_SIGNAL | \
-				_DRM_VBLANK_SECONDARY | _DRM_VBLANK_NEXTONMISS)
+				_DRM_VBLANK_SECONDARY | _DRM_VBLANK_NEXTONMISS | \
+				_DRM_VBLANK_TIMESTAMP_RAW)
 
 struct drm_wait_vblank_request {
 	enum drm_vblank_seq_type type;
@@ -778,6 +780,7 @@ struct drm_event_vblank {
 #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
 #define DRM_CAP_DUMB_PREFER_SHADOW 0x4
 #define DRM_CAP_PRIME 0x5
+#define DRM_CAP_TIMESTAMP_RAW 0x6
 
 #define DRM_PRIME_CAP_IMPORT 0x1
 #define DRM_PRIME_CAP_EXPORT 0x2
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d5f0c16..e2a6599 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -730,6 +730,11 @@ struct drm_bus {
 
 };
 
+struct drm_vblank_time {
+	struct timeval real;
+	struct timeval raw;
+};
+
 /**
  * DRM driver structure. This structure represent the common code for
  * a family of cards. There will one drm_device for each card present
@@ -866,7 +871,7 @@ struct drm_driver {
 	 */
 	int (*get_vblank_timestamp) (struct drm_device *dev, int crtc,
 				     int *max_error,
-				     struct timeval *vblank_time,
+				     struct drm_vblank_time *vblank_time,
 				     unsigned flags);
 
 	/* these have to be filled in */
@@ -1047,6 +1052,7 @@ struct drm_cmdline_mode {
 struct drm_pending_vblank_event {
 	struct drm_pending_event base;
 	int pipe;
+	bool timestamp_raw;
 	struct drm_event_vblank event;
 };
 
@@ -1132,7 +1138,14 @@ struct drm_device {
 
 	wait_queue_head_t *vbl_queue;   /**< VBLANK wait queue */
 	atomic_t *_vblank_count;        /**< number of VBLANK interrupts (driver must alloc the right number of counters) */
-	struct timeval *_vblank_time;   /**< timestamp of current vblank_count (drivers must alloc right number of fields) */
+
+	/*
+	 * timestamp of current vblank_count (drivers must alloc right number
+	 * of fields). We use a raw monotonic timestamp, but for backward
+	 * compatibility with clients depending on the wall time we also store
+	 * the GTOD value.
+	 */
+	struct drm_vblank_time *_vblank_time;
 	spinlock_t vblank_time_lock;    /**< Protects vblank count and time updates during vblank enable/disable */
 	spinlock_t vbl_lock;
 	atomic_t *vblank_refcount;      /* number of users of vblank interruptsper crtc */
@@ -1429,17 +1442,18 @@ extern int drm_wait_vblank(struct drm_device *dev, void *data,
 extern int drm_vblank_wait(struct drm_device *dev, unsigned int *vbl_seq);
 extern u32 drm_vblank_count(struct drm_device *dev, int crtc);
 extern u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
-				     struct timeval *vblanktime);
+				     struct drm_vblank_time *vblanktime);
 extern bool drm_handle_vblank(struct drm_device *dev, int crtc);
 extern int drm_vblank_get(struct drm_device *dev, int crtc);
 extern void drm_vblank_put(struct drm_device *dev, int crtc);
 extern void drm_vblank_off(struct drm_device *dev, int crtc);
 extern void drm_vblank_cleanup(struct drm_device *dev);
 extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
-				     struct timeval *tvblank, unsigned flags);
+				     struct drm_vblank_time *tvblank,
+				     unsigned flags);
 extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 						 int crtc, int *max_error,
-						 struct timeval *vblank_time,
+						 struct drm_vblank_time *vblank_time,
 						 unsigned flags,
 						 struct drm_crtc *refcrtc);
 extern void drm_calc_timestamping_constants(struct drm_crtc *crtc);
@@ -1771,5 +1785,19 @@ static __inline__ bool drm_can_sleep(void)
 	return true;
 }
 
+static inline void drm_set_event_seq_and_time(struct drm_event_vblank *e,
+					      bool timestamp_raw, u32 seq,
+					      struct drm_vblank_time *time)
+{
+	e->sequence = seq;
+	if (timestamp_raw) {
+		e->tv_sec = time->raw.tv_sec;
+		e->tv_usec = time->raw.tv_usec;
+	} else {
+		e->tv_sec = time->real.tv_sec;
+		e->tv_usec = time->real.tv_usec;
+	}
+}
+
 #endif				/* __KERNEL__ */
 #endif
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index 3d6301b..b493011 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -399,7 +399,9 @@ struct drm_mode_crtc_lut {
 };
 
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
-#define DRM_MODE_PAGE_FLIP_FLAGS DRM_MODE_PAGE_FLIP_EVENT
+#define DRM_MODE_PAGE_FLIP_TIMESTAMP_RAW 0x02
+#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
+				  DRM_MODE_PAGE_FLIP_TIMESTAMP_RAW)
 
 /*
  * Request a page flip on the specified crtc.
-- 
1.7.9.5

Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Michel Dänzer]

On Fre, 2012-10-05 at 16:37 +0300, Imre Deak wrote: 
> In practice we never want the timestamps for vblank and page flip events
> to be affected by time adjustments, so in addition to the gettimeofday
> timestamps we used so far add support for raw monotonic timestamps.
> 
> For backward compatibility use flags to select between the old and new
> timestamp format.
> 
> Note that with this change we will save the timestamp in both formats,
> for cases where multiple clients are expecting an event notification in
> different time formats.

I wonder if all this trouble is really necessary. I honestly can't
imagine any user of this API requiring non-monotonic timestamps and
breaking with monotonic ones. I think it was simply a mistake that we
didn't make them monotonic in the first place (or maybe it wasn't even
possible when this API was first introduced).


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer

Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Imre Deak]

On Fri, 2012-10-05 at 15:55 +0200, Michel Dänzer wrote:
> On Fre, 2012-10-05 at 16:37 +0300, Imre Deak wrote: 
> > In practice we never want the timestamps for vblank and page flip events
> > to be affected by time adjustments, so in addition to the gettimeofday
> > timestamps we used so far add support for raw monotonic timestamps.
> > 
> > For backward compatibility use flags to select between the old and new
> > timestamp format.
> > 
> > Note that with this change we will save the timestamp in both formats,
> > for cases where multiple clients are expecting an event notification in
> > different time formats.
> 
> I wonder if all this trouble is really necessary. I honestly can't
> imagine any user of this API requiring non-monotonic timestamps and
> breaking with monotonic ones. I think it was simply a mistake that we
> didn't make them monotonic in the first place (or maybe it wasn't even
> possible when this API was first introduced).

Yea, I'd rather simply switch over to monotonic timestamps too. But that
would break apps that already compare against the wall time for whatever
purpose (for example A/V sync).

--Imre


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Michel Dänzer]

On Fre, 2012-10-05 at 16:59 +0300, Imre Deak wrote: 
> On Fri, 2012-10-05 at 15:55 +0200, Michel Dänzer wrote:
> > On Fre, 2012-10-05 at 16:37 +0300, Imre Deak wrote: 
> > > In practice we never want the timestamps for vblank and page flip events
> > > to be affected by time adjustments, so in addition to the gettimeofday
> > > timestamps we used so far add support for raw monotonic timestamps.
> > > 
> > > For backward compatibility use flags to select between the old and new
> > > timestamp format.
> > > 
> > > Note that with this change we will save the timestamp in both formats,
> > > for cases where multiple clients are expecting an event notification in
> > > different time formats.
> > 
> > I wonder if all this trouble is really necessary. I honestly can't
> > imagine any user of this API requiring non-monotonic timestamps and
> > breaking with monotonic ones. I think it was simply a mistake that we
> > didn't make them monotonic in the first place (or maybe it wasn't even
> > possible when this API was first introduced).
> 
> Yea, I'd rather simply switch over to monotonic timestamps too. But that
> would break apps that already compare against the wall time for whatever
> purpose (for example A/V sync).

Are there actually any such apps in the real world? Do they work when
the wall time jumps?


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer

Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Imre Deak]

On Fri, 2012-10-05 at 16:14 +0200, Michel Dänzer wrote:
> On Fre, 2012-10-05 at 16:59 +0300, Imre Deak wrote: 
> > On Fri, 2012-10-05 at 15:55 +0200, Michel Dänzer wrote:
> > > On Fre, 2012-10-05 at 16:37 +0300, Imre Deak wrote: 
> > > > In practice we never want the timestamps for vblank and page flip events
> > > > to be affected by time adjustments, so in addition to the gettimeofday
> > > > timestamps we used so far add support for raw monotonic timestamps.
> > > > 
> > > > For backward compatibility use flags to select between the old and new
> > > > timestamp format.
> > > > 
> > > > Note that with this change we will save the timestamp in both formats,
> > > > for cases where multiple clients are expecting an event notification in
> > > > different time formats.
> > > 
> > > I wonder if all this trouble is really necessary. I honestly can't
> > > imagine any user of this API requiring non-monotonic timestamps and
> > > breaking with monotonic ones. I think it was simply a mistake that we
> > > didn't make them monotonic in the first place (or maybe it wasn't even
> > > possible when this API was first introduced).
> > 
> > Yea, I'd rather simply switch over to monotonic timestamps too. But that
> > would break apps that already compare against the wall time for whatever
> > purpose (for example A/V sync).
> 
> Are there actually any such apps in the real world?

Tbh, I haven't checked, but we can't be sure in any case.

> Do they work when the wall time jumps?

They will have a problem with that yes. But providing them with a
monotonic clock when they expect otherwise will probably make them
unusable, so I think it's better to avoid that.

--Imre


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/4] time: export getnstime_raw_and_real for DRM

[Kristian Høgsberg]

On Fri, Oct 5, 2012 at 9:36 AM, Imre Deak <imre.deak@intel.com> wrote:
> Needed by the upcoming DRM raw monotonic timestamp support.

I just had a quick look at driver/input/evdev.c, since evdev devices
did a similar change recently to allow evdev timestamp from the
monotonic clock.  They're using a different time API:

	time_mono = ktime_get();
        time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());

and

        event->time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
			                mono : real);

I'm not really up-to-date on kernel time APIs, but I wonder if that
may be better?  At least, I suspect we wouldn't need changes outside
drm if we use this API.

Kristian


> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  kernel/time/timekeeping.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index d3b91e7..073d262 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -365,7 +365,7 @@ void ktime_get_ts(struct timespec *ts)
>  }
>  EXPORT_SYMBOL_GPL(ktime_get_ts);
>
> -#ifdef CONFIG_NTP_PPS
> +#if defined(CONFIG_NTP_PPS) || defined(CONFIG_DRM) || defined(CONFIG_DRM_MODULE)
>
>  /**
>   * getnstime_raw_and_real - get day and raw monotonic time in timespec format
> --
> 1.7.9.5
>

Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Rob Clark]

On Fri, Oct 5, 2012 at 7:37 AM, Imre Deak <imre.deak@intel.com> wrote:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ab1ef15..056e810 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6247,7 +6247,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>         struct intel_unpin_work *work;
>         struct drm_i915_gem_object *obj;
>         struct drm_pending_vblank_event *e;
> -       struct timeval tvbl;
>         unsigned long flags;
>
>         /* Ignore early vblank irqs */
> @@ -6264,12 +6263,13 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>         intel_crtc->unpin_work = NULL;
>
>         if (work->event) {
> -               e = work->event;
> -               e->event.sequence = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
> -
> -               e->event.tv_sec = tvbl.tv_sec;
> -               e->event.tv_usec = tvbl.tv_usec;
> +               struct drm_vblank_time tvbl;
> +               u32 seq;
>
> +               e = work->event;
> +               seq = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
> +               drm_set_event_seq_and_time(&e->event, e->timestamp_raw, seq,
> +                                          &tvbl);
>                 list_add_tail(&e->base.link,
>                               &e->base.file_priv->event_list);
>                 wake_up_interruptible(&e->base.file_priv->event_wait);


btw, I wonder if we could just have a helper like:

int drm_send_page_flip_event(struct drm_device *dev, int crtc,
              struct drm_pending_vblank_event *event);

Since most drivers have pretty much the same code for sending vblank
event after a page flip..

I guess not strictly related to monotonic timestamps, but it has been
a cleanup that I've been meaning to do for a while.. and I guess if
this was done first it wouldn't mean touching each driver (as much) to
add support for monotonic timestamps.

BR,
-R

> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 7ddef8f..55c014a 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -272,7 +272,6 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
>         struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
>         struct radeon_unpin_work *work;
>         struct drm_pending_vblank_event *e;
> -       struct timeval now;
>         unsigned long flags;
>         u32 update_pending;
>         int vpos, hpos;
> @@ -329,10 +328,13 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
>
>         /* wakeup userspace */
>         if (work->event) {
> +               struct drm_vblank_time now;
> +               u32 seq;
> +
>                 e = work->event;
> -               e->event.sequence = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
> -               e->event.tv_sec = now.tv_sec;
> -               e->event.tv_usec = now.tv_usec;
> +               seq = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
> +               drm_set_event_seq_and_time(&e->event, e->timestamp_raw,
> +                                          seq, &now);
>                 list_add_tail(&e->base.link, &e->base.file_priv->event_list);
>                 wake_up_interruptible(&e->base.file_priv->event_wait);
>         }

Re: [RFC 0/4] drm: add raw monotonic timestamp support

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 949 bytes --]

Imre Deak <imre.deak@intel.com> writes:

> 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.
>
> The patchset is based on danvet's dinq branch with the following
> additional patches from the intel-gfx ML applied: 
>     drm/i915: paper over a pipe-enable vs pageflip race
>     drm/i915: don't frob the vblank ts in finish_page_flip
>     drm/i915: call drm_handle_vblank before finish_page_flip

While people are in pageflip code:

It would be really, really cool for application tracing if we could get
timestamps out of our swaps that used the TIMESTAMP register that is the
timer used for event tracing on the GPU using GL_ARB_timer_query.  Then
you could get decent visualizations of the latency of your rendering.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Imre Deak]

On Fri, 2012-10-05 at 16:18 -0600, Rob Clark wrote:
> On Fri, Oct 5, 2012 at 7:37 AM, Imre Deak <imre.deak@intel.com> wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ab1ef15..056e810 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6247,7 +6247,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
> >         struct intel_unpin_work *work;
> >         struct drm_i915_gem_object *obj;
> >         struct drm_pending_vblank_event *e;
> > -       struct timeval tvbl;
> >         unsigned long flags;
> >
> >         /* Ignore early vblank irqs */
> > @@ -6264,12 +6263,13 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
> >         intel_crtc->unpin_work = NULL;
> >
> >         if (work->event) {
> > -               e = work->event;
> > -               e->event.sequence = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
> > -
> > -               e->event.tv_sec = tvbl.tv_sec;
> > -               e->event.tv_usec = tvbl.tv_usec;
> > +               struct drm_vblank_time tvbl;
> > +               u32 seq;
> >
> > +               e = work->event;
> > +               seq = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
> > +               drm_set_event_seq_and_time(&e->event, e->timestamp_raw, seq,
> > +                                          &tvbl);
> >                 list_add_tail(&e->base.link,
> >                               &e->base.file_priv->event_list);
> >                 wake_up_interruptible(&e->base.file_priv->event_wait);
> 
> 
> btw, I wonder if we could just have a helper like:
> 
> int drm_send_page_flip_event(struct drm_device *dev, int crtc,
>               struct drm_pending_vblank_event *event);
> 
> Since most drivers have pretty much the same code for sending vblank
> event after a page flip..
> 
> I guess not strictly related to monotonic timestamps, but it has been
> a cleanup that I've been meaning to do for a while.. and I guess if
> this was done first it wouldn't mean touching each driver (as much) to
> add support for monotonic timestamps.

Good idea, we should do this.

But if we want to reduce the diff from my changes then the proto should
rather be:

int drm_send_page_flip_event(struct drm_device *dev, int crtc,
               struct drm_pending_vblank_event *event,
               int seq, struct timeval *now);

with struct timeval changing to struct drm_vblank_time with my changes.

I can do this if you agree - unless you have something ready.

--Imre

> 
> BR,
> -R
> 
> > diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> > index 7ddef8f..55c014a 100644
> > --- a/drivers/gpu/drm/radeon/radeon_display.c
> > +++ b/drivers/gpu/drm/radeon/radeon_display.c
> > @@ -272,7 +272,6 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
> >         struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
> >         struct radeon_unpin_work *work;
> >         struct drm_pending_vblank_event *e;
> > -       struct timeval now;
> >         unsigned long flags;
> >         u32 update_pending;
> >         int vpos, hpos;
> > @@ -329,10 +328,13 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
> >
> >         /* wakeup userspace */
> >         if (work->event) {
> > +               struct drm_vblank_time now;
> > +               u32 seq;
> > +
> >                 e = work->event;
> > -               e->event.sequence = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
> > -               e->event.tv_sec = now.tv_sec;
> > -               e->event.tv_usec = now.tv_usec;
> > +               seq = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
> > +               drm_set_event_seq_and_time(&e->event, e->timestamp_raw,
> > +                                          seq, &now);
> >                 list_add_tail(&e->base.link, &e->base.file_priv->event_list);
> >                 wake_up_interruptible(&e->base.file_priv->event_wait);
> >         }

Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Rob Clark]

On Fri, Oct 5, 2012 at 5:41 PM, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, 2012-10-05 at 16:18 -0600, Rob Clark wrote:
>> On Fri, Oct 5, 2012 at 7:37 AM, Imre Deak <imre.deak@intel.com> wrote:
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index ab1ef15..056e810 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -6247,7 +6247,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>> >         struct intel_unpin_work *work;
>> >         struct drm_i915_gem_object *obj;
>> >         struct drm_pending_vblank_event *e;
>> > -       struct timeval tvbl;
>> >         unsigned long flags;
>> >
>> >         /* Ignore early vblank irqs */
>> > @@ -6264,12 +6263,13 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>> >         intel_crtc->unpin_work = NULL;
>> >
>> >         if (work->event) {
>> > -               e = work->event;
>> > -               e->event.sequence = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
>> > -
>> > -               e->event.tv_sec = tvbl.tv_sec;
>> > -               e->event.tv_usec = tvbl.tv_usec;
>> > +               struct drm_vblank_time tvbl;
>> > +               u32 seq;
>> >
>> > +               e = work->event;
>> > +               seq = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
>> > +               drm_set_event_seq_and_time(&e->event, e->timestamp_raw, seq,
>> > +                                          &tvbl);
>> >                 list_add_tail(&e->base.link,
>> >                               &e->base.file_priv->event_list);
>> >                 wake_up_interruptible(&e->base.file_priv->event_wait);
>>
>>
>> btw, I wonder if we could just have a helper like:
>>
>> int drm_send_page_flip_event(struct drm_device *dev, int crtc,
>>               struct drm_pending_vblank_event *event);
>>
>> Since most drivers have pretty much the same code for sending vblank
>> event after a page flip..
>>
>> I guess not strictly related to monotonic timestamps, but it has been
>> a cleanup that I've been meaning to do for a while.. and I guess if
>> this was done first it wouldn't mean touching each driver (as much) to
>> add support for monotonic timestamps.
>
> Good idea, we should do this.
>
> But if we want to reduce the diff from my changes then the proto should
> rather be:
>
> int drm_send_page_flip_event(struct drm_device *dev, int crtc,
>                struct drm_pending_vblank_event *event,
>                int seq, struct timeval *now);

do we need 'seq' and 'now'?  I was sort of thinking that could all be
internal to the send_page_flip_event() fxn.. ie. like:

---------
void drm_send_page_flip_event(struct drm_device *dev, int pipe,
		struct drm_crtc *crtc, struct drm_pending_vblank_event *event)
{
	struct timeval tnow, tvbl;

	do_gettimeofday(&tnow);

	event->event.sequence = drm_vblank_count_and_time(dev, pipe, &tvbl);

	/* Called before vblank count and timestamps have
	 * been updated for the vblank interval of flip
	 * completion? Need to increment vblank count and
	 * add one videorefresh duration to returned timestamp
	 * to account for this. We assume this happened if we
	 * get called over 0.9 frame durations after the last
	 * timestamped vblank.
	 *
	 * This calculation can not be used with vrefresh rates
	 * below 5Hz (10Hz to be on the safe side) without
	 * promoting to 64 integers.
	 */
	if (10 * (timeval_to_ns(&tnow) - timeval_to_ns(&tvbl)) >
	    9 * crtc->framedur_ns) {
		event->event.sequence++;
		tvbl = ns_to_timeval(timeval_to_ns(&tvbl) +
				     crtc->framedur_ns);
	}

	event->event.tv_sec = tvbl.tv_sec;
	event->event.tv_usec = tvbl.tv_usec;

	list_add_tail(&event->base.link,
		      &event->base.file_priv->event_list);
	wake_up_interruptible(&event->base.file_priv->event_wait);
}
---------

well, this would be the pre-monotonic timestamp version.. and it is a
bit weird to have to pass in both crtc ptr and pipe #.. I don't see
anything obvious in drm core that links pipe # and crtc ptr, although
userspace seems to make this assumption about order of crtc's.  But I
think that if() statement is only in i915 driver.. I assume because
you don't know if this will get called before or after
drm_handle_vblank()?  I guess that shouldn't hurt for other drivers.

then each driver would just have something like:

---------
	if (work->event)
		drm_send_page_flip_event(dev, intel_crtc->pipe, crtc, work->event);
---------


> with struct timeval changing to struct drm_vblank_time with my changes.
>
> I can do this if you agree - unless you have something ready.

I've locally split out this into a helper fxn in omapdrm, but haven't
got around to pushing it to core and updating other drivers.  If you
want I can send a patch, but I guess it is easier to just include
something like this in your patchset.  I'm ok either way.

BR,
-R

> --Imre
>
>>
>> BR,
>> -R
>>
>> > diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
>> > index 7ddef8f..55c014a 100644
>> > --- a/drivers/gpu/drm/radeon/radeon_display.c
>> > +++ b/drivers/gpu/drm/radeon/radeon_display.c
>> > @@ -272,7 +272,6 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
>> >         struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
>> >         struct radeon_unpin_work *work;
>> >         struct drm_pending_vblank_event *e;
>> > -       struct timeval now;
>> >         unsigned long flags;
>> >         u32 update_pending;
>> >         int vpos, hpos;
>> > @@ -329,10 +328,13 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
>> >
>> >         /* wakeup userspace */
>> >         if (work->event) {
>> > +               struct drm_vblank_time now;
>> > +               u32 seq;
>> > +
>> >                 e = work->event;
>> > -               e->event.sequence = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
>> > -               e->event.tv_sec = now.tv_sec;
>> > -               e->event.tv_usec = now.tv_usec;
>> > +               seq = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
>> > +               drm_set_event_seq_and_time(&e->event, e->timestamp_raw,
>> > +                                          seq, &now);
>> >                 list_add_tail(&e->base.link, &e->base.file_priv->event_list);
>> >                 wake_up_interruptible(&e->base.file_priv->event_wait);
>> >         }
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Imre Deak]

On Fri, 2012-10-05 at 18:09 -0600, Rob Clark wrote:
> On Fri, Oct 5, 2012 at 5:41 PM, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, 2012-10-05 at 16:18 -0600, Rob Clark wrote:
> >> On Fri, Oct 5, 2012 at 7:37 AM, Imre Deak <imre.deak@intel.com> wrote:
> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> > index ab1ef15..056e810 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -6247,7 +6247,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
> >> >         struct intel_unpin_work *work;
> >> >         struct drm_i915_gem_object *obj;
> >> >         struct drm_pending_vblank_event *e;
> >> > -       struct timeval tvbl;
> >> >         unsigned long flags;
> >> >
> >> >         /* Ignore early vblank irqs */
> >> > @@ -6264,12 +6263,13 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
> >> >         intel_crtc->unpin_work = NULL;
> >> >
> >> >         if (work->event) {
> >> > -               e = work->event;
> >> > -               e->event.sequence = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
> >> > -
> >> > -               e->event.tv_sec = tvbl.tv_sec;
> >> > -               e->event.tv_usec = tvbl.tv_usec;
> >> > +               struct drm_vblank_time tvbl;
> >> > +               u32 seq;
> >> >
> >> > +               e = work->event;
> >> > +               seq = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
> >> > +               drm_set_event_seq_and_time(&e->event, e->timestamp_raw, seq,
> >> > +                                          &tvbl);
> >> >                 list_add_tail(&e->base.link,
> >> >                               &e->base.file_priv->event_list);
> >> >                 wake_up_interruptible(&e->base.file_priv->event_wait);
> >>
> >>
> >> btw, I wonder if we could just have a helper like:
> >>
> >> int drm_send_page_flip_event(struct drm_device *dev, int crtc,
> >>               struct drm_pending_vblank_event *event);
> >>
> >> Since most drivers have pretty much the same code for sending vblank
> >> event after a page flip..
> >>
> >> I guess not strictly related to monotonic timestamps, but it has been
> >> a cleanup that I've been meaning to do for a while.. and I guess if
> >> this was done first it wouldn't mean touching each driver (as much) to
> >> add support for monotonic timestamps.
> >
> > Good idea, we should do this.
> >
> > But if we want to reduce the diff from my changes then the proto should
> > rather be:
> >
> > int drm_send_page_flip_event(struct drm_device *dev, int crtc,
> >                struct drm_pending_vblank_event *event,
> >                int seq, struct timeval *now);
> 
> do we need 'seq' and 'now'?  I was sort of thinking that could all be
> internal to the send_page_flip_event() fxn.. ie. like:
> 
> ---------
> void drm_send_page_flip_event(struct drm_device *dev, int pipe,
> 		struct drm_crtc *crtc, struct drm_pending_vblank_event *event)
> {
> 	struct timeval tnow, tvbl;
> 
> 	do_gettimeofday(&tnow);
> 
> 	event->event.sequence = drm_vblank_count_and_time(dev, pipe, &tvbl);

Ah, ok, you are right. I originally thought of using this helper in
drm_handle_vblank_events too, but now I realized it's not quite the same
sequence there.

> 
> 	/* Called before vblank count and timestamps have
> 	 * been updated for the vblank interval of flip
> 	 * completion? Need to increment vblank count and
> 	 * add one videorefresh duration to returned timestamp
> 	 * to account for this. We assume this happened if we
> 	 * get called over 0.9 frame durations after the last
> 	 * timestamped vblank.
> 	 *
> 	 * This calculation can not be used with vrefresh rates
> 	 * below 5Hz (10Hz to be on the safe side) without
> 	 * promoting to 64 integers.
> 	 */
> 	if (10 * (timeval_to_ns(&tnow) - timeval_to_ns(&tvbl)) >
> 	    9 * crtc->framedur_ns) {
> 		event->event.sequence++;
> 		tvbl = ns_to_timeval(timeval_to_ns(&tvbl) +
> 				     crtc->framedur_ns);
> 	}

This has been recently removed by danvet's "drm/i915: don't frob the
vblank ts in finish_page_flip", though not yet committed, so we can do
away with it here too.

> 	event->event.tv_sec = tvbl.tv_sec;
> 	event->event.tv_usec = tvbl.tv_usec;
> 
> 	list_add_tail(&event->base.link,
> 		      &event->base.file_priv->event_list);
> 	wake_up_interruptible(&event->base.file_priv->event_wait);
> }


> ---------
> 
> well, this would be the pre-monotonic timestamp version.. and it is a
> bit weird to have to pass in both crtc ptr and pipe #.. I don't see
> anything obvious in drm core that links pipe # and crtc ptr, although
> userspace seems to make this assumption about order of crtc's.  But I
> think that if() statement is only in i915 driver.. I assume because
> you don't know if this will get called before or after
> drm_handle_vblank()?  I guess that shouldn't hurt for other drivers.
> 
> then each driver would just have something like:
> 
> ---------
> 	if (work->event)
> 		drm_send_page_flip_event(dev, intel_crtc->pipe, crtc, work->event);
> ---------
> 
> 
> > with struct timeval changing to struct drm_vblank_time with my changes.
> >
> > I can do this if you agree - unless you have something ready.
> 
> I've locally split out this into a helper fxn in omapdrm, but haven't
> got around to pushing it to core and updating other drivers.  If you
> want I can send a patch, but I guess it is easier to just include
> something like this in your patchset.  I'm ok either way.

I think the easiest is if you post your updated patch since it can be
anyway applied independently. I'll then rebase my changes on top of
that.

Thanks,
Imre

Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Daniel Vetter]

On Sat, Oct 06, 2012 at 03:49:16AM +0300, Imre Deak wrote:
> On Fri, 2012-10-05 at 18:09 -0600, Rob Clark wrote:
> > 
> > 	/* Called before vblank count and timestamps have
> > 	 * been updated for the vblank interval of flip
> > 	 * completion? Need to increment vblank count and
> > 	 * add one videorefresh duration to returned timestamp
> > 	 * to account for this. We assume this happened if we
> > 	 * get called over 0.9 frame durations after the last
> > 	 * timestamped vblank.
> > 	 *
> > 	 * This calculation can not be used with vrefresh rates
> > 	 * below 5Hz (10Hz to be on the safe side) without
> > 	 * promoting to 64 integers.
> > 	 */
> > 	if (10 * (timeval_to_ns(&tnow) - timeval_to_ns(&tvbl)) >
> > 	    9 * crtc->framedur_ns) {
> > 		event->event.sequence++;
> > 		tvbl = ns_to_timeval(timeval_to_ns(&tvbl) +
> > 				     crtc->framedur_ns);
> > 	}
> 
> This has been recently removed by danvet's "drm/i915: don't frob the
> vblank ts in finish_page_flip", though not yet committed, so we can do
> away with it here too.

Yeah, I'd prefer if the common code doesn't have such hackes - this bit
here papered over some bugs in our driver code, but only partially
successfully ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC 0/4] drm: add raw monotonic timestamp support

[Imre Deak]

On Fri, 2012-10-05 at 16:07 -0700, Eric Anholt wrote:
> Imre Deak <imre.deak@intel.com> writes:
> 
> > 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.
> >
> > The patchset is based on danvet's dinq branch with the following
> > additional patches from the intel-gfx ML applied: 
> >     drm/i915: paper over a pipe-enable vs pageflip race
> >     drm/i915: don't frob the vblank ts in finish_page_flip
> >     drm/i915: call drm_handle_vblank before finish_page_flip
> 
> While people are in pageflip code:
> 
> It would be really, really cool for application tracing if we could get
> timestamps out of our swaps that used the TIMESTAMP register that is the
> timer used for event tracing on the GPU using GL_ARB_timer_query.  Then
> you could get decent visualizations of the latency of your rendering.

I assume this querying wouldn't be done through the wait_for_vblank or
page_flip ioctls, but rather a new ioctl or even through a new perf
event? We could add such a new interface later; this patchset is more
focused on the above two ioctls.

--Imre

Re: [RFC 1/4] time: export getnstime_raw_and_real for DRM

[Imre Deak]

On Fri, 2012-10-05 at 12:14 -0400, Kristian Høgsberg wrote:
> On Fri, Oct 5, 2012 at 9:36 AM, Imre Deak <imre.deak@intel.com> wrote:
> > Needed by the upcoming DRM raw monotonic timestamp support.
> 
> I just had a quick look at driver/input/evdev.c, since evdev devices
> did a similar change recently to allow evdev timestamp from the
> monotonic clock.  They're using a different time API:
> 
> 	time_mono = ktime_get();
>         time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());
> 
> and
> 
>         event->time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
> 			                mono : real);
> 
> I'm not really up-to-date on kernel time APIs, but I wonder if that
> may be better?  At least, I suspect we wouldn't need changes outside
> drm if we use this API.


Thanks, I will use this instead of getnstime_raw_and_real.

The reason is as discussed on #intel-gfx that this provides a monotonic
vs. raw monotonic time and that as you say it won't require a change in
the time keeping code.

--Imre

> 
> Kristian
> 
> 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  kernel/time/timekeeping.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index d3b91e7..073d262 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -365,7 +365,7 @@ void ktime_get_ts(struct timespec *ts)
> >  }
> >  EXPORT_SYMBOL_GPL(ktime_get_ts);
> >
> > -#ifdef CONFIG_NTP_PPS
> > +#if defined(CONFIG_NTP_PPS) || defined(CONFIG_DRM) || defined(CONFIG_DRM_MODULE)
> >
> >  /**
> >   * getnstime_raw_and_real - get day and raw monotonic time in timespec format
> > --
> > 1.7.9.5
> >


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 0/4] drm: add raw monotonic timestamp support

[Laurent Pinchart]

(CC'ing linux-media)

On Friday 05 October 2012 16:36:58 Imre Deak wrote:
> This is needed to make applications depending on vblank/page flip
> timestamps independent of time ajdustments.

We're in the process to switching to CLOCK_MONOTONIC timestamps in V4L2. The 
reason why we're using CLOCK_MONOTONIC and not CLOCK_MONOTONIC_RAW is that 
ALSA uses the former. It would make sense in my opinion to unify timestamps 
across our media APIs.

> 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.
> 
> The patchset is based on danvet's dinq branch with the following
> additional patches from the intel-gfx ML applied:
>     drm/i915: paper over a pipe-enable vs pageflip race
>     drm/i915: don't frob the vblank ts in finish_page_flip
>     drm/i915: call drm_handle_vblank before finish_page_flip
> 
> Imre Deak (4):
>   time: export getnstime_raw_and_real for DRM
>   drm: make memset/calloc for _vblank_time more robust
>   drm: use raw time in drm_calc_vbltimestamp_from_scanoutpos
>   drm: add support for raw monotonic vblank timestamps
> 
>  drivers/gpu/drm/drm_crtc.c                |    2 +
>  drivers/gpu/drm/drm_ioctl.c               |    3 ++
>  drivers/gpu/drm/drm_irq.c                 |   83 ++++++++++++++------------
>  drivers/gpu/drm/i915/i915_irq.c           |    2 +-
>  drivers/gpu/drm/i915/intel_display.c      |   12 ++---
>  drivers/gpu/drm/radeon/radeon_display.c   |   10 ++--
>  drivers/gpu/drm/radeon/radeon_drv.c       |    2 +-
>  drivers/gpu/drm/radeon/radeon_kms.c       |    2 +-
>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |    9 ++--
>  include/drm/drm.h                         |    5 +-
>  include/drm/drmP.h                        |   38 +++++++++++--
>  include/drm/drm_mode.h                    |    4 +-
>  kernel/time/timekeeping.c                 |    2 +-
>  13 files changed, 113 insertions(+), 61 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [RFC 0/4] drm: add raw monotonic timestamp support

[Laurent Pinchart]

Hi Imre,

On Friday 05 October 2012 16:36:58 Imre Deak wrote:
> 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.
> 
> The patchset is based on danvet's dinq branch with the following
> additional patches from the intel-gfx ML applied:
>     drm/i915: paper over a pipe-enable vs pageflip race
>     drm/i915: don't frob the vblank ts in finish_page_flip
>     drm/i915: call drm_handle_vblank before finish_page_flip
> 
> Imre Deak (4):
>   time: export getnstime_raw_and_real for DRM
>   drm: make memset/calloc for _vblank_time more robust
>   drm: use raw time in drm_calc_vbltimestamp_from_scanoutpos
>   drm: add support for raw monotonic vblank timestamps

Could you please also update Documentation/DocBook/drm.tmpl to document that 
the event timestamp is a raw monotonic clock timestamp ?

>  drivers/gpu/drm/drm_crtc.c                |    2 +
>  drivers/gpu/drm/drm_ioctl.c               |    3 ++
>  drivers/gpu/drm/drm_irq.c                 |   83 +++++++++++++-------------
>  drivers/gpu/drm/i915/i915_irq.c           |    2 +-
>  drivers/gpu/drm/i915/intel_display.c      |   12 ++---
>  drivers/gpu/drm/radeon/radeon_display.c   |   10 ++--
>  drivers/gpu/drm/radeon/radeon_drv.c       |    2 +-
>  drivers/gpu/drm/radeon/radeon_kms.c       |    2 +-
>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |    9 ++--
>  include/drm/drm.h                         |    5 +-
>  include/drm/drmP.h                        |   38 +++++++++++--
>  include/drm/drm_mode.h                    |    4 +-
>  kernel/time/timekeeping.c                 |    2 +-
>  13 files changed, 113 insertions(+), 61 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [RFC 0/4] drm: add raw monotonic timestamp support

[Imre Deak]

Hi,

On Thu, 2012-10-11 at 12:29 +0200, Laurent Pinchart wrote:
> (CC'ing linux-media)
> 
> On Friday 05 October 2012 16:36:58 Imre Deak wrote:
> > This is needed to make applications depending on vblank/page flip
> > timestamps independent of time ajdustments.
> 
> We're in the process to switching to CLOCK_MONOTONIC timestamps in V4L2. The 
> reason why we're using CLOCK_MONOTONIC and not CLOCK_MONOTONIC_RAW is that 
> ALSA uses the former. It would make sense in my opinion to unify timestamps 
> across our media APIs.

yes, thanks for pointing this out.

Since I posted the RFC we made the decision to use CLOCK_MONOTONIC too.
CLOCK_MONOTONIC_RAW is not adjusted against HW clock frequency
variations, so it's not ideal for us. CLOCK_MONOTONIC doesn't have this
problem, neither does it jump so it seems to be the ideal choice.

--Imre

> > 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.
> > 
> > The patchset is based on danvet's dinq branch with the following
> > additional patches from the intel-gfx ML applied:
> >     drm/i915: paper over a pipe-enable vs pageflip race
> >     drm/i915: don't frob the vblank ts in finish_page_flip
> >     drm/i915: call drm_handle_vblank before finish_page_flip
> > 
> > Imre Deak (4):
> >   time: export getnstime_raw_and_real for DRM
> >   drm: make memset/calloc for _vblank_time more robust
> >   drm: use raw time in drm_calc_vbltimestamp_from_scanoutpos
> >   drm: add support for raw monotonic vblank timestamps
> > 
> >  drivers/gpu/drm/drm_crtc.c                |    2 +
> >  drivers/gpu/drm/drm_ioctl.c               |    3 ++
> >  drivers/gpu/drm/drm_irq.c                 |   83 ++++++++++++++------------
> >  drivers/gpu/drm/i915/i915_irq.c           |    2 +-
> >  drivers/gpu/drm/i915/intel_display.c      |   12 ++---
> >  drivers/gpu/drm/radeon/radeon_display.c   |   10 ++--
> >  drivers/gpu/drm/radeon/radeon_drv.c       |    2 +-
> >  drivers/gpu/drm/radeon/radeon_kms.c       |    2 +-
> >  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |    9 ++--
> >  include/drm/drm.h                         |    5 +-
> >  include/drm/drmP.h                        |   38 +++++++++++--
> >  include/drm/drm_mode.h                    |    4 +-
> >  kernel/time/timekeeping.c                 |    2 +-
> >  13 files changed, 113 insertions(+), 61 deletions(-)
> 

Re: [RFC 0/4] drm: add raw monotonic timestamp support

[Imre Deak]

On Thu, 2012-10-11 at 12:32 +0200, Laurent Pinchart wrote:
> Hi Imre,
> 
> On Friday 05 October 2012 16:36:58 Imre Deak wrote:
> > 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.
> > 
> > The patchset is based on danvet's dinq branch with the following
> > additional patches from the intel-gfx ML applied:
> >     drm/i915: paper over a pipe-enable vs pageflip race
> >     drm/i915: don't frob the vblank ts in finish_page_flip
> >     drm/i915: call drm_handle_vblank before finish_page_flip
> > 
> > Imre Deak (4):
> >   time: export getnstime_raw_and_real for DRM
> >   drm: make memset/calloc for _vblank_time more robust
> >   drm: use raw time in drm_calc_vbltimestamp_from_scanoutpos
> >   drm: add support for raw monotonic vblank timestamps
> 
> Could you please also update Documentation/DocBook/drm.tmpl to document that 
> the event timestamp is a raw monotonic clock timestamp ?

Ok, will fix up the documentation.

--Imre

> 
> >  drivers/gpu/drm/drm_crtc.c                |    2 +
> >  drivers/gpu/drm/drm_ioctl.c               |    3 ++
> >  drivers/gpu/drm/drm_irq.c                 |   83 +++++++++++++-------------
> >  drivers/gpu/drm/i915/i915_irq.c           |    2 +-
> >  drivers/gpu/drm/i915/intel_display.c      |   12 ++---
> >  drivers/gpu/drm/radeon/radeon_display.c   |   10 ++--
> >  drivers/gpu/drm/radeon/radeon_drv.c       |    2 +-
> >  drivers/gpu/drm/radeon/radeon_kms.c       |    2 +-
> >  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |    9 ++--
> >  include/drm/drm.h                         |    5 +-
> >  include/drm/drmP.h                        |   38 +++++++++++--
> >  include/drm/drm_mode.h                    |    4 +-
> >  kernel/time/timekeeping.c                 |    2 +-
> >  13 files changed, 113 insertions(+), 61 deletions(-)
> 

[PATCH 0/2] drm: add monotonic timestamp support

[Imre Deak]

This is needed to make applications using wait-for-vblank/page-flip
timestamps independent of time ajdustments other than adjustments for
HW clock jitter.

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.

The patchset is based on the nightly branch of [1], with Rob Clark's
"drm: add drm_send_vblank_event() helper" patchset applied.

Changes since the RFC:
- use monotonic instead of raw monotonic time
- use a module global flag instead of a per-client flag to set the
  compatibility mode with GTOD timestamps
- rebased on top of Rob Clark's drm_send_vblank_event patchset

[1] git://people.freedesktop.org/~danvet/drm-intel

Imre Deak (2):
  drm: use monotonic time in drm_calc_vbltimestamp_from_scanoutpos
  drm: add support for monotonic vblank timestamps

 drivers/gpu/drm/drm_ioctl.c |    3 +++
 drivers/gpu/drm/drm_irq.c   |   39 ++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/drm_stub.c  |    8 ++++++++
 include/drm/drmP.h          |    1 +
 include/uapi/drm/drm.h      |    1 +
 5 files changed, 43 insertions(+), 9 deletions(-)

-- 
1.7.9.5

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

[Imre Deak]

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

[PATCH 2/2] drm: add support for monotonic vblank timestamps

[Imre Deak]

Jumps in the vblank and page flip event timestamps cause trouble for
clients, so we should avoid them. The timestamp we get currently with
gettimeofday can jump, so use instead monotonic timestamps.

For backward compatibility use a module flag to revert back to using
gettimeofday timestamps. Add also a DRM_CAP_TIMESTAMP_MONOTONIC flag
that is simply a read only version of the module flag, so that clients
can query this without depending on sysfs.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_ioctl.c |    3 +++
 drivers/gpu/drm/drm_irq.c   |   25 ++++++++++++++++++++-----
 drivers/gpu/drm/drm_stub.c  |    8 ++++++++
 include/drm/drmP.h          |    1 +
 include/uapi/drm/drm.h      |    1 +
 5 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 23dd975..e77bd8b 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -287,6 +287,9 @@ int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 		req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
 		req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0;
 		break;
+	case DRM_CAP_TIMESTAMP_MONOTONIC:
+		req->value = drm_timestamp_monotonic;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 7dc203d..37e58df7 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -633,7 +633,8 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 
 		/* Get system timestamp after query. */
 		etime = ktime_get();
-		mono_time_offset = ktime_get_monotonic_offset();
+		if (!drm_timestamp_monotonic)
+			mono_time_offset = ktime_get_monotonic_offset();
 
 		preempt_enable();
 
@@ -691,7 +692,9 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 		vbl_status |= 0x8;
 	}
 
-	etime = ktime_sub(etime, mono_time_offset);
+	if (!drm_timestamp_monotonic)
+		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
@@ -714,6 +717,17 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 }
 EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
 
+static struct timeval get_drm_timestamp(void)
+{
+	ktime_t now;
+
+	now = ktime_get();
+	if (!drm_timestamp_monotonic)
+		now = ktime_sub(now, ktime_get_monotonic_offset());
+
+	return ktime_to_timeval(now);
+}
+
 /**
  * drm_get_last_vbltimestamp - retrieve raw timestamp for the most recent
  * vblank interval.
@@ -751,9 +765,9 @@ u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
 	}
 
 	/* GPU high precision timestamp query unsupported or failed.
-	 * Return gettimeofday timestamp as best estimate.
+	 * Return current monotonic/gettimeofday timestamp as best estimate.
 	 */
-	do_gettimeofday(tvblank);
+	*tvblank = get_drm_timestamp();
 
 	return 0;
 }
@@ -842,7 +856,8 @@ void drm_send_vblank_event(struct drm_device *dev, int crtc,
 		seq = drm_vblank_count_and_time(dev, crtc, &now);
 	} else {
 		seq = 0;
-		do_gettimeofday(&now);
+
+		now = get_drm_timestamp();
 	}
 	send_vblank_event(dev, e, seq, &now);
 }
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index c236fd2..4433453 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -46,16 +46,24 @@ EXPORT_SYMBOL(drm_vblank_offdelay);
 unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
 EXPORT_SYMBOL(drm_timestamp_precision);
 
+/*
+ * Default to use monotonic timestamps for wait-for-vblank and page-flip
+ * complete events.
+ */
+unsigned int drm_timestamp_monotonic = 1;
+
 MODULE_AUTHOR(CORE_AUTHOR);
 MODULE_DESCRIPTION(CORE_DESC);
 MODULE_LICENSE("GPL and additional rights");
 MODULE_PARM_DESC(debug, "Enable debug output");
 MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs]");
 MODULE_PARM_DESC(timestamp_precision_usec, "Max. error on timestamps [usecs]");
+MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");
 
 module_param_named(debug, drm_debug, int, 0600);
 module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600);
 module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600);
+module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
 
 struct idr drm_minors_idr;
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 29eb237..fad21c9 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1505,6 +1505,7 @@ extern unsigned int drm_debug;
 
 extern unsigned int drm_vblank_offdelay;
 extern unsigned int drm_timestamp_precision;
+extern unsigned int drm_timestamp_monotonic;
 
 extern struct class *drm_class;
 extern struct proc_dir_entry *drm_proc_root;
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 1e3481e..8d1e2bb 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -778,6 +778,7 @@ struct drm_event_vblank {
 #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
 #define DRM_CAP_DUMB_PREFER_SHADOW 0x4
 #define DRM_CAP_PRIME 0x5
+#define DRM_CAP_TIMESTAMP_MONOTONIC 0x6
 
 #define DRM_PRIME_CAP_IMPORT 0x1
 #define DRM_PRIME_CAP_EXPORT 0x2
-- 
1.7.9.5

Re: [PATCH 2/2] drm: add support for monotonic vblank timestamps

[Michel Dänzer]

On Die, 2012-10-23 at 21:53 +0300, Imre Deak wrote: 
> Jumps in the vblank and page flip event timestamps cause trouble for
> clients, so we should avoid them. The timestamp we get currently with
> gettimeofday can jump, so use instead monotonic timestamps.
> 
> For backward compatibility use a module flag to revert back to using
> gettimeofday timestamps. Add also a DRM_CAP_TIMESTAMP_MONOTONIC flag
> that is simply a read only version of the module flag, so that clients
> can query this without depending on sysfs.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

drm_timestamp_monotonic should probably be a bool rather than an int.

Looks good to me otherwise, as does patch 1.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer

Re: [PATCH 2/2] drm: add support for monotonic vblank timestamps

[Imre Deak]

On Wed, 2012-10-24 at 10:08 +0200, Michel Dänzer wrote:
> On Die, 2012-10-23 at 21:53 +0300, Imre Deak wrote: 
> > Jumps in the vblank and page flip event timestamps cause trouble for
> > clients, so we should avoid them. The timestamp we get currently with
> > gettimeofday can jump, so use instead monotonic timestamps.
> > 
> > For backward compatibility use a module flag to revert back to using
> > gettimeofday timestamps. Add also a DRM_CAP_TIMESTAMP_MONOTONIC flag
> > that is simply a read only version of the module flag, so that clients
> > can query this without depending on sysfs.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> drm_timestamp_monotonic should probably be a bool rather than an int.

Ok, will change that. Also just realized that the permission could be
0644 isntead of 0600.

--Imre  

> 
> Looks good to me otherwise, as does patch 1.
> 
> 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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);
>
>

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

[Imre Deak]

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

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

[Mario Kleiner]

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