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

[Michel Dänzer]

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);
+        }
+    }

     target_msc = present_window_to_crtc_msc(window, target_crtc, window_msc, crtc_msc);




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