[PATCH xserver] modesetting: Fix and improve ms_kernel_msc_to_crtc_msc()
Mike Lothian
mike at fireburn.co.uk
Mon May 7 08:58:07 UTC 2018
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 <mario.kleiner.de at gmail.com>
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 <mario.kleiner.de at gmail.com>
> Cc: Adam Jackson <ajax at redhat.com>
> Cc: Keith Packard <keithp at keithp.com>
> Cc: Michel Dänzer <michel.daenzer at amd.com>
> ---
> 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
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.x.org/archives/xorg-devel/attachments/20180507/a6d5f1a2/attachment-0001.html>
More information about the xorg-devel
mailing list