On 29.09.2015 11:55, Fredrik Höglund wrote: > On Friday 11 September 2015, Michel Dänzer wrote: >> On 11.09.2015 06:33, Fredrik Höglund wrote: >>> Otherwise we stash an uninitalized value, and later use it to compute >>> the msc_offset for the window. Also initialize ust and crtc_msc so we >>> never use uninitalized values when present_get_ust_msc fails. >>> >>> This fixes clients getting stuck waiting indefinitely for an idle >>> event when a CRTC is turned off. >>> >>> Signed-off-by: Fredrik Höglund <[email protected]> >>> --- >>> present/present.c | 14 ++++++++------ >>> 1 file changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/present/present.c b/present/present.c >>> index a634601..7ddffbd 100644 >>> --- a/present/present.c >>> +++ b/present/present.c >>> @@ -710,9 +710,9 @@ present_pixmap(WindowPtr window, >>> present_notify_ptr notifies, >>> int num_notifies) >>> { >>> - uint64_t ust; >>> + uint64_t ust = 0; >>> uint64_t target_msc; >>> - uint64_t crtc_msc; >>> + uint64_t crtc_msc = 0; >>> int ret; >>> present_vblank_ptr vblank, tmp; >>> ScreenPtr screen = window->drawable.pScreen; >>> @@ -734,13 +734,15 @@ present_pixmap(WindowPtr window, >>> target_crtc = present_get_crtc(window); >>> } >>> >>> - present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc); >>> + ret = present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc); >>> >>> target_msc = present_window_to_crtc_msc(window, target_crtc, >>> window_msc, crtc_msc); >>> >>> - /* Stash the current MSC away in case we need it later >>> - */ >>> - window_priv->msc = crtc_msc; >>> + if (ret == Success) { >>> + /* Stash the current MSC away in case we need it later >>> + */ >>> + window_priv->msc = crtc_msc; >>> + } >>> >>> /* Adjust target_msc to match modulus >>> */ >>> >> >> My only doubt is about present_window_to_crtc_msc(). If target_msc >> doesn't match window_priv->crtc, presumably present_get_ust_msc() will >> fail there as well, so window_priv->msc (the last recorded valid MSC >> value for the window) will be added to window_priv->msc_offset. I >> suspect that's not right and might still cause similar trouble down the >> road. > > window_priv->msc is updated after the call to present_window_to_crtc_msc(), > so the old MSC value will hopefully be valid in that call. The bigger > issue is that crtc_msc is not valid, so we still set msc_offset to an > incorrect value. > > I'm not sure if it's possible to fully fix this issue while still allowing > present_get_ust_msc() to fail.
Maybe not. > The deadlock I'm seeing happens with an application that alternates > between calling GetSyncValuesOML() and SwapBuffersMscOML(). In the > GetSyncValuesOML() call, pixmap is NULL, so present_pixmap() picks the > CRTC stored in window_priv. It's when that CRTC is off that we end up > storing an uninitialized value in window_priv->msc. Note that > present_window_to_crtc_msc() doesn't update window_priv->msc_offset > in this case since the CRTC's match. In the next call to SwapBuffersOML(), > pixmap is not NULL, so present_pixmap() calls present_get_crtc() which > returns NULL. This causes present_window_to_crtc_msc() to update > window_priv->msc_offset using the now invalid window_priv->msc value. > As a result we compute a target MSC for the vblank that's several weeks > into the future. Thanks for the detailed explanation. It might be nice to work that into the Git commit log, but either way, Reviewed-by: Michel Dänzer <[email protected]> -- 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
