[PATCH xserver] present: Handle wraparound when comparing MSC values

[Keith Packard]

Michel Dänzer <michel at daenzer.net> writes:

> From: Michel Dänzer <michel.daenzer at amd.com>
>
> When a window moves from one CRTC to another, present_window_to_crtc_msc
> updates window_priv->msc_offset according to the delta between the
> current MSC values of the old and new CRTC:
>
>             window_priv->msc_offset += new_msc - old_msc;
>
> window_priv->msc_offset is initially 0, so if new_msc < old_msc,
> window_priv->msc_offset wraps around and becomes a large number. If the
> window_msc parameter passed in is small (in particular if it's 0, such as
> is the case when the client just wants to know the current window MSC
> value), the returned CRTC MSC value may still be a large number. In that
> case, the existing MSC comparisons in pixmap_present weren't working as
> intended, resulting in scheduling a wait far into the future when the
> target MSC had actually already passed. This would result in the client
> (e.g. the Chromium browser) hanging when moving its window between CRTCs.
>
> In order to fix this, introduce msc_is_(equal_or_)after helper functions
> which take the wraparound into account for comparing two MSC values.
>
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>



> ---
>  present/present.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/present/present.c b/present/present.c
> index 66e0f21..8cf3b6f 100644
> --- a/present/present.c
> +++ b/present/present.c
> @@ -717,6 +717,28 @@ present_execute(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc)
>      present_vblank_destroy(vblank);
>  }
>  
> +/*
> + * Returns:
> + * TRUE if the first MSC value is after the second one
> + * FALSE if the first MSC value is equal to or before the second one
> + */
> +static Bool
> +msc_is_after(uint64_t test, uint64_t reference)
> +{
> +    return (int64_t)(test - reference) > 0;
> +}
> +
> +/*
> + * Returns:
> + * TRUE if the first MSC value is equal to or after the second one
> + * FALSE if the first MSC value is before the second one
> + */
> +static Bool
> +msc_is_equal_or_after(uint64_t test, uint64_t reference)
> +{
> +    return (int64_t)(test - reference) >= 0;
> +}
> +

These should probably get declared as inline, although the compiler will
probably just inline them anyways.

I also won't be surprised if we end needing to expose these in a header
for other code to use too. Should we just do that now so that someone
needing to compare values finds them?

In any case; this all lgtm.

Reviewed-by: Keith Packard <keithp at keithp.com>

-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20160115/c5360428/attachment.sig>