Daniel Vetter daniel@ffwll.ch writes:
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.
Thanks for your kind words.
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).
In the absence of a suitable EGL api, I'm not sure what to suggest, other than fixing EGL instead of blaming the kernel...
However, for the leasing stuff, this doesn't really matter as I've got a master FD to play with, so if you wanted to restrict it to master, that'd be fine by me.
+/*
- 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.
I'm just trying to follow along with the local "conventions" in the file. Let me know if you have a future plan to make this better and I'll just reformat to suit.
+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.
Like this?
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index e39b2bd074e4..3738ff484f36 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1712,6 +1712,9 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, struct drm_crtc_get_sequence *get_seq = data; struct timespec now;
+ if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + if (!dev->irq_enabled) return -EINVAL;
@@ -1749,6 +1752,9 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data, int ret; unsigned long spin_flags;
+ if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + if (!dev->irq_enabled) return -EINVAL;
- 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.
Right, I saw that code in the wait_vblank case and forgot to carry it over. Here's a duplicate of what that function does; we'll need this code in any case for drivers which don't provide the necessary support for accurate vblank counts:
--- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1711,6 +1711,8 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, int pipe; struct drm_crtc_get_sequence *get_seq = data; struct timespec now; + bool vblank_enabled; + int ret;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; @@ -1724,8 +1726,19 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
pipe = drm_crtc_index(crtc);
+ vblank_enabled = dev->vblank_disable_immediately && READ_ONCE(vblank->enabled); + + if (!vblank_enabled) { + ret = drm_vblank_get(dev, pipe); + if (ret) { + DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret); + return ret; + } + } get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now); get_seq->sequence_ns = timespec_to_ns(&now); + if (!vblank_enabled) + drm_vblank_put(dev, pipe); return 0; }
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.
Hrm. It's certainly easy to do, however an application using this without knowing the enabled state of the crtc is probably not doing the right thing...
Here's what that looks like, I think, in case we want to do this:
--- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1735,6 +1735,12 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, 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 = timespec_to_ns(&now); if (!vblank_enabled)
- /* 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?
It's part of Vulkan, so I want to expose it in the kernel API. And, making sure user-space isn't setting unexpected bits seems like a good idea.
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.
Sure. I'll note that this is just a wrapper around drm_vblank_get/put at this point :-)
I think here there's no need for the accurate version, since we force-enable the vblanks already.
Agreed.
- 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 feared changing the size of the event and so I left that out. And, yes, userspace will almost certainly encode a pointer in the user_data, which can include whatever information it needs.
But might be useful just for consistency.
This would require making the event larger, which seems like a bad idea...
+#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.
Vulkan has this single define, but may have more in the future so I wanted to make sure the application was able to tell if the kernel supported new modes if/when they appear. Reliably returning -EINVAL today when the application asks for something which isn't supported seems like good practice.
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.
Good idea. Above, you asked me to return whether the crtc was active in the get_sequence ioctl; I suggested putting that in the existing pad field, which would leave the whole structure defined.
I've got tiny patches for each piece which I've stuck on my drm-sequence-64 branch at
git://people.freedesktop.org/~keithp/linux.git drm-sequence-64
Most of those are included above, with the exception of the drm_crtc_vblank_get/put changes.
Thanks much for your careful review.