[PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Arnd Bergmann]

The drm vblank handling uses 'timeval' to store timestamps in either
monotonic or wall-clock time base. In either case, it reads the current
time as a ktime_t in get_drm_timestamp() and converts it from there.

This is a bit suspicious, as users of 'timeval' often suffer from
the time_t overflow in y2038. I have gone through this code and
found that it is unlikely to cause problems here:

- The user space ABI does not use time_t or timeval, but uses
  'u32' and 'long' as the types. This means at least that rebuilding
  user programs against a new libc with 64-bit time_t does not
  change the ABI.

- As of commit c61eef726a78 ("drm: add support for monotonic vblank
  timestamps") in linux-3.8, the monotonic timestamp is the default
  and can only get reverted to wall-clock through a module-parameter.

- With the default monotonic timestamps, there is no problem at all.

- The drm_wait_vblank_ioctl() interface is alway safe on 64-bit
  architectures, on 32-bit it might overflow the 'long' timestamps
  in 2038 with wall-clock timestamps.

- The event handling uses 'u32' seconds, which overflow in 2106
  on both 32-bit and 64-bit machines, when wall-clock timestamps
  are used.

- The effect of overflowing either of the two is only temporary
  (during the overflow, and is likely to keep working again
  afterwards. It is likely the same problem as observing a
  'settimeofday()' call, which was the reason for moving to the
  monotonic timestamps in the first place.

Overall, this seems good enough, so my patch removes the use of
'timeval' from the vblank handling altogether and uses ktime_t
consistently, except for the part where we copy the data to user
space structures in the existing format.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/drm_vblank.c | 123 +++++++++++++++++++++++--------------------
 include/drm/drm_drv.h        |   2 +-
 include/drm/drm_vblank.h     |   6 +--
 3 files changed, 71 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 70f2b9593edc..c605c3ad6b6e 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -78,7 +78,7 @@
 
 static bool
 drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
-			  struct timeval *tvblank, bool in_vblank_irq);
+			  ktime_t *tvblank, bool in_vblank_irq);
 
 static unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
 
@@ -99,7 +99,7 @@ MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");
 
 static void store_vblank(struct drm_device *dev, unsigned int pipe,
 			 u32 vblank_count_inc,
-			 struct timeval *t_vblank, u32 last)
+			 ktime_t t_vblank, u32 last)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 
@@ -108,7 +108,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
 	vblank->last = last;
 
 	write_seqlock(&vblank->seqlock);
-	vblank->time = *t_vblank;
+	vblank->time = t_vblank;
 	vblank->count += vblank_count_inc;
 	write_sequnlock(&vblank->seqlock);
 }
@@ -151,7 +151,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
 {
 	u32 cur_vblank;
 	bool rc;
-	struct timeval t_vblank;
+	ktime_t t_vblank;
 	int count = DRM_TIMESTAMP_MAXRETRIES;
 
 	spin_lock(&dev->vblank_time_lock);
@@ -171,13 +171,13 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
 	 * interrupt and assign 0 for now, to mark the vblanktimestamp as invalid.
 	 */
 	if (!rc)
-		t_vblank = (struct timeval) {0, 0};
+		t_vblank = 0;
 
 	/*
 	 * +1 to make sure user will never see the same
 	 * vblank counter value before and after a modeset
 	 */
-	store_vblank(dev, pipe, 1, &t_vblank, cur_vblank);
+	store_vblank(dev, pipe, 1, t_vblank, cur_vblank);
 
 	spin_unlock(&dev->vblank_time_lock);
 }
@@ -200,7 +200,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	u32 cur_vblank, diff;
 	bool rc;
-	struct timeval t_vblank;
+	ktime_t t_vblank;
 	int count = DRM_TIMESTAMP_MAXRETRIES;
 	int framedur_ns = vblank->framedur_ns;
 
@@ -225,11 +225,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 		/* trust the hw counter when it's around */
 		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
 	} else if (rc && framedur_ns) {
-		const struct timeval *t_old;
-		u64 diff_ns;
-
-		t_old = &vblank->time;
-		diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
+		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
 
 		/*
 		 * Figure out how many vblanks we've missed based
@@ -278,9 +274,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	 * for now, to mark the vblanktimestamp as invalid.
 	 */
 	if (!rc && !in_vblank_irq)
-		t_vblank = (struct timeval) {0, 0};
+		t_vblank = 0;
 
-	store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);
+	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
 }
 
 static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
@@ -556,7 +552,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  * @pipe: index of CRTC whose 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 time which should receive the timestamp
  * @in_vblank_irq:
  *     True when called from drm_crtc_handle_vblank().  Some drivers
  *     need to apply some workarounds for gpu-specific vblank irq quirks
@@ -584,10 +580,10 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
 bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 					   unsigned int pipe,
 					   int *max_error,
-					   struct timeval *vblank_time,
+					   ktime_t *vblank_time,
 					   bool in_vblank_irq)
 {
-	struct timeval tv_etime;
+	struct timespec64 ts_etime, ts_vblank_time;
 	ktime_t stime, etime;
 	bool vbl_status;
 	struct drm_crtc *crtc;
@@ -680,29 +676,27 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 		etime = ktime_mono_to_real(etime);
 
 	/* save this only for debugging purposes */
-	tv_etime = ktime_to_timeval(etime);
+	ts_etime = ktime_to_timespec64(etime);
+	ts_vblank_time = ktime_to_timespec64(*vblank_time);
 	/* Subtract time delta from raw timestamp to get final
 	 * vblank_time timestamp for end of vblank.
 	 */
 	etime = ktime_sub_ns(etime, delta_ns);
-	*vblank_time = ktime_to_timeval(etime);
+	*vblank_time = etime;
 
-	DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
+	DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %lld.%06ld -> %lld.%06ld [e %d us, %d rep]\n",
 		      pipe, hpos, vpos,
-		      (long)tv_etime.tv_sec, (long)tv_etime.tv_usec,
-		      (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
-		      duration_ns/1000, i);
+		      (u64)ts_etime.tv_sec, ts_etime.tv_nsec / 1000,
+		      (u64)ts_vblank_time.tv_sec, ts_vblank_time.tv_nsec / 1000,
+		      duration_ns / 1000, i);
 
 	return true;
 }
 EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
 
-static struct timeval get_drm_timestamp(void)
+static ktime_t get_drm_timestamp(void)
 {
-	ktime_t now;
-
-	now = drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
-	return ktime_to_timeval(now);
+	return drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
 }
 
 /**
@@ -710,7 +704,7 @@ static struct timeval get_drm_timestamp(void)
  *                             vblank interval
  * @dev: DRM device
  * @pipe: index of CRTC whose vblank timestamp to retrieve
- * @tvblank: Pointer to target struct timeval which should receive the timestamp
+ * @tvblank: Pointer to target time which should receive the timestamp
  * @in_vblank_irq:
  *     True when called from drm_crtc_handle_vblank().  Some drivers
  *     need to apply some workarounds for gpu-specific vblank irq quirks
@@ -728,7 +722,7 @@ static struct timeval get_drm_timestamp(void)
  */
 static bool
 drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
-			  struct timeval *tvblank, bool in_vblank_irq)
+			  ktime_t *tvblank, bool in_vblank_irq)
 {
 	bool ret = false;
 
@@ -769,14 +763,14 @@ u32 drm_crtc_vblank_count(struct drm_crtc *crtc)
 EXPORT_SYMBOL(drm_crtc_vblank_count);
 
 static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
-				     struct timeval *vblanktime)
+				     ktime_t *vblanktime)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	u32 vblank_count;
 	unsigned int seq;
 
 	if (WARN_ON(pipe >= dev->num_crtcs)) {
-		*vblanktime = (struct timeval) { 0 };
+		*vblanktime = 0;
 		return 0;
 	}
 
@@ -793,7 +787,7 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
  * drm_crtc_vblank_count_and_time - retrieve "cooked" vblank counter value
  *     and the system timestamp corresponding to that vblank counter value
  * @crtc: which counter to retrieve
- * @vblanktime: Pointer to struct timeval to receive the vblank timestamp.
+ * @vblanktime: Pointer to time to receive the vblank timestamp.
  *
  * Fetches the "cooked" vblank count value that represents the number of
  * vblank events since the system was booted, including lost events due to
@@ -801,7 +795,7 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
  * of the vblank interval that corresponds to the current vblank counter value.
  */
 u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
-				   struct timeval *vblanktime)
+				   ktime_t *vblanktime)
 {
 	return drm_vblank_count_and_time(crtc->dev, drm_crtc_index(crtc),
 					 vblanktime);
@@ -810,11 +804,18 @@ EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
 
 static void send_vblank_event(struct drm_device *dev,
 		struct drm_pending_vblank_event *e,
-		unsigned long seq, struct timeval *now)
+		unsigned long seq, ktime_t now)
 {
+	struct timespec64 tv = ktime_to_timespec64(now);
+
 	e->event.sequence = seq;
-	e->event.tv_sec = now->tv_sec;
-	e->event.tv_usec = now->tv_usec;
+	/*
+	 * e->event is a user space structure, with hardcoded unsigned
+	 * 32-bit seconds/microseconds. This will overflow in 2106 for
+	 * drm_timestamp_monotonic==0, but not with drm_timestamp_monotonic==1
+	 */
+	e->event.tv_sec = tv.tv_sec;
+	e->event.tv_usec = tv.tv_nsec / 1000;
 
 	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
 					 e->event.sequence);
