[PATCH xserver] modesetting: Fix and improve ms_kernel_msc_to_crtc_msc()
Mario Kleiner
mario.kleiner.de at gmail.com
Mon May 7 15:42:46 UTC 2018
On Mon, May 7, 2018 at 10:58 AM, Mike Lothian <mike at fireburn.co.uk> wrote:
> Hi
>
> This doesn't seem to apply cleanly to master or 1.19.6, am I missing
> something?
>
> Cheers
>
> Mike
>
Hi Mike,
it needs the previous patch "[PATCH xserver] modesetting: Remove
ms_crtc_msc_to_kernel_msc()." applied to master first. That one should
also apply to 1.19, but this one is for 1.20 only.
-mario
> 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
More information about the xorg-devel
mailing list