glXSwapBuffers fix for moving between crtcs is not following the OML_sync_control specification

[Pauli Nieminen]

Hi,

commit 75beadd766fed7b12a76e59e57c244e297c2d2cb
Author: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
Date:   Sun Jun 13 18:05:26 2010 +0200

    DRI2/xserver: Don't hang in glXSwapBuffers if drawable moves
between crtc's (bug 28383)

    Detect if a drawable has been moved from an original crtc to a new crtc
    with a lower current vblank count than the original crtc inbetween
    glXSwapBuffers() calls. Reinitialize drawable's last_swap_target
    before scheduling next swap if such a move has taken place.

    last_swap_target defines the baseline for scheduling the next swap.
    If a movement between crtc's is not taken into account, the swap may
    schedule for a vblank count on the new crtc far in the future, resulting
    in a apparent "hang" of the drawable for a long time.

    Fixes Bugzilla bug #28383.

    Signed-off-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
    Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
    Signed-off-by: Keith Packard <keithp at keithp.com>

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index a8dedfa..f9ba8e7 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -759,6 +759,7 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr
pDraw, CARD64 target_msc,
     DRI2DrawablePtr pPriv;
     DRI2BufferPtr   pDestBuffer = NULL, pSrcBuffer = NULL;
     int             ret, i;
+    CARD64          ust, current_msc;

     pPriv = DRI2GetDrawable(pDraw);
     if (pPriv == NULL) {
@@ -803,12 +804,26 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr
pDraw, CARD64 target_msc,
      * need to schedule a swap for the last swap target + the swap interval.
      */
     if (target_msc == 0 && divisor == 0 && remainder == 0) {
+       /* If the current vblank count of the drawable's crtc is lower
+        * than the count stored in last_swap_target from a previous swap
+        * then reinitialize last_swap_target to the current crtc's msc,
+        * otherwise the swap will hang. This will happen if the drawable
+        * is moved to a crtc with a lower refresh rate, or a crtc that just
+        * got enabled.
+        */
+       if (!(*ds->GetMSC)(pDraw, &ust, &current_msc))
+           pPriv->last_swap_target = 0;
+
+       if (current_msc < pPriv->last_swap_target)
+           pPriv->last_swap_target = current_msc;
+
        /*
         * Swap target for this swap is last swap target + swap interval since
         * we have to account for the current swap count, interval, and the
         * number of pending swaps.
         */
        *swap_target = pPriv->last_swap_target + pPriv->swap_interval;
+
     } else {
        /* glXSwapBuffersMscOML could have a 0 target_msc, honor it */


This commit assumes that MSC maybe different depending on where the
drawable is. While specification says "For a multi-monitor system, the
monitor used to determine MSC is screen 0 of <display>." which IMO
means that there should be single MSC everywhere were target drawable
can move. So correct fix would be making MSC value in-depend of CRTC
where drawable is.

I don't know if correct place for that is DRI2 or driver code.  In
current DRI2 way od doing stuff it would be driver code but this would
results duplicating same code to all ddx drivers. I would prefer DRI2
side implementation helpers that can be configured easily by driver to
work correctly with underlying hw.

But actual patch has two problems. First, GetMSC is not NULL checked
before calling it. This causes crash with driver that is using
flipping without OML_sync_control.  Second, same freeze would happen
for application that is using OML_sync_control if it was moved between
areas where is different MSC. In my option actual patch should be
reverted because it is not correct fix for the bug.

Pauli