Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752701AbdHBJZN (ORCPT ); Wed, 2 Aug 2017 05:25:13 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:38059 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751145AbdHBJZM (ORCPT ); Wed, 2 Aug 2017 05:25:12 -0400 Date: Wed, 2 Aug 2017 11:25:05 +0200 From: Daniel Vetter To: Keith Packard Cc: linux-kernel@vger.kernel.org, Dave Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org Subject: Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2] Message-ID: <20170802092505.ynlpy6dxf3lxziim@phenom.ffwll.local> Mail-Followup-To: Keith Packard , linux-kernel@vger.kernel.org, Dave Airlie , dri-devel@lists.freedesktop.org References: <20170705221013.27940-1-keithp@keithp.com> <20170801050306.24423-1-keithp@keithp.com> <20170801050306.24423-4-keithp@keithp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170801050306.24423-4-keithp@keithp.com> X-Operating-System: Linux phenom 4.11.0-2-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12450 Lines: 377 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 > Suggested-by: Ville Syrj?l? > Signed-off-by: Keith Packard 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