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

From: "Keith Packard" <keithp@keithp.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls [v2]
Date: Sat, 05 Aug 2017 23:32:39 -0400	[thread overview]
Message-ID: <87r2wpfk14.fsf@keithp.com> (raw)
In-Reply-To: <20170802092505.ynlpy6dxf3lxziim@phenom.ffwll.local>

[-- Attachment #1: Type: text/plain, Size: 3974 bytes --]

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.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]