@@ -891,7 +892,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	unsigned int seq, pipe = drm_crtc_index(crtc);
-	struct timeval now;
+	ktime_t now;
 
 	if (dev->num_crtcs > 0) {
 		seq = drm_vblank_count_and_time(dev, pipe, &now);
@@ -902,7 +903,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 	}
 	e->pipe = pipe;
 	e->event.crtc_id = crtc->base.id;
-	send_vblank_event(dev, e, seq, &now);
+	send_vblank_event(dev, e, seq, now);
 }
 EXPORT_SYMBOL(drm_crtc_send_vblank_event);
 
@@ -1100,7 +1101,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 	unsigned int pipe = drm_crtc_index(crtc);
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	struct drm_pending_vblank_event *e, *t;
-	struct timeval now;
+
+	ktime_t now;
 	unsigned long irqflags;
 	unsigned int seq;
 
@@ -1141,7 +1143,7 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 			  e->event.sequence, seq);
 		list_del(&e->base.link);
 		drm_vblank_put(dev, pipe);
-		send_vblank_event(dev, e, seq, &now);
+		send_vblank_event(dev, e, seq, now);
 	}
 	spin_unlock_irqrestore(&dev->event_lock, irqflags);
 
@@ -1321,7 +1323,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	struct drm_pending_vblank_event *e;
-	struct timeval now;
+	ktime_t now;
 	unsigned long flags;
 	unsigned int seq;
 	int ret;
@@ -1367,7 +1369,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 	e->event.sequence = vblwait->request.sequence;
 	if (vblank_passed(seq, vblwait->request.sequence)) {
 		drm_vblank_put(dev, pipe);
-		send_vblank_event(dev, e, seq, &now);
+		send_vblank_event(dev, e, seq, now);
 		vblwait->reply.sequence = seq;
 	} else {
 		/* drm_handle_vblank_events will call drm_vblank_put */
@@ -1398,6 +1400,24 @@ static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
 					  _DRM_VBLANK_NEXTONMISS));
 }
 
+static void drm_wait_vblank_reply(struct drm_device *dev, unsigned int pipe,
+				  struct drm_wait_vblank_reply *reply)
+{
+	ktime_t now;
+	struct timespec64 ts;
+
+	/*
+	 * drm_wait_vblank_reply is a UAPI structure that uses 'long'
+	 * to store the seconds. This will overflow in y2038 on 32-bit
+	 * architectures with drm_timestamp_monotonic==0, but not with
+	 * drm_timestamp_monotonic==1 (the default).
+	 */
+	reply->sequence = drm_vblank_count_and_time(dev, pipe, &now);
+	ts = ktime_to_timespec64(now);
+	reply->tval_sec = (u32)ts.tv_sec;
+	reply->tval_usec = ts.tv_nsec / 1000;
+}
+
 int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv)
 {
@@ -1439,12 +1459,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 	if (dev->vblank_disable_immediate &&
 	    drm_wait_vblank_is_query(vblwait) &&
 	    READ_ONCE(vblank->enabled)) {
-		struct timeval now;
-
-		vblwait->reply.sequence =
-			drm_vblank_count_and_time(dev, pipe, &now);
-		vblwait->reply.tval_sec = now.tv_sec;
-		vblwait->reply.tval_usec = now.tv_usec;
+		drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
 		return 0;
 	}
 
@@ -1487,11 +1502,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 	}
 
 	if (ret != -EINTR) {
-		struct timeval now;
-
-		vblwait->reply.sequence = drm_vblank_count_and_time(dev, pipe, &now);
-		vblwait->reply.tval_sec = now.tv_sec;
-		vblwait->reply.tval_usec = now.tv_usec;
+		drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
 
 		DRM_DEBUG("crtc %d returning %u to client\n",
 			  pipe, vblwait->reply.sequence);
@@ -1507,7 +1518,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_pending_vblank_event *e, *t;
-	struct timeval now;
+	ktime_t now;
 	unsigned int seq;
 
 	assert_spin_locked(&dev->event_lock);
@@ -1525,7 +1536,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
 
 		list_del(&e->base.link);
 		drm_vblank_put(dev, pipe);
-		send_vblank_event(dev, e, seq, &now);
+		send_vblank_event(dev, e, seq, now);
 	}
 
 	trace_drm_vblank_event(pipe, seq);
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index ee06ecd6c01f..412e83a4d3db 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -324,7 +324,7 @@ struct drm_driver {
 	 */
 	bool (*get_vblank_timestamp) (struct drm_device *dev, unsigned int pipe,
 				     int *max_error,
-				     struct timeval *vblank_time,
+				     ktime_t *vblank_time,
 				     bool in_vblank_irq);
 
 	/**
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 7fba9efe4951..6a58e2e91a0f 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -92,7 +92,7 @@ struct drm_vblank_crtc {
 	/**
 	 * @time: Vblank timestamp corresponding to @count.
 	 */
-	struct timeval time;
+	ktime_t time;
 
 	/**
 	 * @refcount: Number of users/waiters of the vblank interrupt. Only when
@@ -154,7 +154,7 @@ struct drm_vblank_crtc {
 int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs);
 u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
 u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
-				   struct timeval *vblanktime);
+				   ktime_t *vblanktime);
 void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 			       struct drm_pending_vblank_event *e);
 void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
@@ -172,7 +172,7 @@ u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
 
 bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 					   unsigned int pipe, int *max_error,
-					   struct timeval *vblank_time,
+					   ktime_t *vblank_time,
 					   bool in_vblank_irq);
 void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode);
-- 
2.9.0

[PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Arnd Bergmann]

The drm vblank handling uses 'timeval' to store timestamps in either
monotonic or wall-clock time base. In either case, it reads the current
time as a ktime_t in get_drm_timestamp() and converts it from there.

This is a bit suspicious, as users of 'timeval' often suffer from
the time_t overflow in y2038. I have gone through this code and
found that it is unlikely to cause problems here:

- The user space ABI does not use time_t or timeval, but uses
  'u32' and 'long' as the types. This means at least that rebuilding
  user programs against a new libc with 64-bit time_t does not
  change the ABI.

- As of commit c61eef726a78 ("drm: add support for monotonic vblank
  timestamps") in linux-3.8, the monotonic timestamp is the default
  and can only get reverted to wall-clock through a module-parameter.

- With the default monotonic timestamps, there is no problem at all.

- The drm_wait_vblank_ioctl() interface is alway safe on 64-bit
  architectures, on 32-bit it might overflow the 'long' timestamps
  in 2038 with wall-clock timestamps.

- The event handling uses 'u32' seconds, which overflow in 2106
  on both 32-bit and 64-bit machines, when wall-clock timestamps
  are used.

- The effect of overflowing either of the two is only temporary
  (during the overflow, and is likely to keep working again
  afterwards. It is likely the same problem as observing a
  'settimeofday()' call, which was the reason for moving to the
  monotonic timestamps in the first place.

Overall, this seems good enough, so my patch removes the use of
'timeval' from the vblank handling altogether and uses ktime_t
consistently, except for the part where we copy the data to user
space structures in the existing format.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/drm_vblank.c | 123 +++++++++++++++++++++++--------------------
 include/drm/drm_drv.h        |   2 +-
 include/drm/drm_vblank.h     |   6 +--
 3 files changed, 71 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 70f2b9593edc..c605c3ad6b6e 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -78,7 +78,7 @@
 
 static bool
 drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
-			  struct timeval *tvblank, bool in_vblank_irq);
+			  ktime_t *tvblank, bool in_vblank_irq);
 
 static unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
 
@@ -99,7 +99,7 @@ MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");
 
 static void store_vblank(struct drm_device *dev, unsigned int pipe,
 			 u32 vblank_count_inc,
-			 struct timeval *t_vblank, u32 last)
+			 ktime_t t_vblank, u32 last)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 
@@ -108,7 +108,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
 	vblank->last = last;
 
 	write_seqlock(&vblank->seqlock);
-	vblank->time = *t_vblank;
+	vblank->time = t_vblank;
 	vblank->count += vblank_count_inc;
 	write_sequnlock(&vblank->seqlock);
 }
@@ -151,7 +151,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
 {
 	u32 cur_vblank;
 	bool rc;
-	struct timeval t_vblank;
+	ktime_t t_vblank;
 	int count = DRM_TIMESTAMP_MAXRETRIES;
 
 	spin_lock(&dev->vblank_time_lock);
@@ -171,13 +171,13 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
 	 * interrupt and assign 0 for now, to mark the vblanktimestamp as invalid.
 	 */
 	if (!rc)
-		t_vblank = (struct timeval) {0, 0};
+		t_vblank = 0;
 
 	/*
 	 * +1 to make sure user will never see the same
 	 * vblank counter value before and after a modeset
 	 */
-	store_vblank(dev, pipe, 1, &t_vblank, cur_vblank);
+	store_vblank(dev, pipe, 1, t_vblank, cur_vblank);
 
 	spin_unlock(&dev->vblank_time_lock);
 }
@@ -200,7 +200,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	u32 cur_vblank, diff;
 	bool rc;
-	struct timeval t_vblank;
+	ktime_t t_vblank;
 	int count = DRM_TIMESTAMP_MAXRETRIES;
 	int framedur_ns = vblank->framedur_ns;
 
