On 16.01.2016 02:03, Keith Packard wrote: > Michel Dänzer <[email protected]> writes: > >> From: Michel Dänzer <[email protected]> >> >> 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 <[email protected]> > > > >> --- >> 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 <[email protected]>
Thanks. May I push myself, or any takers? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
