| Message ID | 20170801050306.24423-4-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
On 01/08/17 02:03 PM, 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> [...] > +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */ Do you have userspace making use of DRM_CRTC_SEQUENCE_NEXT_ON_MISS? If not, drop it. > +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */ I thought there was consensus that this flag is pointless.
Daniel Vetter <daniel@ffwll.ch> writes: > 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? Well, given that we'll have to keep the old API around for older kernels, at least for a decade or so, I'm not sure why we'd actually want that anytime soon, if ever? I guess it does provide 64-bit sequence numbers, which Present wants? > This way we could land this before the lease stuff for the > vk extension is all solid&ready. Do you think there's a pile more work to be done for the lease changes in the kernel? Or are you just trying to separate the work flows? I can go re-write the modesetting present support to use this new API and use that for testing the kernel, if you think that would help move the kernel bits along. >> + 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. So, in reality, the client can more-or-less tell that the crtc is disabled because the call fails? Sounds like I can just remove the little dance to get the CRTC enabled state entirely. I don't understand your comment about READ_ONCE(vblank->enabled); that doesn't relate to the crtc enabled state, I don't think? >> + >> +/* 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 ... (NEXT_ON_MISS is not used by the new Vulkan code; I added it only to keep compatibility with the old API, in case we want to switch someday). FIRST_PIXEL_OUT is an attempt to signal to the kernel that the application really wants to see the event when the first pixel hits the display. I assume the important thing here is the timestamp in the event and not the actual delivery, but I don't actually know that. If the timestamp is the only important thing, it sounds like the kernel already satisfies that, which is cool. If Vulkan really wants the event to be delivered when the first pixel is displayed, then having this bit in the ioctl means we can let drivers continue to do whatever they are now when the bit isn't set, but try harder to deliver the event at first-pixel when requested. So, I think what I want to do is leave the bit in the request so that drivers can at least see what user space is asking for, and if we learn that it's important to deliver the event at the requested time, we can go fix drivers later.
Michel Dänzer <michel@daenzer.net> writes: > [...] > >> +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */ > > Do you have userspace making use of DRM_CRTC_SEQUENCE_NEXT_ON_MISS? If > not, drop it. I added this so that the new ioctl would be compatible with the old ioctl; do you think that's unnecessary? > >> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */ > > I thought there was consensus that this flag is pointless. I just wrote a note to Daniel about this; I think it is useful in that applications could specify that they actually want the event delivered at first pixel out in accordance with the Vulkan spec, even if we can't do that (yet). I definitely agree that requiring the bit be set is ridiculous and should be removed. Two choices 1) Remove the code which checks whether the flag is set. Make Vulkan set the flag signaling what it wants. Plan on doing the actual driver work if we find that it's necessary. 2) Remove the flag entirely. Any preference?
On 06/08/17 12:32 PM, Keith Packard wrote: > Daniel Vetter <daniel@ffwll.ch> writes: > >>> +#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 ... > > [...] > > FIRST_PIXEL_OUT is an attempt to signal to the kernel that the > application really wants to see the event when the first pixel hits the > display. I assume the important thing here is the timestamp in the > event and not the actual delivery, but I don't actually know that. > > If the timestamp is the only important thing, it sounds like the kernel > already satisfies that, which is cool. > > If Vulkan really wants the event to be delivered when the first pixel is > displayed, then having this bit in the ioctl means we can let drivers > continue to do whatever they are now when the bit isn't set, but try > harder to deliver the event at first-pixel when requested. I don't see the point of giving this choice to userspace. The event timestamp specifies when first-pixel occurs; if it's in the future, userspace can use other functionality to wait until then if needed (though it's hard to imagine why it would be). > So, I think what I want to do is leave the bit in the request so that > drivers can at least see what user space is asking for, and if we learn > that it's important to deliver the event at the requested time, we can > go fix drivers later. This seems like a very bad idea: Having a flag which doesn't have any effect at first will result in userspace randomly setting the flag or not. If we were to then change the behaviour with the flag (not) set, some userspace will almost certainly break. So effectively we can never make the flag have any effect. The way to go here is to drop the flag for now and document the behaviour explicitly. If unexpectedly a real need for different behaviour comes up in the future, we can add a flag for it at that time.
On 06/08/17 12:42 PM, Keith Packard wrote: > Michel Dänzer <michel@daenzer.net> writes: > >> [...] >> >>> +#define DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002 /* Use next sequence if we've missed */ >> >> Do you have userspace making use of DRM_CRTC_SEQUENCE_NEXT_ON_MISS? If >> not, drop it. > > I added this so that the new ioctl would be compatible with the old > ioctl; do you think that's unnecessary? I do. I don't know if there's ever been any real-world usage of DRM_VBLANK_NEXTONMISS. Let's not repeat my mistake in the new interface.
On Sun, Aug 6, 2017 at 5:32 AM, Keith Packard <keithp@keithp.com> wrote: > Daniel Vetter <daniel@ffwll.ch> writes: > >> 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? > > Well, given that we'll have to keep the old API around for older > kernels, at least for a decade or so, I'm not sure why we'd actually > want that anytime soon, if ever? I guess it does provide 64-bit sequence > numbers, which Present wants? I just figured that -modesetting would be the simplest domenstration vehicle, since the vulkan patches don't look ready yet. I need fully reviewed&tested userspace before we can land any kernel stuff. Doing the quick modesetting conversion would unblock. >> This way we could land this before the lease stuff for the >> vk extension is all solid&ready. > > Do you think there's a pile more work to be done for the lease changes > in the kernel? Or are you just trying to separate the work flows? > > I can go re-write the modesetting present support to use this new API > and use that for testing the kernel, if you think that would help move > the kernel bits along. Just trying to separate flows and get stuff landed as soon as it's ready. There's always the chance someone rewrites the code meanwhile if we wait until all the vk stuff is ready. >>> + 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. > > So, in reality, the client can more-or-less tell that the crtc is > disabled because the call fails? Sounds like I can just remove the > little dance to get the CRTC enabled state entirely. I don't understand > your comment about READ_ONCE(vblank->enabled); that doesn't relate to > the crtc enabled state, I don't think? It's supposed to, at least for atomic drivers. For legacy kms drivers they're supposed to reject the enable attempt with some error code, when the CRTC is off. It's all pretty awkward ad-hoc uabi :-( I'm leaning more-and-more towards just dropping this part as a bad idea from my side. At least until we have someone who really needs this. >>> + >>> +/* 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 ... > > (NEXT_ON_MISS is not used by the new Vulkan code; I added it only to keep > compatibility with the old API, in case we want to switch someday). > > FIRST_PIXEL_OUT is an attempt to signal to the kernel that the > application really wants to see the event when the first pixel hits the > display. I assume the important thing here is the timestamp in the > event and not the actual delivery, but I don't actually know that. > > If the timestamp is the only important thing, it sounds like the kernel > already satisfies that, which is cool. Would be good to confirm that. If it's not, we have a problem. > If Vulkan really wants the event to be delivered when the first pixel is > displayed, then having this bit in the ioctl means we can let drivers > continue to do whatever they are now when the bit isn't set, but try > harder to deliver the event at first-pixel when requested. > > So, I think what I want to do is leave the bit in the request so that > drivers can at least see what user space is asking for, and if we learn > that it's important to deliver the event at the requested time, we can > go fix drivers later. Not sure that's a good idea without fixing up drivers. Asking for something that's not delivered just makes that bit meaningless. Atm the ioctl is also rejected if you don't set this flag, so it essentially means whatever current drivers do. And I think it'd be good to at least document that, and maybe even drop the bitflag (since it doesn't encode anything, at least in the current patch). -Daniel
Daniel Vetter <daniel@ffwll.ch> writes: > I just figured that -modesetting would be the simplest domenstration > vehicle, since the vulkan patches don't look ready yet. I need fully > reviewed&tested userspace before we can land any kernel stuff. Doing > the quick modesetting conversion would unblock. I've provided a patch for the modesetting driver and the preliminary bits are merged, leaving only a fairly straightforward addition of the new ioctls to that code. I'm not sure how to make more progress there at this point; that code would need testing, and it requires a hand-patched kernel to test. I also posted IGT tests for the new functionality, again, getting those reviewed and tested depends on someone willing to build a patched kernel. Dave Airlie has started trying to get that done. >> (regarding FIRST_PIXEL_OUT): >> >> If the timestamp is the only important thing, it sounds like the kernel >> already satisfies that, which is cool. > > Would be good to confirm that. If it's not, we have a problem. Michel Dänzer says that the timestamp provided is computed to be first_pixel_out for all hardware. Given that, I suggest we specify that in the documentation and remove this bit from the API. Seem reasonable?
On Mon, Oct 09, 2017 at 10:18:30AM -0700, Keith Packard wrote: > Daniel Vetter <daniel@ffwll.ch> writes: > > > I just figured that -modesetting would be the simplest domenstration > > vehicle, since the vulkan patches don't look ready yet. I need fully > > reviewed&tested userspace before we can land any kernel stuff. Doing > > the quick modesetting conversion would unblock. > > I've provided a patch for the modesetting driver and the preliminary > bits are merged, leaving only a fairly straightforward addition of the > new ioctls to that code. I'm not sure how to make more progress there at > this point; that code would need testing, and it requires a hand-patched > kernel to test. > > I also posted IGT tests for the new functionality, again, getting those > reviewed and tested depends on someone willing to build a patched > kernel. Dave Airlie has started trying to get that done. > > >> (regarding FIRST_PIXEL_OUT): > >> > >> If the timestamp is the only important thing, it sounds like the kernel > >> already satisfies that, which is cool. > > > > Would be good to confirm that. If it's not, we have a problem. > > Michel Dänzer says that the timestamp provided is computed to be > first_pixel_out for all hardware. Given that, I suggest we specify that > in the documentation and remove this bit from the API. > > Seem reasonable? Ack on all. It looks like Dave is taking care of merging too, in that case ack on all these patches from my side. Thanks, Daniel
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); + 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 */ + +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;
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> --- 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(+)