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

[Martin Peres]

On 15/01/16 19: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>

So far, it seems to fix an issue I have had intermittently on my home 
machine which made it very hard to debug. I will keep you updated if I 
ever get a hang again with your patch applied.

>
>> ---
>>   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?

Sounds reasonable, indeed. Which header do you have in mind?

>
> In any case; this all lgtm.
>
> Reviewed-by: Keith Packard <keithp at keithp.com>

Same here, thanks for tracking this down!

Reviewed-by: Martin Peres <martin.peres at linux.intel.com>