On 20/01/16 03:57, Michel Dänzer wrote:
On 16.01.2016 02: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>
---
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 <kei...@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.
_______________________________________________
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