@@ -225,11 +225,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 		/* trust the hw counter when it's around */
 		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
 	} else if (rc && framedur_ns) {
-		const struct timeval *t_old;
-		u64 diff_ns;
-
-		t_old = &vblank->time;
-		diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
+		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
 
 		/*
 		 * Figure out how many vblanks we've missed based
@@ -278,9 +274,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	 * for now, to mark the vblanktimestamp as invalid.
 	 */
 	if (!rc && !in_vblank_irq)
-		t_vblank = (struct timeval) {0, 0};
+		t_vblank = 0;
 
-	store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);
+	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
 }
 
 static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
@@ -556,7 +552,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  * @pipe: index of CRTC whose 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 time which should receive the timestamp
  * @in_vblank_irq:
  *     True when called from drm_crtc_handle_vblank().  Some drivers
  *     need to apply some workarounds for gpu-specific vblank irq quirks
@@ -584,10 +580,10 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
 bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 					   unsigned int pipe,
 					   int *max_error,
-					   struct timeval *vblank_time,
+					   ktime_t *vblank_time,
 					   bool in_vblank_irq)
 {
-	struct timeval tv_etime;
+	struct timespec64 ts_etime, ts_vblank_time;
 	ktime_t stime, etime;
 	bool vbl_status;
 	struct drm_crtc *crtc;
@@ -680,29 +676,27 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 		etime = ktime_mono_to_real(etime);
 
 	/* save this only for debugging purposes */
-	tv_etime = ktime_to_timeval(etime);
+	ts_etime = ktime_to_timespec64(etime);
+	ts_vblank_time = ktime_to_timespec64(*vblank_time);
 	/* Subtract time delta from raw timestamp to get final
 	 * vblank_time timestamp for end of vblank.
 	 */
 	etime = ktime_sub_ns(etime, delta_ns);
-	*vblank_time = ktime_to_timeval(etime);
+	*vblank_time = etime;
 
-	DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
+	DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %lld.%06ld -> %lld.%06ld [e %d us, %d rep]\n",
 		      pipe, hpos, vpos,
-		      (long)tv_etime.tv_sec, (long)tv_etime.tv_usec,
-		      (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
-		      duration_ns/1000, i);
+		      (u64)ts_etime.tv_sec, ts_etime.tv_nsec / 1000,
+		      (u64)ts_vblank_time.tv_sec, ts_vblank_time.tv_nsec / 1000,
+		      duration_ns / 1000, i);
 
 	return true;
 }
 EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
 
-static struct timeval get_drm_timestamp(void)
+static ktime_t get_drm_timestamp(void)
 {
-	ktime_t now;
-
-	now = drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
-	return ktime_to_timeval(now);
+	return drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
 }
 
 /**
@@ -710,7 +704,7 @@ static struct timeval get_drm_timestamp(void)
  *                             vblank interval
  * @dev: DRM device
  * @pipe: index of CRTC whose vblank timestamp to retrieve
- * @tvblank: Pointer to target struct timeval which should receive the timestamp
+ * @tvblank: Pointer to target time which should receive the timestamp
  * @in_vblank_irq:
  *     True when called from drm_crtc_handle_vblank().  Some drivers
  *     need to apply some workarounds for gpu-specific vblank irq quirks
@@ -728,7 +722,7 @@ static struct timeval get_drm_timestamp(void)
  */
 static bool
 drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
-			  struct timeval *tvblank, bool in_vblank_irq)
+			  ktime_t *tvblank, bool in_vblank_irq)
 {
 	bool ret = false;
 
@@ -769,14 +763,14 @@ u32 drm_crtc_vblank_count(struct drm_crtc *crtc)
 EXPORT_SYMBOL(drm_crtc_vblank_count);
 
 static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
-				     struct timeval *vblanktime)
+				     ktime_t *vblanktime)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	u32 vblank_count;
 	unsigned int seq;
 
 	if (WARN_ON(pipe >= dev->num_crtcs)) {
-		*vblanktime = (struct timeval) { 0 };
+		*vblanktime = 0;
 		return 0;
 	}
 
@@ -793,7 +787,7 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
  * drm_crtc_vblank_count_and_time - retrieve "cooked" vblank counter value
  *     and the system timestamp corresponding to that vblank counter value
  * @crtc: which counter to retrieve
- * @vblanktime: Pointer to struct timeval to receive the vblank timestamp.
+ * @vblanktime: Pointer to time to receive the vblank timestamp.
  *
  * Fetches the "cooked" vblank count value that represents the number of
  * vblank events since the system was booted, including lost events due to
@@ -801,7 +795,7 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
  * of the vblank interval that corresponds to the current vblank counter value.
  */
 u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
-				   struct timeval *vblanktime)
+				   ktime_t *vblanktime)
 {
 	return drm_vblank_count_and_time(crtc->dev, drm_crtc_index(crtc),
 					 vblanktime);
@@ -810,11 +804,18 @@ EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
 
 static void send_vblank_event(struct drm_device *dev,
 		struct drm_pending_vblank_event *e,
-		unsigned long seq, struct timeval *now)
+		unsigned long seq, ktime_t now)
 {
+	struct timespec64 tv = ktime_to_timespec64(now);
+
 	e->event.sequence = seq;
-	e->event.tv_sec = now->tv_sec;
-	e->event.tv_usec = now->tv_usec;
+	/*
+	 * e->event is a user space structure, with hardcoded unsigned
+	 * 32-bit seconds/microseconds. This will overflow in 2106 for
+	 * drm_timestamp_monotonic==0, but not with drm_timestamp_monotonic==1
+	 */
+	e->event.tv_sec = tv.tv_sec;
+	e->event.tv_usec = tv.tv_nsec / 1000;
 
 	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
 					 e->event.sequence);
@@ -891,7 +892,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	unsigned int seq, pipe = drm_crtc_index(crtc);
-	struct timeval now;
+	ktime_t now;
 
 	if (dev->num_crtcs > 0) {
 		seq = drm_vblank_count_and_time(dev, pipe, &now);
@@ -902,7 +903,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 	}
 	e->pipe = pipe;
 	e->event.crtc_id = crtc->base.id;
-	send_vblank_event(dev, e, seq, &now);
+	send_vblank_event(dev, e, seq, now);
 }
 EXPORT_SYMBOL(drm_crtc_send_vblank_event);
 
@@ -1100,7 +1101,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 	unsigned int pipe = drm_crtc_index(crtc);
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	struct drm_pending_vblank_event *e, *t;
-	struct timeval now;
+
+	ktime_t now;
 	unsigned long irqflags;
 	unsigned int seq;
 
@@ -1141,7 +1143,7 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 			  e->event.sequence, seq);
 		list_del(&e->base.link);
 		drm_vblank_put(dev, pipe);
-		send_vblank_event(dev, e, seq, &now);
+		send_vblank_event(dev, e, seq, now);
 	}
 	spin_unlock_irqrestore(&dev->event_lock, irqflags);
 
@@ -1321,7 +1323,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	struct drm_pending_vblank_event *e;
-	struct timeval now;
+	ktime_t now;
 	unsigned long flags;
 	unsigned int seq;
 	int ret;
@@ -1367,7 +1369,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 	e->event.sequence = vblwait->request.sequence;
 	if (vblank_passed(seq, vblwait->request.sequence)) {
 		drm_vblank_put(dev, pipe);
-		send_vblank_event(dev, e, seq, &now);
+		send_vblank_event(dev, e, seq, now);
 		vblwait->reply.sequence = seq;
 	} else {
 		/* drm_handle_vblank_events will call drm_vblank_put */
@@ -1398,6 +1400,24 @@ static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
 					  _DRM_VBLANK_NEXTONMISS));
 }
 
+static void drm_wait_vblank_reply(struct drm_device *dev, unsigned int pipe,
+				  struct drm_wait_vblank_reply *reply)
+{
+	ktime_t now;
+	struct timespec64 ts;
+
+	/*
+	 * drm_wait_vblank_reply is a UAPI structure that uses 'long'
+	 * to store the seconds. This will overflow in y2038 on 32-bit
+	 * architectures with drm_timestamp_monotonic==0, but not with
+	 * drm_timestamp_monotonic==1 (the default).
+	 */
+	reply->sequence = drm_vblank_count_and_time(dev, pipe, &now);
+	ts = ktime_to_timespec64(now);
+	reply->tval_sec = (u32)ts.tv_sec;
+	reply->tval_usec = ts.tv_nsec / 1000;
+}
+
 int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv)
 {
@@ -1439,12 +1459,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 	if (dev->vblank_disable_immediate &&
 	    drm_wait_vblank_is_query(vblwait) &&
 	    READ_ONCE(vblank->enabled)) {
-		struct timeval now;
-
-		vblwait->reply.sequence =
-			drm_vblank_count_and_time(dev, pipe, &now);
-		vblwait->reply.tval_sec = now.tv_sec;
-		vblwait->reply.tval_usec = now.tv_usec;
+		drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
 		return 0;
 	}
 
@@ -1487,11 +1502,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 	}
 
 	if (ret != -EINTR) {
-		struct timeval now;
-
-		vblwait->reply.sequence = drm_vblank_count_and_time(dev, pipe, &now);
-		vblwait->reply.tval_sec = now.tv_sec;
-		vblwait->reply.tval_usec = now.tv_usec;
+		drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
 
 		DRM_DEBUG("crtc %d returning %u to client\n",
 			  pipe, vblwait->reply.sequence);
@@ -1507,7 +1518,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_pending_vblank_event *e, *t;
-	struct timeval now;
+	ktime_t now;
 	unsigned int seq;
 
 	assert_spin_locked(&dev->event_lock);
@@ -1525,7 +1536,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
 
 		list_del(&e->base.link);
 		drm_vblank_put(dev, pipe);
-		send_vblank_event(dev, e, seq, &now);
+		send_vblank_event(dev, e, seq, now);
 	}
 
 	trace_drm_vblank_event(pipe, seq);
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index ee06ecd6c01f..412e83a4d3db 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -324,7 +324,7 @@ struct drm_driver {
 	 */
 	bool (*get_vblank_timestamp) (struct drm_device *dev, unsigned int pipe,
 				     int *max_error,
-				     struct timeval *vblank_time,
+				     ktime_t *vblank_time,
 				     bool in_vblank_irq);
 
 	/**
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 7fba9efe4951..6a58e2e91a0f 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -92,7 +92,7 @@ struct drm_vblank_crtc {
 	/**
 	 * @time: Vblank timestamp corresponding to @count.
 	 */
-	struct timeval time;
+	ktime_t time;
 
 	/**
 	 * @refcount: Number of users/waiters of the vblank interrupt. Only when
@@ -154,7 +154,7 @@ struct drm_vblank_crtc {
 int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs);
 u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
 u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
-				   struct timeval *vblanktime);
+				   ktime_t *vblanktime);
 void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 			       struct drm_pending_vblank_event *e);
 void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
@@ -172,7 +172,7 @@ u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
 
 bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 					   unsigned int pipe, int *max_error,
-					   struct timeval *vblank_time,
+					   ktime_t *vblank_time,
 					   bool in_vblank_irq);
 void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode);
-- 
2.9.0

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

[PATCH 2/2] drm: vblank: remove drm_timestamp_monotonic parameter

[Arnd Bergmann]

There is a risk of overflowing vblank timestamps in 2038 or 2106 if
someone sets the drm_timestamp_monotonic module parameter to zero.

I found no indication of anyone ever setting the parameter, or
complaining about the default being wrong, after it was introduced
as a way to handle backwards-compatibility with linux prior to
c61eef726a78 ("drm: add support for monotonic vblank timestamps"),
so it's probably safer to just remove the parameter completely
and only allowing the default behavior.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/drm_internal.h |  1 -
 drivers/gpu/drm/drm_ioctl.c    |  2 +-
 drivers/gpu/drm/drm_vblank.c   | 29 ++++++-----------------------
 3 files changed, 7 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index fbc3f308fa19..edd921adcf33 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -55,7 +55,6 @@ int drm_clients_info(struct seq_file *m, void* data);
 int drm_gem_name_info(struct seq_file *m, void *data);
 
 /* drm_vblank.c */
-extern unsigned int drm_timestamp_monotonic;
 void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe);
 void drm_vblank_cleanup(struct drm_device *dev);
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a9ae6dd2d593..a78f03155466 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -235,7 +235,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 	/* Only some caps make sense with UMS/render-only drivers. */
 	switch (req->capability) {
 	case DRM_CAP_TIMESTAMP_MONOTONIC:
-		req->value = drm_timestamp_monotonic;
+		req->value = 1;
 		return 0;
 	case DRM_CAP_PRIME:
 		req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index c605c3ad6b6e..810a93fc558b 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -82,20 +82,12 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
 
 static unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
 
-/*
- * Default to use monotonic timestamps for wait-for-vblank and page-flip
- * complete events.
- */
-unsigned int drm_timestamp_monotonic = 1;
-
 static int drm_vblank_offdelay = 5000;    /* Default to 5000 msecs. */
 
 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);
 MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs] (0: never disable, <0: disable immediately)");
 MODULE_PARM_DESC(timestamp_precision_usec, "Max. error on timestamps [usecs]");
-MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");
 
 static void store_vblank(struct drm_device *dev, unsigned int pipe,
 			 u32 vblank_count_inc,
@@ -672,9 +664,6 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 	delta_ns = div_s64(1000000LL * (vpos * mode->crtc_htotal + hpos),
 			   mode->crtc_clock);
 
-	if (!drm_timestamp_monotonic)
-		etime = ktime_mono_to_real(etime);
-
 	/* save this only for debugging purposes */
 	ts_etime = ktime_to_timespec64(etime);
 	ts_vblank_time = ktime_to_timespec64(*vblank_time);
@@ -694,11 +683,6 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
 
-static ktime_t get_drm_timestamp(void)
-{
-	return drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
-}
-
 /**
  * drm_get_last_vbltimestamp - retrieve raw timestamp for the most recent
  *                             vblank interval
@@ -738,7 +722,7 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
 	 * Return current monotonic/gettimeofday timestamp as best estimate.
 	 */
 	if (!ret)
-		*tvblank = get_drm_timestamp();
+		*tvblank = ktime_get();
 
 	return ret;
 }
@@ -811,8 +795,8 @@ static void send_vblank_event(struct drm_device *dev,
 	e->event.sequence = seq;
 	/*
 	 * e->event is a user space structure, with hardcoded unsigned
-	 * 32-bit seconds/microseconds. This will overflow in 2106 for
-	 * drm_timestamp_monotonic==0, but not with drm_timestamp_monotonic==1
+	 * 32-bit seconds/microseconds. This is safe as we always use
+	 * monotonic timestamps since linux-4.15
 	 */
 	e->event.tv_sec = tv.tv_sec;
 	e->event.tv_usec = tv.tv_nsec / 1000;
@@ -899,7 +883,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 	} else {
 		seq = 0;
 
-		now = get_drm_timestamp();
+		now = ktime_get();
 	}
 	e->pipe = pipe;
 	e->event.crtc_id = crtc->base.id;
@@ -1408,9 +1392,8 @@ static void drm_wait_vblank_reply(struct drm_device *dev, unsigned int pipe,
 
 	/*
 	 * drm_wait_vblank_reply is a UAPI structure that uses 'long'
-	 * to store the seconds. This will overflow in y2038 on 32-bit
-	 * architectures with drm_timestamp_monotonic==0, but not with
-	 * drm_timestamp_monotonic==1 (the default).
+	 * to store the seconds. This is safe as we always use monotonic
+	 * timestamps since linux-4.15.
 	 */
 	reply->sequence = drm_vblank_count_and_time(dev, pipe, &now);
 	ts = ktime_to_timespec64(now);
-- 
2.9.0

Re: [PATCH 2/2] drm: vblank: remove drm_timestamp_monotonic parameter

[Daniel Stone]

On 11 October 2017 at 16:20, Arnd Bergmann <arnd@arndb.de> wrote:
> There is a risk of overflowing vblank timestamps in 2038 or 2106 if
> someone sets the drm_timestamp_monotonic module parameter to zero.
>
> I found no indication of anyone ever setting the parameter, or
> complaining about the default being wrong, after it was introduced
> as a way to handle backwards-compatibility with linux prior to
> c61eef726a78 ("drm: add support for monotonic vblank timestamps"),
> so it's probably safer to just remove the parameter completely
> and only allowing the default behavior.

I don't think there's any reason to remain suspicious of
CLOCK_MONOTONIC in 2017. Not to mention that removing it wrecks
Weston/etc's ability to give clients present-timing feedback, so
removing it is a net uABI improvement.

Acked-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel

Re: [PATCH 2/2] drm: vblank: remove drm_timestamp_monotonic parameter

[Daniel Stone]

On 11 October 2017 at 16:20, Arnd Bergmann <arnd@arndb.de> wrote:
> There is a risk of overflowing vblank timestamps in 2038 or 2106 if
> someone sets the drm_timestamp_monotonic module parameter to zero.
>
> I found no indication of anyone ever setting the parameter, or
> complaining about the default being wrong, after it was introduced
> as a way to handle backwards-compatibility with linux prior to
> c61eef726a78 ("drm: add support for monotonic vblank timestamps"),
> so it's probably safer to just remove the parameter completely
> and only allowing the default behavior.

I don't think there's any reason to remain suspicious of
CLOCK_MONOTONIC in 2017. Not to mention that removing it wrecks
Weston/etc's ability to give clients present-timing feedback, so
removing it is a net uABI improvement.

Acked-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Sean Paul]

