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

[Daniel Vetter]

On Sun, Aug 6, 2017 at 5:32 AM, Keith Packard <keithp@xxxxxxxxxx> wrote:
> Daniel Vetter <daniel@xxxxxxxx> 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
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch