[PATCH] Revert "DRI2: re-allocate DRI2 drawable if pixmap serial changes"

Eric Anholt eric at anholt.net
Wed Jun 5 12:34:24 PDT 2013


This reverts commit 3209b094a3b1466b579e8020e12a4f3fa78a5f3f.  After a
long debug session by Paul Berry, it appears that this was the commit
that has been producing sporadic failures in piglit front buffer
rendering tests for the last several years.

GetBuffers may return fresh buffers with invalid contents at a couple
reasonable times:

- When first asked for a non-fake-front buffer.
- When the drawable size is changed, an Invalidate has been sent, and
  obviously the app needs to redraw the whole buffer.
- After a glXSwapBuffers(), GL allows the backbuffer to be undefined,
  and an Invalidate was sent to tell the GL that it should grab these
  appropriate new buffers to avoid stalling.

But with the patch being reverted, GetBuffers would also return fresh
invalid buffers when the drawable serial number changed, which is
approximately "whenever, for any reason".  The app is not expecting
invalid buffer contents "whenever", nor is it valid.  Because the GL
usually only GetBuffers after an Invalidate is sent, and the new
buffer allocation only happened during a GetBuffers, most apps saw no
problems.  But apps that do (fake-)frontbuffer rendering do frequently
ask the server for the front buffer (since we drop the fake front
allocation when we're not doing front buffer rendering), and if the
drawable serial got bumped midway through a draw, the server would
pointlessly ditch the front *and* backbuffer full of important
drawing, resulting in bad rendering.

The patch was originally to fix bugzilla:
https://bugs.freedesktop.org/show_bug.cgi?id=28365
Specifically:

    To reproduce, start with a large-ish display (i.e. 1680x1050 on my
    laptop), use the patched glxgears from bug 28252 to add the
    -override option.  Then run glxgears -override -geometry 640x480
    to create a 640x480 window in the top left corner, which will work
    fine.  Next, run xrandr -s 640x480 and watch the fireworks.

I've tested with an override-redirect glxgears, both with vblank sync
enabled and disabled, both with gnome-shell and no window manager at
all, before and after this patch.  The only problem observed was that
before and after the revert, sometimes when alt-tabbing to kill my
gears after completing the test gnome-shell would get confused about
override-redirectness of the glxgears window (according to a log
message) and apparently not bother doing any further compositing.
---
 hw/xfree86/dri2/dri2.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 40963c3..0b047f0 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -99,7 +99,6 @@ typedef struct _DRI2Drawable {
     CARD64 last_swap_msc;       /* msc at completion of most recent swap */
     CARD64 last_swap_ust;       /* ust at completion of most recent swap */
     int swap_limit;             /* for N-buffering */
-    unsigned long serialNumber;
     Bool needInvalidate;
     int prime_id;
     PixmapPtr prime_slave_pixmap;
@@ -189,19 +188,6 @@ DRI2GetDrawable(DrawablePtr pDraw)
     }
 }
 
-static unsigned long
-DRI2DrawableSerial(DrawablePtr pDraw)
-{
-    ScreenPtr pScreen = pDraw->pScreen;
-    PixmapPtr pPix;
-
-    if (pDraw->type != DRAWABLE_WINDOW)
-        return pDraw->serialNumber;
-
-    pPix = pScreen->GetWindowPixmap((WindowPtr) pDraw);
-    return pPix->drawable.serialNumber;
-}
-
 static DRI2DrawablePtr
 DRI2AllocateDrawable(DrawablePtr pDraw)
 {
@@ -235,7 +221,6 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
     pPriv->last_swap_msc = 0;
     pPriv->last_swap_ust = 0;
     xorg_list_init(&pPriv->reference_list);
-    pPriv->serialNumber = DRI2DrawableSerial(pDraw);
     pPriv->needInvalidate = FALSE;
     pPriv->redirectpixmap = NULL;
     pPriv->prime_slave_pixmap = NULL;
@@ -493,7 +478,6 @@ allocate_or_reuse_buffer(DrawablePtr pDraw, DRI2ScreenPtr ds,
         || attachment == DRI2BufferFrontLeft
         || !dimensions_match || (pPriv->buffers[old_buf]->format != format)) {
         *buffer = create_buffer (pDraw, attachment, format);
-        pPriv->serialNumber = DRI2DrawableSerial(pDraw);
         return TRUE;
 
     }
@@ -559,8 +543,7 @@ do_get_buffers(DrawablePtr pDraw, int *width, int *height,
     ds = DRI2GetScreen(pDraw->pScreen);
 
     dimensions_match = (pDraw->width == pPriv->width)
-        && (pDraw->height == pPriv->height)
-        && (pPriv->serialNumber == DRI2DrawableSerial(pDraw));
+        && (pDraw->height == pPriv->height);
 
     buffers = calloc((count + 1), sizeof(buffers[0]));
     if (!buffers)
-- 
1.8.3.rc0



More information about the xorg-devel mailing list