On Wed, Oct 11, 2017 at 05:20:12PM +0200, Arnd Bergmann wrote:
> The drm vblank handling uses 'timeval' to store timestamps in either
> monotonic or wall-clock time base. In either case, it reads the current
> time as a ktime_t in get_drm_timestamp() and converts it from there.
> 
> This is a bit suspicious, as users of 'timeval' often suffer from
> the time_t overflow in y2038. I have gone through this code and
> found that it is unlikely to cause problems here:
> 
> - The user space ABI does not use time_t or timeval, but uses
>   'u32' and 'long' as the types. This means at least that rebuilding
>   user programs against a new libc with 64-bit time_t does not
>   change the ABI.
> 
> - As of commit c61eef726a78 ("drm: add support for monotonic vblank
>   timestamps") in linux-3.8, the monotonic timestamp is the default
>   and can only get reverted to wall-clock through a module-parameter.
> 
> - With the default monotonic timestamps, there is no problem at all.
> 
> - The drm_wait_vblank_ioctl() interface is alway safe on 64-bit
>   architectures, on 32-bit it might overflow the 'long' timestamps
>   in 2038 with wall-clock timestamps.
> 
> - The event handling uses 'u32' seconds, which overflow in 2106
>   on both 32-bit and 64-bit machines, when wall-clock timestamps
>   are used.
> 
> - The effect of overflowing either of the two is only temporary
>   (during the overflow, and is likely to keep working again
>   afterwards. It is likely the same problem as observing a
>   'settimeofday()' call, which was the reason for moving to the
>   monotonic timestamps in the first place.
> 
> Overall, this seems good enough, so my patch removes the use of
> 'timeval' from the vblank handling altogether and uses ktime_t
> consistently, except for the part where we copy the data to user
> space structures in the existing format.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Hi Arnd,
Keith posted something very similar with:
<20171011004514.9500-2-keithp@keithp.com> "drm: Widen vblank UAPI to 64 bits.
Change vblank time to ktime_t"

It looks like perhaps Keith missed one of the comment tweaks that you have
below.

Keith, perhaps you can rebase your widening patch on this one?

Sean


> ---
>  drivers/gpu/drm/drm_vblank.c | 123 +++++++++++++++++++++++--------------------
>  include/drm/drm_drv.h        |   2 +-
>  include/drm/drm_vblank.h     |   6 +--
>  3 files changed, 71 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 70f2b9593edc..c605c3ad6b6e 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -78,7 +78,7 @@
>  
>  static bool
>  drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> -			  struct timeval *tvblank, bool in_vblank_irq);
> +			  ktime_t *tvblank, bool in_vblank_irq);
>  
>  static unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
>  
> @@ -99,7 +99,7 @@ MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");
>  
>  static void store_vblank(struct drm_device *dev, unsigned int pipe,
>  			 u32 vblank_count_inc,
> -			 struct timeval *t_vblank, u32 last)
> +			 ktime_t t_vblank, u32 last)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  
> @@ -108,7 +108,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
>  	vblank->last = last;
>  
>  	write_seqlock(&vblank->seqlock);
> -	vblank->time = *t_vblank;
> +	vblank->time = t_vblank;
>  	vblank->count += vblank_count_inc;
>  	write_sequnlock(&vblank->seqlock);
>  }
> @@ -151,7 +151,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
>  {
>  	u32 cur_vblank;
>  	bool rc;
> -	struct timeval t_vblank;
> +	ktime_t t_vblank;
>  	int count = DRM_TIMESTAMP_MAXRETRIES;
>  
>  	spin_lock(&dev->vblank_time_lock);
> @@ -171,13 +171,13 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
>  	 * interrupt and assign 0 for now, to mark the vblanktimestamp as invalid.
>  	 */
>  	if (!rc)
> -		t_vblank = (struct timeval) {0, 0};
> +		t_vblank = 0;
>  
>  	/*
>  	 * +1 to make sure user will never see the same
>  	 * vblank counter value before and after a modeset
>  	 */
> -	store_vblank(dev, pipe, 1, &t_vblank, cur_vblank);
> +	store_vblank(dev, pipe, 1, t_vblank, cur_vblank);
>  
>  	spin_unlock(&dev->vblank_time_lock);
>  }
> @@ -200,7 +200,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	u32 cur_vblank, diff;
>  	bool rc;
> -	struct timeval t_vblank;
> +	ktime_t t_vblank;
>  	int count = DRM_TIMESTAMP_MAXRETRIES;
>  	int framedur_ns = vblank->framedur_ns;
>  
> @@ -225,11 +225,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  		/* trust the hw counter when it's around */
>  		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
>  	} else if (rc && framedur_ns) {
> -		const struct timeval *t_old;
> -		u64 diff_ns;
> -
> -		t_old = &vblank->time;
> -		diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
> +		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
>  
>  		/*
>  		 * Figure out how many vblanks we've missed based
> @@ -278,9 +274,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	 * for now, to mark the vblanktimestamp as invalid.
>  	 */
>  	if (!rc && !in_vblank_irq)
> -		t_vblank = (struct timeval) {0, 0};
> +		t_vblank = 0;
>  
> -	store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);
> +	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
>  }
>  
>  static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> @@ -556,7 +552,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
>   * @pipe: index of CRTC whose 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 time which should receive the timestamp
>   * @in_vblank_irq:
>   *     True when called from drm_crtc_handle_vblank().  Some drivers
>   *     need to apply some workarounds for gpu-specific vblank irq quirks
> @@ -584,10 +580,10 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
>  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  					   unsigned int pipe,
>  					   int *max_error,
> -					   struct timeval *vblank_time,
> +					   ktime_t *vblank_time,
>  					   bool in_vblank_irq)
>  {
> -	struct timeval tv_etime;
> +	struct timespec64 ts_etime, ts_vblank_time;
>  	ktime_t stime, etime;
>  	bool vbl_status;
>  	struct drm_crtc *crtc;
> @@ -680,29 +676,27 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  		etime = ktime_mono_to_real(etime);
>  
>  	/* save this only for debugging purposes */
> -	tv_etime = ktime_to_timeval(etime);
> +	ts_etime = ktime_to_timespec64(etime);
> +	ts_vblank_time = ktime_to_timespec64(*vblank_time);
>  	/* Subtract time delta from raw timestamp to get final
>  	 * vblank_time timestamp for end of vblank.
>  	 */
>  	etime = ktime_sub_ns(etime, delta_ns);
> -	*vblank_time = ktime_to_timeval(etime);
> +	*vblank_time = etime;
>  
> -	DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
> +	DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %lld.%06ld -> %lld.%06ld [e %d us, %d rep]\n",
>  		      pipe, hpos, vpos,
> -		      (long)tv_etime.tv_sec, (long)tv_etime.tv_usec,
> -		      (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
> -		      duration_ns/1000, i);
> +		      (u64)ts_etime.tv_sec, ts_etime.tv_nsec / 1000,
> +		      (u64)ts_vblank_time.tv_sec, ts_vblank_time.tv_nsec / 1000,
> +		      duration_ns / 1000, i);
>  
>  	return true;
>  }
>  EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
>  
> -static struct timeval get_drm_timestamp(void)
> +static ktime_t get_drm_timestamp(void)
>  {
> -	ktime_t now;
> -
> -	now = drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
> -	return ktime_to_timeval(now);
> +	return drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
>  }
>  
>  /**
> @@ -710,7 +704,7 @@ static struct timeval get_drm_timestamp(void)
>   *                             vblank interval
>   * @dev: DRM device
>   * @pipe: index of CRTC whose vblank timestamp to retrieve
> - * @tvblank: Pointer to target struct timeval which should receive the timestamp
> + * @tvblank: Pointer to target time which should receive the timestamp
>   * @in_vblank_irq:
>   *     True when called from drm_crtc_handle_vblank().  Some drivers
>   *     need to apply some workarounds for gpu-specific vblank irq quirks
> @@ -728,7 +722,7 @@ static struct timeval get_drm_timestamp(void)
>   */
>  static bool
>  drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> -			  struct timeval *tvblank, bool in_vblank_irq)
> +			  ktime_t *tvblank, bool in_vblank_irq)
>  {
>  	bool ret = false;
>  
> @@ -769,14 +763,14 @@ u32 drm_crtc_vblank_count(struct drm_crtc *crtc)
>  EXPORT_SYMBOL(drm_crtc_vblank_count);
>  
>  static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> -				     struct timeval *vblanktime)
> +				     ktime_t *vblanktime)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	u32 vblank_count;
>  	unsigned int seq;
>  
>  	if (WARN_ON(pipe >= dev->num_crtcs)) {
> -		*vblanktime = (struct timeval) { 0 };
> +		*vblanktime = 0;
>  		return 0;
>  	}
>  
> @@ -793,7 +787,7 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
>   * drm_crtc_vblank_count_and_time - retrieve "cooked" vblank counter value
>   *     and the system timestamp corresponding to that vblank counter value
>   * @crtc: which counter to retrieve
> - * @vblanktime: Pointer to struct timeval to receive the vblank timestamp.
> + * @vblanktime: Pointer to time to receive the vblank timestamp.
>   *
>   * Fetches the "cooked" vblank count value that represents the number of
>   * vblank events since the system was booted, including lost events due to
> @@ -801,7 +795,7 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
>   * of the vblank interval that corresponds to the current vblank counter value.
>   */
>  u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> -				   struct timeval *vblanktime)
> +				   ktime_t *vblanktime)
>  {
>  	return drm_vblank_count_and_time(crtc->dev, drm_crtc_index(crtc),
>  					 vblanktime);
> @@ -810,11 +804,18 @@ EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
>  
>  static void send_vblank_event(struct drm_device *dev,
>  		struct drm_pending_vblank_event *e,
> -		unsigned long seq, struct timeval *now)
> +		unsigned long seq, ktime_t now)
>  {
> +	struct timespec64 tv = ktime_to_timespec64(now);
> +
>  	e->event.sequence = seq;
> -	e->event.tv_sec = now->tv_sec;
> -	e->event.tv_usec = now->tv_usec;
> +	/*
> +	 * e->event is a user space structure, with hardcoded unsigned
> +	 * 32-bit seconds/microseconds. This will overflow in 2106 for
> +	 * drm_timestamp_monotonic==0, but not with drm_timestamp_monotonic==1
> +	 */
> +	e->event.tv_sec = tv.tv_sec;
> +	e->event.tv_usec = tv.tv_nsec / 1000;
>  
>  	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
>  					 e->event.sequence);
> @@ -891,7 +892,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->dev;
>  	unsigned int seq, pipe = drm_crtc_index(crtc);
> -	struct timeval now;
> +	ktime_t now;
>  
>  	if (dev->num_crtcs > 0) {
>  		seq = drm_vblank_count_and_time(dev, pipe, &now);
> @@ -902,7 +903,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  	}
>  	e->pipe = pipe;
>  	e->event.crtc_id = crtc->base.id;
> -	send_vblank_event(dev, e, seq, &now);
> +	send_vblank_event(dev, e, seq, now);
>  }
>  EXPORT_SYMBOL(drm_crtc_send_vblank_event);
>  
> @@ -1100,7 +1101,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
>  	unsigned int pipe = drm_crtc_index(crtc);
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	struct drm_pending_vblank_event *e, *t;
> -	struct timeval now;
> +
> +	ktime_t now;
>  	unsigned long irqflags;
>  	unsigned int seq;
>  
> @@ -1141,7 +1143,7 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
>  			  e->event.sequence, seq);
>  		list_del(&e->base.link);
>  		drm_vblank_put(dev, pipe);
> -		send_vblank_event(dev, e, seq, &now);
> +		send_vblank_event(dev, e, seq, now);
>  	}
>  	spin_unlock_irqrestore(&dev->event_lock, irqflags);
>  
> @@ -1321,7 +1323,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	struct drm_pending_vblank_event *e;
> -	struct timeval now;
> +	ktime_t now;
>  	unsigned long flags;
>  	unsigned int seq;
>  	int ret;
> @@ -1367,7 +1369,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	e->event.sequence = vblwait->request.sequence;
>  	if (vblank_passed(seq, vblwait->request.sequence)) {
>  		drm_vblank_put(dev, pipe);
> -		send_vblank_event(dev, e, seq, &now);
> +		send_vblank_event(dev, e, seq, now);
>  		vblwait->reply.sequence = seq;
>  	} else {
>  		/* drm_handle_vblank_events will call drm_vblank_put */
> @@ -1398,6 +1400,24 @@ static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
>  					  _DRM_VBLANK_NEXTONMISS));
>  }
>  
> +static void drm_wait_vblank_reply(struct drm_device *dev, unsigned int pipe,
> +				  struct drm_wait_vblank_reply *reply)
> +{
> +	ktime_t now;
> +	struct timespec64 ts;
> +
> +	/*
> +	 * drm_wait_vblank_reply is a UAPI structure that uses 'long'
> +	 * to store the seconds. This will overflow in y2038 on 32-bit
> +	 * architectures with drm_timestamp_monotonic==0, but not with
> +	 * drm_timestamp_monotonic==1 (the default).
> +	 */
> +	reply->sequence = drm_vblank_count_and_time(dev, pipe, &now);
> +	ts = ktime_to_timespec64(now);
> +	reply->tval_sec = (u32)ts.tv_sec;
> +	reply->tval_usec = ts.tv_nsec / 1000;
> +}
> +
>  int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  			  struct drm_file *file_priv)
>  {
> @@ -1439,12 +1459,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  	if (dev->vblank_disable_immediate &&
>  	    drm_wait_vblank_is_query(vblwait) &&
>  	    READ_ONCE(vblank->enabled)) {
> -		struct timeval now;
> -
> -		vblwait->reply.sequence =
> -			drm_vblank_count_and_time(dev, pipe, &now);
> -		vblwait->reply.tval_sec = now.tv_sec;
> -		vblwait->reply.tval_usec = now.tv_usec;
> +		drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
>  		return 0;
>  	}
>  
> @@ -1487,11 +1502,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	if (ret != -EINTR) {
> -		struct timeval now;
> -
> -		vblwait->reply.sequence = drm_vblank_count_and_time(dev, pipe, &now);
> -		vblwait->reply.tval_sec = now.tv_sec;
> -		vblwait->reply.tval_usec = now.tv_usec;
> +		drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
>  
>  		DRM_DEBUG("crtc %d returning %u to client\n",
>  			  pipe, vblwait->reply.sequence);
> @@ -1507,7 +1518,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_pending_vblank_event *e, *t;
> -	struct timeval now;
> +	ktime_t now;
>  	unsigned int seq;
>  
>  	assert_spin_locked(&dev->event_lock);
> @@ -1525,7 +1536,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
>  
>  		list_del(&e->base.link);
>  		drm_vblank_put(dev, pipe);
> -		send_vblank_event(dev, e, seq, &now);
> +		send_vblank_event(dev, e, seq, now);
>  	}
>  
>  	trace_drm_vblank_event(pipe, seq);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index ee06ecd6c01f..412e83a4d3db 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -324,7 +324,7 @@ struct drm_driver {
>  	 */
>  	bool (*get_vblank_timestamp) (struct drm_device *dev, unsigned int pipe,
>  				     int *max_error,
> -				     struct timeval *vblank_time,
> +				     ktime_t *vblank_time,
>  				     bool in_vblank_irq);
>  
>  	/**
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 7fba9efe4951..6a58e2e91a0f 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -92,7 +92,7 @@ struct drm_vblank_crtc {
>  	/**
>  	 * @time: Vblank timestamp corresponding to @count.
>  	 */
> -	struct timeval time;
> +	ktime_t time;
>  
>  	/**
>  	 * @refcount: Number of users/waiters of the vblank interrupt. Only when
> @@ -154,7 +154,7 @@ struct drm_vblank_crtc {
>  int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs);
>  u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
>  u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> -				   struct timeval *vblanktime);
> +				   ktime_t *vblanktime);
>  void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  			       struct drm_pending_vblank_event *e);
>  void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> @@ -172,7 +172,7 @@ u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
>  
>  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  					   unsigned int pipe, int *max_error,
> -					   struct timeval *vblank_time,
> +					   ktime_t *vblank_time,
>  					   bool in_vblank_irq);
>  void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  				     const struct drm_display_mode *mode);
> -- 
> 2.9.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Sean Paul]

On Wed, Oct 11, 2017 at 05:20:12PM +0200, Arnd Bergmann wrote:
> The drm vblank handling uses 'timeval' to store timestamps in either
> monotonic or wall-clock time base. In either case, it reads the current
> time as a ktime_t in get_drm_timestamp() and converts it from there.
> 
> This is a bit suspicious, as users of 'timeval' often suffer from
> the time_t overflow in y2038. I have gone through this code and
> found that it is unlikely to cause problems here:
> 
> - The user space ABI does not use time_t or timeval, but uses
>   'u32' and 'long' as the types. This means at least that rebuilding
>   user programs against a new libc with 64-bit time_t does not
>   change the ABI.
> 
> - As of commit c61eef726a78 ("drm: add support for monotonic vblank
>   timestamps") in linux-3.8, the monotonic timestamp is the default
>   and can only get reverted to wall-clock through a module-parameter.
> 
> - With the default monotonic timestamps, there is no problem at all.
> 
> - The drm_wait_vblank_ioctl() interface is alway safe on 64-bit
>   architectures, on 32-bit it might overflow the 'long' timestamps
>   in 2038 with wall-clock timestamps.
> 
> - The event handling uses 'u32' seconds, which overflow in 2106
>   on both 32-bit and 64-bit machines, when wall-clock timestamps
>   are used.
> 
> - The effect of overflowing either of the two is only temporary
>   (during the overflow, and is likely to keep working again
>   afterwards. It is likely the same problem as observing a
>   'settimeofday()' call, which was the reason for moving to the
>   monotonic timestamps in the first place.
> 
> Overall, this seems good enough, so my patch removes the use of
> 'timeval' from the vblank handling altogether and uses ktime_t
> consistently, except for the part where we copy the data to user
> space structures in the existing format.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Hi Arnd,
Keith posted something very similar with:
<20171011004514.9500-2-keithp@keithp.com> "drm: Widen vblank UAPI to 64 bits.
Change vblank time to ktime_t"

It looks like perhaps Keith missed one of the comment tweaks that you have
below.

Keith, perhaps you can rebase your widening patch on this one?

Sean


> ---
>  drivers/gpu/drm/drm_vblank.c | 123 +++++++++++++++++++++++--------------------
>  include/drm/drm_drv.h        |   2 +-
>  include/drm/drm_vblank.h     |   6 +--
>  3 files changed, 71 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 70f2b9593edc..c605c3ad6b6e 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -78,7 +78,7 @@
>  
>  static bool
>  drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> -			  struct timeval *tvblank, bool in_vblank_irq);
> +			  ktime_t *tvblank, bool in_vblank_irq);
>  
>  static unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
>  
> @@ -99,7 +99,7 @@ MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");
>  
>  static void store_vblank(struct drm_device *dev, unsigned int pipe,
>  			 u32 vblank_count_inc,
> -			 struct timeval *t_vblank, u32 last)
> +			 ktime_t t_vblank, u32 last)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  
> @@ -108,7 +108,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
>  	vblank->last = last;
>  
>  	write_seqlock(&vblank->seqlock);
> -	vblank->time = *t_vblank;
> +	vblank->time = t_vblank;
>  	vblank->count += vblank_count_inc;
>  	write_sequnlock(&vblank->seqlock);
>  }
> @@ -151,7 +151,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
>  {
>  	u32 cur_vblank;
>  	bool rc;
> -	struct timeval t_vblank;
> +	ktime_t t_vblank;
>  	int count = DRM_TIMESTAMP_MAXRETRIES;
>  
>  	spin_lock(&dev->vblank_time_lock);
> @@ -171,13 +171,13 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
>  	 * interrupt and assign 0 for now, to mark the vblanktimestamp as invalid.
>  	 */
>  	if (!rc)
> -		t_vblank = (struct timeval) {0, 0};
> +		t_vblank = 0;
>  
>  	/*
>  	 * +1 to make sure user will never see the same
>  	 * vblank counter value before and after a modeset
>  	 */
> -	store_vblank(dev, pipe, 1, &t_vblank, cur_vblank);
> +	store_vblank(dev, pipe, 1, t_vblank, cur_vblank);
>  
>  	spin_unlock(&dev->vblank_time_lock);
>  }
> @@ -200,7 +200,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	u32 cur_vblank, diff;
>  	bool rc;
> -	struct timeval t_vblank;
> +	ktime_t t_vblank;
>  	int count = DRM_TIMESTAMP_MAXRETRIES;
>  	int framedur_ns = vblank->framedur_ns;
>  
> @@ -225,11 +225,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  		/* trust the hw counter when it's around */
>  		diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
>  	} else if (rc && framedur_ns) {
> -		const struct timeval *t_old;
> -		u64 diff_ns;
> -
> -		t_old = &vblank->time;
> -		diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
> +		u64 diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
>  
>  		/*
>  		 * Figure out how many vblanks we've missed based
> @@ -278,9 +274,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  	 * for now, to mark the vblanktimestamp as invalid.
>  	 */
>  	if (!rc && !in_vblank_irq)
> -		t_vblank = (struct timeval) {0, 0};
> +		t_vblank = 0;
>  
> -	store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);
> +	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
>  }
>  
>  static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
> @@ -556,7 +552,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
>   * @pipe: index of CRTC whose 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 time which should receive the timestamp
>   * @in_vblank_irq:
>   *     True when called from drm_crtc_handle_vblank().  Some drivers
>   *     need to apply some workarounds for gpu-specific vblank irq quirks
> @@ -584,10 +580,10 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
>  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  					   unsigned int pipe,
>  					   int *max_error,
> -					   struct timeval *vblank_time,
> +					   ktime_t *vblank_time,
>  					   bool in_vblank_irq)
>  {
> -	struct timeval tv_etime;
> +	struct timespec64 ts_etime, ts_vblank_time;
>  	ktime_t stime, etime;
>  	bool vbl_status;
>  	struct drm_crtc *crtc;
> @@ -680,29 +676,27 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  		etime = ktime_mono_to_real(etime);
>  
>  	/* save this only for debugging purposes */
> -	tv_etime = ktime_to_timeval(etime);
> +	ts_etime = ktime_to_timespec64(etime);
> +	ts_vblank_time = ktime_to_timespec64(*vblank_time);
>  	/* Subtract time delta from raw timestamp to get final
>  	 * vblank_time timestamp for end of vblank.
>  	 */
>  	etime = ktime_sub_ns(etime, delta_ns);
> -	*vblank_time = ktime_to_timeval(etime);
> +	*vblank_time = etime;
>  
> -	DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
> +	DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %lld.%06ld -> %lld.%06ld [e %d us, %d rep]\n",
>  		      pipe, hpos, vpos,
> -		      (long)tv_etime.tv_sec, (long)tv_etime.tv_usec,
> -		      (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
> -		      duration_ns/1000, i);
> +		      (u64)ts_etime.tv_sec, ts_etime.tv_nsec / 1000,
> +		      (u64)ts_vblank_time.tv_sec, ts_vblank_time.tv_nsec / 1000,
> +		      duration_ns / 1000, i);
>  
>  	return true;
>  }
>  EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
>  
> -static struct timeval get_drm_timestamp(void)
> +static ktime_t get_drm_timestamp(void)
>  {
> -	ktime_t now;
> -
> -	now = drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
> -	return ktime_to_timeval(now);
> +	return drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
>  }
>  
>  /**
> @@ -710,7 +704,7 @@ static struct timeval get_drm_timestamp(void)
>   *                             vblank interval
>   * @dev: DRM device
>   * @pipe: index of CRTC whose vblank timestamp to retrieve
> - * @tvblank: Pointer to target struct timeval which should receive the timestamp
> + * @tvblank: Pointer to target time which should receive the timestamp
>   * @in_vblank_irq:
>   *     True when called from drm_crtc_handle_vblank().  Some drivers
>   *     need to apply some workarounds for gpu-specific vblank irq quirks
> @@ -728,7 +722,7 @@ static struct timeval get_drm_timestamp(void)
>   */
>  static bool
>  drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> -			  struct timeval *tvblank, bool in_vblank_irq)
> +			  ktime_t *tvblank, bool in_vblank_irq)
>  {
>  	bool ret = false;
>  
> @@ -769,14 +763,14 @@ u32 drm_crtc_vblank_count(struct drm_crtc *crtc)
>  EXPORT_SYMBOL(drm_crtc_vblank_count);
>  
>  static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> -				     struct timeval *vblanktime)
> +				     ktime_t *vblanktime)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	u32 vblank_count;
>  	unsigned int seq;
>  
>  	if (WARN_ON(pipe >= dev->num_crtcs)) {
> -		*vblanktime = (struct timeval) { 0 };
> +		*vblanktime = 0;
>  		return 0;
>  	}
>  
> @@ -793,7 +787,7 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
>   * drm_crtc_vblank_count_and_time - retrieve "cooked" vblank counter value
>   *     and the system timestamp corresponding to that vblank counter value
>   * @crtc: which counter to retrieve
> - * @vblanktime: Pointer to struct timeval to receive the vblank timestamp.
> + * @vblanktime: Pointer to time to receive the vblank timestamp.
>   *
>   * Fetches the "cooked" vblank count value that represents the number of
>   * vblank events since the system was booted, including lost events due to
> @@ -801,7 +795,7 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
>   * of the vblank interval that corresponds to the current vblank counter value.
>   */
>  u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> -				   struct timeval *vblanktime)
> +				   ktime_t *vblanktime)
>  {
>  	return drm_vblank_count_and_time(crtc->dev, drm_crtc_index(crtc),
>  					 vblanktime);
> @@ -810,11 +804,18 @@ EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
>  
>  static void send_vblank_event(struct drm_device *dev,
>  		struct drm_pending_vblank_event *e,
> -		unsigned long seq, struct timeval *now)
> +		unsigned long seq, ktime_t now)
>  {
> +	struct timespec64 tv = ktime_to_timespec64(now);
> +
>  	e->event.sequence = seq;
> -	e->event.tv_sec = now->tv_sec;
> -	e->event.tv_usec = now->tv_usec;
> +	/*
> +	 * e->event is a user space structure, with hardcoded unsigned
> +	 * 32-bit seconds/microseconds. This will overflow in 2106 for
> +	 * drm_timestamp_monotonic==0, but not with drm_timestamp_monotonic==1
> +	 */
> +	e->event.tv_sec = tv.tv_sec;
> +	e->event.tv_usec = tv.tv_nsec / 1000;
>  
>  	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
>  					 e->event.sequence);
> @@ -891,7 +892,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->dev;
>  	unsigned int seq, pipe = drm_crtc_index(crtc);
> -	struct timeval now;
> +	ktime_t now;
>  
>  	if (dev->num_crtcs > 0) {
>  		seq = drm_vblank_count_and_time(dev, pipe, &now);
> @@ -902,7 +903,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  	}
>  	e->pipe = pipe;
>  	e->event.crtc_id = crtc->base.id;
> -	send_vblank_event(dev, e, seq, &now);
> +	send_vblank_event(dev, e, seq, now);
>  }
>  EXPORT_SYMBOL(drm_crtc_send_vblank_event);
>  
> @@ -1100,7 +1101,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
>  	unsigned int pipe = drm_crtc_index(crtc);
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	struct drm_pending_vblank_event *e, *t;
> -	struct timeval now;
> +
> +	ktime_t now;
>  	unsigned long irqflags;
>  	unsigned int seq;
>  
> @@ -1141,7 +1143,7 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
>  			  e->event.sequence, seq);
>  		list_del(&e->base.link);
>  		drm_vblank_put(dev, pipe);
> -		send_vblank_event(dev, e, seq, &now);
> +		send_vblank_event(dev, e, seq, now);
>  	}
>  	spin_unlock_irqrestore(&dev->event_lock, irqflags);
>  
> @@ -1321,7 +1323,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	struct drm_pending_vblank_event *e;
> -	struct timeval now;
> +	ktime_t now;
>  	unsigned long flags;
>  	unsigned int seq;
>  	int ret;
> @@ -1367,7 +1369,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	e->event.sequence = vblwait->request.sequence;
>  	if (vblank_passed(seq, vblwait->request.sequence)) {
>  		drm_vblank_put(dev, pipe);
> -		send_vblank_event(dev, e, seq, &now);
> +		send_vblank_event(dev, e, seq, now);
>  		vblwait->reply.sequence = seq;
>  	} else {
>  		/* drm_handle_vblank_events will call drm_vblank_put */
> @@ -1398,6 +1400,24 @@ static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
>  					  _DRM_VBLANK_NEXTONMISS));
>  }
>  
> +static void drm_wait_vblank_reply(struct drm_device *dev, unsigned int pipe,
> +				  struct drm_wait_vblank_reply *reply)
> +{
> +	ktime_t now;
> +	struct timespec64 ts;
> +
> +	/*
> +	 * drm_wait_vblank_reply is a UAPI structure that uses 'long'
> +	 * to store the seconds. This will overflow in y2038 on 32-bit
> +	 * architectures with drm_timestamp_monotonic==0, but not with
> +	 * drm_timestamp_monotonic==1 (the default).
> +	 */
> +	reply->sequence = drm_vblank_count_and_time(dev, pipe, &now);
> +	ts = ktime_to_timespec64(now);
> +	reply->tval_sec = (u32)ts.tv_sec;
> +	reply->tval_usec = ts.tv_nsec / 1000;
> +}
> +
>  int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  			  struct drm_file *file_priv)
>  {
> @@ -1439,12 +1459,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  	if (dev->vblank_disable_immediate &&
>  	    drm_wait_vblank_is_query(vblwait) &&
>  	    READ_ONCE(vblank->enabled)) {
> -		struct timeval now;
> -
> -		vblwait->reply.sequence =
> -			drm_vblank_count_and_time(dev, pipe, &now);
> -		vblwait->reply.tval_sec = now.tv_sec;
> -		vblwait->reply.tval_usec = now.tv_usec;
> +		drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
>  		return 0;
>  	}
>  
> @@ -1487,11 +1502,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	if (ret != -EINTR) {
> -		struct timeval now;
> -
> -		vblwait->reply.sequence = drm_vblank_count_and_time(dev, pipe, &now);
> -		vblwait->reply.tval_sec = now.tv_sec;
> -		vblwait->reply.tval_usec = now.tv_usec;
> +		drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
>  
>  		DRM_DEBUG("crtc %d returning %u to client\n",
>  			  pipe, vblwait->reply.sequence);
> @@ -1507,7 +1518,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_pending_vblank_event *e, *t;
> -	struct timeval now;
> +	ktime_t now;
>  	unsigned int seq;
>  
>  	assert_spin_locked(&dev->event_lock);
> @@ -1525,7 +1536,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
>  
>  		list_del(&e->base.link);
>  		drm_vblank_put(dev, pipe);
> -		send_vblank_event(dev, e, seq, &now);
> +		send_vblank_event(dev, e, seq, now);
>  	}
>  
>  	trace_drm_vblank_event(pipe, seq);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index ee06ecd6c01f..412e83a4d3db 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -324,7 +324,7 @@ struct drm_driver {
>  	 */
>  	bool (*get_vblank_timestamp) (struct drm_device *dev, unsigned int pipe,
>  				     int *max_error,
> -				     struct timeval *vblank_time,
> +				     ktime_t *vblank_time,
>  				     bool in_vblank_irq);
>  
>  	/**
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 7fba9efe4951..6a58e2e91a0f 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -92,7 +92,7 @@ struct drm_vblank_crtc {
>  	/**
>  	 * @time: Vblank timestamp corresponding to @count.
>  	 */
> -	struct timeval time;
> +	ktime_t time;
>  
>  	/**
>  	 * @refcount: Number of users/waiters of the vblank interrupt. Only when
> @@ -154,7 +154,7 @@ struct drm_vblank_crtc {
>  int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs);
>  u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
>  u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> -				   struct timeval *vblanktime);
> +				   ktime_t *vblanktime);
>  void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  			       struct drm_pending_vblank_event *e);
>  void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> @@ -172,7 +172,7 @@ u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
>  
>  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  					   unsigned int pipe, int *max_error,
> -					   struct timeval *vblank_time,
> +					   ktime_t *vblank_time,
>  					   bool in_vblank_irq);
>  void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  				     const struct drm_display_mode *mode);
> -- 
> 2.9.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Arnd Bergmann]

On Wed, Oct 11, 2017 at 7:36 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Wed, Oct 11, 2017 at 05:20:12PM +0200, Arnd Bergmann wrote:
>
> Hi Arnd,
> Keith posted something very similar with:
> <20171011004514.9500-2-keithp@keithp.com> "drm: Widen vblank UAPI to 64 bits.
> Change vblank time to ktime_t"
>
> It looks like perhaps Keith missed one of the comment tweaks that you have
> below.

That is an interesting coincidence, I created my patch earlier this week
without having any idea that others were looking at the same files.

      Arnd

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Arnd Bergmann]

On Wed, Oct 11, 2017 at 7:36 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Wed, Oct 11, 2017 at 05:20:12PM +0200, Arnd Bergmann wrote:
>
> Hi Arnd,
> Keith posted something very similar with:
> <20171011004514.9500-2-keithp@keithp.com> "drm: Widen vblank UAPI to 64 bits.
> Change vblank time to ktime_t"
>
> It looks like perhaps Keith missed one of the comment tweaks that you have
> below.

That is an interesting coincidence, I created my patch earlier this week
without having any idea that others were looking at the same files.

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

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 549 bytes --]

Arnd Bergmann <arnd@arndb.de> writes:

> That is an interesting coincidence, I created my patch earlier this week
> without having any idea that others were looking at the same files.

My requirements were to support 64-bit vblank counts and ns precision
vblank timing for Vulkan; obviously using ktime_t was a good cleaup at
the same time.

My patch also widens the sequence numbers to 64-bits, so it's a superset
of Arnd's patch; rebasing should be straightforward (although not
automatic) if that's what we want to do.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

Sean Paul <seanpaul@chromium.org> writes:

> It looks like perhaps Keith missed one of the comment tweaks that you have
> below.
>
> Keith, perhaps you can rebase your widening patch on this one?

I'm easy; either order works for me just fine. Having the time
change separated from the sequence change might be nice?

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Keith Packard]

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

Sean Paul <seanpaul@chromium.org> writes:

> It looks like perhaps Keith missed one of the comment tweaks that you have
> below.
>
> Keith, perhaps you can rebase your widening patch on this one?

I'm easy; either order works for me just fine. Having the time
change separated from the sequence change might be nice?

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Sean Paul]

On Wed, Oct 11, 2017 at 4:18 PM, Keith Packard <keithp@keithp.com> wrote:
> Sean Paul <seanpaul@chromium.org> writes:
>
>> It looks like perhaps Keith missed one of the comment tweaks that you have
>> below.
>>
>> Keith, perhaps you can rebase your widening patch on this one?
>
> I'm easy; either order works for me just fine. Having the time
> change separated from the sequence change might be nice?
>

Yeah, that's what I was thinking. I'd normally default to first come
first serve, but since your patch is a superset, rebasing has the nice
feature of teasing out the widening change.

Would it be easier for you to respin this into your series, or for me
to just apply it to drm-misc-next?

Sean


> --
> -keith

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

Sean Paul <seanpaul@chromium.org> writes:

> Would it be easier for you to respin this into your series, or for me
> to just apply it to drm-misc-next?

Yeah, I don't see any particular hurry to getting just the time widening
patch merged as it doesn't change any external interfaces. I'll go ahead
and respin my series using Arnd's time patch first. If Dave decides to
delay the vblank series for a long time, we can ask him to just pull the
time widening patch in sooner.

I won't get to this until tomorrow though.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Sean Paul]

On Wed, Oct 11, 2017 at 04:28:39PM -0400, Sean Paul wrote:
> On Wed, Oct 11, 2017 at 4:18 PM, Keith Packard <keithp@keithp.com> wrote:
> > Sean Paul <seanpaul@chromium.org> writes:
> >
> >> It looks like perhaps Keith missed one of the comment tweaks that you have
> >> below.
> >>
> >> Keith, perhaps you can rebase your widening patch on this one?
> >
> > I'm easy; either order works for me just fine. Having the time
> > change separated from the sequence change might be nice?
> >
> 
> Yeah, that's what I was thinking. I'd normally default to first come
> first serve, but since your patch is a superset, rebasing has the nice
> feature of teasing out the widening change.
> 
> Would it be easier for you to respin this into your series, or for me
> to just apply it to drm-misc-next?
> 

It seems like we have consensus, and Daniel has offered to pick both of Arnd's
patches into -misc-next.

So thanks to Keith for being flexible, and

Reviewed-by: Sean Paul <seanpaul@chromium.org>

Sean


> Sean
> 
> 
> > --
> > -keith

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Sean Paul]

On Wed, Oct 11, 2017 at 04:28:39PM -0400, Sean Paul wrote:
> On Wed, Oct 11, 2017 at 4:18 PM, Keith Packard <keithp@keithp.com> wrote:
> > Sean Paul <seanpaul@chromium.org> writes:
> >
> >> It looks like perhaps Keith missed one of the comment tweaks that you have
> >> below.
> >>
> >> Keith, perhaps you can rebase your widening patch on this one?
> >
> > I'm easy; either order works for me just fine. Having the time
> > change separated from the sequence change might be nice?
> >
> 
> Yeah, that's what I was thinking. I'd normally default to first come
> first serve, but since your patch is a superset, rebasing has the nice
> feature of teasing out the widening change.
> 
> Would it be easier for you to respin this into your series, or for me
> to just apply it to drm-misc-next?
> 

It seems like we have consensus, and Daniel has offered to pick both of Arnd's
patches into -misc-next.

So thanks to Keith for being flexible, and

Reviewed-by: Sean Paul <seanpaul@chromium.org>

Sean


> Sean
> 
> 
> > --
> > -keith

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

Arnd Bergmann <arnd@arndb.de> writes:

> Overall, this seems good enough, so my patch removes the use of
> 'timeval' from the vblank handling altogether and uses ktime_t
> consistently, except for the part where we copy the data to user
> space structures in the existing format.

This patch is better than the portion of my patch which does the same
thing as it uses the ktime APIs consistently and doesn't assume that
ktime_t is in ns. Thanks much!

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/2] drm: vblank: use ktime_t instead of timeval

[Keith Packard]

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

Arnd Bergmann <arnd@arndb.de> writes:

> Overall, this seems good enough, so my patch removes the use of
> 'timeval' from the vblank handling altogether and uses ktime_t
> consistently, except for the part where we copy the data to user
> space structures in the existing format.

This patch is better than the portion of my patch which does the same
thing as it uses the ktime APIs consistently and doesn't assume that
ktime_t is in ns. Thanks much!

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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