lkml.org 
[lkml]   [2017]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls
    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

    \
     
     \ /
      Last update: 2017-07-06 12:16    [W:6.435 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site