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

[Michel Dänzer]

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?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer