On 15/01/16 19:03, Keith Packard wrote:
Michel Dänzer <mic...@daenzer.net> writes:From: Michel Dänzer <michel.daen...@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.daen...@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 <kei...@keithp.com>
Same here, thanks for tracking this down! Reviewed-by: Martin Peres <martin.pe...@linux.intel.com> _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
