| Message ID | 86y3s1bkf4.fsf@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 6, 2017 at 6:27 PM, Keith Packard <keithp@keithp.com> wrote: > 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. Was just a thought, I don't mind either way really I think. >>> + >>> +/* >>> + * 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. Yeah that \param stuff is all back from really old libdrm. Just shows how old this code is :-) >>> + >>> +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? Yup. > > 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; > } Yeah looks good. >> 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: Yeah the annoying bit is that we have to grab the mutex. I think better to postpone this (we can always add a flag) and only do it when there's a real need. Was just an idea that might be useful. > --- 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. So the event we generate should be for the very first pixel iirc, or top of frame or whatever OML_sync says, so it should match vulkan I think. I just meant we can remove the #define since it's rejected anyway (we reject all unknown flags), and we can easily add it later on. >> 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 :-) Yeah I'm not there yet :-) >> 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. I think the size should be a problem, since the old vblank ioctl uses sizeof(e->event.vbl), extending event.seq shouldn't be a problem. But we can also postpone this, since iirc we've done the events correctly and you can extend them at the end. >> 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. As long as we reject any noise in unused bits/members we can extend them. No need to have an explicit #define to reject a special bit. >> 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. tbh that "is the crtc active" was just an idea, I think if you don't have an immediate use for it in the vk extension we can leave it for later. -Daniel
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)