Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700633 > unrolled thread

[PATCH 0/3] drm: Add CRTC-id based ioctls for vblank query/event

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/3] drm: Add CRTC-id based ioctls for vblank query/event Keith Packard <keithp@keithp.com> - 2017-08-01 07:10 +0200
    [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2] Keith Packard <keithp@keithp.com> - 2017-08-01 07:10 +0200
      Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls [v2] Daniel Vetter <daniel@ffwll.ch> - 2017-08-02 11:30 +0200
        Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2] "Keith Packard" <keithp@keithp.com> - 2017-08-06 07:50 +0200
          Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls [v2] Michel Dänzer <michel@daenzer.net> - 2017-08-07 05:10 +0200
          Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls [v2] Daniel Vetter <daniel@ffwll.ch> - 2017-08-07 10:40 +0200
      Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls [v2] Michel Dänzer <michel@daenzer.net> - 2017-08-02 11:50 +0200
        Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2] "Keith Packard" <keithp@keithp.com> - 2017-08-06 07:50 +0200
          Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE  ioctls [v2] Michel Dänzer <michel@daenzer.net> - 2017-08-07 05:10 +0200
    [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2] Keith Packard <keithp@keithp.com> - 2017-08-01 07:10 +0200
      Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank  time to ktime_t [v2] Daniel Vetter <daniel@ffwll.ch> - 2017-08-02 11:00 +0200
        Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time  to ktime_t [v2] Michel Dänzer <michel@daenzer.net> - 2017-08-02 11:50 +0200
        Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2] "Keith Packard" <keithp@keithp.com> - 2017-08-06 19:40 +0200
    [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types [v2] Keith Packard <keithp@keithp.com> - 2017-08-01 07:10 +0200
      Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future  event types [v2] Daniel Vetter <daniel@ffwll.ch> - 2017-08-02 11:10 +0200

#1700633 — [PATCH 0/3] drm: Add CRTC-id based ioctls for vblank query/event

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 <daniel@ffwll.ch>
	Michel Dänzer <michel@daenzer.net>
	Ville Syrjälä <ville.syrjala@linux.intel.com>

-keith

[toc] | [next] | [standalone]

#1700634 — [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]

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 <daniel@ffwll.ch>
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1701949 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]

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 <daniel@ffwll.ch>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Keith Packard <keithp@keithp.com>

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

[toc] | [prev] | [next] | [standalone]

#1704748 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]

[Multipart message — attachments visible in raw view] — view raw

Daniel Vetter <daniel@ffwll.ch> 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

[toc] | [prev] | [next] | [standalone]

#1705068 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]

[Multipart message — attachments visible in raw view] — view raw

On 06/08/17 12:32 PM, Keith Packard wrote:
> Daniel Vetter <daniel@ffwll.ch> 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

[toc] | [prev] | [next] | [standalone]

#1705244 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]

On Sun, Aug 6, 2017 at 5:32 AM, Keith Packard <keithp@keithp.com> wrote:
> Daniel Vetter <daniel@ffwll.ch> 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

[toc] | [prev] | [next] | [standalone]

#1701960 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]

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 <daniel@ffwll.ch>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Keith Packard <keithp@keithp.com>

[...]

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

[toc] | [prev] | [next] | [standalone]

#1704747 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]

[Multipart message — attachments visible in raw view] — view raw

Michel Dänzer <michel@daenzer.net> 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

[toc] | [prev] | [next] | [standalone]

#1705069 — Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]

[Multipart message — attachments visible in raw view] — view raw

On 06/08/17 12:42 PM, Keith Packard wrote:
> Michel Dänzer <michel@daenzer.net> 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

[toc] | [prev] | [next] | [standalone]

#1700635 — [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2]

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 <michel@daenzer.net>
Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Keith Packard <keithp@keithp.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1701919 — Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2]

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 <michel@daenzer.net>
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Keith Packard <keithp@keithp.com>

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 <daniel.vetter@ffwll.ch>

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

[toc] | [prev] | [next] | [standalone]

#1701958 — Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2]

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 <michel@daenzer.net>
>> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Keith Packard <keithp@keithp.com>
> 
> 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 <daniel.vetter@ffwll.ch>
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1704930 — Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2]

[Multipart message — attachments visible in raw view] — view raw

Daniel Vetter <daniel@ffwll.ch> writes:

> Subject is a bit confusing since you say uapi, but this is just the
> internal prep work. Dropping UAPI fixes that. With that fixed:

Yeah, thanks.

> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Added.

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

I also need to re-write the reply.sequence value in the queue
function; seems like passing in the vblwait is a simpler plan.

>> +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) ?

Oh, yes, that makes perfect sense -- an int32_t will obviously hold the
shortest distance between the two, whether negative or positive. Of
course, '(uint32_t) wide' is just 'narrow'.

> But then it took me way too long to think about this one, so maybe leave
> it at that.

Your version is a lot shorter, and I think it's actually clearer. How
about

static inline uint64_t widen_32_to_64(uint32_t narrow, uint64_t near)
{
	return near + (int32_t) (narrow - (uint32_t) near);
}

Here's a test program which validates the widen function.

[toc] | [prev] | [next] | [standalone]

#1700637 — [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types [v2]

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 <keithp@keithp.com>
---
 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

[toc] | [prev] | [next] | [standalone]

#1701939 — Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types [v2]

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 <keithp@keithp.com>
> ---
>  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 <daniel.vetter@ffwll.ch>

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web