Hi This doesn't seem to apply cleanly to master or 1.19.6, am I missing something?
Cheers Mike On Sun, 6 May 2018 at 06:35 Mario Kleiner <[email protected]> wrote: > The old 32-Bit wraparound handling didn't actually work, > due to some integer casting bug, and the mapping was ill > equipped to deal with input from the new true 64-bit > GetCrtcSequence/QueueCrtcSequence api's introduced in Linux > 4.15. > > For 32-Bit truncated input from pageflip events and old vblank > events and old drmWaitVblank ioctl, implement new wraparound > handling, which also allows to deal with wraparound in the other > direction, e.g., if a 32-Bit truncated sequence value is passed > in, whose true 64-Bit in-kernel hw value is within 2^30 counts > of the previous processed value, but whose 32-bit truncated > sequence value happens to lie just above or below a 2^32 > boundary, iow. one of the two values 'sequence' vs. 'msc_prev' > lies above a 2^32 border, the other one below it. > > The method is directly translated from Mesa's proven implementation > of the INTEL_swap_events extension, where a true underlying > 64-Bit wide swapbuffers count (SBC) needs to get reconstructed > from a 32-Bit LSB truncated SBC transported over the X11 protocol > wire. Same conditions apply, ie. successive true 64-Bit SBC > values are close to each other, but don't always get received > in strictly monotonically increasing order. See Mesa commit > cc5ddd584d17abd422ae4d8e83805969485740d9 ("glx: Handle > out-of-sequence swap completion events correctly. (v2)") for > explanation. > > Additionally add a separate path for true 64-bit msc input > originating from Linux 4.15+ drmCrtcGetSequence/QueueSequence > ioctl's and corresponding 64-bit vblank events. True 64-bit > msc's don't need remapping and must be passed through. > > As a reliability bonus, they are also used here to update the > tracking values msc_prev and ms_high with perfect 64-Bit ground > truth as baseline for mapping msc from pageflip completion events, > because pageflip events are always 32-bit wide, even when the new > kernel api's are used. Because each pageflip(-event) is always > preceeded close in time (and vblank count) by a drmCrtcQueueSequence > queued event or drmCrtcGetSequence query as part of DRI2 or DRI3+Present > swap scheduling, we can be certain that each pageflip event will get > its truncated 32-bit msc remapped reliably to the true 64-bit msc of > flip completion whenever the sequence api is available, ie. on Linux > 4.15 or later. > > Note: In principle at least the 32-bit mapping path could also > be backported to earlier server branches, as this seems to be > broken for at least server 1.16 to 1.19. > > Signed-off-by: Mario Kleiner <[email protected]> > Cc: Adam Jackson <[email protected]> > Cc: Keith Packard <[email protected]> > Cc: Michel Dänzer <[email protected]> > --- > hw/xfree86/drivers/modesetting/driver.h | 2 +- > hw/xfree86/drivers/modesetting/vblank.c | 66 > ++++++++++++++++++++++++++------- > 2 files changed, 54 insertions(+), 14 deletions(-) > > diff --git a/hw/xfree86/drivers/modesetting/driver.h > b/hw/xfree86/drivers/modesetting/driver.h > index 3a81d4d..55f3400 100644 > --- a/hw/xfree86/drivers/modesetting/driver.h > +++ b/hw/xfree86/drivers/modesetting/driver.h > @@ -149,7 +149,7 @@ xf86CrtcPtr ms_dri2_crtc_covering_drawable(DrawablePtr > pDraw); > > int ms_get_crtc_ust_msc(xf86CrtcPtr crtc, CARD64 *ust, CARD64 *msc); > > -uint64_t ms_kernel_msc_to_crtc_msc(xf86CrtcPtr crtc, uint64_t sequence); > +uint64_t ms_kernel_msc_to_crtc_msc(xf86CrtcPtr crtc, uint64_t sequence, > Bool is64bit); > > > Bool ms_dri2_screen_init(ScreenPtr screen); > diff --git a/hw/xfree86/drivers/modesetting/vblank.c > b/hw/xfree86/drivers/modesetting/vblank.c > index d1a6fc1..561229f 100644 > --- a/hw/xfree86/drivers/modesetting/vblank.c > +++ b/hw/xfree86/drivers/modesetting/vblank.c > @@ -239,7 +239,7 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags, > drm_flags, msc, &kernel_queued, > seq); > if (ret == 0) { > if (msc_queued) > - *msc_queued = ms_kernel_msc_to_crtc_msc(crtc, > kernel_queued); > + *msc_queued = ms_kernel_msc_to_crtc_msc(crtc, > kernel_queued, TRUE); > ms->has_queue_sequence = TRUE; > return TRUE; > } > @@ -262,7 +262,7 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags, > ret = drmWaitVBlank(ms->fd, &vbl); > if (ret == 0) { > if (msc_queued) > - *msc_queued = ms_kernel_msc_to_crtc_msc(crtc, > vbl.reply.sequence); > + *msc_queued = ms_kernel_msc_to_crtc_msc(crtc, > vbl.reply.sequence, FALSE); > return TRUE; > } > check: > @@ -275,28 +275,59 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag > flags, > } > > /** > - * Convert a 32-bit kernel MSC sequence number to a 64-bit local sequence > - * number, adding in the high 32 bits, and dealing with 64-bit wrapping. > + * Convert a 32-bit or 64-bit kernel MSC sequence number to a 64-bit local > + * sequence number, adding in the high 32 bits, and dealing with 32-bit > + * wrapping if needed. > */ > uint64_t > -ms_kernel_msc_to_crtc_msc(xf86CrtcPtr crtc, uint64_t sequence) > +ms_kernel_msc_to_crtc_msc(xf86CrtcPtr crtc, uint64_t sequence, Bool > is64bit) > { > drmmode_crtc_private_rec *drmmode_crtc = crtc->driver_private; > > - if ((int32_t) (sequence - drmmode_crtc->msc_prev) < -0x40000000) > - drmmode_crtc->msc_high += 0x100000000L; > + if (!is64bit) { > + /* sequence is provided as a 32 bit value from one of the 32 bit > apis, > + * e.g., drmWaitVBlank(), classic vblank events, or pageflip > events. > + * > + * Track and handle 32-Bit wrapping, somewhat robust against > occasional > + * out-of-order not always monotonically increasing sequence > values. > + */ > + if ((int64_t) sequence < ((int64_t) drmmode_crtc->msc_prev - > 0x40000000)) > + drmmode_crtc->msc_high += 0x100000000L; > + > + if ((int64_t) sequence > ((int64_t) drmmode_crtc->msc_prev + > 0x40000000)) > + drmmode_crtc->msc_high -= 0x100000000L; > + > + drmmode_crtc->msc_prev = sequence; > + > + return drmmode_crtc->msc_high + sequence; > + } > + > + /* True 64-Bit sequence from Linux 4.15+ 64-Bit drmCrtcGetSequence / > + * drmCrtcQueueSequence apis and events. Pass through sequence > unmodified, > + * but update the 32-bit tracking variables with reliable ground > truth. > + * > + * With 64-Bit api in use, the only !is64bit input is from pageflip > events, > + * and any pageflip event is usually preceeded by some is64bit input > from > + * swap scheduling, so this should provide reliable mapping for > pageflip > + * events based on true 64-bit input as baseline as well. > + */ > drmmode_crtc->msc_prev = sequence; > - return drmmode_crtc->msc_high + sequence; > + drmmode_crtc->msc_high = sequence & 0xffffffff00000000; > + > + return sequence; > } > > int > ms_get_crtc_ust_msc(xf86CrtcPtr crtc, CARD64 *ust, CARD64 *msc) > { > + ScreenPtr screen = crtc->randr_crtc->pScreen; > + ScrnInfoPtr scrn = xf86ScreenToScrn(screen); > + modesettingPtr ms = modesettingPTR(scrn); > uint64_t kernel_msc; > > if (!ms_get_kernel_ust_msc(crtc, &kernel_msc, ust)) > return BadMatch; > - *msc = ms_kernel_msc_to_crtc_msc(crtc, kernel_msc); > + *msc = ms_kernel_msc_to_crtc_msc(crtc, kernel_msc, > ms->has_queue_sequence); > > return Success; > } > @@ -416,7 +447,7 @@ ms_drm_abort(ScrnInfoPtr scrn, Bool (*match)(void > *data, void *match_data), > * drm event queue and calls the handler for it. > */ > static void > -ms_drm_sequence_handler(int fd, uint64_t frame, uint64_t ns, uint64_t > user_data) > +ms_drm_sequence_handler(int fd, uint64_t frame, uint64_t ns, Bool > is64bit, uint64_t user_data) > { > struct ms_drm_queue *q, *tmp; > uint32_t seq = (uint32_t) user_data; > @@ -425,7 +456,7 @@ ms_drm_sequence_handler(int fd, uint64_t frame, > uint64_t ns, uint64_t user_data) > if (q->seq == seq) { > uint64_t msc; > > - msc = ms_kernel_msc_to_crtc_msc(q->crtc, frame); > + msc = ms_kernel_msc_to_crtc_msc(q->crtc, frame, is64bit); > xorg_list_del(&q->list); > q->handler(msc, ns / 1000, q->data); > free(q); > @@ -435,10 +466,19 @@ ms_drm_sequence_handler(int fd, uint64_t frame, > uint64_t ns, uint64_t user_data) > } > > static void > +ms_drm_sequence_handler_64bit(int fd, uint64_t frame, uint64_t ns, > uint64_t user_data) > +{ > + /* frame is true 64 bit wrapped into 64 bit */ > + ms_drm_sequence_handler(fd, frame, ns, TRUE, user_data); > +} > + > +static void > ms_drm_handler(int fd, uint32_t frame, uint32_t sec, uint32_t usec, > void *user_ptr) > { > - ms_drm_sequence_handler(fd, frame, ((uint64_t) sec * 1000000 + usec) > * 1000, (uint32_t) (uintptr_t) user_ptr); > + /* frame is 32 bit wrapped into 64 bit */ > + ms_drm_sequence_handler(fd, frame, ((uint64_t) sec * 1000000 + usec) > * 1000, > + FALSE, (uint32_t) (uintptr_t) user_ptr); > } > > Bool > @@ -452,7 +492,7 @@ ms_vblank_screen_init(ScreenPtr screen) > ms->event_context.version = 4; > ms->event_context.vblank_handler = ms_drm_handler; > ms->event_context.page_flip_handler = ms_drm_handler; > - ms->event_context.sequence_handler = ms_drm_sequence_handler; > + ms->event_context.sequence_handler = ms_drm_sequence_handler_64bit; > > /* We need to re-register the DRM fd for the synchronisation > * feedback on every server generation, so perform the > -- > 2.7.4 > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
