[PATCH] present: Don't stash the MSC value when present_get_ust_msc fails

[Fredrik Höglund]

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 <fredrik at kde.org>
> > ---
> >  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.

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.

> That said, it's probably an unlikely scenario, and your change certainly
> looks like an improvement.
> 
> 
> 

Fredrik