Date: Thu, 6 Jul 2017 13:16:04 +0300
From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
To: Keith Packard <keithp@keithp.com>, linux-kernel@vger.kernel.org,
        Dave Airlie <airlied@redhat.com>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE
 ioctls
Message-ID: <20170706101604.GY12629@intel.com>
References: <20170705221013.27940-1-keithp@keithp.com>
 <20170705221013.27940-4-keithp@keithp.com>
 <20170706075313.bn2exiklfabgc25t@phenom.ffwll.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20170706075313.bn2exiklfabgc25t@phenom.ffwll.local>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 13375
Lines: 382

On Thu, Jul 06, 2017 at 09:53:13AM +0200, Daniel Vetter wrote:
> On Wed, Jul 05, 2017 at 03:10:13PM -0700, Keith Packard wrote:
> > These provide crtc-id based functions instead of pipe-number, while
> > also offering higher resolution time (ns) and wider frame count (64)
> > as required by the Vulkan API.
> > 
> > Signed-off-by: Keith Packard <keithp@keithp.com>
> 
> I very much like this since the old ioctl really is a rather bad horror
> show. And since it's tied in with ums drivers everything is complicated.
> 
> \o/ for much cleaner ioctls.
> 
> Bunch of comments below, but looks good overall.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_internal.h |   6 ++
> >  drivers/gpu/drm/drm_ioctl.c    |   2 +
> >  drivers/gpu/drm/drm_vblank.c   | 148 +++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_vblank.h       |   1 +
> >  include/uapi/drm/drm.h         |  32 +++++++++
> >  5 files changed, 189 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > index 5cecc974d2f9..b68a193b7907 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -65,6 +65,12 @@ int drm_legacy_irq_control(struct drm_device *dev, void *data,
> >  int drm_legacy_modeset_ctl(struct drm_device *dev, void *data,
> >  			   struct drm_file *file_priv);
> >  
> > +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> > +				struct drm_file *filp);
> > +
> > +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> > +				  struct drm_file *filp);
> > +
> >  /* drm_auth.c */
> >  int drm_getmagic(struct drm_device *dev, void *data,
> >  		 struct drm_file *file_priv);
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index f1e568176da9..63016cf3e224 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> >  		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
> >  	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
> >  		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
> > +	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
> > +	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
> 
> I started a discussion a while back whether these should be restricted to
> DRM_MASTER (i.e. the modeset resource owner) or available to everyone.
> Since it's read-only I guess we can keep it accessible to everyone, but it
> has a bit the problem that client app developers see this, think it does
> what it does and then use it to schedule frames without asking the
> compositor. Which sometimes even works, but isn't really proper design.
> The reasons seems to be that on X11 there's no EGL extension for accurate
> timing frame updates (DRI2/3 can do it ofc, and glx exposes it, but glx is
> uncool or something like that).
> 
> >  };
> >  
> >  #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 9ae170857ef6..93004b1bf84c 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -817,6 +817,12 @@ static void send_vblank_event(struct drm_device *dev,
> >  			e->event.vbl.tv_usec = now->tv_nsec / 1000;
> >  		}
> >  		break;
> > +	case DRM_EVENT_CRTC_SEQUENCE:
> > +		if (seq)
> > +			e->event.seq.sequence = seq;
> > +		if (now)
> > +			e->event.seq.time_ns = timespec_to_ns(now);
> > +		break;
> >  	}
> >  	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
> >  	drm_send_event_locked(dev, &e->base);
> > @@ -1516,6 +1522,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> >  		DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> >  		return ret;
> >  	}
> > +
> >  	seq = drm_vblank_count(dev, pipe);
> >  
> >  	switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
> > @@ -1676,3 +1683,144 @@ bool drm_crtc_handle_vblank(struct drm_crtc *crtc)
> >  	return drm_handle_vblank(crtc->dev, drm_crtc_index(crtc));
> >  }
> >  EXPORT_SYMBOL(drm_crtc_handle_vblank);
> > +
> > +/*
> > + * Get crtc VBLANK count.
> > + *
> > + * \param dev DRM device
> > + * \param data user arguement, pointing to a drm_crtc_get_sequence structure.
> > + * \param file_priv drm file private for the user's open file descriptor
> > + */
> 
> Since this stuff isn't parsed by kerneldoc I tend to just free-form ioctl
> comments completely. Someday maybe someone even gets around to doing
> proper uabi documentation :-) Just an aside.
> > +
> > +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> > +				struct drm_file *file_priv)
> > +{
> > +	struct drm_crtc *crtc;
> > +	int pipe;
> > +	struct drm_crtc_get_sequence *get_seq = data;
> > +	struct timespec now;
> > +
> 
> You need a DRIVER_MODESET check here or the drm_crtc_find will oops. Same
> below.
> 
> > +	if (!dev->irq_enabled)
> > +		return -EINVAL;
> > +
> > +	crtc = drm_crtc_find(dev, get_seq->crtc_id);
> > +	if (!crtc)
> > +		return -ENOENT;
> > +
> > +	pipe = drm_crtc_index(crtc);
> > +
> > +	get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now);
> 
> This can give you and old vblank if the vblank is off (i.e. sw state
> hasn't be regularly updated). I think we want a new
> drm_crtc_accurate_vblank_count_and_time variant.

Or better yet just do what Chris did for the old ioctl in commit
b33b02707ba3 ("drm: Peek at the current counter/timestamp for vblank queries")

> 
> Another thing that is very ill-defined in the old vblank ioctl and that we
> could fix here: Asking for vblanks when the CRTC is off is a bit silly.
> Asking for the sequence when it's off makes some sense, but might still be
> good to give userspace some indication in the new struct? This also from
> the pov of the unpriviledge vblank waiter use-case that I wondered about
> earlier.
> 
> > +	get_seq->sequence_ns = timespec_to_ns(&now);
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Queue a event for VBLANK sequence
> > + *
> > + * \param dev DRM device
> > + * \param data user arguement, pointing to a drm_crtc_queue_sequence structure.
> > + * \param file_priv drm file private for the user's open file descriptor
> > + */
> > +
> > +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> > +				  struct drm_file *file_priv)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_vblank_crtc *vblank;
> > +	int pipe;
> > +	struct drm_crtc_queue_sequence *queue_seq = data;
> > +	struct timespec now;
> > +	struct drm_pending_vblank_event *e;
> > +	u32 flags;
> > +	u64 seq;
> > +	u64 req_seq;
> > +	int ret;
> > +	unsigned long spin_flags;
> > +
> > +	if (!dev->irq_enabled)
> > +		return -EINVAL;
> > +
> > +	crtc = drm_crtc_find(dev, queue_seq->crtc_id);
> > +	if (!crtc)
> > +		return -ENOENT;
> > +
> > +	flags = queue_seq->flags;
> > +	/* Check valid flag bits */
> > +	if (flags & ~(DRM_CRTC_SEQUENCE_RELATIVE|
> > +		      DRM_CRTC_SEQUENCE_NEXT_ON_MISS|
> > +		      DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
> > +		return -EINVAL;
> > +
> > +	/* Check for valid signal edge */
> > +	if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT))
> > +		return -EINVAL;
> 
> This seems new, maybe drop it until we need it?
> 
> > +
> > +	pipe = drm_crtc_index(crtc);
> > +
> > +	vblank = &dev->vblank[pipe];
> > +
> > +	e = kzalloc(sizeof(*e), GFP_KERNEL);
> > +	if (e == NULL)
> > +		return -ENOMEM;
> > +
> > +	ret = drm_vblank_get(dev, pipe);
> 
> drm_crtc_vblank_get pls, would be sweet if we can avoid the (dev, pipe)
> pairs as much as possible. Same for all the others.
> 
> > +	if (ret) {
> > +		DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> > +		goto err_free;
> > +	}
> > +
> > +	seq = drm_vblank_count_and_time(dev, pipe, &now);
> 
> I think here there's no need for the accurate version, since we
> force-enable the vblanks already.
> 
> > +	req_seq = queue_seq->sequence;
> > +
> > +	if (flags & DRM_CRTC_SEQUENCE_RELATIVE)
> > +		req_seq += seq;
> > +
> > +	if ((flags & DRM_CRTC_SEQUENCE_NEXT_ON_MISS) && vblank_passed(seq, req_seq))
> > +		req_seq = seq + 1;
> > +
> > +	e->pipe = pipe;
> > +	e->event.base.type = DRM_EVENT_CRTC_SEQUENCE;
> > +	e->event.base.length = sizeof(e->event.seq);
> > +	e->event.seq.user_data = queue_seq->user_data;
> 
> No need for crtc_id in this event? Or do we expect userspace to encode
> that in the user_data somehow? I don't think it's a real problem since
> we'll only deliver one event per request, so clear for which crtc it is.
> In atomic we might deliver multiple events (one for each crtc) so that's
> why it's needed there.
> 
> But might be useful just for consistency.
> 
> > +
> > +	spin_lock_irqsave(&dev->event_lock, spin_flags);
> > +
> > +	/*
> > +	 * drm_crtc_vblank_off() might have been called after we called
> > +	 * drm_vblank_get(). drm_crtc_vblank_off() holds event_lock around the
> > +	 * vblank disable, so no need for further locking.  The reference from
> > +	 * drm_vblank_get() protects against vblank disable from another source.
> > +	 */
> > +	if (!READ_ONCE(vblank->enabled)) {
> > +		ret = -EINVAL;
> > +		goto err_unlock;
> > +	}
> > +
> > +	ret = drm_event_reserve_init_locked(dev, file_priv, &e->base,
> > +					    &e->event.base);
> > +
> > +	if (ret)
> > +		goto err_unlock;
> > +
> > +	e->sequence = req_seq;
> > +
> > +	if (vblank_passed(seq, req_seq)) {
> > +		drm_vblank_put(dev, pipe);
> > +		send_vblank_event(dev, e, seq, &now);
> > +		queue_seq->sequence = seq;
> > +	} else {
> > +		/* drm_handle_vblank_events will call drm_vblank_put */
> > +		list_add_tail(&e->base.link, &dev->vblank_event_list);
> > +		queue_seq->sequence = req_seq;
> > +	}
> > +
> > +	spin_unlock_irqrestore(&dev->event_lock, spin_flags);
> > +	return 0;
> > +
> > +err_unlock:
> > +	spin_unlock_irqrestore(&dev->event_lock, spin_flags);
> > +	drm_vblank_put(dev, pipe);
> > +err_free:
> > +	kfree(e);
> > +	return ret;
> > +}
> > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > index 3ef7ddc5db5f..8a5e1dfe3be7 100644
> > --- a/include/drm/drm_vblank.h
> > +++ b/include/drm/drm_vblank.h
> > @@ -57,6 +57,7 @@ struct drm_pending_vblank_event {
> >  	union {
> >  		struct drm_event base;
> >  		struct drm_event_vblank vbl;
> > +		struct drm_event_crtc_sequence seq;
> >  	} event;
> >  };
> >  
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 101593ab10ac..dc16d42a88c7 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -718,6 +718,27 @@ struct drm_syncobj_handle {
> >  	__u32 pad;
> >  };
> >  
> > +/* Query current scanout sequence number */
> > +struct drm_crtc_get_sequence {
> > +	__u32 crtc_id;
> > +	__u32 pad;
> > +	__u64 sequence;
> > +	__u64 sequence_ns;
> > +};
> > +
> > +/* Queue event to be delivered at specified sequence */
> > +
> > +#define DRM_CRTC_SEQUENCE_RELATIVE		0x00000001	/* sequence is relative to current */
> > +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS		0x00000002	/* Use next sequence if we've missed */
> > +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT	0x00000004	/* Signal when first pixel is displayed */
> 
> I thought this is already the semantics our current vblank events have (in
> the timestamp, when exactly the event comes out isn't defined further than
> "somewhere around vblank"). Since it's unsed I'd just remove this #define.
> 
> > +
> > +struct drm_crtc_queue_sequence {
> > +	__u32 crtc_id;
> > +	__u32 flags;
> > +	__u64 sequence;		/* on input, target sequence. on output, actual sequence */
> > +	__u64 user_data;	/* user data passed to event */
> > +};
> 
> In both ioctl handlers pls make sure everything you don't look at is 0,
> including unused stuff like pad. Otherwise userspace might fail to clear
> them, and we can never use them in the future. Maybe just rename pad to
> flags right away.
> 
> > +
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> > @@ -800,6 +821,9 @@ extern "C" {
> >  
> >  #define DRM_IOCTL_WAIT_VBLANK		DRM_IOWR(0x3a, union drm_wait_vblank)
> >  
> > +#define DRM_IOCTL_CRTC_GET_SEQUENCE	DRM_IOWR(0x3b, struct drm_crtc_get_sequence)
> > +#define DRM_IOCTL_CRTC_QUEUE_SEQUENCE	DRM_IOWR(0x3c, struct drm_crtc_queue_sequence)
> > +
> >  #define DRM_IOCTL_UPDATE_DRAW		DRM_IOW(0x3f, struct drm_update_draw)
> >  
> >  #define DRM_IOCTL_MODE_GETRESOURCES	DRM_IOWR(0xA0, struct drm_mode_card_res)
> > @@ -871,6 +895,7 @@ struct drm_event {
> >  
> >  #define DRM_EVENT_VBLANK 0x01
> >  #define DRM_EVENT_FLIP_COMPLETE 0x02
> > +#define DRM_EVENT_CRTC_SEQUENCE	0x03
> >  
> >  struct drm_event_vblank {
> >  	struct drm_event base;
> > @@ -881,6 +906,13 @@ struct drm_event_vblank {
> >  	__u32 crtc_id; /* 0 on older kernels that do not support this */
> >  };
> >  
> > +struct drm_event_crtc_sequence {
> > +	struct drm_event	base;
> > +	__u64			user_data;
> > +	__u64			time_ns;
> > +	__u64			sequence;
> > +};
> > +
> >  /* typedef area */
> >  #ifndef __KERNEL__
> >  typedef struct drm_clip_rect drm_clip_rect_t;
> > -- 
> > 2.11.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj?l?
Intel OTC