[PATCH 2/3] present: Query the Window's CRTC every time

[Michel Dänzer]

On 28.05.2015 17:38, Chris Wilson wrote:
> On Thu, May 28, 2015 at 04:59:14PM +0900, Michel Dänzer wrote:
>> On 27.05.2015 15:51, Chris Wilson wrote:
>>> On Tue, May 26, 2015 at 02:30:32PM -0700, Keith Packard wrote:
>>>> Michel Dänzer <michel at daenzer.net> writes:
>>>>
>>>>> The old code also called present_get_crtc() unless pixmap == NULL, so
>>>>> the problem couldn't affect flips but only MSC waits.
>>>>
>>>> The original code was trying to let the client control which CRTC to
>>>> track for each window. For PresentPixmap requests, there's an explicit
>>>> CRTC argument, which allows the client to select the right one. For
>>>> PresentNotifyMSC, there's no such argument.
>>>>
>>>> So, the code was trying to respect the client's wishes by using
>>>> whichever CRTC was last associated with the window -- either implicitly
>>>> through the last call to present_get_crtc or explicitly from the last
>>>> crtc passed to PresentPixmap for the same window.
>>>>
>>>> Probably the right thing to do is to add an explicit CRTC parameter to
>>>> PresentNotifyMSC, and then have both requests call present_get_crtc when
>>>> an explicit CRTC is not provided.
>>>>
>>>> That doesn't solve the problem when an explicitly requested CRTC is
>>>> disabled though, so this fix doesn't address the root cause, only one of
>>>> the symptoms.
>>>>
>>>>> The problem I was hitting was that this code was running for an MSC wait
>>>>> when the CRTC referenced by window_priv->crtc was already disabled for
>>>>> DPMS off. This caused hangs at the GNOME lock screen. This patch seems
>>>>> to fix that problem.
>>>>
>>>> Why isn't your queue_vblank function bailing when asked to queue a
>>>> request for a CRTC which is disabled? If it simply fails,
>>>> present_execute will get called immediately and the client would
>>>> continue happily along.
>>>
>>> Oh, but it does fail gracefully. The problem is not that but that it
>>> sends a garbage msc to a valid CRTC.
>>
>> The patch below is an alternative fix for the problem I'm seeing, while
>> preserving the window CRTC for MSC waits when possible. Your
>> description sounds like it might work for you as well, Chris. Can you
>> try it?
>>
>>
>> diff --git a/present/present.c b/present/present.c
>> index a634601..dc58f25 100644
>> --- a/present/present.c
>> +++ b/present/present.c
>> @@ -713,6 +713,7 @@ present_pixmap(WindowPtr window,
>>      uint64_t                    ust;
>>      uint64_t                    target_msc;
>>      uint64_t                    crtc_msc;
>> +    RRCrtcPtr                   new_crtc;
>>      int                         ret;
>>      present_vblank_ptr          vblank, tmp;
>>      ScreenPtr                   screen = window->drawable.pScreen;
>> @@ -734,7 +735,21 @@ present_pixmap(WindowPtr window,
>>              target_crtc = present_get_crtc(window);
>>      }
>>
>> -    present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc);
>> +    if (present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc) != Success) {
>> +        /* Maybe we need to try a different CRTC?
>> +         */
>> +        new_crtc = present_get_crtc(window);
>> +
>> +        if (new_crtc != target_crtc &&
>> +            present_get_ust_msc(screen, new_crtc, &ust, &crtc_msc) == Success)
>> +            target_crtc = new_crtc;
>> +        else {
>> +            /* Fall back to fake MSC handling
>> +             */
>> +            target_crtc = NULL;
>> +            present_fake_get_ust_msc(screen, &ust, &crtc_msc);
>> +        }
>> +    }
> 
> It survived for one more CRTC change, but still feeds passed msc into
> the wait_vblank.

By how much is it off? Does it cause a hang?

What's your test-case? Mine is the GNOME lock screen, i.e. basically
DPMS off vs vblank waits and flips.


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