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? > 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:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
