Bad caching of the scratch pixmap

[Daniel Drake]

Hi,

At http://dev.laptop.org/ticket/12542 we are seeing a problem where on
OLPC XO-1.75 and XO-4, after rotating the screen with RandR, pixmaps
often get drawn badly.

e.g. launching the following via xinit shows the problem:
	#!/bin/bash
	xrandr -o left
	xrandr -o normal
	./pixbuf.py

where pixbuf.py is a simple pygtk app that draws an image pixbuf on the
screen. In this case the image does not show, it is either all-black or
heavily corrupted in some way.

The hardware here is a vivante GPU embedded into a Marvell ARM SoC.
The DDX is http://dev.laptop.org/git/projects/xf86-video-dove/
This calls into a "galcore" GPU driver on the kernel side via libgfx
http://dev.laptop.org/git/olpc-kernel/tree/drivers/gpu/galcore?h=arm-3.5
Sadly libgfx is proprietary (grumble) - sorry.

The DDX implements its own pixmap management (via CreatePixmap2) and RandR
rotation is done with EXA (we support composite operations including
a transformation matrix that results in rotation). The CreatePixmap2
implementation creates some kind of GPU object to track the pixmap.

Adding this patch solves the issue:
http://lists.x.org/archives/xorg-devel/2011-June/023004.html
http://lists.x.org/archives/xorg-devel/2011-June/023008.html

Does it sound likely that we are hitting the same type of issue
described there (is there any easy way to verify this)? If so, can we
move forward with this patch? It looks like it was previously half-acked:
http://lists.x.org/archives/xorg-devel/2011-June/023014.html

Here is the updated patch for latest xserver. The totalPixmapSize move
into screenInfo in the original patch doesn't seem appropriate any more,
as PRIVATE_PIXMAP is now a screen-specific private element.

---
 dix/main.c           |  2 --
 dix/pixmap.c         | 40 +++++++++++++++-------------------------
 include/pixmap.h     |  2 --
 include/scrnintstr.h |  4 ++--
 4 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/dix/main.c b/dix/main.c
index bea1a8d..4fccdd4 100644
--- a/dix/main.c
+++ b/dix/main.c
@@ -333,7 +333,6 @@ main(int argc, char *argv[], char *envp[])
 
         for (i = screenInfo.numGPUScreens - 1; i >= 0; i--) {
             ScreenPtr pScreen = screenInfo.gpuscreens[i];
-            FreeScratchPixmapsForScreen(pScreen);
             (*pScreen->CloseScreen) (pScreen);
             dixFreePrivates(pScreen->devPrivates, PRIVATE_SCREEN);
             free(pScreen);
@@ -341,7 +340,6 @@ main(int argc, char *argv[], char *envp[])
         }
 
         for (i = screenInfo.numScreens - 1; i >= 0; i--) {
-            FreeScratchPixmapsForScreen(screenInfo.screens[i]);
             FreeGCperDepth(i);
             FreeDefaultStipple(i);
             dixFreeScreenSpecificPrivates(screenInfo.screens[i]);
diff --git a/dix/pixmap.c b/dix/pixmap.c
index 2418812..3cc8239 100644
--- a/dix/pixmap.c
+++ b/dix/pixmap.c
@@ -51,20 +51,18 @@ PixmapPtr
 GetScratchPixmapHeader(ScreenPtr pScreen, int width, int height, int depth,
                        int bitsPerPixel, int devKind, pointer pPixData)
 {
-    PixmapPtr pPixmap = pScreen->pScratchPixmap;
+    PixmapPtr pPixmap;
 
-    if (pPixmap)
-        pScreen->pScratchPixmap = NULL;
-    else
-        /* width and height of 0 means don't allocate any pixmap data */
-        pPixmap = (*pScreen->CreatePixmap) (pScreen, 0, 0, depth, 0);
+    /* width and height of 0 means don't allocate any pixmap data */
+    pPixmap = (*pScreen->CreatePixmap) (pScreen, 0, 0, depth, 0);
+    if (pPixmap == NullPixmap)
+	return NullPixmap;
 
-    if (pPixmap) {
-        if ((*pScreen->ModifyPixmapHeader) (pPixmap, width, height, depth,
-                                            bitsPerPixel, devKind, pPixData))
-            return pPixmap;
-        (*pScreen->DestroyPixmap) (pPixmap);
-    }
+    if ((*pScreen->ModifyPixmapHeader)(pPixmap, width, height, depth,
+				       bitsPerPixel, devKind, pPixData))
+	return pPixmap;
+
+    (*pScreen->DestroyPixmap)(pPixmap);
     return NullPixmap;
 }
 
@@ -75,11 +73,11 @@ FreeScratchPixmapHeader(PixmapPtr pPixmap)
     if (pPixmap) {
         ScreenPtr pScreen = pPixmap->drawable.pScreen;
 
-        pPixmap->devPrivate.ptr = NULL; /* lest ddx chases bad ptr */
-        if (pScreen->pScratchPixmap)
-            (*pScreen->DestroyPixmap) (pPixmap);
-        else
-            pScreen->pScratchPixmap = pPixmap;
+	if (pPixmap->refcnt != 1)
+		FatalError("Scratch pixmap still in use when finalized, refcnt=%d\n",
+			   pPixmap->refcnt);
+
+	(*pScreen->DestroyPixmap)(pPixmap);
     }
 }
 
@@ -92,17 +90,9 @@ CreateScratchPixmapsForScreen(ScreenPtr pScreen)
     pScreen->totalPixmapSize =
         BitmapBytePad(pixmap_size * 8);
 
-    /* let it be created on first use */
-    pScreen->pScratchPixmap = NULL;
     return TRUE;
 }
 
-void
-FreeScratchPixmapsForScreen(ScreenPtr pScreen)
-{
-    FreeScratchPixmapHeader(pScreen->pScratchPixmap);
-}
-
 /* callable by ddx */
 PixmapPtr
 AllocatePixmap(ScreenPtr pScreen, int pixDataSize)
diff --git a/include/pixmap.h b/include/pixmap.h
index 921a94d..e40463c 100644
--- a/include/pixmap.h
+++ b/include/pixmap.h
@@ -105,8 +105,6 @@ extern _X_EXPORT void FreeScratchPixmapHeader(PixmapPtr /*pPixmap */ );
 
 extern _X_EXPORT Bool CreateScratchPixmapsForScreen(ScreenPtr /*pScreen */ );
 
-extern _X_EXPORT void FreeScratchPixmapsForScreen(ScreenPtr /*pScreen */ );
-
 extern _X_EXPORT PixmapPtr AllocatePixmap(ScreenPtr /*pScreen */ ,
                                           int /*pixDataSize */ );
 
diff --git a/include/scrnintstr.h b/include/scrnintstr.h
index df74073..dcbc61d 100644
--- a/include/scrnintstr.h
+++ b/include/scrnintstr.h
@@ -181,6 +181,8 @@ typedef void (*ClipNotifyProcPtr) (WindowPtr /*pWindow */ ,
 #define CREATE_PIXMAP_USAGE_GLYPH_PICTURE               3
 /* pixmap will be shared */
 #define CREATE_PIXMAP_USAGE_SHARED                      4
+/* pixmap will only be as a header for transient (e.g. on-stack) pixels */
+#define CREATE_PIXMAP_USAGE_SCRATCH_HEADER              5
 
 typedef PixmapPtr (*CreatePixmapProcPtr) (ScreenPtr /*pScreen */ ,
                                           int /*width */ ,
@@ -464,8 +466,6 @@ typedef struct _Screen {
     GetScreenPixmapProcPtr GetScreenPixmap;
     SetScreenPixmapProcPtr SetScreenPixmap;
 
-    PixmapPtr pScratchPixmap;   /* scratch pixmap "pool" */
-
     unsigned int totalPixmapSize;
 
     MarkWindowProcPtr MarkWindow;
-- 
1.8.1.4