| Message ID | 20170705221013.27940-4-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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 >
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
On Thu, Jul 6, 2017 at 12:16 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > + if (!dev->irq_enabled) >> > + return -EINVAL; >> > + >> > + crtc = drm_crtc_find(dev, get_seq->crtc_id); >> > + if (!crtc) >> > + return -ENOENT; >> > + >> > + pipe = drm_crtc_index(crtc); >> > + >> > + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now); >> >> This can give you and old vblank if the vblank is off (i.e. sw state >> hasn't be regularly updated). I think we want a new >> drm_crtc_accurate_vblank_count_and_time variant. > > Or better yet just do what Chris did for the old ioctl in commit > b33b02707ba3 ("drm: Peek at the current counter/timestamp for vblank queries") Yeah the READ_ONCE(vblank->enabled) is a nice fastpath. But we still need the accurate one as slowpath in case the vblank irq is off. -Daniel
On Thu, Jul 06, 2017 at 01:04:18PM +0200, Daniel Vetter wrote: > On Thu, Jul 6, 2017 at 12:16 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > >> > + if (!dev->irq_enabled) > >> > + return -EINVAL; > >> > + > >> > + crtc = drm_crtc_find(dev, get_seq->crtc_id); > >> > + if (!crtc) > >> > + return -ENOENT; > >> > + > >> > + pipe = drm_crtc_index(crtc); > >> > + > >> > + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now); > >> > >> This can give you and old vblank if the vblank is off (i.e. sw state > >> hasn't be regularly updated). I think we want a new > >> drm_crtc_accurate_vblank_count_and_time variant. > > > > Or better yet just do what Chris did for the old ioctl in commit > > b33b02707ba3 ("drm: Peek at the current counter/timestamp for vblank queries") > > Yeah the READ_ONCE(vblank->enabled) is a nice fastpath. But we still > need the accurate one as slowpath in case the vblank irq is off. Maybe, or maybe we want to turn the interrupt on in that case? That's what the old ioctl does.
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > Maybe, or maybe we want to turn the interrupt on in that case? That's > what the old ioctl does. That's what I suggested in my reply to Daniel's review. Even if we add the accurate function, we'll still need the interrupt-enable case as a fallback for drivers which don't support the accurate path, right?
On Thu, Jul 06, 2017 at 09:28:40AM -0700, Keith Packard wrote: > Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > > > Maybe, or maybe we want to turn the interrupt on in that case? That's > > what the old ioctl does. > > That's what I suggested in my reply to Daniel's review. Even if we add > the accurate function, we'll still need the interrupt-enable case as a > fallback for drivers which don't support the accurate path, right? TBH I didn't even consider that case, but yeah makes sense. Otherwise the counter won't start to tick and the result of the query is pretty much useless. I was mostly thinking of the 'seq = query(); wait(seq + n);' pattern where we can avoid doing the full update more than once if we enable the interrupt already during the query.
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > I was mostly thinking of the 'seq = query(); wait(seq + n);' pattern > where we can avoid doing the full update more than once if we enable > the interrupt already during the query. Don't we still wait 5 seconds before disabling vblank? In that case, the chances of hitting an idle vblank are pretty slim if the application is actually busy.
On Thu, Jul 06, 2017 at 11:22:43AM -0700, Keith Packard wrote: > Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > > > I was mostly thinking of the 'seq = query(); wait(seq + n);' pattern > > where we can avoid doing the full update more than once if we enable > > the interrupt already during the query. > > Don't we still wait 5 seconds before disabling vblank? In that case, the > chances of hitting an idle vblank are pretty slim if the application is > actually busy. With the disable_immediate thing we only wait until the next vblank before disabling the irq again.
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > With the disable_immediate thing we only wait until the next vblank > before disabling the irq again. Ok, still sounds like we'll be doing fine if the application does a get immediately followed by a queue event. At least most of the time.
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 5cecc974d2f9..b68a193b7907 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -65,6 +65,12 @@ int drm_legacy_irq_control(struct drm_device *dev, void *data, int drm_legacy_modeset_ctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, + struct drm_file *filp); + +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data, + struct drm_file *filp); + /* drm_auth.c */ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv); diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index f1e568176da9..63016cf3e224 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED), }; #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 9ae170857ef6..93004b1bf84c 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -817,6 +817,12 @@ static void send_vblank_event(struct drm_device *dev, e->event.vbl.tv_usec = now->tv_nsec / 1000; } break; + case DRM_EVENT_CRTC_SEQUENCE: + if (seq) + e->event.seq.sequence = seq; + if (now) + e->event.seq.time_ns = timespec_to_ns(now); + break; } trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq); drm_send_event_locked(dev, &e->base); @@ -1516,6 +1522,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret); return ret; } + seq = drm_vblank_count(dev, pipe); switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) { @@ -1676,3 +1683,144 @@ bool drm_crtc_handle_vblank(struct drm_crtc *crtc) return drm_handle_vblank(crtc->dev, drm_crtc_index(crtc)); } EXPORT_SYMBOL(drm_crtc_handle_vblank); + +/* + * Get crtc VBLANK count. + * + * \param dev DRM device + * \param data user arguement, pointing to a drm_crtc_get_sequence structure. + * \param file_priv drm file private for the user's open file descriptor + */ + +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + struct drm_crtc *crtc; + int pipe; + struct drm_crtc_get_sequence *get_seq = data; + struct timespec now; + + if (!dev->irq_enabled) + return -EINVAL; + + crtc = drm_crtc_find(dev, get_seq->crtc_id); + if (!crtc) + return -ENOENT; + + pipe = drm_crtc_index(crtc); + + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now); + get_seq->sequence_ns = timespec_to_ns(&now); + return 0; +} + +/* + * Queue a event for VBLANK sequence + * + * \param dev DRM device + * \param data user arguement, pointing to a drm_crtc_queue_sequence structure. + * \param file_priv drm file private for the user's open file descriptor + */ + +int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + struct drm_crtc *crtc; + struct drm_vblank_crtc *vblank; + int pipe; + struct drm_crtc_queue_sequence *queue_seq = data; + struct timespec now; + struct drm_pending_vblank_event *e; + u32 flags; + u64 seq; + u64 req_seq; + int ret; + unsigned long spin_flags; + + if (!dev->irq_enabled) + return -EINVAL; + + crtc = drm_crtc_find(dev, queue_seq->crtc_id); + if (!crtc) + return -ENOENT; + + flags = queue_seq->flags; + /* Check valid flag bits */ + if (flags & ~(DRM_CRTC_SEQUENCE_RELATIVE| + DRM_CRTC_SEQUENCE_NEXT_ON_MISS| + DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT)) + return -EINVAL; + + /* Check for valid signal edge */ + if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT)) + return -EINVAL; + + pipe = drm_crtc_index(crtc); + + vblank = &dev->vblank[pipe]; + + e = kzalloc(sizeof(*e), GFP_KERNEL); + if (e == NULL) + return -ENOMEM; + + ret = drm_vblank_get(dev, pipe); + if (ret) { + DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret); + goto err_free; + } + + seq = drm_vblank_count_and_time(dev, pipe, &now); + req_seq = queue_seq->sequence; + + if (flags & DRM_CRTC_SEQUENCE_RELATIVE) + req_seq += seq; + + if ((flags & DRM_CRTC_SEQUENCE_NEXT_ON_MISS) && vblank_passed(seq, req_seq)) + req_seq = seq + 1; + + e->pipe = pipe; + e->event.base.type = DRM_EVENT_CRTC_SEQUENCE; + e->event.base.length = sizeof(e->event.seq); + e->event.seq.user_data = queue_seq->user_data; + + spin_lock_irqsave(&dev->event_lock, spin_flags); + + /* + * drm_crtc_vblank_off() might have been called after we called + * drm_vblank_get(). drm_crtc_vblank_off() holds event_lock around the + * vblank disable, so no need for further locking. The reference from + * drm_vblank_get() protects against vblank disable from another source. + */ + if (!READ_ONCE(vblank->enabled)) { + ret = -EINVAL; + goto err_unlock; + } + + ret = drm_event_reserve_init_locked(dev, file_priv, &e->base, + &e->event.base); + + if (ret) + goto err_unlock; + + e->sequence = req_seq; + + if (vblank_passed(seq, req_seq)) { + drm_vblank_put(dev, pipe); + send_vblank_event(dev, e, seq, &now); + queue_seq->sequence = seq; + } else { + /* drm_handle_vblank_events will call drm_vblank_put */ + list_add_tail(&e->base.link, &dev->vblank_event_list); + queue_seq->sequence = req_seq; + } + + spin_unlock_irqrestore(&dev->event_lock, spin_flags); + return 0; + +err_unlock: + spin_unlock_irqrestore(&dev->event_lock, spin_flags); + drm_vblank_put(dev, pipe); +err_free: + kfree(e); + return ret; +} diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index 3ef7ddc5db5f..8a5e1dfe3be7 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -57,6 +57,7 @@ struct drm_pending_vblank_event { union { struct drm_event base; struct drm_event_vblank vbl; + struct drm_event_crtc_sequence seq; } event; }; diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 101593ab10ac..dc16d42a88c7 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -718,6 +718,27 @@ struct drm_syncobj_handle { __u32 pad; }; +/* Query current scanout sequence number */ +struct drm_crtc_get_sequence { + __u32 crtc_id; + __u32 pad; + __u64 sequence; + __u64 sequence_ns; +}; + +/* Queue event to be delivered at specified sequence */ + +#define DRM_CRTC_SEQUENCE_RELATIVE 0x00000001 /* sequence is relative to current */ +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */ +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */ + +struct drm_crtc_queue_sequence { + __u32 crtc_id; + __u32 flags; + __u64 sequence; /* on input, target sequence. on output, actual sequence */ + __u64 user_data; /* user data passed to event */ +}; + #if defined(__cplusplus) } #endif @@ -800,6 +821,9 @@ extern "C" { #define DRM_IOCTL_WAIT_VBLANK DRM_IOWR(0x3a, union drm_wait_vblank) +#define DRM_IOCTL_CRTC_GET_SEQUENCE DRM_IOWR(0x3b, struct drm_crtc_get_sequence) +#define DRM_IOCTL_CRTC_QUEUE_SEQUENCE DRM_IOWR(0x3c, struct drm_crtc_queue_sequence) + #define DRM_IOCTL_UPDATE_DRAW DRM_IOW(0x3f, struct drm_update_draw) #define DRM_IOCTL_MODE_GETRESOURCES DRM_IOWR(0xA0, struct drm_mode_card_res) @@ -871,6 +895,7 @@ struct drm_event { #define DRM_EVENT_VBLANK 0x01 #define DRM_EVENT_FLIP_COMPLETE 0x02 +#define DRM_EVENT_CRTC_SEQUENCE 0x03 struct drm_event_vblank { struct drm_event base; @@ -881,6 +906,13 @@ struct drm_event_vblank { __u32 crtc_id; /* 0 on older kernels that do not support this */ }; +struct drm_event_crtc_sequence { + struct drm_event base; + __u64 user_data; + __u64 time_ns; + __u64 sequence; +}; + /* typedef area */ #ifndef __KERNEL__ typedef struct drm_clip_rect drm_clip_rect_t;
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> --- 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(+)