Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls

[Keith Packard]

Daniel Vetter <daniel@xxxxxxxx> 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.

-- 
-keith

Attachment: signature.asc
Description: PGP signature