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