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

Hi Arnd,
Keith posted something very similar with:
<20171011004514.9500-2-keithp@xxxxxxxxxx> "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