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

[Martin Peres]

On 20/01/16 03:57, Michel Dänzer wrote:
> On 16.01.2016 02:03, Keith Packard wrote:
>> 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>
>
> Thanks. May I push myself, or any takers?

Please also include in the stable branches :)

Got a hang this morning though, but not as bad as usual and ... I had my 
GPU driver in a borked state to begin with and I failed to get a 
backtrace. Will try to reproduce for a little longer to see if it fixes 
some bugs so I can close them.