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

From: Daniel Vetter <daniel@ffwll.ch>
To: Keith Packard <keithp@keithp.com>
Cc: linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]
Date: Wed, 2 Aug 2017 11:25:05 +0200	[thread overview]
Message-ID: <20170802092505.ynlpy6dxf3lxziim@phenom.ffwll.local> (raw)
In-Reply-To: <20170801050306.24423-4-keithp@keithp.com>

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