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

[Michel Dänzer]

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 <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.

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 <michel.daenzer at amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer