This patch series provides a new interface which fixes three issues
with the current VBLANK_WAIT ioctl:
1) CRTC indices to select a target.
2) 32-bits of count resolution.
3) Microsecond time resolution.
The first makes it quite difficult to use this interface from a leased
DRM device; without the ability to see all of the crtcs for a DRM
device, it's not possible to compute the right index into the array of
them for this interface.
2) and 3) prevent the API from matching the requirements for Vulkan,
which asks for 64-bits of counter and nano-second time resolution.
I've split this series into three pieces, the first two adjust the
internal APIs without exposing new functionality, the third adds the
new IOCTLs.
[PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank time
This changes all DRM-level internal interfaces to 64-bit vblank
counters and nano-second time resolution. Changing the driver
interface to 64-bits seems like the right plan, but as no drivers
currently support anything wider than 32-bits, it may be that we don't
want to bother at this point.
[PATCH 2/3] drm: Reorganize drm_pending_event to support future event
This sticks the vblank event in a union inside of the pending event
structure so that I can add another parallel event in the next
patch. Importantly, this required that I move the assignment of the
event crtc_id field from event deliver to event allocation. That
"shouldn't" matter, but it should also be looked at with some care.
[PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls
With the internal APIs ready, this patch is pretty simple.
-keith
These provide crtc-id based functions instead of pipe-number, while
also offering higher resolution time (ns) and wider frame count (64)
as required by the Vulkan API.
Signed-off-by: Keith Packard <[email protected]>
---
drivers/gpu/drm/drm_internal.h | 6 ++
drivers/gpu/drm/drm_ioctl.c | 2 +
drivers/gpu/drm/drm_vblank.c | 148 +++++++++++++++++++++++++++++++++++++++++
include/drm/drm_vblank.h | 1 +
include/uapi/drm/drm.h | 32 +++++++++
5 files changed, 189 insertions(+)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 5cecc974d2f9..b68a193b7907 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -65,6 +65,12 @@ int drm_legacy_irq_control(struct drm_device *dev, void *data,
int drm_legacy_modeset_ctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *filp);
+
+int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *filp);
+
/* drm_auth.c */
int drm_getmagic(struct drm_device *dev, void *data,
struct drm_file *file_priv);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f1e568176da9..63016cf3e224 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
+ DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
};
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 9ae170857ef6..93004b1bf84c 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -817,6 +817,12 @@ static void send_vblank_event(struct drm_device *dev,
e->event.vbl.tv_usec = now->tv_nsec / 1000;
}
break;
+ case DRM_EVENT_CRTC_SEQUENCE:
+ if (seq)
+ e->event.seq.sequence = seq;
+ if (now)
+ e->event.seq.time_ns = timespec_to_ns(now);
+ break;
}
trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
drm_send_event_locked(dev, &e->base);
@@ -1516,6 +1522,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
return ret;
}
+
seq = drm_vblank_count(dev, pipe);
switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
@@ -1676,3 +1683,144 @@ bool drm_crtc_handle_vblank(struct drm_crtc *crtc)
return drm_handle_vblank(crtc->dev, drm_crtc_index(crtc));
}
EXPORT_SYMBOL(drm_crtc_handle_vblank);
+
+/*
+ * Get crtc VBLANK count.
+ *
+ * \param dev DRM device
+ * \param data user arguement, pointing to a drm_crtc_get_sequence structure.
+ * \param file_priv drm file private for the user's open file descriptor
+ */
+
+int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv)
+{
+ struct drm_crtc *crtc;
+ int pipe;
+ struct drm_crtc_get_sequence *get_seq = data;
+ struct timespec now;
+
+ if (!dev->irq_enabled)
+ return -EINVAL;
+
+ crtc = drm_crtc_find(dev, get_seq->crtc_id);
+ if (!crtc)
+ return -ENOENT;
+
+ pipe = drm_crtc_index(crtc);
+
+ get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
+ get_seq->sequence_ns = timespec_to_ns(&now);
+ return 0;
+}
+
+/*
+ * Queue a event for VBLANK sequence
+ *
+ * \param dev DRM device
+ * \param data user arguement, pointing to a drm_crtc_queue_sequence structure.
+ * \param file_priv drm file private for the user's open file descriptor
+ */
+
+int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv)
+{
+ struct drm_crtc *crtc;
+ struct drm_vblank_crtc *vblank;
+ int pipe;
+ struct drm_crtc_queue_sequence *queue_seq = data;
+ struct timespec now;
+ struct drm_pending_vblank_event *e;
+ u32 flags;
+ u64 seq;
+ u64 req_seq;
+ int ret;
+ unsigned long spin_flags;
+
+ if (!dev->irq_enabled)
+ return -EINVAL;
+
+ crtc = drm_crtc_find(dev, queue_seq->crtc_id);
+ if (!crtc)
+ return -ENOENT;
+
+ flags = queue_seq->flags;
+ /* Check valid flag bits */
+ if (flags & ~(DRM_CRTC_SEQUENCE_RELATIVE|
+ DRM_CRTC_SEQUENCE_NEXT_ON_MISS|
+ DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
+ return -EINVAL;
+
+ /* Check for valid signal edge */
+ if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
+ return -EINVAL;
+
+ pipe = drm_crtc_index(crtc);
+
+ vblank = &dev->vblank[pipe];
+
+ e = kzalloc(sizeof(*e), GFP_KERNEL);
+ if (e == NULL)
+ return -ENOMEM;
+
+ ret = drm_vblank_get(dev, pipe);
+ if (ret) {
+ DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
+ goto err_free;
+ }
+
+ seq = drm_vblank_count_and_time(dev, pipe, &now);
+ req_seq = queue_seq->sequence;
+
+ if (flags & DRM_CRTC_SEQUENCE_RELATIVE)
+ req_seq += seq;
+
+ if ((flags & DRM_CRTC_SEQUENCE_NEXT_ON_MISS) && vblank_passed(seq, req_seq))
+ req_seq = seq + 1;
+
+ e->pipe = pipe;
+ e->event.base.type = DRM_EVENT_CRTC_SEQUENCE;
+ e->event.base.length = sizeof(e->event.seq);
+ e->event.seq.user_data = queue_seq->user_data;
+
+ spin_lock_irqsave(&dev->event_lock, spin_flags);
+
+ /*
+ * drm_crtc_vblank_off() might have been called after we called
+ * drm_vblank_get(). drm_crtc_vblank_off() holds event_lock around the
+ * vblank disable, so no need for further locking. The reference from
+ * drm_vblank_get() protects against vblank disable from another source.
+ */
+ if (!READ_ONCE(vblank->enabled)) {
+ ret = -EINVAL;
+ goto err_unlock;
+ }
+
+ ret = drm_event_reserve_init_locked(dev, file_priv, &e->base,
+ &e->event.base);
+
+ if (ret)
+ goto err_unlock;
+
+ e->sequence = req_seq;
+
+ if (vblank_passed(seq, req_seq)) {
+ drm_vblank_put(dev, pipe);
+ send_vblank_event(dev, e, seq, &now);
+ queue_seq->sequence = seq;
+ } else {
+ /* drm_handle_vblank_events will call drm_vblank_put */
+ list_add_tail(&e->base.link, &dev->vblank_event_list);
+ queue_seq->sequence = req_seq;
+ }
+
+ spin_unlock_irqrestore(&dev->event_lock, spin_flags);
+ return 0;
+
+err_unlock:
+ spin_unlock_irqrestore(&dev->event_lock, spin_flags);
+ drm_vblank_put(dev, pipe);
+err_free:
+ kfree(e);
+ return ret;
+}
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 3ef7ddc5db5f..8a5e1dfe3be7 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -57,6 +57,7 @@ struct drm_pending_vblank_event {
union {
struct drm_event base;
struct drm_event_vblank vbl;
+ struct drm_event_crtc_sequence seq;
} event;
};
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 101593ab10ac..dc16d42a88c7 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -718,6 +718,27 @@ struct drm_syncobj_handle {
__u32 pad;
};
+/* Query current scanout sequence number */
+struct drm_crtc_get_sequence {
+ __u32 crtc_id;
+ __u32 pad;
+ __u64 sequence;
+ __u64 sequence_ns;
+};
+
+/* Queue event to be delivered at specified sequence */
+
+#define DRM_CRTC_SEQUENCE_RELATIVE 0x00000001 /* sequence is relative to current */
+#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
+#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
+
+struct drm_crtc_queue_sequence {
+ __u32 crtc_id;
+ __u32 flags;
+ __u64 sequence; /* on input, target sequence. on output, actual sequence */
+ __u64 user_data; /* user data passed to event */
+};
+
#if defined(__cplusplus)
}
#endif
@@ -800,6 +821,9 @@ extern "C" {
#define DRM_IOCTL_WAIT_VBLANK DRM_IOWR(0x3a, union drm_wait_vblank)
+#define DRM_IOCTL_CRTC_GET_SEQUENCE DRM_IOWR(0x3b, struct drm_crtc_get_sequence)
+#define DRM_IOCTL_CRTC_QUEUE_SEQUENCE DRM_IOWR(0x3c, struct drm_crtc_queue_sequence)
+
#define DRM_IOCTL_UPDATE_DRAW DRM_IOW(0x3f, struct drm_update_draw)
#define DRM_IOCTL_MODE_GETRESOURCES DRM_IOWR(0xA0, struct drm_mode_card_res)
@@ -871,6 +895,7 @@ struct drm_event {
#define DRM_EVENT_VBLANK 0x01
#define DRM_EVENT_FLIP_COMPLETE 0x02
+#define DRM_EVENT_CRTC_SEQUENCE 0x03
struct drm_event_vblank {
struct drm_event base;
@@ -881,6 +906,13 @@ struct drm_event_vblank {
__u32 crtc_id; /* 0 on older kernels that do not support this */
};
+struct drm_event_crtc_sequence {
+ struct drm_event base;
+ __u64 user_data;
+ __u64 time_ns;
+ __u64 sequence;
+};
+
/* typedef area */
#ifndef __KERNEL__
typedef struct drm_clip_rect drm_clip_rect_t;
--
2.11.0
Place drm_event_vblank in a new union that includes that and a bare
drm_event structure. This will allow new members of that union to be
added in the future without changing code related to the existing vbl
event type.
Assignments to the crtc_id field are now done when the event is
allocated, rather than when delievered. This way, delivery doesn't
need to have the crtc ID available.
Signed-off-by: Keith Packard <[email protected]>
---
drivers/gpu/drm/drm_atomic.c | 7 ++++---
drivers/gpu/drm/drm_plane.c | 2 +-
drivers/gpu/drm/drm_vblank.c | 27 ++++++++++++++++-----------
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 ++--
include/drm/drm_vblank.h | 8 +++++++-
6 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c0f336d23f9c..f569d7f03f3c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1839,7 +1839,7 @@ int drm_atomic_debugfs_init(struct drm_minor *minor)
*/
static struct drm_pending_vblank_event *create_vblank_event(
- struct drm_device *dev, uint64_t user_data)
+ struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data)
{
struct drm_pending_vblank_event *e = NULL;
@@ -1849,7 +1849,8 @@ static struct drm_pending_vblank_event *create_vblank_event(
e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
e->event.base.length = sizeof(e->event);
- e->event.user_data = user_data;
+ e->event.vbl.crtc_id = crtc->base.id;
+ e->event.vbl.user_data = user_data;
return e;
}
@@ -2052,7 +2053,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
struct drm_pending_vblank_event *e;
- e = create_vblank_event(dev, arg->user_data);
+ e = create_vblank_event(dev, crtc, arg->user_data);
if (!e)
return -ENOMEM;
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 5dc8c4350602..fe9f31285bc2 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -918,7 +918,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
}
e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
e->event.base.length = sizeof(e->event);
- e->event.user_data = page_flip->user_data;
+ e->event.vbl.user_data = page_flip->user_data;
ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
if (ret) {
kfree(e);
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f55f997c0b8f..9ae170857ef6 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -807,13 +807,18 @@ static void send_vblank_event(struct drm_device *dev,
struct drm_pending_vblank_event *e,
u64 seq, struct timespec *now)
{
- e->event.sequence = seq;
- e->event.tv_sec = now->tv_sec;
- e->event.tv_usec = now->tv_nsec / 1000;
-
- trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
- e->event.sequence);
-
+ switch (e->event.base.type) {
+ case DRM_EVENT_VBLANK:
+ case DRM_EVENT_FLIP_COMPLETE:
+ if (seq)
+ e->event.vbl.sequence = (u32) seq;
+ if (now) {
+ e->event.vbl.tv_sec = now->tv_sec;
+ e->event.vbl.tv_usec = now->tv_nsec / 1000;
+ }
+ break;
+ }
+ trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
drm_send_event_locked(dev, &e->base);
}
@@ -865,7 +870,6 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
e->pipe = pipe;
e->sequence = drm_vblank_count(dev, pipe);
- e->event.crtc_id = crtc->base.id;
list_add_tail(&e->base.link, &dev->vblank_event_list);
}
EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
@@ -897,7 +901,6 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
now = get_drm_timestamp();
}
e->pipe = pipe;
- e->event.crtc_id = crtc->base.id;
send_vblank_event(dev, e, seq, &now);
}
EXPORT_SYMBOL(drm_crtc_send_vblank_event);
@@ -1342,6 +1345,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
union drm_wait_vblank *vblwait,
struct drm_file *file_priv)
{
+ struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
struct drm_pending_vblank_event *e;
struct timespec now;
@@ -1357,8 +1361,9 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
e->pipe = pipe;
e->event.base.type = DRM_EVENT_VBLANK;
- e->event.base.length = sizeof(e->event);
- e->event.user_data = vblwait->request.signal;
+ e->event.base.length = sizeof(e->event.vbl);
+ e->event.vbl.user_data = vblwait->request.signal;
+ e->event.vbl.crtc_id = crtc ? crtc->base.id : 0;
spin_lock_irqsave(&dev->event_lock, flags);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 8d7dc9def7c2..c13b97338310 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -358,8 +358,8 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
ret = vmw_event_fence_action_queue(file_priv, fence,
&event->base,
- &event->event.tv_sec,
- &event->event.tv_usec,
+ &event->event.vbl.tv_sec,
+ &event->event.vbl.tv_usec,
true);
}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index bad31bdf09b6..4e329588ce9c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -544,8 +544,8 @@ static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc,
ret = vmw_event_fence_action_queue(file_priv, fence,
&event->base,
- &event->event.tv_sec,
- &event->event.tv_usec,
+ &event->event.vbl.tv_sec,
+ &event->event.vbl.tv_usec,
true);
vmw_fence_obj_unreference(&fence);
} else {
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 68e99177fff3..3ef7ddc5db5f 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -54,7 +54,10 @@ struct drm_pending_vblank_event {
/**
* @event: Actual event which will be sent to userspace.
*/
- struct drm_event_vblank event;
+ union {
+ struct drm_event base;
+ struct drm_event_vblank vbl;
+ } event;
};
/**
@@ -163,6 +166,9 @@ 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,
struct drm_pending_vblank_event *e);
+void drm_vblank_set_event(struct drm_pending_vblank_event *e,
+ u64 *seq,
+ struct timespec *now);
bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
int drm_crtc_vblank_get(struct drm_crtc *crtc);
--
2.11.0
This modifies the datatypes used by the vblank code to provide both 64
bits of vblank count and to increase the resolution of the vblank
timestamp from microseconds to nanoseconds.
The driver interfaces have also been changed to return 64-bits of
vblank count; fortunately all of the code necessary to widen that value
was already included to handle devices returning fewer than 32-bits.
This will provide the necessary datatypes for the Vulkan API.
Signed-off-by: Keith Packard <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
drivers/gpu/drm/drm_vblank.c | 159 ++++++++++++++++++-------------
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 +-
drivers/gpu/drm/gma500/psb_drv.h | 2 +-
drivers/gpu/drm/gma500/psb_irq.c | 2 +-
drivers/gpu/drm/gma500/psb_irq.h | 2 +-
drivers/gpu/drm/i915/i915_irq.c | 4 +-
drivers/gpu/drm/mga/mga_drv.h | 2 +-
drivers/gpu/drm/mga/mga_irq.c | 2 +-
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 2 +-
drivers/gpu/drm/r128/r128_drv.h | 2 +-
drivers/gpu/drm/r128/r128_irq.c | 2 +-
drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
drivers/gpu/drm/radeon/radeon_kms.c | 2 +-
drivers/gpu/drm/tegra/dc.c | 2 +-
drivers/gpu/drm/via/via_drv.h | 2 +-
drivers/gpu/drm/via/via_irq.c | 5 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +-
include/drm/drmP.h | 2 +-
include/drm/drm_crtc.h | 2 +-
include/drm/drm_drv.h | 4 +-
include/drm/drm_vblank.h | 18 ++--
24 files changed, 130 insertions(+), 98 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e0adad590ecb..860f5e194864 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1979,7 +1979,7 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
int amdgpu_suspend(struct amdgpu_device *adev);
int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon);
int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon);
-u32 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
+u64 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
int amdgpu_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
long amdgpu_kms_compat_ioctl(struct file *filp, unsigned int cmd,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 12497a40ef92..f8c814c9c91a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -922,7 +922,7 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
* Gets the frame count on the requested crtc (all asics).
* Returns frame count on success, -EINVAL on failure.
*/
-u32 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe)
+u64 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe)
{
struct amdgpu_device *adev = dev->dev_private;
int vpos, hpos, stat;
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 463e4d81fb0d..f55f997c0b8f 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -43,7 +43,7 @@
static bool
drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
- struct timeval *tvblank, bool in_vblank_irq);
+ struct timespec *tvblank, bool in_vblank_irq);
static unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */
@@ -63,8 +63,8 @@ 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,
- struct timeval *t_vblank, u32 last)
+ u64 vblank_count_inc,
+ struct timespec *t_vblank, u64 last)
{
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
@@ -82,13 +82,13 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
* "No hw counter" fallback implementation of .get_vblank_counter() hook,
* if there is no useable hardware frame counter available.
*/
-static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
+static u64 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
{
WARN_ON_ONCE(dev->max_vblank_count != 0);
return 0;
}
-static u32 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+static u64 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
{
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
@@ -114,9 +114,9 @@ static u32 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
*/
static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe)
{
- u32 cur_vblank;
+ u64 cur_vblank;
bool rc;
- struct timeval t_vblank;
+ struct timespec t_vblank;
int count = DRM_TIMESTAMP_MAXRETRIES;
spin_lock(&dev->vblank_time_lock);
@@ -136,7 +136,7 @@ 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 = (struct timespec) {0, 0};
/*
* +1 to make sure user will never see the same
@@ -163,9 +163,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
bool in_vblank_irq)
{
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
- u32 cur_vblank, diff;
+ u64 cur_vblank, diff;
bool rc;
- struct timeval t_vblank;
+ struct timespec t_vblank;
int count = DRM_TIMESTAMP_MAXRETRIES;
int framedur_ns = vblank->framedur_ns;
@@ -190,11 +190,11 @@ 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;
+ const struct timespec *t_old;
u64 diff_ns;
t_old = &vblank->time;
- diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
+ diff_ns = timespec_to_ns(&t_vblank) - timespec_to_ns(t_old);
/*
* Figure out how many vblanks we've missed based
@@ -222,13 +222,13 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
* random large forward jumps of the software vblank counter.
*/
if (diff > 1 && (vblank->inmodeset & 0x2)) {
- DRM_DEBUG_VBL("clamping vblank bump to 1 on crtc %u: diffr=%u"
+ DRM_DEBUG_VBL("clamping vblank bump to 1 on crtc %u: diffr=%llu"
" due to pre-modeset.\n", pipe, diff);
diff = 1;
}
DRM_DEBUG_VBL("updating vblank count on crtc %u:"
- " current=%u, diff=%u, hw=%u hw_last=%u\n",
+ " current=%llu, diff=%llu, hw=%llu hw_last=%llu\n",
pipe, vblank->count, diff, cur_vblank, vblank->last);
if (diff == 0) {
@@ -243,7 +243,7 @@ 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 = (struct timespec) {0, 0};
store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);
}
@@ -567,10 +567,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,
+ struct timespec *vblank_time,
bool in_vblank_irq)
{
- struct timeval tv_etime;
+ struct timespec tv_etime;
ktime_t stime, etime;
bool vbl_status;
struct drm_crtc *crtc;
@@ -663,29 +663,29 @@ 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);
+ tv_etime = ktime_to_timespec(etime);
/* 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 = ktime_to_timespec(etime);
DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %ld.%ld -> %ld.%ld [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,
+ (long)tv_etime.tv_sec, (long)tv_etime.tv_nsec,
+ (long)vblank_time->tv_sec, (long)vblank_time->tv_nsec,
duration_ns/1000, i);
return true;
}
EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
-static struct timeval get_drm_timestamp(void)
+static struct timespec get_drm_timestamp(void)
{
ktime_t now;
now = drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
- return ktime_to_timeval(now);
+ return ktime_to_timespec(now);
}
/**
@@ -711,7 +711,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)
+ struct timespec *tvblank, bool in_vblank_irq)
{
bool ret = false;
@@ -743,7 +743,7 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
* Returns:
* The software vblank counter.
*/
-u32 drm_crtc_vblank_count(struct drm_crtc *crtc)
+u64 drm_crtc_vblank_count(struct drm_crtc *crtc)
{
return drm_vblank_count(crtc->dev, drm_crtc_index(crtc));
}
@@ -763,15 +763,15 @@ EXPORT_SYMBOL(drm_crtc_vblank_count);
*
* This is the legacy version of drm_crtc_vblank_count_and_time().
*/
-static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
- struct timeval *vblanktime)
+static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
+ struct timespec *vblanktime)
{
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
- u32 vblank_count;
+ u64 vblank_count;
unsigned int seq;
if (WARN_ON(pipe >= dev->num_crtcs)) {
- *vblanktime = (struct timeval) { 0 };
+ *vblanktime = (struct timespec) { 0 };
return 0;
}
@@ -795,8 +795,8 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
* modesetting activity. Returns corresponding system timestamp of the time
* 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)
+u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
+ struct timespec *vblanktime)
{
return drm_vblank_count_and_time(crtc->dev, drm_crtc_index(crtc),
vblanktime);
@@ -805,11 +805,11 @@ 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)
+ u64 seq, struct timespec *now)
{
e->event.sequence = seq;
e->event.tv_sec = now->tv_sec;
- e->event.tv_usec = now->tv_usec;
+ e->event.tv_usec = now->tv_nsec / 1000;
trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
e->event.sequence);
@@ -864,7 +864,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
assert_spin_locked(&dev->event_lock);
e->pipe = pipe;
- e->event.sequence = drm_vblank_count(dev, pipe);
+ e->sequence = drm_vblank_count(dev, pipe);
e->event.crtc_id = crtc->base.id;
list_add_tail(&e->base.link, &dev->vblank_event_list);
}
@@ -885,8 +885,9 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
struct drm_pending_vblank_event *e)
{
struct drm_device *dev = crtc->dev;
- unsigned int seq, pipe = drm_crtc_index(crtc);
- struct timeval now;
+ u64 seq;
+ unsigned int pipe = drm_crtc_index(crtc);
+ struct timespec now;
if (dev->num_crtcs > 0) {
seq = drm_vblank_count_and_time(dev, pipe, &now);
@@ -1124,9 +1125,9 @@ 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;
+ struct timespec now;
unsigned long irqflags;
- unsigned int seq;
+ u64 seq;
if (WARN_ON(pipe >= dev->num_crtcs))
return;
@@ -1161,8 +1162,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
if (e->pipe != pipe)
continue;
DRM_DEBUG("Sending premature vblank event on disable: "
- "wanted %u, current %u\n",
- e->event.sequence, seq);
+ "wanted %llu current %llu\n",
+ e->sequence, seq);
list_del(&e->base.link);
drm_vblank_put(dev, pipe);
send_vblank_event(dev, e, seq, &now);
@@ -1331,20 +1332,21 @@ int drm_legacy_modeset_ctl(struct drm_device *dev, void *data,
return 0;
}
-static inline bool vblank_passed(u32 seq, u32 ref)
+static inline bool vblank_passed(u64 seq, u64 ref)
{
return (seq - ref) <= (1 << 23);
}
static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
+ u64 req_seq,
union drm_wait_vblank *vblwait,
struct drm_file *file_priv)
{
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
struct drm_pending_vblank_event *e;
- struct timeval now;
+ struct timespec now;
unsigned long flags;
- unsigned int seq;
+ u64 seq;
int ret;
e = kzalloc(sizeof(*e), GFP_KERNEL);
@@ -1379,21 +1381,20 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
seq = drm_vblank_count_and_time(dev, pipe, &now);
- DRM_DEBUG("event on vblank count %u, current %u, crtc %u\n",
- vblwait->request.sequence, seq, pipe);
+ DRM_DEBUG("event on vblank count %llu, current %llu, crtc %u\n",
+ req_seq, seq, pipe);
- trace_drm_vblank_event_queued(file_priv, pipe,
- vblwait->request.sequence);
+ trace_drm_vblank_event_queued(file_priv, pipe, req_seq);
- e->event.sequence = vblwait->request.sequence;
- if (vblank_passed(seq, vblwait->request.sequence)) {
+ e->sequence = req_seq;
+ if (vblank_passed(seq, req_seq)) {
drm_vblank_put(dev, pipe);
send_vblank_event(dev, e, seq, &now);
vblwait->reply.sequence = seq;
} else {
/* drm_handle_vblank_events will call drm_vblank_put */
list_add_tail(&e->base.link, &dev->vblank_event_list);
- vblwait->reply.sequence = vblwait->request.sequence;
+ vblwait->reply.sequence = req_seq;
}
spin_unlock_irqrestore(&dev->event_lock, flags);
@@ -1420,6 +1421,27 @@ static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
}
/*
+ * Widen a 32-bit param to 64-bits.
+ *
+ * \param narrow 32-bit value (missing upper 32 bits)
+ * \param near 64-bit value that should be 'close' to near
+ *
+ * This function returns a 64-bit value using the lower 32-bits from
+ * 'narrow' and constructing the upper 32-bits so that the result is
+ * as close as possible to 'near'.
+ */
+
+static u64 widen_32_to_64(u32 narrow, u64 near)
+{
+ u64 wide = narrow | (near & 0xffffffff00000000ULL);
+ if ((int64_t) (wide - near) > 0x80000000LL)
+ wide -= 0x100000000ULL;
+ else if ((int64_t) (near - wide) > 0x80000000LL)
+ wide += 0x100000000ULL;
+ return wide;
+}
+
+/*
* Wait for VBLANK.
*
* \param inode device inode.
@@ -1439,6 +1461,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
struct drm_vblank_crtc *vblank;
union drm_wait_vblank *vblwait = data;
int ret;
+ u64 req_seq;
unsigned int flags, seq, pipe, high_pipe;
if (!dev->irq_enabled)
@@ -1474,12 +1497,12 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
if (dev->vblank_disable_immediate &&
drm_wait_vblank_is_query(vblwait) &&
READ_ONCE(vblank->enabled)) {
- struct timeval now;
+ struct timespec 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;
+ vblwait->reply.tval_usec = now.tv_nsec / 1000;
return 0;
}
@@ -1492,9 +1515,11 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
case _DRM_VBLANK_RELATIVE:
- vblwait->request.sequence += seq;
+ req_seq = seq + vblwait->request.sequence;
vblwait->request.type &= ~_DRM_VBLANK_RELATIVE;
+ break;
case _DRM_VBLANK_ABSOLUTE:
+ req_seq = widen_32_to_64(vblwait->request.sequence, seq);
break;
default:
ret = -EINVAL;
@@ -1502,31 +1527,31 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
}
if ((flags & _DRM_VBLANK_NEXTONMISS) &&
- vblank_passed(seq, vblwait->request.sequence))
- vblwait->request.sequence = seq + 1;
+ vblank_passed(seq, req_seq))
+ req_seq = seq + 1;
if (flags & _DRM_VBLANK_EVENT) {
/* must hold on to the vblank ref until the event fires
* drm_vblank_put will be called asynchronously
*/
- return drm_queue_vblank_event(dev, pipe, vblwait, file_priv);
+ return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, file_priv);
}
- if (vblwait->request.sequence != seq) {
- DRM_DEBUG("waiting on vblank count %u, crtc %u\n",
- vblwait->request.sequence, pipe);
+ if (req_seq != seq) {
+ DRM_DEBUG("waiting on vblank count %llu, crtc %u\n",
+ req_seq, pipe);
DRM_WAIT_ON(ret, vblank->queue, 3 * HZ,
vblank_passed(drm_vblank_count(dev, pipe),
- vblwait->request.sequence) ||
+ req_seq) ||
!READ_ONCE(vblank->enabled));
}
if (ret != -EINTR) {
- struct timeval now;
+ struct timespec 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;
+ vblwait->reply.tval_usec = now.tv_nsec / 1000;
DRM_DEBUG("crtc %d returning %u to client\n",
pipe, vblwait->reply.sequence);
@@ -1542,8 +1567,8 @@ int drm_wait_vblank(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;
- unsigned int seq;
+ struct timespec now;
+ u64 seq;
assert_spin_locked(&dev->event_lock);
@@ -1552,11 +1577,11 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
if (e->pipe != pipe)
continue;
- if (!vblank_passed(seq, e->event.sequence))
+ if (!vblank_passed(seq, e->sequence))
continue;
- DRM_DEBUG("vblank event on %u, current %u\n",
- e->event.sequence, seq);
+ DRM_DEBUG("vblank event on %llu, current %llu\n",
+ e->sequence, seq);
list_del(&e->base.link);
drm_vblank_put(dev, pipe);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index d72777f6411a..110beca116f8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -132,7 +132,7 @@ static void exynos_drm_crtc_disable_vblank(struct drm_crtc *crtc)
exynos_crtc->ops->disable_vblank(exynos_crtc);
}
-static u32 exynos_drm_crtc_get_vblank_counter(struct drm_crtc *crtc)
+static u64 exynos_drm_crtc_get_vblank_counter(struct drm_crtc *crtc)
{
struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h
index 83667087d6e5..6e3959da8ec2 100644
--- a/drivers/gpu/drm/gma500/psb_drv.h
+++ b/drivers/gpu/drm/gma500/psb_drv.h
@@ -699,7 +699,7 @@ psb_enable_pipestat(struct drm_psb_private *dev_priv, int pipe, u32 mask);
void
psb_disable_pipestat(struct drm_psb_private *dev_priv, int pipe, u32 mask);
-extern u32 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
+extern u64 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
/* framebuffer.c */
extern int psbfb_probed(struct drm_device *dev);
diff --git a/drivers/gpu/drm/gma500/psb_irq.c b/drivers/gpu/drm/gma500/psb_irq.c
index 78eb10902809..efd2124de971 100644
--- a/drivers/gpu/drm/gma500/psb_irq.c
+++ b/drivers/gpu/drm/gma500/psb_irq.c
@@ -622,7 +622,7 @@ void mdfld_disable_te(struct drm_device *dev, int pipe)
/* Called from drm generic code, passed a 'crtc', which
* we use as a pipe index
*/
-u32 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+u64 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
{
uint32_t high_frame = PIPEAFRAMEHIGH;
uint32_t low_frame = PIPEAFRAMEPIXEL;
diff --git a/drivers/gpu/drm/gma500/psb_irq.h b/drivers/gpu/drm/gma500/psb_irq.h
index e6a81a8c9f35..4ab8af0607a4 100644
--- a/drivers/gpu/drm/gma500/psb_irq.h
+++ b/drivers/gpu/drm/gma500/psb_irq.h
@@ -40,7 +40,7 @@ void psb_irq_turn_on_dpst(struct drm_device *dev);
void psb_irq_turn_off_dpst(struct drm_device *dev);
int psb_enable_vblank(struct drm_device *dev, unsigned int pipe);
void psb_disable_vblank(struct drm_device *dev, unsigned int pipe);
-u32 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
+u64 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
int mdfld_enable_te(struct drm_device *dev, int pipe);
void mdfld_disable_te(struct drm_device *dev, int pipe);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7b7f55a28eec..97c928c823e2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -715,7 +715,7 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
/* Called from drm generic code, passed a 'crtc', which
* we use as a pipe index
*/
-static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+static u64 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
{
struct drm_i915_private *dev_priv = to_i915(dev);
i915_reg_t high_frame, low_frame;
@@ -765,7 +765,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xffffff;
}
-static u32 g4x_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+static u64 g4x_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
{
struct drm_i915_private *dev_priv = to_i915(dev);
diff --git a/drivers/gpu/drm/mga/mga_drv.h b/drivers/gpu/drm/mga/mga_drv.h
index 45cf363d25ad..46adff5d1fc6 100644
--- a/drivers/gpu/drm/mga/mga_drv.h
+++ b/drivers/gpu/drm/mga/mga_drv.h
@@ -185,7 +185,7 @@ extern int mga_warp_init(drm_mga_private_t *dev_priv);
/* mga_irq.c */
extern int mga_enable_vblank(struct drm_device *dev, unsigned int pipe);
extern void mga_disable_vblank(struct drm_device *dev, unsigned int pipe);
-extern u32 mga_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
+extern u64 mga_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
extern int mga_driver_fence_wait(struct drm_device *dev, unsigned int *sequence);
extern int mga_driver_vblank_wait(struct drm_device *dev, unsigned int *sequence);
extern irqreturn_t mga_driver_irq_handler(int irq, void *arg);
diff --git a/drivers/gpu/drm/mga/mga_irq.c b/drivers/gpu/drm/mga/mga_irq.c
index 693ba708cfed..a361db778f6a 100644
--- a/drivers/gpu/drm/mga/mga_irq.c
+++ b/drivers/gpu/drm/mga/mga_irq.c
@@ -35,7 +35,7 @@
#include <drm/mga_drm.h>
#include "mga_drv.h"
-u32 mga_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+u64 mga_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
{
const drm_mga_private_t *const dev_priv =
(drm_mga_private_t *) dev->dev_private;
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
index e2b3346ead48..678f2c03a93a 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
@@ -587,7 +587,7 @@ static bool mdp5_get_scanoutpos(struct drm_device *dev, unsigned int pipe,
return true;
}
-static u32 mdp5_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+static u64 mdp5_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
{
struct msm_drm_private *priv = dev->dev_private;
struct drm_crtc *crtc;
diff --git a/drivers/gpu/drm/r128/r128_drv.h b/drivers/gpu/drm/r128/r128_drv.h
index 09143b840482..3479198774dc 100644
--- a/drivers/gpu/drm/r128/r128_drv.h
+++ b/drivers/gpu/drm/r128/r128_drv.h
@@ -156,7 +156,7 @@ extern int r128_do_cleanup_cce(struct drm_device *dev);
extern int r128_enable_vblank(struct drm_device *dev, unsigned int pipe);
extern void r128_disable_vblank(struct drm_device *dev, unsigned int pipe);
-extern u32 r128_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
+extern u64 r128_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
extern irqreturn_t r128_driver_irq_handler(int irq, void *arg);
extern void r128_driver_irq_preinstall(struct drm_device *dev);
extern int r128_driver_irq_postinstall(struct drm_device *dev);
diff --git a/drivers/gpu/drm/r128/r128_irq.c b/drivers/gpu/drm/r128/r128_irq.c
index 9730f4918944..141d4dfc30b1 100644
--- a/drivers/gpu/drm/r128/r128_irq.c
+++ b/drivers/gpu/drm/r128/r128_irq.c
@@ -34,7 +34,7 @@
#include <drm/r128_drm.h>
#include "r128_drv.h"
-u32 r128_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+u64 r128_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
{
const drm_r128_private_t *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index b23c771f4216..4e2b3fa4293a 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -112,7 +112,7 @@ void radeon_driver_postclose_kms(struct drm_device *dev,
int radeon_suspend_kms(struct drm_device *dev, bool suspend,
bool fbcon, bool freeze);
int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon);
-u32 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
+u64 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
int radeon_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
void radeon_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
void radeon_driver_irq_preinstall_kms(struct drm_device *dev);
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index dfee8f7d94ae..bf6c3bad36c7 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -772,7 +772,7 @@ void radeon_driver_postclose_kms(struct drm_device *dev,
* Gets the frame count on the requested crtc (all asics).
* Returns frame count on success, -EINVAL on failure.
*/
-u32 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe)
+u64 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe)
{
int vpos, hpos, stat;
u32 count;
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 95b373f739f2..9f060ce156b4 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -909,7 +909,7 @@ static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
return 0;
}
-static u32 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
+static u64 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
{
struct tegra_dc *dc = to_tegra_dc(crtc);
diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
index 9873942ca8f4..019769a54a70 100644
--- a/drivers/gpu/drm/via/via_drv.h
+++ b/drivers/gpu/drm/via/via_drv.h
@@ -140,7 +140,7 @@ extern int via_init_context(struct drm_device *dev, int context);
extern int via_final_context(struct drm_device *dev, int context);
extern int via_do_cleanup_map(struct drm_device *dev);
-extern u32 via_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
+extern u64 via_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
extern int via_enable_vblank(struct drm_device *dev, unsigned int pipe);
extern void via_disable_vblank(struct drm_device *dev, unsigned int pipe);
diff --git a/drivers/gpu/drm/via/via_irq.c b/drivers/gpu/drm/via/via_irq.c
index ea8172c747a2..a151c72d148a 100644
--- a/drivers/gpu/drm/via/via_irq.c
+++ b/drivers/gpu/drm/via/via_irq.c
@@ -95,7 +95,7 @@ static unsigned time_diff(struct timeval *now, struct timeval *then)
1000000 - (then->tv_usec - now->tv_usec);
}
-u32 via_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+u64 via_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
{
drm_via_private_t *dev_priv = dev->dev_private;
@@ -317,6 +317,9 @@ int via_driver_irq_postinstall(struct drm_device *dev)
if (!dev_priv)
return -EINVAL;
+ if (dev->driver->get_vblank_counter)
+ dev->max_vblank_count = 0xffffffff;
+
status = VIA_READ(VIA_REG_INTERRUPT);
VIA_WRITE(VIA_REG_INTERRUPT, status | VIA_IRQ_GLOBAL
| dev_priv->irq_enable_mask);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 130d51c5ec6a..adc9d2ae37a4 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -918,7 +918,7 @@ void vmw_kms_idle_workqueues(struct vmw_master *vmaster);
bool vmw_kms_validate_mode_vram(struct vmw_private *dev_priv,
uint32_t pitch,
uint32_t height);
-u32 vmw_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
+u64 vmw_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
int vmw_enable_vblank(struct drm_device *dev, unsigned int pipe);
void vmw_disable_vblank(struct drm_device *dev, unsigned int pipe);
int vmw_kms_present(struct vmw_private *dev_priv,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index a8876b070168..135f5c3dbb6c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1944,7 +1944,7 @@ bool vmw_kms_validate_mode_vram(struct vmw_private *dev_priv,
/**
* Function called by DRM code called with vbl_lock held.
*/
-u32 vmw_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
+u64 vmw_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
{
return 0;
}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 39df16af7a4a..e50cf152f565 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -403,7 +403,7 @@ struct drm_device {
spinlock_t vblank_time_lock; /**< Protects vblank count and time updates during vblank enable/disable */
spinlock_t vbl_lock;
- u32 max_vblank_count; /**< size of vblank counter register */
+ u64 max_vblank_count; /**< size of vblank counter register */
/**
* List of events
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 629a5fe075b3..e866f0007d8a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -689,7 +689,7 @@ struct drm_crtc_funcs {
*
* Raw vblank counter value.
*/
- u32 (*get_vblank_counter)(struct drm_crtc *crtc);
+ u64 (*get_vblank_counter)(struct drm_crtc *crtc);
/**
* @enable_vblank:
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index d855f9ae41a8..e575802fbe4c 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -196,7 +196,7 @@ struct drm_driver {
*
* Raw vblank counter value.
*/
- u32 (*get_vblank_counter) (struct drm_device *dev, unsigned int pipe);
+ u64 (*get_vblank_counter) (struct drm_device *dev, unsigned int pipe);
/**
* @enable_vblank:
@@ -325,7 +325,7 @@ struct drm_driver {
*/
bool (*get_vblank_timestamp) (struct drm_device *dev, unsigned int pipe,
int *max_error,
- struct timeval *vblank_time,
+ struct timespec *vblank_time,
bool in_vblank_irq);
/**
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 4cde47332dfa..68e99177fff3 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -48,6 +48,10 @@ struct drm_pending_vblank_event {
*/
unsigned int pipe;
/**
+ * @sequence: frame event should be triggered at
+ */
+ u64 sequence;
+ /**
* @event: Actual event which will be sent to userspace.
*/
struct drm_event_vblank event;
@@ -88,11 +92,11 @@ struct drm_vblank_crtc {
/**
* @count: Current software vblank counter.
*/
- u32 count;
+ u64 count;
/**
* @time: Vblank timestamp corresponding to @count.
*/
- struct timeval time;
+ struct timespec time;
/**
* @refcount: Number of users/waiters of the vblank interrupt. Only when
@@ -103,7 +107,7 @@ struct drm_vblank_crtc {
/**
* @last: Protected by &drm_device.vbl_lock, used for wraparound handling.
*/
- u32 last;
+ u64 last;
/**
* @inmodeset: Tracks whether the vblank is disabled due to a modeset.
* For legacy driver bit 2 additionally tracks whether an additional
@@ -152,9 +156,9 @@ 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);
+u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
+u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
+ struct timespec *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,
@@ -173,7 +177,7 @@ u32 drm_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,
+ struct timespec *vblank_time,
bool in_vblank_irq);
void drm_calc_timestamping_constants(struct drm_crtc *crtc,
const struct drm_display_mode *mode);
--
2.11.0
On Wed, Jul 05, 2017 at 03:10:11PM -0700, Keith Packard wrote:
> This modifies the datatypes used by the vblank code to provide both 64
> bits of vblank count and to increase the resolution of the vblank
> timestamp from microseconds to nanoseconds.
>
> The driver interfaces have also been changed to return 64-bits of
> vblank count; fortunately all of the code necessary to widen that value
> was already included to handle devices returning fewer than 32-bits.
Extending the reported/sw vblank counter to u64 makes sense imo, but do we
have to extend the driver interfaces too? If there's no 64 bit hw vblank
currently I think I'd be good to postpone that part, simply because I'm
too lazy to audit all the drivers for correctly setting max_vblank_count
after your change :-)
Other thought on this, since you bother to change all the types: Afaik
both timespec and timeval suffer from the 32bit issues. If we bother with
changing everything I think it'd be neat to switch all internal interfaces
over to ktime, and only convert to the userspace types once when we
generate the event. I think that's how cool hackers are supposed to do it,
but not fully sure.
Otherwise looks all good, but haven't yet carefully hunted for fumbles in
review before the above is clear.
-Daniel
> This will provide the necessary datatypes for the Vulkan API.
>
> Signed-off-by: Keith Packard <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
> drivers/gpu/drm/drm_vblank.c | 159 ++++++++++++++++++-------------
> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 +-
> drivers/gpu/drm/gma500/psb_drv.h | 2 +-
> drivers/gpu/drm/gma500/psb_irq.c | 2 +-
> drivers/gpu/drm/gma500/psb_irq.h | 2 +-
> drivers/gpu/drm/i915/i915_irq.c | 4 +-
> drivers/gpu/drm/mga/mga_drv.h | 2 +-
> drivers/gpu/drm/mga/mga_irq.c | 2 +-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 2 +-
> drivers/gpu/drm/r128/r128_drv.h | 2 +-
> drivers/gpu/drm/r128/r128_irq.c | 2 +-
> drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
> drivers/gpu/drm/radeon/radeon_kms.c | 2 +-
> drivers/gpu/drm/tegra/dc.c | 2 +-
> drivers/gpu/drm/via/via_drv.h | 2 +-
> drivers/gpu/drm/via/via_irq.c | 5 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +-
> include/drm/drmP.h | 2 +-
> include/drm/drm_crtc.h | 2 +-
> include/drm/drm_drv.h | 4 +-
> include/drm/drm_vblank.h | 18 ++--
> 24 files changed, 130 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e0adad590ecb..860f5e194864 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1979,7 +1979,7 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
> int amdgpu_suspend(struct amdgpu_device *adev);
> int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon);
> int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon);
> -u32 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
> +u64 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
> int amdgpu_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> long amdgpu_kms_compat_ioctl(struct file *filp, unsigned int cmd,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 12497a40ef92..f8c814c9c91a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -922,7 +922,7 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
> * Gets the frame count on the requested crtc (all asics).
> * Returns frame count on success, -EINVAL on failure.
> */
> -u32 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe)
> +u64 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe)
> {
> struct amdgpu_device *adev = dev->dev_private;
> int vpos, hpos, stat;
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 463e4d81fb0d..f55f997c0b8f 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -43,7 +43,7 @@
>
> static bool
> drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> - struct timeval *tvblank, bool in_vblank_irq);
> + struct timespec *tvblank, bool in_vblank_irq);
>
> static unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */
>
> @@ -63,8 +63,8 @@ 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,
> - struct timeval *t_vblank, u32 last)
> + u64 vblank_count_inc,
> + struct timespec *t_vblank, u64 last)
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> @@ -82,13 +82,13 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> * "No hw counter" fallback implementation of .get_vblank_counter() hook,
> * if there is no useable hardware frame counter available.
> */
> -static u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> +static u64 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe)
> {
> WARN_ON_ONCE(dev->max_vblank_count != 0);
> return 0;
> }
>
> -static u32 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +static u64 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> @@ -114,9 +114,9 @@ static u32 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> */
> static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe)
> {
> - u32 cur_vblank;
> + u64 cur_vblank;
> bool rc;
> - struct timeval t_vblank;
> + struct timespec t_vblank;
> int count = DRM_TIMESTAMP_MAXRETRIES;
>
> spin_lock(&dev->vblank_time_lock);
> @@ -136,7 +136,7 @@ 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 = (struct timespec) {0, 0};
>
> /*
> * +1 to make sure user will never see the same
> @@ -163,9 +163,9 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> bool in_vblank_irq)
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> - u32 cur_vblank, diff;
> + u64 cur_vblank, diff;
> bool rc;
> - struct timeval t_vblank;
> + struct timespec t_vblank;
> int count = DRM_TIMESTAMP_MAXRETRIES;
> int framedur_ns = vblank->framedur_ns;
>
> @@ -190,11 +190,11 @@ 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;
> + const struct timespec *t_old;
> u64 diff_ns;
>
> t_old = &vblank->time;
> - diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
> + diff_ns = timespec_to_ns(&t_vblank) - timespec_to_ns(t_old);
>
> /*
> * Figure out how many vblanks we've missed based
> @@ -222,13 +222,13 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> * random large forward jumps of the software vblank counter.
> */
> if (diff > 1 && (vblank->inmodeset & 0x2)) {
> - DRM_DEBUG_VBL("clamping vblank bump to 1 on crtc %u: diffr=%u"
> + DRM_DEBUG_VBL("clamping vblank bump to 1 on crtc %u: diffr=%llu"
> " due to pre-modeset.\n", pipe, diff);
> diff = 1;
> }
>
> DRM_DEBUG_VBL("updating vblank count on crtc %u:"
> - " current=%u, diff=%u, hw=%u hw_last=%u\n",
> + " current=%llu, diff=%llu, hw=%llu hw_last=%llu\n",
> pipe, vblank->count, diff, cur_vblank, vblank->last);
>
> if (diff == 0) {
> @@ -243,7 +243,7 @@ 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 = (struct timespec) {0, 0};
>
> store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);
> }
> @@ -567,10 +567,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,
> + struct timespec *vblank_time,
> bool in_vblank_irq)
> {
> - struct timeval tv_etime;
> + struct timespec tv_etime;
> ktime_t stime, etime;
> bool vbl_status;
> struct drm_crtc *crtc;
> @@ -663,29 +663,29 @@ 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);
> + tv_etime = ktime_to_timespec(etime);
> /* 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 = ktime_to_timespec(etime);
>
> DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %ld.%ld -> %ld.%ld [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,
> + (long)tv_etime.tv_sec, (long)tv_etime.tv_nsec,
> + (long)vblank_time->tv_sec, (long)vblank_time->tv_nsec,
> duration_ns/1000, i);
>
> return true;
> }
> EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
>
> -static struct timeval get_drm_timestamp(void)
> +static struct timespec get_drm_timestamp(void)
> {
> ktime_t now;
>
> now = drm_timestamp_monotonic ? ktime_get() : ktime_get_real();
> - return ktime_to_timeval(now);
> + return ktime_to_timespec(now);
> }
>
> /**
> @@ -711,7 +711,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)
> + struct timespec *tvblank, bool in_vblank_irq)
> {
> bool ret = false;
>
> @@ -743,7 +743,7 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> * Returns:
> * The software vblank counter.
> */
> -u32 drm_crtc_vblank_count(struct drm_crtc *crtc)
> +u64 drm_crtc_vblank_count(struct drm_crtc *crtc)
> {
> return drm_vblank_count(crtc->dev, drm_crtc_index(crtc));
> }
> @@ -763,15 +763,15 @@ EXPORT_SYMBOL(drm_crtc_vblank_count);
> *
> * This is the legacy version of drm_crtc_vblank_count_and_time().
> */
> -static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> - struct timeval *vblanktime)
> +static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> + struct timespec *vblanktime)
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> - u32 vblank_count;
> + u64 vblank_count;
> unsigned int seq;
>
> if (WARN_ON(pipe >= dev->num_crtcs)) {
> - *vblanktime = (struct timeval) { 0 };
> + *vblanktime = (struct timespec) { 0 };
> return 0;
> }
>
> @@ -795,8 +795,8 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> * modesetting activity. Returns corresponding system timestamp of the time
> * 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)
> +u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> + struct timespec *vblanktime)
> {
> return drm_vblank_count_and_time(crtc->dev, drm_crtc_index(crtc),
> vblanktime);
> @@ -805,11 +805,11 @@ 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)
> + u64 seq, struct timespec *now)
> {
> e->event.sequence = seq;
> e->event.tv_sec = now->tv_sec;
> - e->event.tv_usec = now->tv_usec;
> + e->event.tv_usec = now->tv_nsec / 1000;
>
> trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
> e->event.sequence);
> @@ -864,7 +864,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> assert_spin_locked(&dev->event_lock);
>
> e->pipe = pipe;
> - e->event.sequence = drm_vblank_count(dev, pipe);
> + e->sequence = drm_vblank_count(dev, pipe);
> e->event.crtc_id = crtc->base.id;
> list_add_tail(&e->base.link, &dev->vblank_event_list);
> }
> @@ -885,8 +885,9 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> struct drm_pending_vblank_event *e)
> {
> struct drm_device *dev = crtc->dev;
> - unsigned int seq, pipe = drm_crtc_index(crtc);
> - struct timeval now;
> + u64 seq;
> + unsigned int pipe = drm_crtc_index(crtc);
> + struct timespec now;
>
> if (dev->num_crtcs > 0) {
> seq = drm_vblank_count_and_time(dev, pipe, &now);
> @@ -1124,9 +1125,9 @@ 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;
> + struct timespec now;
> unsigned long irqflags;
> - unsigned int seq;
> + u64 seq;
>
> if (WARN_ON(pipe >= dev->num_crtcs))
> return;
> @@ -1161,8 +1162,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
> if (e->pipe != pipe)
> continue;
> DRM_DEBUG("Sending premature vblank event on disable: "
> - "wanted %u, current %u\n",
> - e->event.sequence, seq);
> + "wanted %llu current %llu\n",
> + e->sequence, seq);
> list_del(&e->base.link);
> drm_vblank_put(dev, pipe);
> send_vblank_event(dev, e, seq, &now);
> @@ -1331,20 +1332,21 @@ int drm_legacy_modeset_ctl(struct drm_device *dev, void *data,
> return 0;
> }
>
> -static inline bool vblank_passed(u32 seq, u32 ref)
> +static inline bool vblank_passed(u64 seq, u64 ref)
> {
> return (seq - ref) <= (1 << 23);
> }
>
> static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
> + u64 req_seq,
> union drm_wait_vblank *vblwait,
> struct drm_file *file_priv)
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> struct drm_pending_vblank_event *e;
> - struct timeval now;
> + struct timespec now;
> unsigned long flags;
> - unsigned int seq;
> + u64 seq;
> int ret;
>
> e = kzalloc(sizeof(*e), GFP_KERNEL);
> @@ -1379,21 +1381,20 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>
> seq = drm_vblank_count_and_time(dev, pipe, &now);
>
> - DRM_DEBUG("event on vblank count %u, current %u, crtc %u\n",
> - vblwait->request.sequence, seq, pipe);
> + DRM_DEBUG("event on vblank count %llu, current %llu, crtc %u\n",
> + req_seq, seq, pipe);
>
> - trace_drm_vblank_event_queued(file_priv, pipe,
> - vblwait->request.sequence);
> + trace_drm_vblank_event_queued(file_priv, pipe, req_seq);
>
> - e->event.sequence = vblwait->request.sequence;
> - if (vblank_passed(seq, vblwait->request.sequence)) {
> + e->sequence = req_seq;
> + if (vblank_passed(seq, req_seq)) {
> drm_vblank_put(dev, pipe);
> send_vblank_event(dev, e, seq, &now);
> vblwait->reply.sequence = seq;
> } else {
> /* drm_handle_vblank_events will call drm_vblank_put */
> list_add_tail(&e->base.link, &dev->vblank_event_list);
> - vblwait->reply.sequence = vblwait->request.sequence;
> + vblwait->reply.sequence = req_seq;
> }
>
> spin_unlock_irqrestore(&dev->event_lock, flags);
> @@ -1420,6 +1421,27 @@ static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
> }
>
> /*
> + * Widen a 32-bit param to 64-bits.
> + *
> + * \param narrow 32-bit value (missing upper 32 bits)
> + * \param near 64-bit value that should be 'close' to near
> + *
> + * This function returns a 64-bit value using the lower 32-bits from
> + * 'narrow' and constructing the upper 32-bits so that the result is
> + * as close as possible to 'near'.
> + */
> +
> +static u64 widen_32_to_64(u32 narrow, u64 near)
> +{
> + u64 wide = narrow | (near & 0xffffffff00000000ULL);
> + if ((int64_t) (wide - near) > 0x80000000LL)
> + wide -= 0x100000000ULL;
> + else if ((int64_t) (near - wide) > 0x80000000LL)
> + wide += 0x100000000ULL;
> + return wide;
> +}
> +
> +/*
> * Wait for VBLANK.
> *
> * \param inode device inode.
> @@ -1439,6 +1461,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> struct drm_vblank_crtc *vblank;
> union drm_wait_vblank *vblwait = data;
> int ret;
> + u64 req_seq;
> unsigned int flags, seq, pipe, high_pipe;
>
> if (!dev->irq_enabled)
> @@ -1474,12 +1497,12 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> if (dev->vblank_disable_immediate &&
> drm_wait_vblank_is_query(vblwait) &&
> READ_ONCE(vblank->enabled)) {
> - struct timeval now;
> + struct timespec 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;
> + vblwait->reply.tval_usec = now.tv_nsec / 1000;
> return 0;
> }
>
> @@ -1492,9 +1515,11 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>
> switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
> case _DRM_VBLANK_RELATIVE:
> - vblwait->request.sequence += seq;
> + req_seq = seq + vblwait->request.sequence;
> vblwait->request.type &= ~_DRM_VBLANK_RELATIVE;
> + break;
> case _DRM_VBLANK_ABSOLUTE:
> + req_seq = widen_32_to_64(vblwait->request.sequence, seq);
> break;
> default:
> ret = -EINVAL;
> @@ -1502,31 +1527,31 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> }
>
> if ((flags & _DRM_VBLANK_NEXTONMISS) &&
> - vblank_passed(seq, vblwait->request.sequence))
> - vblwait->request.sequence = seq + 1;
> + vblank_passed(seq, req_seq))
> + req_seq = seq + 1;
>
> if (flags & _DRM_VBLANK_EVENT) {
> /* must hold on to the vblank ref until the event fires
> * drm_vblank_put will be called asynchronously
> */
> - return drm_queue_vblank_event(dev, pipe, vblwait, file_priv);
> + return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, file_priv);
> }
>
> - if (vblwait->request.sequence != seq) {
> - DRM_DEBUG("waiting on vblank count %u, crtc %u\n",
> - vblwait->request.sequence, pipe);
> + if (req_seq != seq) {
> + DRM_DEBUG("waiting on vblank count %llu, crtc %u\n",
> + req_seq, pipe);
> DRM_WAIT_ON(ret, vblank->queue, 3 * HZ,
> vblank_passed(drm_vblank_count(dev, pipe),
> - vblwait->request.sequence) ||
> + req_seq) ||
> !READ_ONCE(vblank->enabled));
> }
>
> if (ret != -EINTR) {
> - struct timeval now;
> + struct timespec 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;
> + vblwait->reply.tval_usec = now.tv_nsec / 1000;
>
> DRM_DEBUG("crtc %d returning %u to client\n",
> pipe, vblwait->reply.sequence);
> @@ -1542,8 +1567,8 @@ int drm_wait_vblank(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;
> - unsigned int seq;
> + struct timespec now;
> + u64 seq;
>
> assert_spin_locked(&dev->event_lock);
>
> @@ -1552,11 +1577,11 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
> list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
> if (e->pipe != pipe)
> continue;
> - if (!vblank_passed(seq, e->event.sequence))
> + if (!vblank_passed(seq, e->sequence))
> continue;
>
> - DRM_DEBUG("vblank event on %u, current %u\n",
> - e->event.sequence, seq);
> + DRM_DEBUG("vblank event on %llu, current %llu\n",
> + e->sequence, seq);
>
> list_del(&e->base.link);
> drm_vblank_put(dev, pipe);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index d72777f6411a..110beca116f8 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -132,7 +132,7 @@ static void exynos_drm_crtc_disable_vblank(struct drm_crtc *crtc)
> exynos_crtc->ops->disable_vblank(exynos_crtc);
> }
>
> -static u32 exynos_drm_crtc_get_vblank_counter(struct drm_crtc *crtc)
> +static u64 exynos_drm_crtc_get_vblank_counter(struct drm_crtc *crtc)
> {
> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>
> diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h
> index 83667087d6e5..6e3959da8ec2 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.h
> +++ b/drivers/gpu/drm/gma500/psb_drv.h
> @@ -699,7 +699,7 @@ psb_enable_pipestat(struct drm_psb_private *dev_priv, int pipe, u32 mask);
> void
> psb_disable_pipestat(struct drm_psb_private *dev_priv, int pipe, u32 mask);
>
> -extern u32 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> +extern u64 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
>
> /* framebuffer.c */
> extern int psbfb_probed(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/gma500/psb_irq.c b/drivers/gpu/drm/gma500/psb_irq.c
> index 78eb10902809..efd2124de971 100644
> --- a/drivers/gpu/drm/gma500/psb_irq.c
> +++ b/drivers/gpu/drm/gma500/psb_irq.c
> @@ -622,7 +622,7 @@ void mdfld_disable_te(struct drm_device *dev, int pipe)
> /* Called from drm generic code, passed a 'crtc', which
> * we use as a pipe index
> */
> -u32 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +u64 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> uint32_t high_frame = PIPEAFRAMEHIGH;
> uint32_t low_frame = PIPEAFRAMEPIXEL;
> diff --git a/drivers/gpu/drm/gma500/psb_irq.h b/drivers/gpu/drm/gma500/psb_irq.h
> index e6a81a8c9f35..4ab8af0607a4 100644
> --- a/drivers/gpu/drm/gma500/psb_irq.h
> +++ b/drivers/gpu/drm/gma500/psb_irq.h
> @@ -40,7 +40,7 @@ void psb_irq_turn_on_dpst(struct drm_device *dev);
> void psb_irq_turn_off_dpst(struct drm_device *dev);
> int psb_enable_vblank(struct drm_device *dev, unsigned int pipe);
> void psb_disable_vblank(struct drm_device *dev, unsigned int pipe);
> -u32 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> +u64 psb_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
>
> int mdfld_enable_te(struct drm_device *dev, int pipe);
> void mdfld_disable_te(struct drm_device *dev, int pipe);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7b7f55a28eec..97c928c823e2 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -715,7 +715,7 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
> /* Called from drm generic code, passed a 'crtc', which
> * we use as a pipe index
> */
> -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +static u64 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> i915_reg_t high_frame, low_frame;
> @@ -765,7 +765,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xffffff;
> }
>
> -static u32 g4x_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +static u64 g4x_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> diff --git a/drivers/gpu/drm/mga/mga_drv.h b/drivers/gpu/drm/mga/mga_drv.h
> index 45cf363d25ad..46adff5d1fc6 100644
> --- a/drivers/gpu/drm/mga/mga_drv.h
> +++ b/drivers/gpu/drm/mga/mga_drv.h
> @@ -185,7 +185,7 @@ extern int mga_warp_init(drm_mga_private_t *dev_priv);
> /* mga_irq.c */
> extern int mga_enable_vblank(struct drm_device *dev, unsigned int pipe);
> extern void mga_disable_vblank(struct drm_device *dev, unsigned int pipe);
> -extern u32 mga_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> +extern u64 mga_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> extern int mga_driver_fence_wait(struct drm_device *dev, unsigned int *sequence);
> extern int mga_driver_vblank_wait(struct drm_device *dev, unsigned int *sequence);
> extern irqreturn_t mga_driver_irq_handler(int irq, void *arg);
> diff --git a/drivers/gpu/drm/mga/mga_irq.c b/drivers/gpu/drm/mga/mga_irq.c
> index 693ba708cfed..a361db778f6a 100644
> --- a/drivers/gpu/drm/mga/mga_irq.c
> +++ b/drivers/gpu/drm/mga/mga_irq.c
> @@ -35,7 +35,7 @@
> #include <drm/mga_drm.h>
> #include "mga_drv.h"
>
> -u32 mga_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +u64 mga_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> const drm_mga_private_t *const dev_priv =
> (drm_mga_private_t *) dev->dev_private;
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index e2b3346ead48..678f2c03a93a 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -587,7 +587,7 @@ static bool mdp5_get_scanoutpos(struct drm_device *dev, unsigned int pipe,
> return true;
> }
>
> -static u32 mdp5_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +static u64 mdp5_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> struct msm_drm_private *priv = dev->dev_private;
> struct drm_crtc *crtc;
> diff --git a/drivers/gpu/drm/r128/r128_drv.h b/drivers/gpu/drm/r128/r128_drv.h
> index 09143b840482..3479198774dc 100644
> --- a/drivers/gpu/drm/r128/r128_drv.h
> +++ b/drivers/gpu/drm/r128/r128_drv.h
> @@ -156,7 +156,7 @@ extern int r128_do_cleanup_cce(struct drm_device *dev);
>
> extern int r128_enable_vblank(struct drm_device *dev, unsigned int pipe);
> extern void r128_disable_vblank(struct drm_device *dev, unsigned int pipe);
> -extern u32 r128_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> +extern u64 r128_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> extern irqreturn_t r128_driver_irq_handler(int irq, void *arg);
> extern void r128_driver_irq_preinstall(struct drm_device *dev);
> extern int r128_driver_irq_postinstall(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/r128/r128_irq.c b/drivers/gpu/drm/r128/r128_irq.c
> index 9730f4918944..141d4dfc30b1 100644
> --- a/drivers/gpu/drm/r128/r128_irq.c
> +++ b/drivers/gpu/drm/r128/r128_irq.c
> @@ -34,7 +34,7 @@
> #include <drm/r128_drm.h>
> #include "r128_drv.h"
>
> -u32 r128_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +u64 r128_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> const drm_r128_private_t *dev_priv = dev->dev_private;
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index b23c771f4216..4e2b3fa4293a 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -112,7 +112,7 @@ void radeon_driver_postclose_kms(struct drm_device *dev,
> int radeon_suspend_kms(struct drm_device *dev, bool suspend,
> bool fbcon, bool freeze);
> int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon);
> -u32 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
> +u64 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
> int radeon_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> void radeon_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> void radeon_driver_irq_preinstall_kms(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index dfee8f7d94ae..bf6c3bad36c7 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -772,7 +772,7 @@ void radeon_driver_postclose_kms(struct drm_device *dev,
> * Gets the frame count on the requested crtc (all asics).
> * Returns frame count on success, -EINVAL on failure.
> */
> -u32 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe)
> +u64 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe)
> {
> int vpos, hpos, stat;
> u32 count;
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 95b373f739f2..9f060ce156b4 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -909,7 +909,7 @@ static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
> return 0;
> }
>
> -static u32 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
> +static u64 tegra_dc_get_vblank_counter(struct drm_crtc *crtc)
> {
> struct tegra_dc *dc = to_tegra_dc(crtc);
>
> diff --git a/drivers/gpu/drm/via/via_drv.h b/drivers/gpu/drm/via/via_drv.h
> index 9873942ca8f4..019769a54a70 100644
> --- a/drivers/gpu/drm/via/via_drv.h
> +++ b/drivers/gpu/drm/via/via_drv.h
> @@ -140,7 +140,7 @@ extern int via_init_context(struct drm_device *dev, int context);
> extern int via_final_context(struct drm_device *dev, int context);
>
> extern int via_do_cleanup_map(struct drm_device *dev);
> -extern u32 via_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> +extern u64 via_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> extern int via_enable_vblank(struct drm_device *dev, unsigned int pipe);
> extern void via_disable_vblank(struct drm_device *dev, unsigned int pipe);
>
> diff --git a/drivers/gpu/drm/via/via_irq.c b/drivers/gpu/drm/via/via_irq.c
> index ea8172c747a2..a151c72d148a 100644
> --- a/drivers/gpu/drm/via/via_irq.c
> +++ b/drivers/gpu/drm/via/via_irq.c
> @@ -95,7 +95,7 @@ static unsigned time_diff(struct timeval *now, struct timeval *then)
> 1000000 - (then->tv_usec - now->tv_usec);
> }
>
> -u32 via_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +u64 via_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> drm_via_private_t *dev_priv = dev->dev_private;
>
> @@ -317,6 +317,9 @@ int via_driver_irq_postinstall(struct drm_device *dev)
> if (!dev_priv)
> return -EINVAL;
>
> + if (dev->driver->get_vblank_counter)
> + dev->max_vblank_count = 0xffffffff;
> +
> status = VIA_READ(VIA_REG_INTERRUPT);
> VIA_WRITE(VIA_REG_INTERRUPT, status | VIA_IRQ_GLOBAL
> | dev_priv->irq_enable_mask);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 130d51c5ec6a..adc9d2ae37a4 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -918,7 +918,7 @@ void vmw_kms_idle_workqueues(struct vmw_master *vmaster);
> bool vmw_kms_validate_mode_vram(struct vmw_private *dev_priv,
> uint32_t pitch,
> uint32_t height);
> -u32 vmw_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> +u64 vmw_get_vblank_counter(struct drm_device *dev, unsigned int pipe);
> int vmw_enable_vblank(struct drm_device *dev, unsigned int pipe);
> void vmw_disable_vblank(struct drm_device *dev, unsigned int pipe);
> int vmw_kms_present(struct vmw_private *dev_priv,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index a8876b070168..135f5c3dbb6c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -1944,7 +1944,7 @@ bool vmw_kms_validate_mode_vram(struct vmw_private *dev_priv,
> /**
> * Function called by DRM code called with vbl_lock held.
> */
> -u32 vmw_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +u64 vmw_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> return 0;
> }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 39df16af7a4a..e50cf152f565 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -403,7 +403,7 @@ struct drm_device {
> spinlock_t vblank_time_lock; /**< Protects vblank count and time updates during vblank enable/disable */
> spinlock_t vbl_lock;
>
> - u32 max_vblank_count; /**< size of vblank counter register */
> + u64 max_vblank_count; /**< size of vblank counter register */
>
> /**
> * List of events
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 629a5fe075b3..e866f0007d8a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -689,7 +689,7 @@ struct drm_crtc_funcs {
> *
> * Raw vblank counter value.
> */
> - u32 (*get_vblank_counter)(struct drm_crtc *crtc);
> + u64 (*get_vblank_counter)(struct drm_crtc *crtc);
>
> /**
> * @enable_vblank:
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index d855f9ae41a8..e575802fbe4c 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -196,7 +196,7 @@ struct drm_driver {
> *
> * Raw vblank counter value.
> */
> - u32 (*get_vblank_counter) (struct drm_device *dev, unsigned int pipe);
> + u64 (*get_vblank_counter) (struct drm_device *dev, unsigned int pipe);
>
> /**
> * @enable_vblank:
> @@ -325,7 +325,7 @@ struct drm_driver {
> */
> bool (*get_vblank_timestamp) (struct drm_device *dev, unsigned int pipe,
> int *max_error,
> - struct timeval *vblank_time,
> + struct timespec *vblank_time,
> bool in_vblank_irq);
>
> /**
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 4cde47332dfa..68e99177fff3 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -48,6 +48,10 @@ struct drm_pending_vblank_event {
> */
> unsigned int pipe;
> /**
> + * @sequence: frame event should be triggered at
> + */
> + u64 sequence;
> + /**
> * @event: Actual event which will be sent to userspace.
> */
> struct drm_event_vblank event;
> @@ -88,11 +92,11 @@ struct drm_vblank_crtc {
> /**
> * @count: Current software vblank counter.
> */
> - u32 count;
> + u64 count;
> /**
> * @time: Vblank timestamp corresponding to @count.
> */
> - struct timeval time;
> + struct timespec time;
>
> /**
> * @refcount: Number of users/waiters of the vblank interrupt. Only when
> @@ -103,7 +107,7 @@ struct drm_vblank_crtc {
> /**
> * @last: Protected by &drm_device.vbl_lock, used for wraparound handling.
> */
> - u32 last;
> + u64 last;
> /**
> * @inmodeset: Tracks whether the vblank is disabled due to a modeset.
> * For legacy driver bit 2 additionally tracks whether an additional
> @@ -152,9 +156,9 @@ 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);
> +u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
> +u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> + struct timespec *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,
> @@ -173,7 +177,7 @@ u32 drm_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,
> + struct timespec *vblank_time,
> bool in_vblank_irq);
> void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> const struct drm_display_mode *mode);
> --
> 2.11.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Jul 05, 2017 at 03:10:12PM -0700, Keith Packard wrote:
> Place drm_event_vblank in a new union that includes that and a bare
> drm_event structure. This will allow new members of that union to be
> added in the future without changing code related to the existing vbl
> event type.
>
> Assignments to the crtc_id field are now done when the event is
> allocated, rather than when delievered. This way, delivery doesn't
> need to have the crtc ID available.
>
> Signed-off-by: Keith Packard <[email protected]>
A few nits below, but looks good otherwise.
-Daniel
> ---
> drivers/gpu/drm/drm_atomic.c | 7 ++++---
> drivers/gpu/drm/drm_plane.c | 2 +-
> drivers/gpu/drm/drm_vblank.c | 27 ++++++++++++++++-----------
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 ++--
> include/drm/drm_vblank.h | 8 +++++++-
> 6 files changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c0f336d23f9c..f569d7f03f3c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1839,7 +1839,7 @@ int drm_atomic_debugfs_init(struct drm_minor *minor)
> */
>
> static struct drm_pending_vblank_event *create_vblank_event(
> - struct drm_device *dev, uint64_t user_data)
> + struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data)
Nit: Please also drop the dev argument, we have crtc->dev easily
available. That fits better into my long-term goal of getting rid of the
(dev, pipe) pairs everywhere in the vblank code and fully switching over
to drm_crtc *.
> {
> struct drm_pending_vblank_event *e = NULL;
>
> @@ -1849,7 +1849,8 @@ static struct drm_pending_vblank_event *create_vblank_event(
>
> e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
> e->event.base.length = sizeof(e->event);
> - e->event.user_data = user_data;
> + e->event.vbl.crtc_id = crtc->base.id;
> + e->event.vbl.user_data = user_data;
>
> return e;
> }
> @@ -2052,7 +2053,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
> if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
> struct drm_pending_vblank_event *e;
>
> - e = create_vblank_event(dev, arg->user_data);
> + e = create_vblank_event(dev, crtc, arg->user_data);
> if (!e)
> return -ENOMEM;
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 5dc8c4350602..fe9f31285bc2 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -918,7 +918,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> }
> e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
> e->event.base.length = sizeof(e->event);
> - e->event.user_data = page_flip->user_data;
> + e->event.vbl.user_data = page_flip->user_data;
> ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
> if (ret) {
> kfree(e);
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f55f997c0b8f..9ae170857ef6 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -807,13 +807,18 @@ static void send_vblank_event(struct drm_device *dev,
> struct drm_pending_vblank_event *e,
> u64 seq, struct timespec *now)
> {
> - e->event.sequence = seq;
> - e->event.tv_sec = now->tv_sec;
> - e->event.tv_usec = now->tv_nsec / 1000;
> -
> - trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
> - e->event.sequence);
> -
> + switch (e->event.base.type) {
> + case DRM_EVENT_VBLANK:
> + case DRM_EVENT_FLIP_COMPLETE:
> + if (seq)
> + e->event.vbl.sequence = (u32) seq;
> + if (now) {
> + e->event.vbl.tv_sec = now->tv_sec;
> + e->event.vbl.tv_usec = now->tv_nsec / 1000;
> + }
> + break;
> + }
Not sure why this change? Also prep for the new, presumably extended
events? Seems at least slightly inconsistent with other paths, where we
still unconditionally fill it in.
> + trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
> drm_send_event_locked(dev, &e->base);
> }
>
> @@ -865,7 +870,6 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>
> e->pipe = pipe;
> e->sequence = drm_vblank_count(dev, pipe);
> - e->event.crtc_id = crtc->base.id;
> list_add_tail(&e->base.link, &dev->vblank_event_list);
> }
> EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
> @@ -897,7 +901,6 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> now = get_drm_timestamp();
> }
> e->pipe = pipe;
> - e->event.crtc_id = crtc->base.id;
> send_vblank_event(dev, e, seq, &now);
> }
> EXPORT_SYMBOL(drm_crtc_send_vblank_event);
> @@ -1342,6 +1345,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
> union drm_wait_vblank *vblwait,
> struct drm_file *file_priv)
> {
> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
This'll oops on ums drivers since kms isn't set up. And we can call this
from ums drivers in the old vblank ioctl. My long-term goal is to
completely separate these two worlds, and exclusive deal with drm_crtc *
in the kms side (and only compute the pipe index when needed). But we're
not there yet. I also want to split it into two files (drm_crtc_vblank.c
and drm_legacy_vblank.c) and make sure we consistently use
drm_crtc_vblank_ as the new-world prefix.
Probably the simplest option is to extend this to take all three (dev,
pipe, crtc) as arguments, and then pass NULL for the old vblank ioctl, and
the right pointer for the new stuff. Or you stuff a DRIVER_MODESET check
at the right spot somewhere.
Or maybe I shouldn't have told you this and seized this opportunity to
break all the old drivers :-)
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> struct drm_pending_vblank_event *e;
> struct timespec now;
> @@ -1357,8 +1361,9 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>
> e->pipe = pipe;
> e->event.base.type = DRM_EVENT_VBLANK;
> - e->event.base.length = sizeof(e->event);
> - e->event.user_data = vblwait->request.signal;
> + e->event.base.length = sizeof(e->event.vbl);
> + e->event.vbl.user_data = vblwait->request.signal;
> + e->event.vbl.crtc_id = crtc ? crtc->base.id : 0;
>
> spin_lock_irqsave(&dev->event_lock, flags);
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index 8d7dc9def7c2..c13b97338310 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -358,8 +358,8 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
>
> ret = vmw_event_fence_action_queue(file_priv, fence,
> &event->base,
> - &event->event.tv_sec,
> - &event->event.tv_usec,
> + &event->event.vbl.tv_sec,
> + &event->event.vbl.tv_usec,
> true);
> }
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index bad31bdf09b6..4e329588ce9c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -544,8 +544,8 @@ static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc,
>
> ret = vmw_event_fence_action_queue(file_priv, fence,
> &event->base,
> - &event->event.tv_sec,
> - &event->event.tv_usec,
> + &event->event.vbl.tv_sec,
> + &event->event.vbl.tv_usec,
> true);
> vmw_fence_obj_unreference(&fence);
> } else {
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 68e99177fff3..3ef7ddc5db5f 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -54,7 +54,10 @@ struct drm_pending_vblank_event {
> /**
> * @event: Actual event which will be sent to userspace.
> */
> - struct drm_event_vblank event;
> + union {
> + struct drm_event base;
> + struct drm_event_vblank vbl;
> + } event;
> };
>
> /**
> @@ -163,6 +166,9 @@ 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,
> struct drm_pending_vblank_event *e);
> +void drm_vblank_set_event(struct drm_pending_vblank_event *e,
> + u64 *seq,
> + struct timespec *now);
> bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
> bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
> int drm_crtc_vblank_get(struct drm_crtc *crtc);
> --
> 2.11.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 06/07/17 07:10 AM, Keith Packard wrote:
> This modifies the datatypes used by the vblank code to provide both 64
> bits of vblank count and to increase the resolution of the vblank
> timestamp from microseconds to nanoseconds.
>
> The driver interfaces have also been changed to return 64-bits of
> vblank count; fortunately all of the code necessary to widen that value
> was already included to handle devices returning fewer than 32-bits.
>
> This will provide the necessary datatypes for the Vulkan API.
>
> Signed-off-by: Keith Packard <[email protected]>
[...]
> @@ -1492,9 +1515,11 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>
> switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
> case _DRM_VBLANK_RELATIVE:
> - vblwait->request.sequence += seq;
> + req_seq = seq + vblwait->request.sequence;
> vblwait->request.type &= ~_DRM_VBLANK_RELATIVE;
Subtle breakage here: vblwait->request.sequence must still get updated
for _DRM_VBLANK_RELATIVE, in case we're interrupted by a signal.
> @@ -317,6 +317,9 @@ int via_driver_irq_postinstall(struct drm_device *dev)
> if (!dev_priv)
> return -EINVAL;
>
> + if (dev->driver->get_vblank_counter)
> + dev->max_vblank_count = 0xffffffff;
What's the purpose of this? All drivers providing get_vblank_counter
should already initialize max_vblank_count correctly.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On Wed, Jul 05, 2017 at 03:10:13PM -0700, Keith Packard wrote:
> These provide crtc-id based functions instead of pipe-number, while
> also offering higher resolution time (ns) and wider frame count (64)
> as required by the Vulkan API.
>
> Signed-off-by: Keith Packard <[email protected]>
I very much like this since the old ioctl really is a rather bad horror
show. And since it's tied in with ums drivers everything is complicated.
\o/ for much cleaner ioctls.
Bunch of comments below, but looks good overall.
-Daniel
> ---
> drivers/gpu/drm/drm_internal.h | 6 ++
> drivers/gpu/drm/drm_ioctl.c | 2 +
> drivers/gpu/drm/drm_vblank.c | 148 +++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_vblank.h | 1 +
> include/uapi/drm/drm.h | 32 +++++++++
> 5 files changed, 189 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 5cecc974d2f9..b68a193b7907 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -65,6 +65,12 @@ int drm_legacy_irq_control(struct drm_device *dev, void *data,
> int drm_legacy_modeset_ctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
>
> +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *filp);
> +
> +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *filp);
> +
> /* drm_auth.c */
> int drm_getmagic(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index f1e568176da9..63016cf3e224 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
> + DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
I started a discussion a while back whether these should be restricted to
DRM_MASTER (i.e. the modeset resource owner) or available to everyone.
Since it's read-only I guess we can keep it accessible to everyone, but it
has a bit the problem that client app developers see this, think it does
what it does and then use it to schedule frames without asking the
compositor. Which sometimes even works, but isn't really proper design.
The reasons seems to be that on X11 there's no EGL extension for accurate
timing frame updates (DRI2/3 can do it ofc, and glx exposes it, but glx is
uncool or something like that).
> };
>
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 9ae170857ef6..93004b1bf84c 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -817,6 +817,12 @@ static void send_vblank_event(struct drm_device *dev,
> e->event.vbl.tv_usec = now->tv_nsec / 1000;
> }
> break;
> + case DRM_EVENT_CRTC_SEQUENCE:
> + if (seq)
> + e->event.seq.sequence = seq;
> + if (now)
> + e->event.seq.time_ns = timespec_to_ns(now);
> + break;
> }
> trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
> drm_send_event_locked(dev, &e->base);
> @@ -1516,6 +1522,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> return ret;
> }
> +
> seq = drm_vblank_count(dev, pipe);
>
> switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
> @@ -1676,3 +1683,144 @@ bool drm_crtc_handle_vblank(struct drm_crtc *crtc)
> return drm_handle_vblank(crtc->dev, drm_crtc_index(crtc));
> }
> EXPORT_SYMBOL(drm_crtc_handle_vblank);
> +
> +/*
> + * Get crtc VBLANK count.
> + *
> + * \param dev DRM device
> + * \param data user arguement, pointing to a drm_crtc_get_sequence structure.
> + * \param file_priv drm file private for the user's open file descriptor
> + */
Since this stuff isn't parsed by kerneldoc I tend to just free-form ioctl
comments completely. Someday maybe someone even gets around to doing
proper uabi documentation :-) Just an aside.
> +
> +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_crtc *crtc;
> + int pipe;
> + struct drm_crtc_get_sequence *get_seq = data;
> + struct timespec now;
> +
You need a DRIVER_MODESET check here or the drm_crtc_find will oops. Same
below.
> + if (!dev->irq_enabled)
> + return -EINVAL;
> +
> + crtc = drm_crtc_find(dev, get_seq->crtc_id);
> + if (!crtc)
> + return -ENOENT;
> +
> + pipe = drm_crtc_index(crtc);
> +
> + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
This can give you and old vblank if the vblank is off (i.e. sw state
hasn't be regularly updated). I think we want a new
drm_crtc_accurate_vblank_count_and_time variant.
Another thing that is very ill-defined in the old vblank ioctl and that we
could fix here: Asking for vblanks when the CRTC is off is a bit silly.
Asking for the sequence when it's off makes some sense, but might still be
good to give userspace some indication in the new struct? This also from
the pov of the unpriviledge vblank waiter use-case that I wondered about
earlier.
> + get_seq->sequence_ns = timespec_to_ns(&now);
> + return 0;
> +}
> +
> +/*
> + * Queue a event for VBLANK sequence
> + *
> + * \param dev DRM device
> + * \param data user arguement, pointing to a drm_crtc_queue_sequence structure.
> + * \param file_priv drm file private for the user's open file descriptor
> + */
> +
> +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_crtc *crtc;
> + struct drm_vblank_crtc *vblank;
> + int pipe;
> + struct drm_crtc_queue_sequence *queue_seq = data;
> + struct timespec now;
> + struct drm_pending_vblank_event *e;
> + u32 flags;
> + u64 seq;
> + u64 req_seq;
> + int ret;
> + unsigned long spin_flags;
> +
> + if (!dev->irq_enabled)
> + return -EINVAL;
> +
> + crtc = drm_crtc_find(dev, queue_seq->crtc_id);
> + if (!crtc)
> + return -ENOENT;
> +
> + flags = queue_seq->flags;
> + /* Check valid flag bits */
> + if (flags & ~(DRM_CRTC_SEQUENCE_RELATIVE|
> + DRM_CRTC_SEQUENCE_NEXT_ON_MISS|
> + DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
> + return -EINVAL;
> +
> + /* Check for valid signal edge */
> + if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
> + return -EINVAL;
This seems new, maybe drop it until we need it?
> +
> + pipe = drm_crtc_index(crtc);
> +
> + vblank = &dev->vblank[pipe];
> +
> + e = kzalloc(sizeof(*e), GFP_KERNEL);
> + if (e == NULL)
> + return -ENOMEM;
> +
> + ret = drm_vblank_get(dev, pipe);
drm_crtc_vblank_get pls, would be sweet if we can avoid the (dev, pipe)
pairs as much as possible. Same for all the others.
> + if (ret) {
> + DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> + goto err_free;
> + }
> +
> + seq = drm_vblank_count_and_time(dev, pipe, &now);
I think here there's no need for the accurate version, since we
force-enable the vblanks already.
> + req_seq = queue_seq->sequence;
> +
> + if (flags & DRM_CRTC_SEQUENCE_RELATIVE)
> + req_seq += seq;
> +
> + if ((flags & DRM_CRTC_SEQUENCE_NEXT_ON_MISS) && vblank_passed(seq, req_seq))
> + req_seq = seq + 1;
> +
> + e->pipe = pipe;
> + e->event.base.type = DRM_EVENT_CRTC_SEQUENCE;
> + e->event.base.length = sizeof(e->event.seq);
> + e->event.seq.user_data = queue_seq->user_data;
No need for crtc_id in this event? Or do we expect userspace to encode
that in the user_data somehow? I don't think it's a real problem since
we'll only deliver one event per request, so clear for which crtc it is.
In atomic we might deliver multiple events (one for each crtc) so that's
why it's needed there.
But might be useful just for consistency.
> +
> + spin_lock_irqsave(&dev->event_lock, spin_flags);
> +
> + /*
> + * drm_crtc_vblank_off() might have been called after we called
> + * drm_vblank_get(). drm_crtc_vblank_off() holds event_lock around the
> + * vblank disable, so no need for further locking. The reference from
> + * drm_vblank_get() protects against vblank disable from another source.
> + */
> + if (!READ_ONCE(vblank->enabled)) {
> + ret = -EINVAL;
> + goto err_unlock;
> + }
> +
> + ret = drm_event_reserve_init_locked(dev, file_priv, &e->base,
> + &e->event.base);
> +
> + if (ret)
> + goto err_unlock;
> +
> + e->sequence = req_seq;
> +
> + if (vblank_passed(seq, req_seq)) {
> + drm_vblank_put(dev, pipe);
> + send_vblank_event(dev, e, seq, &now);
> + queue_seq->sequence = seq;
> + } else {
> + /* drm_handle_vblank_events will call drm_vblank_put */
> + list_add_tail(&e->base.link, &dev->vblank_event_list);
> + queue_seq->sequence = req_seq;
> + }
> +
> + spin_unlock_irqrestore(&dev->event_lock, spin_flags);
> + return 0;
> +
> +err_unlock:
> + spin_unlock_irqrestore(&dev->event_lock, spin_flags);
> + drm_vblank_put(dev, pipe);
> +err_free:
> + kfree(e);
> + return ret;
> +}
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 3ef7ddc5db5f..8a5e1dfe3be7 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -57,6 +57,7 @@ struct drm_pending_vblank_event {
> union {
> struct drm_event base;
> struct drm_event_vblank vbl;
> + struct drm_event_crtc_sequence seq;
> } event;
> };
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 101593ab10ac..dc16d42a88c7 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -718,6 +718,27 @@ struct drm_syncobj_handle {
> __u32 pad;
> };
>
> +/* Query current scanout sequence number */
> +struct drm_crtc_get_sequence {
> + __u32 crtc_id;
> + __u32 pad;
> + __u64 sequence;
> + __u64 sequence_ns;
> +};
> +
> +/* Queue event to be delivered at specified sequence */
> +
> +#define DRM_CRTC_SEQUENCE_RELATIVE 0x00000001 /* sequence is relative to current */
> +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
I thought this is already the semantics our current vblank events have (in
the timestamp, when exactly the event comes out isn't defined further than
"somewhere around vblank"). Since it's unsed I'd just remove this #define.
> +
> +struct drm_crtc_queue_sequence {
> + __u32 crtc_id;
> + __u32 flags;
> + __u64 sequence; /* on input, target sequence. on output, actual sequence */
> + __u64 user_data; /* user data passed to event */
> +};
In both ioctl handlers pls make sure everything you don't look at is 0,
including unused stuff like pad. Otherwise userspace might fail to clear
them, and we can never use them in the future. Maybe just rename pad to
flags right away.
> +
> #if defined(__cplusplus)
> }
> #endif
> @@ -800,6 +821,9 @@ extern "C" {
>
> #define DRM_IOCTL_WAIT_VBLANK DRM_IOWR(0x3a, union drm_wait_vblank)
>
> +#define DRM_IOCTL_CRTC_GET_SEQUENCE DRM_IOWR(0x3b, struct drm_crtc_get_sequence)
> +#define DRM_IOCTL_CRTC_QUEUE_SEQUENCE DRM_IOWR(0x3c, struct drm_crtc_queue_sequence)
> +
> #define DRM_IOCTL_UPDATE_DRAW DRM_IOW(0x3f, struct drm_update_draw)
>
> #define DRM_IOCTL_MODE_GETRESOURCES DRM_IOWR(0xA0, struct drm_mode_card_res)
> @@ -871,6 +895,7 @@ struct drm_event {
>
> #define DRM_EVENT_VBLANK 0x01
> #define DRM_EVENT_FLIP_COMPLETE 0x02
> +#define DRM_EVENT_CRTC_SEQUENCE 0x03
>
> struct drm_event_vblank {
> struct drm_event base;
> @@ -881,6 +906,13 @@ struct drm_event_vblank {
> __u32 crtc_id; /* 0 on older kernels that do not support this */
> };
>
> +struct drm_event_crtc_sequence {
> + struct drm_event base;
> + __u64 user_data;
> + __u64 time_ns;
> + __u64 sequence;
> +};
> +
> /* typedef area */
> #ifndef __KERNEL__
> typedef struct drm_clip_rect drm_clip_rect_t;
> --
> 2.11.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 06/07/17 04:45 PM, Michel Dänzer wrote:
> On 06/07/17 07:10 AM, Keith Packard wrote:
>> This modifies the datatypes used by the vblank code to provide both 64
>> bits of vblank count and to increase the resolution of the vblank
>> timestamp from microseconds to nanoseconds.
>>
>> The driver interfaces have also been changed to return 64-bits of
>> vblank count; fortunately all of the code necessary to widen that value
>> was already included to handle devices returning fewer than 32-bits.
>>
>> This will provide the necessary datatypes for the Vulkan API.
>>
>> Signed-off-by: Keith Packard <[email protected]>
>
> [...]
>
>> @@ -1492,9 +1515,11 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>>
>> switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
>> case _DRM_VBLANK_RELATIVE:
>> - vblwait->request.sequence += seq;
>> + req_seq = seq + vblwait->request.sequence;
>> vblwait->request.type &= ~_DRM_VBLANK_RELATIVE;
>
> Subtle breakage here: vblwait->request.sequence must still get updated
> for _DRM_VBLANK_RELATIVE, in case we're interrupted by a signal.
BTW, this got me thinking that we should probably treat
_DRM_VBLANK_NEXTONMISS the same way, i.e. clear the flag after updating
vblwait->request.sequence. Otherwise there could theoretically (though
unlikely) be an infinite loop:
ioctl with _DRM_VBLANK_NEXTONMISS, target missed => wait for next vblank
wait interrupted by signal
lather, rinse, repeat
I'd advise against adding a "next on miss" flag for the new ioctl until
there is specific demand for that.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On Thu, Jul 06, 2017 at 09:53:13AM +0200, Daniel Vetter wrote:
> On Wed, Jul 05, 2017 at 03:10:13PM -0700, Keith Packard wrote:
> > These provide crtc-id based functions instead of pipe-number, while
> > also offering higher resolution time (ns) and wider frame count (64)
> > as required by the Vulkan API.
> >
> > Signed-off-by: Keith Packard <[email protected]>
>
> I very much like this since the old ioctl really is a rather bad horror
> show. And since it's tied in with ums drivers everything is complicated.
>
> \o/ for much cleaner ioctls.
>
> Bunch of comments below, but looks good overall.
> -Daniel
>
> > ---
> > drivers/gpu/drm/drm_internal.h | 6 ++
> > drivers/gpu/drm/drm_ioctl.c | 2 +
> > drivers/gpu/drm/drm_vblank.c | 148 +++++++++++++++++++++++++++++++++++++++++
> > include/drm/drm_vblank.h | 1 +
> > include/uapi/drm/drm.h | 32 +++++++++
> > 5 files changed, 189 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > index 5cecc974d2f9..b68a193b7907 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -65,6 +65,12 @@ int drm_legacy_irq_control(struct drm_device *dev, void *data,
> > int drm_legacy_modeset_ctl(struct drm_device *dev, void *data,
> > struct drm_file *file_priv);
> >
> > +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *filp);
> > +
> > +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *filp);
> > +
> > /* drm_auth.c */
> > int drm_getmagic(struct drm_device *dev, void *data,
> > struct drm_file *file_priv);
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index f1e568176da9..63016cf3e224 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> > DRM_UNLOCKED|DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
> > DRM_UNLOCKED|DRM_RENDER_ALLOW),
> > + DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
> > + DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
>
> I started a discussion a while back whether these should be restricted to
> DRM_MASTER (i.e. the modeset resource owner) or available to everyone.
> Since it's read-only I guess we can keep it accessible to everyone, but it
> has a bit the problem that client app developers see this, think it does
> what it does and then use it to schedule frames without asking the
> compositor. Which sometimes even works, but isn't really proper design.
> The reasons seems to be that on X11 there's no EGL extension for accurate
> timing frame updates (DRI2/3 can do it ofc, and glx exposes it, but glx is
> uncool or something like that).
>
> > };
> >
> > #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 9ae170857ef6..93004b1bf84c 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -817,6 +817,12 @@ static void send_vblank_event(struct drm_device *dev,
> > e->event.vbl.tv_usec = now->tv_nsec / 1000;
> > }
> > break;
> > + case DRM_EVENT_CRTC_SEQUENCE:
> > + if (seq)
> > + e->event.seq.sequence = seq;
> > + if (now)
> > + e->event.seq.time_ns = timespec_to_ns(now);
> > + break;
> > }
> > trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
> > drm_send_event_locked(dev, &e->base);
> > @@ -1516,6 +1522,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> > DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> > return ret;
> > }
> > +
> > seq = drm_vblank_count(dev, pipe);
> >
> > switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
> > @@ -1676,3 +1683,144 @@ bool drm_crtc_handle_vblank(struct drm_crtc *crtc)
> > return drm_handle_vblank(crtc->dev, drm_crtc_index(crtc));
> > }
> > EXPORT_SYMBOL(drm_crtc_handle_vblank);
> > +
> > +/*
> > + * Get crtc VBLANK count.
> > + *
> > + * \param dev DRM device
> > + * \param data user arguement, pointing to a drm_crtc_get_sequence structure.
> > + * \param file_priv drm file private for the user's open file descriptor
> > + */
>
> Since this stuff isn't parsed by kerneldoc I tend to just free-form ioctl
> comments completely. Someday maybe someone even gets around to doing
> proper uabi documentation :-) Just an aside.
> > +
> > +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *file_priv)
> > +{
> > + struct drm_crtc *crtc;
> > + int pipe;
> > + struct drm_crtc_get_sequence *get_seq = data;
> > + struct timespec now;
> > +
>
> You need a DRIVER_MODESET check here or the drm_crtc_find will oops. Same
> below.
>
> > + if (!dev->irq_enabled)
> > + return -EINVAL;
> > +
> > + crtc = drm_crtc_find(dev, get_seq->crtc_id);
> > + if (!crtc)
> > + return -ENOENT;
> > +
> > + pipe = drm_crtc_index(crtc);
> > +
> > + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
>
> This can give you and old vblank if the vblank is off (i.e. sw state
> hasn't be regularly updated). I think we want a new
> drm_crtc_accurate_vblank_count_and_time variant.
Or better yet just do what Chris did for the old ioctl in commit
b33b02707ba3 ("drm: Peek at the current counter/timestamp for vblank queries")
>
> Another thing that is very ill-defined in the old vblank ioctl and that we
> could fix here: Asking for vblanks when the CRTC is off is a bit silly.
> Asking for the sequence when it's off makes some sense, but might still be
> good to give userspace some indication in the new struct? This also from
> the pov of the unpriviledge vblank waiter use-case that I wondered about
> earlier.
>
> > + get_seq->sequence_ns = timespec_to_ns(&now);
> > + return 0;
> > +}
> > +
> > +/*
> > + * Queue a event for VBLANK sequence
> > + *
> > + * \param dev DRM device
> > + * \param data user arguement, pointing to a drm_crtc_queue_sequence structure.
> > + * \param file_priv drm file private for the user's open file descriptor
> > + */
> > +
> > +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *file_priv)
> > +{
> > + struct drm_crtc *crtc;
> > + struct drm_vblank_crtc *vblank;
> > + int pipe;
> > + struct drm_crtc_queue_sequence *queue_seq = data;
> > + struct timespec now;
> > + struct drm_pending_vblank_event *e;
> > + u32 flags;
> > + u64 seq;
> > + u64 req_seq;
> > + int ret;
> > + unsigned long spin_flags;
> > +
> > + if (!dev->irq_enabled)
> > + return -EINVAL;
> > +
> > + crtc = drm_crtc_find(dev, queue_seq->crtc_id);
> > + if (!crtc)
> > + return -ENOENT;
> > +
> > + flags = queue_seq->flags;
> > + /* Check valid flag bits */
> > + if (flags & ~(DRM_CRTC_SEQUENCE_RELATIVE|
> > + DRM_CRTC_SEQUENCE_NEXT_ON_MISS|
> > + DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
> > + return -EINVAL;
> > +
> > + /* Check for valid signal edge */
> > + if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
> > + return -EINVAL;
>
> This seems new, maybe drop it until we need it?
>
> > +
> > + pipe = drm_crtc_index(crtc);
> > +
> > + vblank = &dev->vblank[pipe];
> > +
> > + e = kzalloc(sizeof(*e), GFP_KERNEL);
> > + if (e == NULL)
> > + return -ENOMEM;
> > +
> > + ret = drm_vblank_get(dev, pipe);
>
> drm_crtc_vblank_get pls, would be sweet if we can avoid the (dev, pipe)
> pairs as much as possible. Same for all the others.
>
> > + if (ret) {
> > + DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> > + goto err_free;
> > + }
> > +
> > + seq = drm_vblank_count_and_time(dev, pipe, &now);
>
> I think here there's no need for the accurate version, since we
> force-enable the vblanks already.
>
> > + req_seq = queue_seq->sequence;
> > +
> > + if (flags & DRM_CRTC_SEQUENCE_RELATIVE)
> > + req_seq += seq;
> > +
> > + if ((flags & DRM_CRTC_SEQUENCE_NEXT_ON_MISS) && vblank_passed(seq, req_seq))
> > + req_seq = seq + 1;
> > +
> > + e->pipe = pipe;
> > + e->event.base.type = DRM_EVENT_CRTC_SEQUENCE;
> > + e->event.base.length = sizeof(e->event.seq);
> > + e->event.seq.user_data = queue_seq->user_data;
>
> No need for crtc_id in this event? Or do we expect userspace to encode
> that in the user_data somehow? I don't think it's a real problem since
> we'll only deliver one event per request, so clear for which crtc it is.
> In atomic we might deliver multiple events (one for each crtc) so that's
> why it's needed there.
>
> But might be useful just for consistency.
>
> > +
> > + spin_lock_irqsave(&dev->event_lock, spin_flags);
> > +
> > + /*
> > + * drm_crtc_vblank_off() might have been called after we called
> > + * drm_vblank_get(). drm_crtc_vblank_off() holds event_lock around the
> > + * vblank disable, so no need for further locking. The reference from
> > + * drm_vblank_get() protects against vblank disable from another source.
> > + */
> > + if (!READ_ONCE(vblank->enabled)) {
> > + ret = -EINVAL;
> > + goto err_unlock;
> > + }
> > +
> > + ret = drm_event_reserve_init_locked(dev, file_priv, &e->base,
> > + &e->event.base);
> > +
> > + if (ret)
> > + goto err_unlock;
> > +
> > + e->sequence = req_seq;
> > +
> > + if (vblank_passed(seq, req_seq)) {
> > + drm_vblank_put(dev, pipe);
> > + send_vblank_event(dev, e, seq, &now);
> > + queue_seq->sequence = seq;
> > + } else {
> > + /* drm_handle_vblank_events will call drm_vblank_put */
> > + list_add_tail(&e->base.link, &dev->vblank_event_list);
> > + queue_seq->sequence = req_seq;
> > + }
> > +
> > + spin_unlock_irqrestore(&dev->event_lock, spin_flags);
> > + return 0;
> > +
> > +err_unlock:
> > + spin_unlock_irqrestore(&dev->event_lock, spin_flags);
> > + drm_vblank_put(dev, pipe);
> > +err_free:
> > + kfree(e);
> > + return ret;
> > +}
> > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > index 3ef7ddc5db5f..8a5e1dfe3be7 100644
> > --- a/include/drm/drm_vblank.h
> > +++ b/include/drm/drm_vblank.h
> > @@ -57,6 +57,7 @@ struct drm_pending_vblank_event {
> > union {
> > struct drm_event base;
> > struct drm_event_vblank vbl;
> > + struct drm_event_crtc_sequence seq;
> > } event;
> > };
> >
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 101593ab10ac..dc16d42a88c7 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -718,6 +718,27 @@ struct drm_syncobj_handle {
> > __u32 pad;
> > };
> >
> > +/* Query current scanout sequence number */
> > +struct drm_crtc_get_sequence {
> > + __u32 crtc_id;
> > + __u32 pad;
> > + __u64 sequence;
> > + __u64 sequence_ns;
> > +};
> > +
> > +/* Queue event to be delivered at specified sequence */
> > +
> > +#define DRM_CRTC_SEQUENCE_RELATIVE 0x00000001 /* sequence is relative to current */
> > +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
> > +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
>
> I thought this is already the semantics our current vblank events have (in
> the timestamp, when exactly the event comes out isn't defined further than
> "somewhere around vblank"). Since it's unsed I'd just remove this #define.
>
> > +
> > +struct drm_crtc_queue_sequence {
> > + __u32 crtc_id;
> > + __u32 flags;
> > + __u64 sequence; /* on input, target sequence. on output, actual sequence */
> > + __u64 user_data; /* user data passed to event */
> > +};
>
> In both ioctl handlers pls make sure everything you don't look at is 0,
> including unused stuff like pad. Otherwise userspace might fail to clear
> them, and we can never use them in the future. Maybe just rename pad to
> flags right away.
>
> > +
> > #if defined(__cplusplus)
> > }
> > #endif
> > @@ -800,6 +821,9 @@ extern "C" {
> >
> > #define DRM_IOCTL_WAIT_VBLANK DRM_IOWR(0x3a, union drm_wait_vblank)
> >
> > +#define DRM_IOCTL_CRTC_GET_SEQUENCE DRM_IOWR(0x3b, struct drm_crtc_get_sequence)
> > +#define DRM_IOCTL_CRTC_QUEUE_SEQUENCE DRM_IOWR(0x3c, struct drm_crtc_queue_sequence)
> > +
> > #define DRM_IOCTL_UPDATE_DRAW DRM_IOW(0x3f, struct drm_update_draw)
> >
> > #define DRM_IOCTL_MODE_GETRESOURCES DRM_IOWR(0xA0, struct drm_mode_card_res)
> > @@ -871,6 +895,7 @@ struct drm_event {
> >
> > #define DRM_EVENT_VBLANK 0x01
> > #define DRM_EVENT_FLIP_COMPLETE 0x02
> > +#define DRM_EVENT_CRTC_SEQUENCE 0x03
> >
> > struct drm_event_vblank {
> > struct drm_event base;
> > @@ -881,6 +906,13 @@ struct drm_event_vblank {
> > __u32 crtc_id; /* 0 on older kernels that do not support this */
> > };
> >
> > +struct drm_event_crtc_sequence {
> > + struct drm_event base;
> > + __u64 user_data;
> > + __u64 time_ns;
> > + __u64 sequence;
> > +};
> > +
> > /* typedef area */
> > #ifndef __KERNEL__
> > typedef struct drm_clip_rect drm_clip_rect_t;
> > --
> > 2.11.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel OTC
On Thu, Jul 6, 2017 at 12:16 PM, Ville Syrjälä
<[email protected]> wrote:
>> > + if (!dev->irq_enabled)
>> > + return -EINVAL;
>> > +
>> > + crtc = drm_crtc_find(dev, get_seq->crtc_id);
>> > + if (!crtc)
>> > + return -ENOENT;
>> > +
>> > + pipe = drm_crtc_index(crtc);
>> > +
>> > + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
>>
>> This can give you and old vblank if the vblank is off (i.e. sw state
>> hasn't be regularly updated). I think we want a new
>> drm_crtc_accurate_vblank_count_and_time variant.
>
> Or better yet just do what Chris did for the old ioctl in commit
> b33b02707ba3 ("drm: Peek at the current counter/timestamp for vblank queries")
Yeah the READ_ONCE(vblank->enabled) is a nice fastpath. But we still
need the accurate one as slowpath in case the vblank irq is off.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Jul 06, 2017 at 01:04:18PM +0200, Daniel Vetter wrote:
> On Thu, Jul 6, 2017 at 12:16 PM, Ville Syrj?l?
> <[email protected]> wrote:
> >> > + if (!dev->irq_enabled)
> >> > + return -EINVAL;
> >> > +
> >> > + crtc = drm_crtc_find(dev, get_seq->crtc_id);
> >> > + if (!crtc)
> >> > + return -ENOENT;
> >> > +
> >> > + pipe = drm_crtc_index(crtc);
> >> > +
> >> > + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
> >>
> >> This can give you and old vblank if the vblank is off (i.e. sw state
> >> hasn't be regularly updated). I think we want a new
> >> drm_crtc_accurate_vblank_count_and_time variant.
> >
> > Or better yet just do what Chris did for the old ioctl in commit
> > b33b02707ba3 ("drm: Peek at the current counter/timestamp for vblank queries")
>
> Yeah the READ_ONCE(vblank->enabled) is a nice fastpath. But we still
> need the accurate one as slowpath in case the vblank irq is off.
Maybe, or maybe we want to turn the interrupt on in that case? That's
what the old ioctl does.
--
Ville Syrj?l?
Intel OTC
Daniel Vetter <[email protected]> writes:
> Extending the reported/sw vblank counter to u64 makes sense imo, but do we
> have to extend the driver interfaces too? If there's no 64 bit hw vblank
> currently I think I'd be good to postpone that part, simply because I'm
> too lazy to audit all the drivers for correctly setting max_vblank_count
> after your change :-)
As I said, it's easy enough to do that; I figured I'd do the obvious
part and let you decide if you wanted that or not. We could also
set max_vblank_count to 0xffffffff if it wasn't set by the driver,
and/or add a WARN_ON_ONCE if it wasn't set. Given that it takes over two
years to wrap this counter at 60Hz, we're never likely to hit a bug in
testing.
Let me know what you think; I'm not invested in any particular solution
at this point.
> Other thought on this, since you bother to change all the types: Afaik
> both timespec and timeval suffer from the 32bit issues.
I'm not sure what 32bit issues you're concerned about here? We don't
compare these values, just report them up to user space.
> If we bother with changing everything I think it'd be neat to switch
> all internal interfaces over to ktime, and only convert to the
> userspace types once when we generate the event. I think that's how
> cool hackers are supposed to do it, but not fully sure.
Yeah, I can definitely get behind that plan. A simple 64-bit value
instead of a struct with two semi-related values which are hard to do
arithmetic on.
> Otherwise looks all good, but haven't yet carefully hunted for fumbles in
> review before the above is clear.
Thanks. I'll switch over to ktime and wait to hear what your thoughts
are on the vblank count interface changes.
--
-keith
Michel Dänzer <[email protected]> writes:
> Subtle breakage here: vblwait->request.sequence must still get updated
> for _DRM_VBLANK_RELATIVE, in case we're interrupted by a signal.
Thanks for finding this.
I think it might be better to just not modify the request.type field
instead, so that on re-entry it gets recomputed? That would mean that a
signal might cause the value to be different if the application takes a
long time processing the signal, but I'm not sure that's wrong?
>> @@ -317,6 +317,9 @@ int via_driver_irq_postinstall(struct drm_device *dev)
>> if (!dev_priv)
>> return -EINVAL;
>>
>> + if (dev->driver->get_vblank_counter)
>> + dev->max_vblank_count = 0xffffffff;
>
> What's the purpose of this? All drivers providing get_vblank_counter
> should already initialize max_vblank_count correctly.
Yeah, I couldn't prove that this driver did that, and as Daniel says, we
haven't ever audited the drivers to make sure they do.
We have a check to see that they don't set max_vblank_count if they
don't provide a get function, but I can't find the matching check for
drivers that do provide a function and aren't setting max_vblank_count.
Do you have any thoughts on the wisdom of changing this API before we
have a driver that needs it?
And, of course, thanks for your review!
--
-keith
Michel Dänzer <[email protected]> writes:
> BTW, this got me thinking that we should probably treat
> _DRM_VBLANK_NEXTONMISS the same way, i.e. clear the flag after updating
> vblwait->request.sequence. Otherwise there could theoretically (though
> unlikely) be an infinite loop:
I was thinking that we should just re-compute the target sequence from
scratch and not modify the request at all. But, now I see your point --
if the wait is interrupted long after it starts, then we don't want to
change the target number.
I wonder if anyone actually waits for vblank anymore, or if everyone
just uses the event interface...
> ioctl with _DRM_VBLANK_NEXTONMISS, target missed => wait for next vblank
> wait interrupted by signal
> lather, rinse, repeat
Yeah, sounds like a latent bug.
Ok, to retract my last email, I'll go ahead and fix things up so that
the request sequence gets set to the correct absolute value and that any
flags which modify it get cleared.
> I'd advise against adding a "next on miss" flag for the new ioctl until
> there is specific demand for that.
Thanks for your advice :-)
--
-keith
Daniel Vetter <[email protected]> writes:
> A few nits below, but looks good otherwise.
Thanks.
>> static struct drm_pending_vblank_event *create_vblank_event(
>> - struct drm_device *dev, uint64_t user_data)
>> + struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data)
>
> Nit: Please also drop the dev argument, we have crtc->dev easily
> available. That fits better into my long-term goal of getting rid of the
> (dev, pipe) pairs everywhere in the vblank code and fully switching over
> to drm_crtc *.
As 'dev' isn't used anyways, this seems like a fine plan.
>> + switch (e->event.base.type) {
>> + case DRM_EVENT_VBLANK:
>> + case DRM_EVENT_FLIP_COMPLETE:
>> + if (seq)
>> + e->event.vbl.sequence = (u32) seq;
>> + if (now) {
>> + e->event.vbl.tv_sec = now->tv_sec;
>> + e->event.vbl.tv_usec = now->tv_nsec / 1000;
>> + }
>> + break;
>> + }
>
> Not sure why this change? Also prep for the new, presumably extended
> events? Seems at least slightly inconsistent with other paths, where we
> still unconditionally fill it in.
Yes, this prepares for the new events to make that patch smaller. The
places where the data are still unconditionally assigned should know
that the event in the struct is either a VBLANK or FLIP_COMPLETE.
>> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>
> This'll oops on ums drivers since kms isn't set up.
How about this fix?
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 857b7cf011e1..e39b2bd074e4 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1355,7 +1355,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
union drm_wait_vblank *vblwait,
struct drm_file *file_priv)
{
- struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
struct drm_pending_vblank_event *e;
struct timespec now;
@@ -1373,7 +1372,12 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
e->event.base.type = DRM_EVENT_VBLANK;
e->event.base.length = sizeof(e->event.vbl);
e->event.vbl.user_data = vblwait->request.signal;
- e->event.vbl.crtc_id = crtc ? crtc->base.id : 0;
+ e->event.vbl.crtc_id = 0;
+ if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+ struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
+ if (crtc)
+ e->event.vbl.crtc_id = crtc->base.id;
+ }
spin_lock_irqsave(&dev->event_lock, flags);
> Or maybe I shouldn't have told you this and seized this opportunity to
> break all the old drivers :-)
You now know my evil plan :-)
--
-keith
Daniel Vetter <[email protected]> writes:
> I very much like this since the old ioctl really is a rather bad horror
> show. And since it's tied in with ums drivers everything is
> complicated.
Thanks for your kind words.
> I started a discussion a while back whether these should be restricted to
> DRM_MASTER (i.e. the modeset resource owner) or available to everyone.
> Since it's read-only I guess we can keep it accessible to everyone, but it
> has a bit the problem that client app developers see this, think it does
> what it does and then use it to schedule frames without asking the
> compositor. Which sometimes even works, but isn't really proper design.
> The reasons seems to be that on X11 there's no EGL extension for accurate
> timing frame updates (DRI2/3 can do it ofc, and glx exposes it, but glx is
> uncool or something like that).
In the absence of a suitable EGL api, I'm not sure what to suggest,
other than fixing EGL instead of blaming the kernel...
However, for the leasing stuff, this doesn't really matter as I've got a
master FD to play with, so if you wanted to restrict it to master,
that'd be fine by me.
>> +
>> +/*
>> + * Get crtc VBLANK count.
>> + *
>> + * \param dev DRM device
>> + * \param data user arguement, pointing to a drm_crtc_get_sequence structure.
>> + * \param file_priv drm file private for the user's open file descriptor
>> + */
>
> Since this stuff isn't parsed by kerneldoc I tend to just free-form ioctl
> comments completely. Someday maybe someone even gets around to doing
> proper uabi documentation :-) Just an aside.
I'm just trying to follow along with the local "conventions" in the
file. Let me know if you have a future plan to make this better and I'll
just reformat to suit.
>> +
>> +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>> + struct drm_file *file_priv)
>> +{
>> + struct drm_crtc *crtc;
>> + int pipe;
>> + struct drm_crtc_get_sequence *get_seq = data;
>> + struct timespec now;
>> +
>
> You need a DRIVER_MODESET check here or the drm_crtc_find will oops. Same
> below.
Like this?
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index e39b2bd074e4..3738ff484f36 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1712,6 +1712,9 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
struct drm_crtc_get_sequence *get_seq = data;
struct timespec now;
+ if (!drm_core_check_feature(dev, DRIVER_MODESET))
+ return -EINVAL;
+
if (!dev->irq_enabled)
return -EINVAL;
@@ -1749,6 +1752,9 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
int ret;
unsigned long spin_flags;
+ if (!drm_core_check_feature(dev, DRIVER_MODESET))
+ return -EINVAL;
+
if (!dev->irq_enabled)
return -EINVAL;
>> + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
>
> This can give you and old vblank if the vblank is off (i.e. sw state
> hasn't be regularly updated). I think we want a new
> drm_crtc_accurate_vblank_count_and_time variant.
Right, I saw that code in the wait_vblank case and forgot to carry it
over. Here's a duplicate of what that function does; we'll need this
code in any case for drivers which don't provide the necessary support
for accurate vblank counts:
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1711,6 +1711,8 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
int pipe;
struct drm_crtc_get_sequence *get_seq = data;
struct timespec now;
+ bool vblank_enabled;
+ int ret;
if (!drm_core_check_feature(dev, DRIVER_MODESET))
return -EINVAL;
@@ -1724,8 +1726,19 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
pipe = drm_crtc_index(crtc);
+ vblank_enabled = dev->vblank_disable_immediately && READ_ONCE(vblank->enabled);
+
+ if (!vblank_enabled) {
+ ret = drm_vblank_get(dev, pipe);
+ if (ret) {
+ DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
+ return ret;
+ }
+ }
get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
get_seq->sequence_ns = timespec_to_ns(&now);
+ if (!vblank_enabled)
+ drm_vblank_put(dev, pipe);
return 0;
}
> Another thing that is very ill-defined in the old vblank ioctl and that we
> could fix here: Asking for vblanks when the CRTC is off is a bit silly.
> Asking for the sequence when it's off makes some sense, but might still be
> good to give userspace some indication in the new struct? This also from
> the pov of the unpriviledge vblank waiter use-case that I wondered about
> earlier.
Hrm. It's certainly easy to do, however an application using this
without knowing the enabled state of the crtc is probably not doing the
right thing...
Here's what that looks like, I think, in case we want to do this:
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1735,6 +1735,12 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
return ret;
}
}
+ drm_modeset_lock(&crtc->mutex, NULL);
+ if (crtc->state)
+ get_seq->active = crtc->state->enable;
+ else
+ get_seq->active = crtc->enabled;
+ drm_modeset_unlock(&crtc->mutex);
get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
get_seq->sequence_ns = timespec_to_ns(&now);
if (!vblank_enabled)
>> + /* Check for valid signal edge */
>> + if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
>> + return -EINVAL;
>
> This seems new, maybe drop it until we need it?
It's part of Vulkan, so I want to expose it in the kernel API. And,
making sure user-space isn't setting unexpected bits seems like a good
idea.
> drm_crtc_vblank_get pls, would be sweet if we can avoid the (dev, pipe)
> pairs as much as possible. Same for all the others.
Sure. I'll note that this is just a wrapper around drm_vblank_get/put at
this point :-)
> I think here there's no need for the accurate version, since we
> force-enable the vblanks already.
Agreed.
>> + e->pipe = pipe;
>> + e->event.base.type = DRM_EVENT_CRTC_SEQUENCE;
>> + e->event.base.length = sizeof(e->event.seq);
>> + e->event.seq.user_data = queue_seq->user_data;
>
> No need for crtc_id in this event? Or do we expect userspace to encode
> that in the user_data somehow?
I feared changing the size of the event and so I left that out. And,
yes, userspace will almost certainly encode a pointer in the user_data,
which can include whatever information it needs.
> But might be useful just for consistency.
This would require making the event larger, which seems like a bad idea...
>> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
>
> I thought this is already the semantics our current vblank events have (in
> the timestamp, when exactly the event comes out isn't defined further than
> "somewhere around vblank"). Since it's unsed I'd just remove this
> #define.
Vulkan has this single define, but may have more in the future so I
wanted to make sure the application was able to tell if the kernel
supported new modes if/when they appear. Reliably returning -EINVAL
today when the application asks for something which isn't supported
seems like good practice.
> In both ioctl handlers pls make sure everything you don't look at is 0,
> including unused stuff like pad. Otherwise userspace might fail to clear
> them, and we can never use them in the future. Maybe just rename pad to
> flags right away.
Good idea. Above, you asked me to return whether the crtc was active in
the get_sequence ioctl; I suggested putting that in the existing pad
field, which would leave the whole structure defined.
I've got tiny patches for each piece which I've stuck on my
drm-sequence-64 branch at
git://people.freedesktop.org/~keithp/linux.git drm-sequence-64
Most of those are included above, with the exception of the
drm_crtc_vblank_get/put changes.
Thanks much for your careful review.
--
-keith
Ville Syrjälä <[email protected]> writes:
> Maybe, or maybe we want to turn the interrupt on in that case? That's
> what the old ioctl does.
That's what I suggested in my reply to Daniel's review. Even if we add
the accurate function, we'll still need the interrupt-enable case as a
fallback for drivers which don't support the accurate path, right?
--
-keith
On Thu, Jul 06, 2017 at 09:28:40AM -0700, Keith Packard wrote:
> Ville Syrj?l? <[email protected]> writes:
>
> > Maybe, or maybe we want to turn the interrupt on in that case? That's
> > what the old ioctl does.
>
> That's what I suggested in my reply to Daniel's review. Even if we add
> the accurate function, we'll still need the interrupt-enable case as a
> fallback for drivers which don't support the accurate path, right?
TBH I didn't even consider that case, but yeah makes sense. Otherwise
the counter won't start to tick and the result of the query is pretty
much useless.
I was mostly thinking of the 'seq = query(); wait(seq + n);' pattern
where we can avoid doing the full update more than once if we enable
the interrupt already during the query.
--
Ville Syrj?l?
Intel OTC
Ville Syrjälä <[email protected]> writes:
> I was mostly thinking of the 'seq = query(); wait(seq + n);' pattern
> where we can avoid doing the full update more than once if we enable
> the interrupt already during the query.
Don't we still wait 5 seconds before disabling vblank? In that case, the
chances of hitting an idle vblank are pretty slim if the application is
actually busy.
--
-keith
On Thu, Jul 06, 2017 at 11:22:43AM -0700, Keith Packard wrote:
> Ville Syrj?l? <[email protected]> writes:
>
> > I was mostly thinking of the 'seq = query(); wait(seq + n);' pattern
> > where we can avoid doing the full update more than once if we enable
> > the interrupt already during the query.
>
> Don't we still wait 5 seconds before disabling vblank? In that case, the
> chances of hitting an idle vblank are pretty slim if the application is
> actually busy.
With the disable_immediate thing we only wait until the next vblank
before disabling the irq again.
--
Ville Syrj?l?
Intel OTC
Ville Syrjälä <[email protected]> writes:
> With the disable_immediate thing we only wait until the next vblank
> before disabling the irq again.
Ok, still sounds like we'll be doing fine if the application does a
get immediately followed by a queue event. At least most of the time.
--
-keith
On Thu, Jul 6, 2017 at 6:27 PM, Keith Packard <[email protected]> wrote:
> Daniel Vetter <[email protected]> writes:
>
>> I very much like this since the old ioctl really is a rather bad horror
>> show. And since it's tied in with ums drivers everything is
>> complicated.
>
> Thanks for your kind words.
>
>> I started a discussion a while back whether these should be restricted to
>> DRM_MASTER (i.e. the modeset resource owner) or available to everyone.
>> Since it's read-only I guess we can keep it accessible to everyone, but it
>> has a bit the problem that client app developers see this, think it does
>> what it does and then use it to schedule frames without asking the
>> compositor. Which sometimes even works, but isn't really proper design.
>> The reasons seems to be that on X11 there's no EGL extension for accurate
>> timing frame updates (DRI2/3 can do it ofc, and glx exposes it, but glx is
>> uncool or something like that).
>
> In the absence of a suitable EGL api, I'm not sure what to suggest,
> other than fixing EGL instead of blaming the kernel...
>
> However, for the leasing stuff, this doesn't really matter as I've got a
> master FD to play with, so if you wanted to restrict it to master,
> that'd be fine by me.
Was just a thought, I don't mind either way really I think.
>>> +
>>> +/*
>>> + * Get crtc VBLANK count.
>>> + *
>>> + * \param dev DRM device
>>> + * \param data user arguement, pointing to a drm_crtc_get_sequence structure.
>>> + * \param file_priv drm file private for the user's open file descriptor
>>> + */
>>
>> Since this stuff isn't parsed by kerneldoc I tend to just free-form ioctl
>> comments completely. Someday maybe someone even gets around to doing
>> proper uabi documentation :-) Just an aside.
>
> I'm just trying to follow along with the local "conventions" in the
> file. Let me know if you have a future plan to make this better and I'll
> just reformat to suit.
Yeah that \param stuff is all back from really old libdrm. Just shows
how old this code is :-)
>>> +
>>> +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>>> + struct drm_file *file_priv)
>>> +{
>>> + struct drm_crtc *crtc;
>>> + int pipe;
>>> + struct drm_crtc_get_sequence *get_seq = data;
>>> + struct timespec now;
>>> +
>>
>> You need a DRIVER_MODESET check here or the drm_crtc_find will oops. Same
>> below.
>
> Like this?
Yup.
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index e39b2bd074e4..3738ff484f36 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1712,6 +1712,9 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> struct drm_crtc_get_sequence *get_seq = data;
> struct timespec now;
>
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> if (!dev->irq_enabled)
> return -EINVAL;
>
> @@ -1749,6 +1752,9 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> int ret;
> unsigned long spin_flags;
>
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> if (!dev->irq_enabled)
> return -EINVAL;
>
>
>>> + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
>>
>> This can give you and old vblank if the vblank is off (i.e. sw state
>> hasn't be regularly updated). I think we want a new
>> drm_crtc_accurate_vblank_count_and_time variant.
>
> Right, I saw that code in the wait_vblank case and forgot to carry it
> over. Here's a duplicate of what that function does; we'll need this
> code in any case for drivers which don't provide the necessary support
> for accurate vblank counts:
>
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1711,6 +1711,8 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> int pipe;
> struct drm_crtc_get_sequence *get_seq = data;
> struct timespec now;
> + bool vblank_enabled;
> + int ret;
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return -EINVAL;
> @@ -1724,8 +1726,19 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
>
> pipe = drm_crtc_index(crtc);
>
> + vblank_enabled = dev->vblank_disable_immediately && READ_ONCE(vblank->enabled);
> +
> + if (!vblank_enabled) {
> + ret = drm_vblank_get(dev, pipe);
> + if (ret) {
> + DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> + return ret;
> + }
> + }
> get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
> get_seq->sequence_ns = timespec_to_ns(&now);
> + if (!vblank_enabled)
> + drm_vblank_put(dev, pipe);
> return 0;
> }
Yeah looks good.
>> Another thing that is very ill-defined in the old vblank ioctl and that we
>> could fix here: Asking for vblanks when the CRTC is off is a bit silly.
>> Asking for the sequence when it's off makes some sense, but might still be
>> good to give userspace some indication in the new struct? This also from
>> the pov of the unpriviledge vblank waiter use-case that I wondered about
>> earlier.
>
> Hrm. It's certainly easy to do, however an application using this
> without knowing the enabled state of the crtc is probably not doing the
> right thing...
>
> Here's what that looks like, I think, in case we want to do this:
Yeah the annoying bit is that we have to grab the mutex. I think
better to postpone this (we can always add a flag) and only do it when
there's a real need. Was just an idea that might be useful.
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1735,6 +1735,12 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> return ret;
> }
> }
> + drm_modeset_lock(&crtc->mutex, NULL);
> + if (crtc->state)
> + get_seq->active = crtc->state->enable;
> + else
> + get_seq->active = crtc->enabled;
> + drm_modeset_unlock(&crtc->mutex);
> get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
> get_seq->sequence_ns = timespec_to_ns(&now);
> if (!vblank_enabled)
>
>>> + /* Check for valid signal edge */
>>> + if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
>>> + return -EINVAL;
>>
>> This seems new, maybe drop it until we need it?
>
> It's part of Vulkan, so I want to expose it in the kernel API. And,
> making sure user-space isn't setting unexpected bits seems like a good
> idea.
So the event we generate should be for the very first pixel iirc, or
top of frame or whatever OML_sync says, so it should match vulkan I
think. I just meant we can remove the #define since it's rejected
anyway (we reject all unknown flags), and we can easily add it later
on.
>> drm_crtc_vblank_get pls, would be sweet if we can avoid the (dev, pipe)
>> pairs as much as possible. Same for all the others.
>
> Sure. I'll note that this is just a wrapper around drm_vblank_get/put at
> this point :-)
Yeah I'm not there yet :-)
>> I think here there's no need for the accurate version, since we
>> force-enable the vblanks already.
>
> Agreed.
>
>>> + e->pipe = pipe;
>>> + e->event.base.type = DRM_EVENT_CRTC_SEQUENCE;
>>> + e->event.base.length = sizeof(e->event.seq);
>>> + e->event.seq.user_data = queue_seq->user_data;
>>
>> No need for crtc_id in this event? Or do we expect userspace to encode
>> that in the user_data somehow?
>
> I feared changing the size of the event and so I left that out. And,
> yes, userspace will almost certainly encode a pointer in the user_data,
> which can include whatever information it needs.
I think the size should be a problem, since the old vblank ioctl uses
sizeof(e->event.vbl), extending event.seq shouldn't be a problem. But
we can also postpone this, since iirc we've done the events correctly
and you can extend them at the end.
>> But might be useful just for consistency.
>
> This would require making the event larger, which seems like a bad idea...
>
>>> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
>>
>> I thought this is already the semantics our current vblank events have (in
>> the timestamp, when exactly the event comes out isn't defined further than
>> "somewhere around vblank"). Since it's unsed I'd just remove this
>> #define.
>
> Vulkan has this single define, but may have more in the future so I
> wanted to make sure the application was able to tell if the kernel
> supported new modes if/when they appear. Reliably returning -EINVAL
> today when the application asks for something which isn't supported
> seems like good practice.
As long as we reject any noise in unused bits/members we can extend
them. No need to have an explicit #define to reject a special bit.
>> In both ioctl handlers pls make sure everything you don't look at is 0,
>> including unused stuff like pad. Otherwise userspace might fail to clear
>> them, and we can never use them in the future. Maybe just rename pad to
>> flags right away.
>
> Good idea. Above, you asked me to return whether the crtc was active in
> the get_sequence ioctl; I suggested putting that in the existing pad
> field, which would leave the whole structure defined.
>
> I've got tiny patches for each piece which I've stuck on my
> drm-sequence-64 branch at
>
> git://people.freedesktop.org/~keithp/linux.git drm-sequence-64
>
> Most of those are included above, with the exception of the
> drm_crtc_vblank_get/put changes.
tbh that "is the crtc active" was just an idea, I think if you don't
have an immediate use for it in the vk extension we can leave it for
later.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On 07/07/17 12:04 AM, Keith Packard wrote:
> Michel Dänzer <[email protected]> writes:
>
>>> @@ -317,6 +317,9 @@ int via_driver_irq_postinstall(struct drm_device *dev)
>>> if (!dev_priv)
>>> return -EINVAL;
>>>
>>> + if (dev->driver->get_vblank_counter)
>>> + dev->max_vblank_count = 0xffffffff;
>>
>> What's the purpose of this? All drivers providing get_vblank_counter
>> should already initialize max_vblank_count correctly.
>
> Yeah, I couldn't prove that this driver did that,
Which driver?
> and as Daniel says, we haven't ever audited the drivers to make sure
> they do.
I don't think that's what he meant, rather that with the change above,
all drivers have to be audited to make sure the added assignment doesn't
clobber an earlier assignment by the driver.
> We have a check to see that they don't set max_vblank_count if they
> don't provide a get function, but I can't find the matching check for
> drivers that do provide a function and aren't setting max_vblank_count.
I don't think that's necessary, see drm_update_vblank_count:
if (dev->max_vblank_count != 0) {
/* trust the hw counter when it's around */
diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
} else [...]
The hardware vblank counter is only used for updating the DRM vblank
counter if dev->max_vblank_count != 0.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On 07/07/17 10:34 AM, Michel Dänzer wrote:
> On 07/07/17 12:04 AM, Keith Packard wrote:
>> Michel Dänzer <[email protected]> writes:
>>
>>>> @@ -317,6 +317,9 @@ int via_driver_irq_postinstall(struct drm_device *dev)
>>>> if (!dev_priv)
>>>> return -EINVAL;
>>>>
>>>> + if (dev->driver->get_vblank_counter)
>>>> + dev->max_vblank_count = 0xffffffff;
>>>
>>> What's the purpose of this? All drivers providing get_vblank_counter
>>> should already initialize max_vblank_count correctly.
>>
>> Yeah, I couldn't prove that this driver did that,
>
> Which driver?
>
>> and as Daniel says, we haven't ever audited the drivers to make sure
>> they do.
>
> I don't think that's what he meant, rather that with the change above,
> all drivers have to be audited to make sure the added assignment doesn't
> clobber an earlier assignment by the driver.
... and if there are any drivers that set
dev->driver->get_vblank_counter but don't set dev->max_vblank_count to a
non-0 value, that the hardware counter actually has 32 bits.
I'd say don't bother, just drop this hunk.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On Thu, Jul 06, 2017 at 08:36:00AM -0700, Keith Packard wrote:
> Daniel Vetter <[email protected]> writes:
>
> > A few nits below, but looks good otherwise.
>
> Thanks.
>
> >> static struct drm_pending_vblank_event *create_vblank_event(
> >> - struct drm_device *dev, uint64_t user_data)
> >> + struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data)
> >
> > Nit: Please also drop the dev argument, we have crtc->dev easily
> > available. That fits better into my long-term goal of getting rid of the
> > (dev, pipe) pairs everywhere in the vblank code and fully switching over
> > to drm_crtc *.
>
> As 'dev' isn't used anyways, this seems like a fine plan.
>
> >> + switch (e->event.base.type) {
> >> + case DRM_EVENT_VBLANK:
> >> + case DRM_EVENT_FLIP_COMPLETE:
> >> + if (seq)
> >> + e->event.vbl.sequence = (u32) seq;
> >> + if (now) {
> >> + e->event.vbl.tv_sec = now->tv_sec;
> >> + e->event.vbl.tv_usec = now->tv_nsec / 1000;
> >> + }
> >> + break;
> >> + }
> >
> > Not sure why this change? Also prep for the new, presumably extended
> > events? Seems at least slightly inconsistent with other paths, where we
> > still unconditionally fill it in.
>
> Yes, this prepares for the new events to make that patch smaller. The
> places where the data are still unconditionally assigned should know
> that the event in the struct is either a VBLANK or FLIP_COMPLETE.
Yeah, I realized that after reading the next patch carefully.
> >> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> >
> > This'll oops on ums drivers since kms isn't set up.
>
> How about this fix?
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 857b7cf011e1..e39b2bd074e4 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1355,7 +1355,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
> union drm_wait_vblank *vblwait,
> struct drm_file *file_priv)
> {
> - struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> struct drm_pending_vblank_event *e;
> struct timespec now;
> @@ -1373,7 +1372,12 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
> e->event.base.type = DRM_EVENT_VBLANK;
> e->event.base.length = sizeof(e->event.vbl);
> e->event.vbl.user_data = vblwait->request.signal;
> - e->event.vbl.crtc_id = crtc ? crtc->base.id : 0;
> + e->event.vbl.crtc_id = 0;
> + if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> + if (crtc)
> + e->event.vbl.crtc_id = crtc->base.id;
> + }
>
> spin_lock_irqsave(&dev->event_lock, flags);
lgtm.
> > Or maybe I shouldn't have told you this and seized this opportunity to
> > break all the old drivers :-)
>
> You now know my evil plan :-)
:-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Jul 06, 2017 at 07:59:51AM -0700, Keith Packard wrote:
> Daniel Vetter <[email protected]> writes:
>
> > Extending the reported/sw vblank counter to u64 makes sense imo, but do we
> > have to extend the driver interfaces too? If there's no 64 bit hw vblank
> > currently I think I'd be good to postpone that part, simply because I'm
> > too lazy to audit all the drivers for correctly setting max_vblank_count
> > after your change :-)
>
> As I said, it's easy enough to do that; I figured I'd do the obvious
> part and let you decide if you wanted that or not. We could also
> set max_vblank_count to 0xffffffff if it wasn't set by the driver,
> and/or add a WARN_ON_ONCE if it wasn't set. Given that it takes over two
> years to wrap this counter at 60Hz, we're never likely to hit a bug in
> testing.
>
> Let me know what you think; I'm not invested in any particular solution
> at this point.
Setting a default would be what I suggested if it's possible. But for hw
without a vblank counter (gen2, and apparently every armsoc display ip
under the sun, dunno why), we need to set it to 0, while vblanks are
otherwise fully supported. Given that vblank counters aren't a thing
everywhere, and that it takes them forever to wrap, I don't think hw will
ever gain 64bit vblank counters.
I'd drop that part (but keep 64 everywhere else ofc).
> > Other thought on this, since you bother to change all the types: Afaik
> > both timespec and timeval suffer from the 32bit issues.
>
> I'm not sure what 32bit issues you're concerned about here? We don't
> compare these values, just report them up to user space.
Year 2038 32bit wrap-around bug. Yes I believe/fear drm will still be
around then :-)
> > If we bother with changing everything I think it'd be neat to switch
> > all internal interfaces over to ktime, and only convert to the
> > userspace types once when we generate the event. I think that's how
> > cool hackers are supposed to do it, but not fully sure.
>
> Yeah, I can definitely get behind that plan. A simple 64-bit value
> instead of a struct with two semi-related values which are hard to do
> arithmetic on.
So Arnd said on irc yesterday that one downside of ktime is that you get
to do a 64bit division when talking to old userspace interfaces that still
use the second/nanoseconds split. For super high-perf stuff where you need
to support old userspace interfaces there's a ktime_get_ts64 to optimize
that a bit. Given that we report vblank events on the order of 60fps to
userspace I think we can ignore that.
Arnd also promised to update Documentation/ioctl/botching-up-ioctls.txt.
Anyway, ktime internally, converting to timeval/spec (old events) or s64 ns (new
events) sounds like the the approach to pick here.
> > Otherwise looks all good, but haven't yet carefully hunted for fumbles in
> > review before the above is clear.
>
> Thanks. I'll switch over to ktime and wait to hear what your thoughts
> are on the vblank count interface changes.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Daniel Vetter <[email protected]> writes:
> I'd drop that part (but keep 64 everywhere else ofc).
Yeah, we only ever ask drivers for a delta anyways, so keeping an
internal 64-bit value while retaining the 32-bit driver API is
easy to manage.
>> > Other thought on this, since you bother to change all the types: Afaik
>> > both timespec and timeval suffer from the 32bit issues.
>>
>> I'm not sure what 32bit issues you're concerned about here? We don't
>> compare these values, just report them up to user space.
>
> Year 2038 32bit wrap-around bug. Yes I believe/fear drm will still be
> around then :-)
A fine point. I've switched to ktime_t, which has additional benefits in
making the internal interfaces a bit cleaner with pass-by-value possible
in more cases now.
I've pushed incremental patches out for all of the suggested changes to
my drm-sequence-64 branch. I think I'll send those out as incremental
patches and then also send out a squashed version from the original
branch point.
--
-keith
Place drm_event_vblank in a new union that includes that and a bare
drm_event structure. This will allow new members of that union to be
added in the future without changing code related to the existing vbl
event type.
Assignments to the crtc_id field are now done when the event is
allocated, rather than when delievered. This way, delivery doesn't
need to have the crtc ID available.
v2:
* Remove 'dev' argument from create_vblank_event
It wasn't being used anyways, and if we need it in the future,
we can always get it from crtc->dev.
* Check for MODESETTING before looking for crtc in queue_vblank_event
UMS drivers will oops if we try to get a crtc, so make sure
we're modesetting before we try to find a crtc_id to fill into
the event.
Signed-off-by: Keith Packard <[email protected]>
---
drivers/gpu/drm/drm_atomic.c | 7 ++++---
drivers/gpu/drm/drm_plane.c | 2 +-
drivers/gpu/drm/drm_vblank.c | 30 ++++++++++++++++++------------
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 ++--
include/drm/drm_vblank.h | 8 +++++++-
6 files changed, 34 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c0f336d23f9c..272b83ea9369 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1839,7 +1839,7 @@ int drm_atomic_debugfs_init(struct drm_minor *minor)
*/
static struct drm_pending_vblank_event *create_vblank_event(
- struct drm_device *dev, uint64_t user_data)
+ struct drm_crtc *crtc, uint64_t user_data)
{
struct drm_pending_vblank_event *e = NULL;
@@ -1849,7 +1849,8 @@ static struct drm_pending_vblank_event *create_vblank_event(
e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
e->event.base.length = sizeof(e->event);
- e->event.user_data = user_data;
+ e->event.vbl.crtc_id = crtc->base.id;
+ e->event.vbl.user_data = user_data;
return e;
}
@@ -2052,7 +2053,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
struct drm_pending_vblank_event *e;
- e = create_vblank_event(dev, arg->user_data);
+ e = create_vblank_event(crtc, arg->user_data);
if (!e)
return -ENOMEM;
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 5dc8c4350602..fe9f31285bc2 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -918,7 +918,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
}
e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
e->event.base.length = sizeof(e->event);
- e->event.user_data = page_flip->user_data;
+ e->event.vbl.user_data = page_flip->user_data;
ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
if (ret) {
kfree(e);
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 346601ad698d..7e7119a5ada3 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -804,14 +804,16 @@ static void send_vblank_event(struct drm_device *dev,
{
struct timeval tv;
- tv = ktime_to_timeval(now);
- e->event.sequence = seq;
- e->event.tv_sec = tv.tv_sec;
- e->event.tv_usec = tv.tv_usec;
-
- trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
- e->event.sequence);
-
+ switch (e->event.base.type) {
+ case DRM_EVENT_VBLANK:
+ case DRM_EVENT_FLIP_COMPLETE:
+ tv = ktime_to_timeval(now);
+ e->event.vbl.sequence = seq;
+ e->event.vbl.tv_sec = tv.tv_sec;
+ e->event.vbl.tv_usec = tv.tv_usec;
+ break;
+ }
+ trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
drm_send_event_locked(dev, &e->base);
}
@@ -863,7 +865,6 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
e->pipe = pipe;
e->sequence = drm_vblank_count(dev, pipe);
- e->event.crtc_id = crtc->base.id;
list_add_tail(&e->base.link, &dev->vblank_event_list);
}
EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
@@ -894,7 +895,6 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
now = get_drm_timestamp();
}
e->pipe = pipe;
- e->event.crtc_id = crtc->base.id;
send_vblank_event(dev, e, seq, now);
}
EXPORT_SYMBOL(drm_crtc_send_vblank_event);
@@ -1354,8 +1354,14 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
e->pipe = pipe;
e->event.base.type = DRM_EVENT_VBLANK;
- e->event.base.length = sizeof(e->event);
- e->event.user_data = vblwait->request.signal;
+ e->event.base.length = sizeof(e->event.vbl);
+ e->event.vbl.user_data = vblwait->request.signal;
+ e->event.vbl.crtc_id = 0;
+ if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+ struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
+ if (crtc)
+ e->event.vbl.crtc_id = crtc->base.id;
+ }
spin_lock_irqsave(&dev->event_lock, flags);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 8d7dc9def7c2..c13b97338310 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -358,8 +358,8 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
ret = vmw_event_fence_action_queue(file_priv, fence,
&event->base,
- &event->event.tv_sec,
- &event->event.tv_usec,
+ &event->event.vbl.tv_sec,
+ &event->event.vbl.tv_usec,
true);
}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index bad31bdf09b6..4e329588ce9c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -544,8 +544,8 @@ static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc,
ret = vmw_event_fence_action_queue(file_priv, fence,
&event->base,
- &event->event.tv_sec,
- &event->event.tv_usec,
+ &event->event.vbl.tv_sec,
+ &event->event.vbl.tv_usec,
true);
vmw_fence_obj_unreference(&fence);
} else {
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index e809ab244919..3013c55aec1d 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -54,7 +54,10 @@ struct drm_pending_vblank_event {
/**
* @event: Actual event which will be sent to userspace.
*/
- struct drm_event_vblank event;
+ union {
+ struct drm_event base;
+ struct drm_event_vblank vbl;
+ } event;
};
/**
@@ -163,6 +166,9 @@ 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,
struct drm_pending_vblank_event *e);
+void drm_vblank_set_event(struct drm_pending_vblank_event *e,
+ u64 *seq,
+ ktime_t *now);
bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
int drm_crtc_vblank_get(struct drm_crtc *crtc);
--
2.13.3
These provide crtc-id based functions instead of pipe-number, while
also offering higher resolution time (ns) and wider frame count (64)
as required by the Vulkan API.
v2:
* Check for DRIVER_MODESET in new crtc-based vblank ioctls
Failing to check this will oops the driver.
* Ensure vblank interupt is running in crtc_get_sequence ioctl
The sequence and timing values are not correct while the
interrupt is off, so make sure it's running before asking for
them.
* Short-circuit get_sequence if the counter is enabled and accurate
Steal the idea from the code in wait_vblank to avoid the
expense of drm_vblank_get/put
* Return active state of crtc in crtc_get_sequence ioctl
Might be useful for applications that aren't in charge of
modesetting?
* Use drm_crtc_vblank_get/put in new crtc-based vblank sequence ioctls
Daniel Vetter prefers these over the old drm_vblank_put/get
APIs.
* Return s64 ns instead of u64 in new sequence event
Suggested-by: Daniel Vetter <[email protected]>
Suggested-by: Ville Syrjälä <[email protected]>
Signed-off-by: Keith Packard <[email protected]>
---
drivers/gpu/drm/drm_internal.h | 6 ++
drivers/gpu/drm/drm_ioctl.c | 2 +
drivers/gpu/drm/drm_vblank.c | 173 +++++++++++++++++++++++++++++++++++++++++
include/drm/drm_vblank.h | 1 +
include/uapi/drm/drm.h | 32 ++++++++
5 files changed, 214 insertions(+)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 5cecc974d2f9..b68a193b7907 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -65,6 +65,12 @@ int drm_legacy_irq_control(struct drm_device *dev, void *data,
int drm_legacy_modeset_ctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *filp);
+
+int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *filp);
+
/* drm_auth.c */
int drm_getmagic(struct drm_device *dev, void *data,
struct drm_file *file_priv);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f1e568176da9..63016cf3e224 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
+ DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
};
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 7e7119a5ada3..69b8c92cdd3a 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -812,6 +812,11 @@ static void send_vblank_event(struct drm_device *dev,
e->event.vbl.tv_sec = tv.tv_sec;
e->event.vbl.tv_usec = tv.tv_usec;
break;
+ case DRM_EVENT_CRTC_SEQUENCE:
+ if (seq)
+ e->event.seq.sequence = seq;
+ e->event.seq.time_ns = ktime_to_ns(now);
+ break;
}
trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
drm_send_event_locked(dev, &e->base);
@@ -1682,3 +1687,171 @@ bool drm_crtc_handle_vblank(struct drm_crtc *crtc)
return drm_handle_vblank(crtc->dev, drm_crtc_index(crtc));
}
EXPORT_SYMBOL(drm_crtc_handle_vblank);
+
+/*
+ * Get crtc VBLANK count.
+ *
+ * \param dev DRM device
+ * \param data user arguement, pointing to a drm_crtc_get_sequence structure.
+ * \param file_priv drm file private for the user's open file descriptor
+ */
+
+int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv)
+{
+ struct drm_crtc *crtc;
+ struct drm_vblank_crtc *vblank;
+ int pipe;
+ struct drm_crtc_get_sequence *get_seq = data;
+ ktime_t now;
+ bool vblank_enabled;
+ int ret;
+
+ if (!drm_core_check_feature(dev, DRIVER_MODESET))
+ return -EINVAL;
+
+ if (!dev->irq_enabled)
+ return -EINVAL;
+
+ crtc = drm_crtc_find(dev, get_seq->crtc_id);
+ if (!crtc)
+ return -ENOENT;
+
+ pipe = drm_crtc_index(crtc);
+
+ vblank = &dev->vblank[pipe];
+ vblank_enabled = dev->vblank_disable_immediate && READ_ONCE(vblank->enabled);
+
+ if (!vblank_enabled) {
+ ret = drm_crtc_vblank_get(crtc);
+ if (ret) {
+ DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
+ return ret;
+ }
+ }
+ drm_modeset_lock(&crtc->mutex, NULL);
+ if (crtc->state)
+ get_seq->active = crtc->state->enable;
+ else
+ get_seq->active = crtc->enabled;
+ drm_modeset_unlock(&crtc->mutex);
+ get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
+ get_seq->sequence_ns = ktime_to_ns(now);
+ if (!vblank_enabled)
+ drm_crtc_vblank_put(crtc);
+ return 0;
+}
+
+/*
+ * Queue a event for VBLANK sequence
+ *
+ * \param dev DRM device
+ * \param data user arguement, pointing to a drm_crtc_queue_sequence structure.
+ * \param file_priv drm file private for the user's open file descriptor
+ */
+
+int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv)
+{
+ struct drm_crtc *crtc;
+ struct drm_vblank_crtc *vblank;
+ int pipe;
+ struct drm_crtc_queue_sequence *queue_seq = data;
+ ktime_t now;
+ struct drm_pending_vblank_event *e;
+ u32 flags;
+ u64 seq;
+ u64 req_seq;
+ int ret;
+ unsigned long spin_flags;
+
+ if (!drm_core_check_feature(dev, DRIVER_MODESET))
+ return -EINVAL;
+
+ if (!dev->irq_enabled)
+ return -EINVAL;
+
+ crtc = drm_crtc_find(dev, queue_seq->crtc_id);
+ if (!crtc)
+ return -ENOENT;
+
+ flags = queue_seq->flags;
+ /* Check valid flag bits */
+ if (flags & ~(DRM_CRTC_SEQUENCE_RELATIVE|
+ DRM_CRTC_SEQUENCE_NEXT_ON_MISS|
+ DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
+ return -EINVAL;
+
+ /* Check for valid signal edge */
+ if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
+ return -EINVAL;
+
+ pipe = drm_crtc_index(crtc);
+
+ vblank = &dev->vblank[pipe];
+
+ e = kzalloc(sizeof(*e), GFP_KERNEL);
+ if (e == NULL)
+ return -ENOMEM;
+
+ ret = drm_crtc_vblank_get(crtc);
+ if (ret) {
+ DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
+ goto err_free;
+ }
+
+ seq = drm_vblank_count_and_time(dev, pipe, &now);
+ req_seq = queue_seq->sequence;
+
+ if (flags & DRM_CRTC_SEQUENCE_RELATIVE)
+ req_seq += seq;
+
+ if ((flags & DRM_CRTC_SEQUENCE_NEXT_ON_MISS) && vblank_passed(seq, req_seq))
+ req_seq = seq + 1;
+
+ e->pipe = pipe;
+ e->event.base.type = DRM_EVENT_CRTC_SEQUENCE;
+ e->event.base.length = sizeof(e->event.seq);
+ e->event.seq.user_data = queue_seq->user_data;
+
+ spin_lock_irqsave(&dev->event_lock, spin_flags);
+
+ /*
+ * drm_crtc_vblank_off() might have been called after we called
+ * drm_crtc_vblank_get(). drm_crtc_vblank_off() holds event_lock around the
+ * vblank disable, so no need for further locking. The reference from
+ * drm_crtc_vblank_get() protects against vblank disable from another source.
+ */
+ if (!READ_ONCE(vblank->enabled)) {
+ ret = -EINVAL;
+ goto err_unlock;
+ }
+
+ ret = drm_event_reserve_init_locked(dev, file_priv, &e->base,
+ &e->event.base);
+
+ if (ret)
+ goto err_unlock;
+
+ e->sequence = req_seq;
+
+ if (vblank_passed(seq, req_seq)) {
+ drm_crtc_vblank_put(crtc);
+ send_vblank_event(dev, e, seq, now);
+ queue_seq->sequence = seq;
+ } else {
+ /* drm_handle_vblank_events will call drm_vblank_put */
+ list_add_tail(&e->base.link, &dev->vblank_event_list);
+ queue_seq->sequence = req_seq;
+ }
+
+ spin_unlock_irqrestore(&dev->event_lock, spin_flags);
+ return 0;
+
+err_unlock:
+ spin_unlock_irqrestore(&dev->event_lock, spin_flags);
+ drm_crtc_vblank_put(crtc);
+err_free:
+ kfree(e);
+ return ret;
+}
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 3013c55aec1d..2029313bce89 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -57,6 +57,7 @@ struct drm_pending_vblank_event {
union {
struct drm_event base;
struct drm_event_vblank vbl;
+ struct drm_event_crtc_sequence seq;
} event;
};
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 101593ab10ac..25478560512a 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -718,6 +718,27 @@ struct drm_syncobj_handle {
__u32 pad;
};
+/* Query current scanout sequence number */
+struct drm_crtc_get_sequence {
+ __u32 crtc_id; /* requested crtc_id */
+ __u32 active; /* return: crtc output is active */
+ __u64 sequence; /* return: most recent vblank sequence */
+ __s64 sequence_ns; /* return: most recent vblank time */
+};
+
+/* Queue event to be delivered at specified sequence */
+
+#define DRM_CRTC_SEQUENCE_RELATIVE 0x00000001 /* sequence is relative to current */
+#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
+#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
+
+struct drm_crtc_queue_sequence {
+ __u32 crtc_id;
+ __u32 flags;
+ __u64 sequence; /* on input, target sequence. on output, actual sequence */
+ __u64 user_data; /* user data passed to event */
+};
+
#if defined(__cplusplus)
}
#endif
@@ -800,6 +821,9 @@ extern "C" {
#define DRM_IOCTL_WAIT_VBLANK DRM_IOWR(0x3a, union drm_wait_vblank)
+#define DRM_IOCTL_CRTC_GET_SEQUENCE DRM_IOWR(0x3b, struct drm_crtc_get_sequence)
+#define DRM_IOCTL_CRTC_QUEUE_SEQUENCE DRM_IOWR(0x3c, struct drm_crtc_queue_sequence)
+
#define DRM_IOCTL_UPDATE_DRAW DRM_IOW(0x3f, struct drm_update_draw)
#define DRM_IOCTL_MODE_GETRESOURCES DRM_IOWR(0xA0, struct drm_mode_card_res)
@@ -871,6 +895,7 @@ struct drm_event {
#define DRM_EVENT_VBLANK 0x01
#define DRM_EVENT_FLIP_COMPLETE 0x02
+#define DRM_EVENT_CRTC_SEQUENCE 0x03
struct drm_event_vblank {
struct drm_event base;
@@ -881,6 +906,13 @@ struct drm_event_vblank {
__u32 crtc_id; /* 0 on older kernels that do not support this */
};
+struct drm_event_crtc_sequence {
+ struct drm_event base;
+ __u64 user_data;
+ __s64 time_ns;
+ __u64 sequence;
+};
+
/* typedef area */
#ifndef __KERNEL__
typedef struct drm_clip_rect drm_clip_rect_t;
--
2.13.3
Here's an updated series for the proposed new IOCTLs. Major changes
since last time:
* Leave driver API with 32-bit vblank counts
* Use ktime_t instead of struct timespec.
* Check for MODESETTING before using modesetting APIs
* Ensure vblank is running in new get_sequence ioctl
There are other minor changes noted in each patch.
Thanks to helpful review from:
Daniel Vetter <[email protected]>
Michel Dänzer <[email protected]>
Ville Syrjälä <[email protected]>
-keith
This modifies the datatypes used by the vblank code to provide both 64
bits of vblank count and switch to using ktime_t for timestamps to
increase resolution from microseconds to nanoseconds.
The driver interfaces have been left using 32 bits of vblank count;
all of the code necessary to widen that value for the user API was
already included to handle devices returning fewer than 32-bits.
This will provide the necessary datatypes for the Vulkan API.
v2:
* Re-write wait_vblank ioctl to ABSOLUTE sequence
When an application uses the WAIT_VBLANK ioctl with RELATIVE
or NEXTONMISS bits set, the target vblank interval is updated
within the kernel. We need to write that target back to the
ioctl buffer and update the flags bits so that if the wait is
interrupted by a signal, when it is re-started, it will target
precisely the same vblank count as before.
* Leave driver API with 32-bit vblank count
Suggested-by: Michel Dänzer <[email protected]>
Suggested-by: Daniel Vetter <[email protected]>
Signed-off-by: Keith Packard <[email protected]>
---
drivers/gpu/drm/drm_vblank.c | 186 +++++++++++++++++++++++++------------------
include/drm/drmP.h | 2 +-
include/drm/drm_drv.h | 2 +-
include/drm/drm_vblank.h | 16 ++--
4 files changed, 120 insertions(+), 86 deletions(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 463e4d81fb0d..346601ad698d 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -43,7 +43,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. */
@@ -64,7 +64,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];
@@ -73,7 +73,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);
}
@@ -116,7 +116,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);
@@ -136,13 +136,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);
}
@@ -165,7 +165,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;
@@ -190,11 +190,9 @@ 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;
+ ktime_t diff_ns;
- t_old = &vblank->time;
- diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
+ diff_ns = t_vblank - vblank->time;
/*
* Figure out how many vblanks we've missed based
@@ -228,7 +226,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
}
DRM_DEBUG_VBL("updating vblank count on crtc %u:"
- " current=%u, diff=%u, hw=%u hw_last=%u\n",
+ " current=%llu, diff=%u, hw=%u hw_last=%u\n",
pipe, vblank->count, diff, cur_vblank, vblank->last);
if (diff == 0) {
@@ -243,9 +241,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)
@@ -567,10 +565,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;
+ ktime_t prev_etime;
ktime_t stime, etime;
bool vbl_status;
struct drm_crtc *crtc;
@@ -663,29 +661,26 @@ 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);
+ prev_etime = etime;
/* 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 -> %lld [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,
+ (long long) prev_etime,
+ (long long) etime,
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();
}
/**
@@ -711,7 +706,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;
@@ -743,7 +738,7 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
* Returns:
* The software vblank counter.
*/
-u32 drm_crtc_vblank_count(struct drm_crtc *crtc)
+u64 drm_crtc_vblank_count(struct drm_crtc *crtc)
{
return drm_vblank_count(crtc->dev, drm_crtc_index(crtc));
}
@@ -763,15 +758,15 @@ EXPORT_SYMBOL(drm_crtc_vblank_count);
*
* This is the legacy version of drm_crtc_vblank_count_and_time().
*/
-static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
- struct timeval *vblanktime)
+static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
+ ktime_t *vblanktime)
{
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
- u32 vblank_count;
+ u64 vblank_count;
unsigned int seq;
if (WARN_ON(pipe >= dev->num_crtcs)) {
- *vblanktime = (struct timeval) { 0 };
+ *vblanktime = 0;
return 0;
}
@@ -795,8 +790,8 @@ static u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
* modesetting activity. Returns corresponding system timestamp of the time
* 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)
+u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
+ ktime_t *vblanktime)
{
return drm_vblank_count_and_time(crtc->dev, drm_crtc_index(crtc),
vblanktime);
@@ -805,11 +800,14 @@ 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)
+ u64 seq, ktime_t now)
{
+ struct timeval tv;
+
+ tv = ktime_to_timeval(now);
e->event.sequence = seq;
- e->event.tv_sec = now->tv_sec;
- e->event.tv_usec = now->tv_usec;
+ e->event.tv_sec = tv.tv_sec;
+ e->event.tv_usec = tv.tv_usec;
trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
e->event.sequence);
@@ -864,7 +862,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
assert_spin_locked(&dev->event_lock);
e->pipe = pipe;
- e->event.sequence = drm_vblank_count(dev, pipe);
+ e->sequence = drm_vblank_count(dev, pipe);
e->event.crtc_id = crtc->base.id;
list_add_tail(&e->base.link, &dev->vblank_event_list);
}
@@ -885,19 +883,19 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
struct drm_pending_vblank_event *e)
{
struct drm_device *dev = crtc->dev;
- unsigned int seq, pipe = drm_crtc_index(crtc);
- struct timeval now;
+ u64 seq;
+ unsigned int pipe = drm_crtc_index(crtc);
+ ktime_t now;
if (dev->num_crtcs > 0) {
seq = drm_vblank_count_and_time(dev, pipe, &now);
} else {
seq = 0;
-
now = get_drm_timestamp();
}
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);
@@ -1124,9 +1122,9 @@ 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;
+ u64 seq;
if (WARN_ON(pipe >= dev->num_crtcs))
return;
@@ -1161,11 +1159,11 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
if (e->pipe != pipe)
continue;
DRM_DEBUG("Sending premature vblank event on disable: "
- "wanted %u, current %u\n",
- e->event.sequence, seq);
+ "wanted %llu current %llu\n",
+ e->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);
@@ -1331,20 +1329,21 @@ int drm_legacy_modeset_ctl(struct drm_device *dev, void *data,
return 0;
}
-static inline bool vblank_passed(u32 seq, u32 ref)
+static inline bool vblank_passed(u64 seq, u64 ref)
{
return (seq - ref) <= (1 << 23);
}
static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
+ u64 req_seq,
union drm_wait_vblank *vblwait,
struct drm_file *file_priv)
{
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;
+ u64 seq;
int ret;
e = kzalloc(sizeof(*e), GFP_KERNEL);
@@ -1379,21 +1378,20 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
seq = drm_vblank_count_and_time(dev, pipe, &now);
- DRM_DEBUG("event on vblank count %u, current %u, crtc %u\n",
- vblwait->request.sequence, seq, pipe);
+ DRM_DEBUG("event on vblank count %llu, current %llu, crtc %u\n",
+ req_seq, seq, pipe);
- trace_drm_vblank_event_queued(file_priv, pipe,
- vblwait->request.sequence);
+ trace_drm_vblank_event_queued(file_priv, pipe, req_seq);
- e->event.sequence = vblwait->request.sequence;
- if (vblank_passed(seq, vblwait->request.sequence)) {
+ e->sequence = req_seq;
+ if (vblank_passed(seq, req_seq)) {
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 */
list_add_tail(&e->base.link, &dev->vblank_event_list);
- vblwait->reply.sequence = vblwait->request.sequence;
+ vblwait->reply.sequence = req_seq;
}
spin_unlock_irqrestore(&dev->event_lock, flags);
@@ -1420,6 +1418,27 @@ static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
}
/*
+ * Widen a 32-bit param to 64-bits.
+ *
+ * \param narrow 32-bit value (missing upper 32 bits)
+ * \param near 64-bit value that should be 'close' to near
+ *
+ * This function returns a 64-bit value using the lower 32-bits from
+ * 'narrow' and constructing the upper 32-bits so that the result is
+ * as close as possible to 'near'.
+ */
+
+static u64 widen_32_to_64(u32 narrow, u64 near)
+{
+ u64 wide = narrow | (near & 0xffffffff00000000ULL);
+ if ((int64_t) (wide - near) > 0x80000000LL)
+ wide -= 0x100000000ULL;
+ else if ((int64_t) (near - wide) > 0x80000000LL)
+ wide += 0x100000000ULL;
+ return wide;
+}
+
+/*
* Wait for VBLANK.
*
* \param inode device inode.
@@ -1439,6 +1458,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
struct drm_vblank_crtc *vblank;
union drm_wait_vblank *vblwait = data;
int ret;
+ u64 req_seq;
unsigned int flags, seq, pipe, high_pipe;
if (!dev->irq_enabled)
@@ -1474,12 +1494,14 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
if (dev->vblank_disable_immediate &&
drm_wait_vblank_is_query(vblwait) &&
READ_ONCE(vblank->enabled)) {
- struct timeval now;
+ ktime_t now;
+ struct timeval tv;
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;
+ tv = ktime_to_timeval(now);
+ vblwait->reply.tval_sec = tv.tv_sec;
+ vblwait->reply.tval_usec = tv.tv_usec;
return 0;
}
@@ -1492,9 +1514,12 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
case _DRM_VBLANK_RELATIVE:
- vblwait->request.sequence += seq;
+ req_seq = seq + vblwait->request.sequence;
vblwait->request.type &= ~_DRM_VBLANK_RELATIVE;
+ vblwait->request.sequence = req_seq;
+ break;
case _DRM_VBLANK_ABSOLUTE:
+ req_seq = widen_32_to_64(vblwait->request.sequence, seq);
break;
default:
ret = -EINVAL;
@@ -1502,31 +1527,36 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
}
if ((flags & _DRM_VBLANK_NEXTONMISS) &&
- vblank_passed(seq, vblwait->request.sequence))
- vblwait->request.sequence = seq + 1;
+ vblank_passed(seq, req_seq)) {
+ req_seq = seq + 1;
+ vblwait->request.type &= ~_DRM_VBLANK_NEXTONMISS;
+ vblwait->request.sequence = req_seq;
+ }
if (flags & _DRM_VBLANK_EVENT) {
/* must hold on to the vblank ref until the event fires
* drm_vblank_put will be called asynchronously
*/
- return drm_queue_vblank_event(dev, pipe, vblwait, file_priv);
+ return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, file_priv);
}
- if (vblwait->request.sequence != seq) {
- DRM_DEBUG("waiting on vblank count %u, crtc %u\n",
- vblwait->request.sequence, pipe);
+ if (req_seq != seq) {
+ DRM_DEBUG("waiting on vblank count %llu, crtc %u\n",
+ req_seq, pipe);
DRM_WAIT_ON(ret, vblank->queue, 3 * HZ,
vblank_passed(drm_vblank_count(dev, pipe),
- vblwait->request.sequence) ||
+ req_seq) ||
!READ_ONCE(vblank->enabled));
}
if (ret != -EINTR) {
- struct timeval now;
+ ktime_t now;
+ struct timeval tv;
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;
+ tv = ktime_to_timeval(now);
+ vblwait->reply.tval_sec = tv.tv_sec;
+ vblwait->reply.tval_usec = tv.tv_usec;
DRM_DEBUG("crtc %d returning %u to client\n",
pipe, vblwait->reply.sequence);
@@ -1542,8 +1572,8 @@ int drm_wait_vblank(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;
- unsigned int seq;
+ ktime_t now;
+ u64 seq;
assert_spin_locked(&dev->event_lock);
@@ -1552,15 +1582,15 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
if (e->pipe != pipe)
continue;
- if (!vblank_passed(seq, e->event.sequence))
+ if (!vblank_passed(seq, e->sequence))
continue;
- DRM_DEBUG("vblank event on %u, current %u\n",
- e->event.sequence, seq);
+ DRM_DEBUG("vblank event on %llu, current %llu\n",
+ e->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);
}
trace_drm_vblank_event(pipe, seq);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 39df16af7a4a..e50cf152f565 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -403,7 +403,7 @@ struct drm_device {
spinlock_t vblank_time_lock; /**< Protects vblank count and time updates during vblank enable/disable */
spinlock_t vbl_lock;
- u32 max_vblank_count; /**< size of vblank counter register */
+ u64 max_vblank_count; /**< size of vblank counter register */
/**
* List of events
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index d855f9ae41a8..2e4e425b5fba 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -325,7 +325,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 4cde47332dfa..e809ab244919 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -48,6 +48,10 @@ struct drm_pending_vblank_event {
*/
unsigned int pipe;
/**
+ * @sequence: frame event should be triggered at
+ */
+ u64 sequence;
+ /**
* @event: Actual event which will be sent to userspace.
*/
struct drm_event_vblank event;
@@ -88,11 +92,11 @@ struct drm_vblank_crtc {
/**
* @count: Current software vblank counter.
*/
- u32 count;
+ u64 count;
/**
* @time: Vblank timestamp corresponding to @count.
*/
- struct timeval time;
+ ktime_t time;
/**
* @refcount: Number of users/waiters of the vblank interrupt. Only when
@@ -152,9 +156,9 @@ 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);
+u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
+u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
+ 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,
@@ -173,7 +177,7 @@ u32 drm_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.13.3
On Mon, Jul 31, 2017 at 10:03:04PM -0700, Keith Packard wrote:
> This modifies the datatypes used by the vblank code to provide both 64
> bits of vblank count and switch to using ktime_t for timestamps to
> increase resolution from microseconds to nanoseconds.
>
> The driver interfaces have been left using 32 bits of vblank count;
> all of the code necessary to widen that value for the user API was
> already included to handle devices returning fewer than 32-bits.
>
> This will provide the necessary datatypes for the Vulkan API.
>
> v2:
>
> * Re-write wait_vblank ioctl to ABSOLUTE sequence
>
> When an application uses the WAIT_VBLANK ioctl with RELATIVE
> or NEXTONMISS bits set, the target vblank interval is updated
> within the kernel. We need to write that target back to the
> ioctl buffer and update the flags bits so that if the wait is
> interrupted by a signal, when it is re-started, it will target
> precisely the same vblank count as before.
>
> * Leave driver API with 32-bit vblank count
>
> Suggested-by: Michel D?nzer <[email protected]>
> Suggested-by: Daniel Vetter <[email protected]>
> Signed-off-by: Keith Packard <[email protected]>
Subject is a bit confusing since you say uapi, but this is just the
internal prep work. Dropping UAPI fixes that. With that fixed:
Reviewed-by: Daniel Vetter <[email protected]>
Two more optional comments below, feel free to adapt or ignore. I'll wait
for Michel's r-b before merging either way.
> static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
> + u64 req_seq,
> union drm_wait_vblank *vblwait,
Minor bikeshed: Since you pass the requested vblank number explicit, mabye
also pass the user_data explicit and remove the vblwait struct from the
parameter list? Restricts the old uapi cruft a bit.
> /*
> + * Widen a 32-bit param to 64-bits.
> + *
> + * \param narrow 32-bit value (missing upper 32 bits)
> + * \param near 64-bit value that should be 'close' to near
> + *
> + * This function returns a 64-bit value using the lower 32-bits from
> + * 'narrow' and constructing the upper 32-bits so that the result is
> + * as close as possible to 'near'.
> + */
> +
> +static u64 widen_32_to_64(u32 narrow, u64 near)
> +{
> + u64 wide = narrow | (near & 0xffffffff00000000ULL);
> + if ((int64_t) (wide - near) > 0x80000000LL)
> + wide -= 0x100000000ULL;
> + else if ((int64_t) (near - wide) > 0x80000000LL)
> + wide += 0x100000000ULL;
> + return wide;
return near + (int32_s) ((uint32_t)wide - near) ?
But then it took me way too long to think about this one, so maybe leave
it at that.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, Jul 31, 2017 at 10:03:05PM -0700, Keith Packard wrote:
> Place drm_event_vblank in a new union that includes that and a bare
> drm_event structure. This will allow new members of that union to be
> added in the future without changing code related to the existing vbl
> event type.
>
> Assignments to the crtc_id field are now done when the event is
> allocated, rather than when delievered. This way, delivery doesn't
> need to have the crtc ID available.
>
> v2:
> * Remove 'dev' argument from create_vblank_event
>
> It wasn't being used anyways, and if we need it in the future,
> we can always get it from crtc->dev.
>
> * Check for MODESETTING before looking for crtc in queue_vblank_event
>
> UMS drivers will oops if we try to get a crtc, so make sure
> we're modesetting before we try to find a crtc_id to fill into
> the event.
>
> Signed-off-by: Keith Packard <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic.c | 7 ++++---
> drivers/gpu/drm/drm_plane.c | 2 +-
> drivers/gpu/drm/drm_vblank.c | 30 ++++++++++++++++++------------
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 ++--
> include/drm/drm_vblank.h | 8 +++++++-
> 6 files changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c0f336d23f9c..272b83ea9369 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1839,7 +1839,7 @@ int drm_atomic_debugfs_init(struct drm_minor *minor)
> */
>
> static struct drm_pending_vblank_event *create_vblank_event(
> - struct drm_device *dev, uint64_t user_data)
> + struct drm_crtc *crtc, uint64_t user_data)
> {
> struct drm_pending_vblank_event *e = NULL;
>
> @@ -1849,7 +1849,8 @@ static struct drm_pending_vblank_event *create_vblank_event(
>
> e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
> e->event.base.length = sizeof(e->event);
> - e->event.user_data = user_data;
> + e->event.vbl.crtc_id = crtc->base.id;
> + e->event.vbl.user_data = user_data;
>
> return e;
> }
> @@ -2052,7 +2053,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
> if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
> struct drm_pending_vblank_event *e;
>
> - e = create_vblank_event(dev, arg->user_data);
> + e = create_vblank_event(crtc, arg->user_data);
> if (!e)
> return -ENOMEM;
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 5dc8c4350602..fe9f31285bc2 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -918,7 +918,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> }
> e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
> e->event.base.length = sizeof(e->event);
> - e->event.user_data = page_flip->user_data;
You missed assigning crtc_id here. With that fixes:
Reviewed-by: Daniel Vetter <[email protected]>
Might also be good to check igt coverage for the various corner-cases
here.
> + e->event.vbl.user_data = page_flip->user_data;
> ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
> if (ret) {
> kfree(e);
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 346601ad698d..7e7119a5ada3 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -804,14 +804,16 @@ static void send_vblank_event(struct drm_device *dev,
> {
> struct timeval tv;
>
> - tv = ktime_to_timeval(now);
> - e->event.sequence = seq;
> - e->event.tv_sec = tv.tv_sec;
> - e->event.tv_usec = tv.tv_usec;
> -
> - trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
> - e->event.sequence);
> -
> + switch (e->event.base.type) {
> + case DRM_EVENT_VBLANK:
> + case DRM_EVENT_FLIP_COMPLETE:
> + tv = ktime_to_timeval(now);
> + e->event.vbl.sequence = seq;
> + e->event.vbl.tv_sec = tv.tv_sec;
> + e->event.vbl.tv_usec = tv.tv_usec;
> + break;
> + }
> + trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
> drm_send_event_locked(dev, &e->base);
> }
>
> @@ -863,7 +865,6 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>
> e->pipe = pipe;
> e->sequence = drm_vblank_count(dev, pipe);
> - e->event.crtc_id = crtc->base.id;
> list_add_tail(&e->base.link, &dev->vblank_event_list);
> }
> EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
> @@ -894,7 +895,6 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> now = get_drm_timestamp();
> }
> e->pipe = pipe;
> - e->event.crtc_id = crtc->base.id;
> send_vblank_event(dev, e, seq, now);
> }
> EXPORT_SYMBOL(drm_crtc_send_vblank_event);
> @@ -1354,8 +1354,14 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>
> e->pipe = pipe;
> e->event.base.type = DRM_EVENT_VBLANK;
> - e->event.base.length = sizeof(e->event);
> - e->event.user_data = vblwait->request.signal;
> + e->event.base.length = sizeof(e->event.vbl);
> + e->event.vbl.user_data = vblwait->request.signal;
> + e->event.vbl.crtc_id = 0;
> + if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> + if (crtc)
> + e->event.vbl.crtc_id = crtc->base.id;
> + }
>
> spin_lock_irqsave(&dev->event_lock, flags);
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index 8d7dc9def7c2..c13b97338310 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -358,8 +358,8 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
>
> ret = vmw_event_fence_action_queue(file_priv, fence,
> &event->base,
> - &event->event.tv_sec,
> - &event->event.tv_usec,
> + &event->event.vbl.tv_sec,
> + &event->event.vbl.tv_usec,
> true);
> }
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index bad31bdf09b6..4e329588ce9c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -544,8 +544,8 @@ static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc,
>
> ret = vmw_event_fence_action_queue(file_priv, fence,
> &event->base,
> - &event->event.tv_sec,
> - &event->event.tv_usec,
> + &event->event.vbl.tv_sec,
> + &event->event.vbl.tv_usec,
> true);
> vmw_fence_obj_unreference(&fence);
> } else {
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index e809ab244919..3013c55aec1d 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -54,7 +54,10 @@ struct drm_pending_vblank_event {
> /**
> * @event: Actual event which will be sent to userspace.
> */
> - struct drm_event_vblank event;
> + union {
> + struct drm_event base;
> + struct drm_event_vblank vbl;
> + } event;
> };
>
> /**
> @@ -163,6 +166,9 @@ 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,
> struct drm_pending_vblank_event *e);
> +void drm_vblank_set_event(struct drm_pending_vblank_event *e,
> + u64 *seq,
> + ktime_t *now);
> bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
> bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
> int drm_crtc_vblank_get(struct drm_crtc *crtc);
> --
> 2.13.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, Jul 31, 2017 at 10:03:06PM -0700, Keith Packard wrote:
> These provide crtc-id based functions instead of pipe-number, while
> also offering higher resolution time (ns) and wider frame count (64)
> as required by the Vulkan API.
>
> v2:
>
> * Check for DRIVER_MODESET in new crtc-based vblank ioctls
>
> Failing to check this will oops the driver.
>
> * Ensure vblank interupt is running in crtc_get_sequence ioctl
>
> The sequence and timing values are not correct while the
> interrupt is off, so make sure it's running before asking for
> them.
>
> * Short-circuit get_sequence if the counter is enabled and accurate
>
> Steal the idea from the code in wait_vblank to avoid the
> expense of drm_vblank_get/put
>
> * Return active state of crtc in crtc_get_sequence ioctl
>
> Might be useful for applications that aren't in charge of
> modesetting?
>
> * Use drm_crtc_vblank_get/put in new crtc-based vblank sequence ioctls
>
> Daniel Vetter prefers these over the old drm_vblank_put/get
> APIs.
>
> * Return s64 ns instead of u64 in new sequence event
>
> Suggested-by: Daniel Vetter <[email protected]>
> Suggested-by: Ville Syrj?l? <[email protected]>
> Signed-off-by: Keith Packard <[email protected]>
Since I missed all the details Michel spotted, so I'll defer to his r-b.
Also, before merging we need the userspace user. Do we have e.g.
-modesetting patch for this, fully reviewed&ready for merging, just as
demonstration? This way we could land this before the lease stuff for the
vk extension is all solid&ready.
A few minor things below.
-Daniel
> ---
> drivers/gpu/drm/drm_internal.h | 6 ++
> drivers/gpu/drm/drm_ioctl.c | 2 +
> drivers/gpu/drm/drm_vblank.c | 173 +++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_vblank.h | 1 +
> include/uapi/drm/drm.h | 32 ++++++++
> 5 files changed, 214 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 5cecc974d2f9..b68a193b7907 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -65,6 +65,12 @@ int drm_legacy_irq_control(struct drm_device *dev, void *data,
> int drm_legacy_modeset_ctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
>
> +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *filp);
> +
> +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *filp);
> +
> /* drm_auth.c */
> int drm_getmagic(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index f1e568176da9..63016cf3e224 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
> + DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
> };
>
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 7e7119a5ada3..69b8c92cdd3a 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -812,6 +812,11 @@ static void send_vblank_event(struct drm_device *dev,
> e->event.vbl.tv_sec = tv.tv_sec;
> e->event.vbl.tv_usec = tv.tv_usec;
> break;
> + case DRM_EVENT_CRTC_SEQUENCE:
> + if (seq)
> + e->event.seq.sequence = seq;
> + e->event.seq.time_ns = ktime_to_ns(now);
> + break;
> }
> trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
> drm_send_event_locked(dev, &e->base);
> @@ -1682,3 +1687,171 @@ bool drm_crtc_handle_vblank(struct drm_crtc *crtc)
> return drm_handle_vblank(crtc->dev, drm_crtc_index(crtc));
> }
> EXPORT_SYMBOL(drm_crtc_handle_vblank);
> +
> +/*
> + * Get crtc VBLANK count.
> + *
> + * \param dev DRM device
> + * \param data user arguement, pointing to a drm_crtc_get_sequence structure.
> + * \param file_priv drm file private for the user's open file descriptor
> + */
> +
> +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_crtc *crtc;
> + struct drm_vblank_crtc *vblank;
> + int pipe;
> + struct drm_crtc_get_sequence *get_seq = data;
> + ktime_t now;
> + bool vblank_enabled;
> + int ret;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> + if (!dev->irq_enabled)
> + return -EINVAL;
> +
> + crtc = drm_crtc_find(dev, get_seq->crtc_id);
> + if (!crtc)
> + return -ENOENT;
> +
> + pipe = drm_crtc_index(crtc);
> +
> + vblank = &dev->vblank[pipe];
> + vblank_enabled = dev->vblank_disable_immediate && READ_ONCE(vblank->enabled);
> +
> + if (!vblank_enabled) {
> + ret = drm_crtc_vblank_get(crtc);
> + if (ret) {
> + DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> + return ret;
> + }
> + }
> + drm_modeset_lock(&crtc->mutex, NULL);
> + if (crtc->state)
> + get_seq->active = crtc->state->enable;
> + else
> + get_seq->active = crtc->enabled;
> + drm_modeset_unlock(&crtc->mutex);
This is really heavywheight, given the lockless dance we attempt above.
Also, when the crtc is off the vblank_get will fail, so you never get
here. I guess my idea wasn't all that useful and well-thought out, or we
need to be a bit more clever about this. To fix this we need to continue
even when vblank_get fails (but only call vblank_put if ret == 0 ofc). And
to avoid the locking you can use READ_ONCE(vblank->enabled) instead.
> + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
> + get_seq->sequence_ns = ktime_to_ns(now);
> + if (!vblank_enabled)
> + drm_crtc_vblank_put(crtc);
> + return 0;
> +}
> +
> +/*
> + * Queue a event for VBLANK sequence
> + *
> + * \param dev DRM device
> + * \param data user arguement, pointing to a drm_crtc_queue_sequence structure.
> + * \param file_priv drm file private for the user's open file descriptor
> + */
> +
> +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_crtc *crtc;
> + struct drm_vblank_crtc *vblank;
> + int pipe;
> + struct drm_crtc_queue_sequence *queue_seq = data;
> + ktime_t now;
> + struct drm_pending_vblank_event *e;
> + u32 flags;
> + u64 seq;
> + u64 req_seq;
> + int ret;
> + unsigned long spin_flags;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> + if (!dev->irq_enabled)
> + return -EINVAL;
> +
> + crtc = drm_crtc_find(dev, queue_seq->crtc_id);
> + if (!crtc)
> + return -ENOENT;
> +
> + flags = queue_seq->flags;
> + /* Check valid flag bits */
> + if (flags & ~(DRM_CRTC_SEQUENCE_RELATIVE|
> + DRM_CRTC_SEQUENCE_NEXT_ON_MISS|
> + DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
> + return -EINVAL;
> +
> + /* Check for valid signal edge */
> + if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
> + return -EINVAL;
> +
> + pipe = drm_crtc_index(crtc);
> +
> + vblank = &dev->vblank[pipe];
> +
> + e = kzalloc(sizeof(*e), GFP_KERNEL);
> + if (e == NULL)
> + return -ENOMEM;
> +
> + ret = drm_crtc_vblank_get(crtc);
> + if (ret) {
> + DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> + goto err_free;
> + }
> +
> + seq = drm_vblank_count_and_time(dev, pipe, &now);
> + req_seq = queue_seq->sequence;
> +
> + if (flags & DRM_CRTC_SEQUENCE_RELATIVE)
> + req_seq += seq;
> +
> + if ((flags & DRM_CRTC_SEQUENCE_NEXT_ON_MISS) && vblank_passed(seq, req_seq))
> + req_seq = seq + 1;
> +
> + e->pipe = pipe;
> + e->event.base.type = DRM_EVENT_CRTC_SEQUENCE;
> + e->event.base.length = sizeof(e->event.seq);
> + e->event.seq.user_data = queue_seq->user_data;
> +
> + spin_lock_irqsave(&dev->event_lock, spin_flags);
> +
> + /*
> + * drm_crtc_vblank_off() might have been called after we called
> + * drm_crtc_vblank_get(). drm_crtc_vblank_off() holds event_lock around the
> + * vblank disable, so no need for further locking. The reference from
> + * drm_crtc_vblank_get() protects against vblank disable from another source.
> + */
> + if (!READ_ONCE(vblank->enabled)) {
> + ret = -EINVAL;
> + goto err_unlock;
> + }
> +
> + ret = drm_event_reserve_init_locked(dev, file_priv, &e->base,
> + &e->event.base);
> +
> + if (ret)
> + goto err_unlock;
> +
> + e->sequence = req_seq;
> +
> + if (vblank_passed(seq, req_seq)) {
> + drm_crtc_vblank_put(crtc);
> + send_vblank_event(dev, e, seq, now);
> + queue_seq->sequence = seq;
> + } else {
> + /* drm_handle_vblank_events will call drm_vblank_put */
> + list_add_tail(&e->base.link, &dev->vblank_event_list);
> + queue_seq->sequence = req_seq;
> + }
> +
> + spin_unlock_irqrestore(&dev->event_lock, spin_flags);
> + return 0;
> +
> +err_unlock:
> + spin_unlock_irqrestore(&dev->event_lock, spin_flags);
> + drm_crtc_vblank_put(crtc);
> +err_free:
> + kfree(e);
> + return ret;
> +}
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 3013c55aec1d..2029313bce89 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -57,6 +57,7 @@ struct drm_pending_vblank_event {
> union {
> struct drm_event base;
> struct drm_event_vblank vbl;
> + struct drm_event_crtc_sequence seq;
> } event;
> };
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 101593ab10ac..25478560512a 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -718,6 +718,27 @@ struct drm_syncobj_handle {
> __u32 pad;
> };
>
> +/* Query current scanout sequence number */
> +struct drm_crtc_get_sequence {
> + __u32 crtc_id; /* requested crtc_id */
> + __u32 active; /* return: crtc output is active */
> + __u64 sequence; /* return: most recent vblank sequence */
> + __s64 sequence_ns; /* return: most recent vblank time */
> +};
> +
> +/* Queue event to be delivered at specified sequence */
> +
> +#define DRM_CRTC_SEQUENCE_RELATIVE 0x00000001 /* sequence is relative to current */
> +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
Note that right now vblank events are defined as:
- The even will be delivered "somewhen" around vblank (right before up to
first pixel are all things current drivers implement).
- An atomic update or pageflip ioctl call right after a vblank event will
hit (assuming no stalls) sequence + 1. radeon/amdgpu have some sw hacks
to handle this because their vblank event gets delivered before the last
possible time to update the next frame.
- The timestamp is corrected to be top-of-frame.
Would be a good time to document this a bit better, and might not exactly
match what vk expects ...
> +
> +struct drm_crtc_queue_sequence {
> + __u32 crtc_id;
> + __u32 flags;
> + __u64 sequence; /* on input, target sequence. on output, actual sequence */
> + __u64 user_data; /* user data passed to event */
> +};
> +
> #if defined(__cplusplus)
> }
> #endif
> @@ -800,6 +821,9 @@ extern "C" {
>
> #define DRM_IOCTL_WAIT_VBLANK DRM_IOWR(0x3a, union drm_wait_vblank)
>
> +#define DRM_IOCTL_CRTC_GET_SEQUENCE DRM_IOWR(0x3b, struct drm_crtc_get_sequence)
> +#define DRM_IOCTL_CRTC_QUEUE_SEQUENCE DRM_IOWR(0x3c, struct drm_crtc_queue_sequence)
> +
> #define DRM_IOCTL_UPDATE_DRAW DRM_IOW(0x3f, struct drm_update_draw)
>
> #define DRM_IOCTL_MODE_GETRESOURCES DRM_IOWR(0xA0, struct drm_mode_card_res)
> @@ -871,6 +895,7 @@ struct drm_event {
>
> #define DRM_EVENT_VBLANK 0x01
> #define DRM_EVENT_FLIP_COMPLETE 0x02
> +#define DRM_EVENT_CRTC_SEQUENCE 0x03
>
> struct drm_event_vblank {
> struct drm_event base;
> @@ -881,6 +906,13 @@ struct drm_event_vblank {
> __u32 crtc_id; /* 0 on older kernels that do not support this */
> };
>
> +struct drm_event_crtc_sequence {
> + struct drm_event base;
> + __u64 user_data;
> + __s64 time_ns;
> + __u64 sequence;
> +};
> +
> /* typedef area */
> #ifndef __KERNEL__
> typedef struct drm_clip_rect drm_clip_rect_t;
> --
> 2.13.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 02/08/17 05:53 PM, Daniel Vetter wrote:
> On Mon, Jul 31, 2017 at 10:03:04PM -0700, Keith Packard wrote:
>> This modifies the datatypes used by the vblank code to provide both 64
>> bits of vblank count and switch to using ktime_t for timestamps to
>> increase resolution from microseconds to nanoseconds.
>>
>> The driver interfaces have been left using 32 bits of vblank count;
>> all of the code necessary to widen that value for the user API was
>> already included to handle devices returning fewer than 32-bits.
>>
>> This will provide the necessary datatypes for the Vulkan API.
>>
>> v2:
>>
>> * Re-write wait_vblank ioctl to ABSOLUTE sequence
>>
>> When an application uses the WAIT_VBLANK ioctl with RELATIVE
>> or NEXTONMISS bits set, the target vblank interval is updated
>> within the kernel. We need to write that target back to the
>> ioctl buffer and update the flags bits so that if the wait is
>> interrupted by a signal, when it is re-started, it will target
>> precisely the same vblank count as before.
>>
>> * Leave driver API with 32-bit vblank count
>>
>> Suggested-by: Michel Dänzer <[email protected]>
>> Suggested-by: Daniel Vetter <[email protected]>
>> Signed-off-by: Keith Packard <[email protected]>
>
> Subject is a bit confusing since you say uapi, but this is just the
> internal prep work. Dropping UAPI fixes that. With that fixed:
>
> Reviewed-by: Daniel Vetter <[email protected]>
>
> Two more optional comments below, feel free to adapt or ignore. I'll wait
> for Michel's r-b before merging either way.
I don't think changing max_vblank_count to u64 is necessary/useful;
other than that, AFAICT the issues I raised before for this patch have
been addressed. I'm afraid I don't know if/when I'll get a chance to
review the whole patch in detail though.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On 01/08/17 02:03 PM, Keith Packard wrote:
> These provide crtc-id based functions instead of pipe-number, while
> also offering higher resolution time (ns) and wider frame count (64)
> as required by the Vulkan API.
>
> v2:
>
> * Check for DRIVER_MODESET in new crtc-based vblank ioctls
>
> Failing to check this will oops the driver.
>
> * Ensure vblank interupt is running in crtc_get_sequence ioctl
>
> The sequence and timing values are not correct while the
> interrupt is off, so make sure it's running before asking for
> them.
>
> * Short-circuit get_sequence if the counter is enabled and accurate
>
> Steal the idea from the code in wait_vblank to avoid the
> expense of drm_vblank_get/put
>
> * Return active state of crtc in crtc_get_sequence ioctl
>
> Might be useful for applications that aren't in charge of
> modesetting?
>
> * Use drm_crtc_vblank_get/put in new crtc-based vblank sequence ioctls
>
> Daniel Vetter prefers these over the old drm_vblank_put/get
> APIs.
>
> * Return s64 ns instead of u64 in new sequence event
>
> Suggested-by: Daniel Vetter <[email protected]>
> Suggested-by: Ville Syrjälä <[email protected]>
> Signed-off-by: Keith Packard <[email protected]>
[...]
> +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
Do you have userspace making use of DRM_CRTC_SEQUENCE_NEXT_ON_MISS? If
not, drop it.
> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
I thought there was consensus that this flag is pointless.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
Michel Dänzer <[email protected]> writes:
> [...]
>
>> +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
>
> Do you have userspace making use of DRM_CRTC_SEQUENCE_NEXT_ON_MISS? If
> not, drop it.
I added this so that the new ioctl would be compatible with the old
ioctl; do you think that's unnecessary?
>
>> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
>
> I thought there was consensus that this flag is pointless.
I just wrote a note to Daniel about this; I think it is useful in that
applications could specify that they actually want the event delivered
at first pixel out in accordance with the Vulkan spec, even if we can't
do that (yet). I definitely agree that requiring the bit be set is
ridiculous and should be removed.
Two choices
1) Remove the code which checks whether the flag is set.
Make Vulkan set the flag signaling what it wants.
Plan on doing the actual driver work if we find that it's necessary.
2) Remove the flag entirely.
Any preference?
--
-keith
Daniel Vetter <[email protected]> writes:
> Since I missed all the details Michel spotted, so I'll defer to his r-b.
> Also, before merging we need the userspace user. Do we have e.g.
> -modesetting patch for this, fully reviewed&ready for merging, just as
> demonstration?
Well, given that we'll have to keep the old API around for older
kernels, at least for a decade or so, I'm not sure why we'd actually
want that anytime soon, if ever? I guess it does provide 64-bit sequence
numbers, which Present wants?
> This way we could land this before the lease stuff for the
> vk extension is all solid&ready.
Do you think there's a pile more work to be done for the lease changes
in the kernel? Or are you just trying to separate the work flows?
I can go re-write the modesetting present support to use this new API
and use that for testing the kernel, if you think that would help move
the kernel bits along.
>> + drm_modeset_lock(&crtc->mutex, NULL);
>> + if (crtc->state)
>> + get_seq->active = crtc->state->enable;
>> + else
>> + get_seq->active = crtc->enabled;
>> + drm_modeset_unlock(&crtc->mutex);
>
> This is really heavywheight, given the lockless dance we attempt above.
> Also, when the crtc is off the vblank_get will fail, so you never get
> here. I guess my idea wasn't all that useful and well-thought out, or we
> need to be a bit more clever about this. To fix this we need to continue
> even when vblank_get fails (but only call vblank_put if ret == 0 ofc). And
> to avoid the locking you can use READ_ONCE(vblank->enabled) instead.
So, in reality, the client can more-or-less tell that the crtc is
disabled because the call fails? Sounds like I can just remove the
little dance to get the CRTC enabled state entirely. I don't understand
your comment about READ_ONCE(vblank->enabled); that doesn't relate to
the crtc enabled state, I don't think?
>> +
>> +/* Queue event to be delivered at specified sequence */
>> +
>> +#define DRM_CRTC_SEQUENCE_RELATIVE 0x00000001 /* sequence is relative to current */
>> +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
>> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
>
> Note that right now vblank events are defined as:
> - The even will be delivered "somewhen" around vblank (right before up to
> first pixel are all things current drivers implement).
> - An atomic update or pageflip ioctl call right after a vblank event will
> hit (assuming no stalls) sequence + 1. radeon/amdgpu have some sw hacks
> to handle this because their vblank event gets delivered before the last
> possible time to update the next frame.
> - The timestamp is corrected to be top-of-frame.
>
> Would be a good time to document this a bit better, and might not exactly
> match what vk expects ...
(NEXT_ON_MISS is not used by the new Vulkan code; I added it only to keep
compatibility with the old API, in case we want to switch someday).
FIRST_PIXEL_OUT is an attempt to signal to the kernel that the
application really wants to see the event when the first pixel hits the
display. I assume the important thing here is the timestamp in the
event and not the actual delivery, but I don't actually know that.
If the timestamp is the only important thing, it sounds like the kernel
already satisfies that, which is cool.
If Vulkan really wants the event to be delivered when the first pixel is
displayed, then having this bit in the ioctl means we can let drivers
continue to do whatever they are now when the bit isn't set, but try
harder to deliver the event at first-pixel when requested.
So, I think what I want to do is leave the bit in the request so that
drivers can at least see what user space is asking for, and if we learn
that it's important to deliver the event at the requested time, we can
go fix drivers later.
--
-keith
On 06/08/17 12:32 PM, Keith Packard wrote:
> Daniel Vetter <[email protected]> writes:
>
>>> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
>>
>> Note that right now vblank events are defined as:
>> - The even will be delivered "somewhen" around vblank (right before up to
>> first pixel are all things current drivers implement).
>> - An atomic update or pageflip ioctl call right after a vblank event will
>> hit (assuming no stalls) sequence + 1. radeon/amdgpu have some sw hacks
>> to handle this because their vblank event gets delivered before the last
>> possible time to update the next frame.
>> - The timestamp is corrected to be top-of-frame.
>>
>> Would be a good time to document this a bit better, and might not exactly
>> match what vk expects ...
>
> [...]
>
> FIRST_PIXEL_OUT is an attempt to signal to the kernel that the
> application really wants to see the event when the first pixel hits the
> display. I assume the important thing here is the timestamp in the
> event and not the actual delivery, but I don't actually know that.
>
> If the timestamp is the only important thing, it sounds like the kernel
> already satisfies that, which is cool.
>
> If Vulkan really wants the event to be delivered when the first pixel is
> displayed, then having this bit in the ioctl means we can let drivers
> continue to do whatever they are now when the bit isn't set, but try
> harder to deliver the event at first-pixel when requested.
I don't see the point of giving this choice to userspace. The event
timestamp specifies when first-pixel occurs; if it's in the future,
userspace can use other functionality to wait until then if needed
(though it's hard to imagine why it would be).
> So, I think what I want to do is leave the bit in the request so that
> drivers can at least see what user space is asking for, and if we learn
> that it's important to deliver the event at the requested time, we can
> go fix drivers later.
This seems like a very bad idea: Having a flag which doesn't have any
effect at first will result in userspace randomly setting the flag or
not. If we were to then change the behaviour with the flag (not) set,
some userspace will almost certainly break. So effectively we can never
make the flag have any effect.
The way to go here is to drop the flag for now and document the
behaviour explicitly. If unexpectedly a real need for different
behaviour comes up in the future, we can add a flag for it at that time.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On 06/08/17 12:42 PM, Keith Packard wrote:
> Michel Dänzer <[email protected]> writes:
>
>> [...]
>>
>>> +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
>>
>> Do you have userspace making use of DRM_CRTC_SEQUENCE_NEXT_ON_MISS? If
>> not, drop it.
>
> I added this so that the new ioctl would be compatible with the old
> ioctl; do you think that's unnecessary?
I do. I don't know if there's ever been any real-world usage of
DRM_VBLANK_NEXTONMISS. Let's not repeat my mistake in the new interface.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
On Sun, Aug 6, 2017 at 5:32 AM, Keith Packard <[email protected]> wrote:
> Daniel Vetter <[email protected]> writes:
>
>> Since I missed all the details Michel spotted, so I'll defer to his r-b.
>> Also, before merging we need the userspace user. Do we have e.g.
>> -modesetting patch for this, fully reviewed&ready for merging, just as
>> demonstration?
>
> Well, given that we'll have to keep the old API around for older
> kernels, at least for a decade or so, I'm not sure why we'd actually
> want that anytime soon, if ever? I guess it does provide 64-bit sequence
> numbers, which Present wants?
I just figured that -modesetting would be the simplest domenstration
vehicle, since the vulkan patches don't look ready yet. I need fully
reviewed&tested userspace before we can land any kernel stuff. Doing
the quick modesetting conversion would unblock.
>> This way we could land this before the lease stuff for the
>> vk extension is all solid&ready.
>
> Do you think there's a pile more work to be done for the lease changes
> in the kernel? Or are you just trying to separate the work flows?
>
> I can go re-write the modesetting present support to use this new API
> and use that for testing the kernel, if you think that would help move
> the kernel bits along.
Just trying to separate flows and get stuff landed as soon as it's
ready. There's always the chance someone rewrites the code meanwhile
if we wait until all the vk stuff is ready.
>>> + drm_modeset_lock(&crtc->mutex, NULL);
>>> + if (crtc->state)
>>> + get_seq->active = crtc->state->enable;
>>> + else
>>> + get_seq->active = crtc->enabled;
>>> + drm_modeset_unlock(&crtc->mutex);
>>
>> This is really heavywheight, given the lockless dance we attempt above.
>> Also, when the crtc is off the vblank_get will fail, so you never get
>> here. I guess my idea wasn't all that useful and well-thought out, or we
>> need to be a bit more clever about this. To fix this we need to continue
>> even when vblank_get fails (but only call vblank_put if ret == 0 ofc). And
>> to avoid the locking you can use READ_ONCE(vblank->enabled) instead.
>
> So, in reality, the client can more-or-less tell that the crtc is
> disabled because the call fails? Sounds like I can just remove the
> little dance to get the CRTC enabled state entirely. I don't understand
> your comment about READ_ONCE(vblank->enabled); that doesn't relate to
> the crtc enabled state, I don't think?
It's supposed to, at least for atomic drivers. For legacy kms drivers
they're supposed to reject the enable attempt with some error code,
when the CRTC is off. It's all pretty awkward ad-hoc uabi :-(
I'm leaning more-and-more towards just dropping this part as a bad
idea from my side. At least until we have someone who really needs
this.
>>> +
>>> +/* Queue event to be delivered at specified sequence */
>>> +
>>> +#define DRM_CRTC_SEQUENCE_RELATIVE 0x00000001 /* sequence is relative to current */
>>> +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */
>>> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */
>>
>> Note that right now vblank events are defined as:
>> - The even will be delivered "somewhen" around vblank (right before up to
>> first pixel are all things current drivers implement).
>> - An atomic update or pageflip ioctl call right after a vblank event will
>> hit (assuming no stalls) sequence + 1. radeon/amdgpu have some sw hacks
>> to handle this because their vblank event gets delivered before the last
>> possible time to update the next frame.
>> - The timestamp is corrected to be top-of-frame.
>>
>> Would be a good time to document this a bit better, and might not exactly
>> match what vk expects ...
>
> (NEXT_ON_MISS is not used by the new Vulkan code; I added it only to keep
> compatibility with the old API, in case we want to switch someday).
>
> FIRST_PIXEL_OUT is an attempt to signal to the kernel that the
> application really wants to see the event when the first pixel hits the
> display. I assume the important thing here is the timestamp in the
> event and not the actual delivery, but I don't actually know that.
>
> If the timestamp is the only important thing, it sounds like the kernel
> already satisfies that, which is cool.
Would be good to confirm that. If it's not, we have a problem.
> If Vulkan really wants the event to be delivered when the first pixel is
> displayed, then having this bit in the ioctl means we can let drivers
> continue to do whatever they are now when the bit isn't set, but try
> harder to deliver the event at first-pixel when requested.
>
> So, I think what I want to do is leave the bit in the request so that
> drivers can at least see what user space is asking for, and if we learn
> that it's important to deliver the event at the requested time, we can
> go fix drivers later.
Not sure that's a good idea without fixing up drivers. Asking for
something that's not delivered just makes that bit meaningless. Atm
the ioctl is also rejected if you don't set this flag, so it
essentially means whatever current drivers do. And I think it'd be
good to at least document that, and maybe even drop the bitflag (since
it doesn't encode anything, at least in the current patch).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch