[PATCH 3/3] exa: properly wrap Create/DestroyPixmap in ExaCheckPolyArc and fix GC (un)wrapping.

Maarten Maathuis madman2003 at gmail.com
Sat Nov 7 04:32:41 PST 2009


- Fix the system that is required for it to actually work.
- The comment near the new macros explains the problem that existed before.
- Why it didn't cause issues before i don't know.
- We cannot unwrap the GC funcs, because they do prepare/finish access too.
- Protect against setting CreatePixmap twice (which would break swapping) in GC
  functions.
- Remove one (now invalid) use of ExaDoPrepareAccess.

Signed-off-by: Maarten Maathuis <madman2003 at gmail.com>
---
 exa/exa.c         |   99 ++++++++++++++++++++--------------------------------
 exa/exa_priv.h    |  100 ++++++++++++++++++++++++++++++++++++++--------------
 exa/exa_unaccel.c |    9 +++++
 3 files changed, 120 insertions(+), 88 deletions(-)

diff --git a/exa/exa.c b/exa/exa.c
index 46e9182..3ff7f22 100644
--- a/exa/exa.c
+++ b/exa/exa.c
@@ -481,11 +481,10 @@ const GCFuncs exaGCFuncs = {
 };
 
 /*
- * This wrapper exists to allow fbValidateGC to work.
  * Note that we no longer assume newly created pixmaps to be in normal ram.
  * This assumption is certainly not garuanteed with driver allocated pixmaps.
  */
-static PixmapPtr
+PixmapPtr
 exaCreatePixmapWithPrepare(ScreenPtr pScreen, int w, int h, int depth,
 		unsigned usage_hint)
 {
@@ -495,24 +494,19 @@ exaCreatePixmapWithPrepare(ScreenPtr pScreen, int w, int h, int depth,
     /* This swaps between this function and the real upper layer function.
      * Normally this would swap to the fb layer pointer, this is a very special case.
      */
-    swap(pExaScr, pScreen, CreatePixmap);
+    swap_special(pExaScr, pScreen, CreatePixmap);
     pPixmap = pScreen->CreatePixmap(pScreen, w, h, depth, usage_hint);
-    swap(pExaScr, pScreen, CreatePixmap);
+    swap_special(pExaScr, pScreen, CreatePixmap);
 
     if (!pPixmap)
 	return NULL;
 
-    /* Note the usage of ExaDoPrepareAccess, this allowed because:
-     * The pixmap is new, so not offscreen in the classic exa case.
-     * For EXA_HANDLES_PIXMAPS the driver will handle whatever is needed.
-     * We want to signal that the pixmaps will be used as destination.
-     */
-    ExaDoPrepareAccess(pPixmap, EXA_PREPARE_AUX_DEST);
+    exaPrepareAccess(&pPixmap->drawable, EXA_PREPARE_AUX_DEST);
 
     return pPixmap;
 }
 
-static Bool
+Bool
 exaDestroyPixmapWithFinish(PixmapPtr pPixmap)
 {
     ScreenPtr pScreen = pPixmap->drawable.pScreen;
@@ -524,9 +518,9 @@ exaDestroyPixmapWithFinish(PixmapPtr pPixmap)
     /* This swaps between this function and the real upper layer function.
      * Normally this would swap to the fb layer pointer, this is a very special case.
      */
-    swap(pExaScr, pScreen, DestroyPixmap);
+    swap_special(pExaScr, pScreen, DestroyPixmap);
     ret = pScreen->DestroyPixmap(pPixmap);
-    swap(pExaScr, pScreen, DestroyPixmap);
+    swap_special(pExaScr, pScreen, DestroyPixmap);
 
     return ret;
 }
@@ -542,20 +536,17 @@ exaValidateGC(GCPtr pGC,
 
     ScreenPtr pScreen = pDrawable->pScreen;
     ExaScreenPriv(pScreen);
-    CreatePixmapProcPtr old_ptr = NULL;
-    DestroyPixmapProcPtr old_ptr2 = NULL;
     PixmapPtr pTile = NULL;
-    EXA_GC_PROLOGUE(pGC);
-
-    /* save the "fb" pointer. */
-    old_ptr = pExaScr->SavedCreatePixmap;
-    /* create a new upper layer pointer. */
-    wrap(pExaScr, pScreen, CreatePixmap, exaCreatePixmapWithPrepare);
-
-    /* save the "fb" pointer. */
-    old_ptr2 = pExaScr->SavedDestroyPixmap;
-    /* create a new upper layer pointer. */
-    wrap(pExaScr, pScreen, DestroyPixmap, exaDestroyPixmapWithFinish);
+    Bool restore = FALSE;
+    EXA_GC_PROLOGUE_FUNC(pGC);
+
+    /* create a new upper layer pointer.  Warning, CreatePixmap may already be
+        wrapped from another place. */
+    if (pScreen->CreatePixmap != exaCreatePixmapWithPrepare) {
+	restore = TRUE;
+	wrap_special(pExaScr, pScreen, CreatePixmap, exaCreatePixmapWithPrepare);
+	wrap_special(pExaScr, pScreen, DestroyPixmap, exaDestroyPixmapWithFinish);
+    }
 
     /* Either of these conditions is enough to trigger access to a tile pixmap. */
     /* With pGC->tileIsPixel == 1, you run the risk of dereferencing an invalid tile pixmap pointer. */
@@ -587,34 +578,30 @@ exaValidateGC(GCPtr pGC,
         exaFinishAccess(&pGC->stipple->drawable, EXA_PREPARE_MASK);
 
     /* switch back to the normal upper layer. */
-    unwrap(pExaScr, pScreen, CreatePixmap);
-    /* restore copy of fb layer pointer. */
-    pExaScr->SavedCreatePixmap = old_ptr;
-
-    /* switch back to the normal upper layer. */
-    unwrap(pExaScr, pScreen, DestroyPixmap);
-    /* restore copy of fb layer pointer. */
-    pExaScr->SavedDestroyPixmap = old_ptr2;
+    if (restore) {
+	unwrap_special(pExaScr, pScreen, CreatePixmap);
+	unwrap_special(pExaScr, pScreen, DestroyPixmap);
+    }
 
-    EXA_GC_EPILOGUE(pGC);
+    EXA_GC_EPILOGUE_FUNC(pGC);
 }
 
 /* Is exaPrepareAccessGC() needed? */
 static void
 exaDestroyGC(GCPtr pGC)
 {
-    EXA_GC_PROLOGUE (pGC);
+    EXA_GC_PROLOGUE_FUNC (pGC);
     (*pGC->funcs->DestroyGC)(pGC);
-    EXA_GC_EPILOGUE (pGC);
+    EXA_GC_EPILOGUE_FUNC (pGC);
 }
 
 static void
 exaChangeGC (GCPtr pGC,
 		unsigned long mask)
 {
-    EXA_GC_PROLOGUE (pGC);
+    EXA_GC_PROLOGUE_FUNC (pGC);
     (*pGC->funcs->ChangeGC) (pGC, mask);
-    EXA_GC_EPILOGUE (pGC);
+    EXA_GC_EPILOGUE_FUNC (pGC);
 }
 
 static void
@@ -622,9 +609,9 @@ exaCopyGC (GCPtr pGCSrc,
 	      unsigned long mask,
 	      GCPtr	 pGCDst)
 {
-    EXA_GC_PROLOGUE (pGCDst);
+    EXA_GC_PROLOGUE_FUNC (pGCDst);
     (*pGCDst->funcs->CopyGC) (pGCSrc, mask, pGCDst);
-    EXA_GC_EPILOGUE (pGCDst);
+    EXA_GC_EPILOGUE_FUNC (pGCDst);
 }
 
 static void
@@ -633,25 +620,25 @@ exaChangeClip (GCPtr pGC,
 		pointer pvalue,
 		int nrects)
 {
-    EXA_GC_PROLOGUE (pGC);
+    EXA_GC_PROLOGUE_FUNC (pGC);
     (*pGC->funcs->ChangeClip) (pGC, type, pvalue, nrects);
-    EXA_GC_EPILOGUE (pGC);
+    EXA_GC_EPILOGUE_FUNC (pGC);
 }
 
 static void
 exaCopyClip(GCPtr pGCDst, GCPtr pGCSrc)
 {
-    EXA_GC_PROLOGUE (pGCDst);
+    EXA_GC_PROLOGUE_FUNC (pGCDst);
     (*pGCDst->funcs->CopyClip)(pGCDst, pGCSrc);
-    EXA_GC_EPILOGUE (pGCDst);
+    EXA_GC_EPILOGUE_FUNC(pGCDst);
 }
 
 static void
 exaDestroyClip(GCPtr pGC)
 {
-    EXA_GC_PROLOGUE (pGC);
+    EXA_GC_PROLOGUE_FUNC (pGC);
     (*pGC->funcs->DestroyClip)(pGC);
-    EXA_GC_EPILOGUE (pGC);
+    EXA_GC_EPILOGUE_FUNC (pGC);
 }
 
 /**
@@ -682,18 +669,12 @@ exaChangeWindowAttributes(WindowPtr pWin, unsigned long mask)
     Bool ret;
     ScreenPtr pScreen = pWin->drawable.pScreen;
     ExaScreenPriv(pScreen);
-    CreatePixmapProcPtr old_ptr = NULL;
-    DestroyPixmapProcPtr old_ptr2 = NULL;
 
-    /* save the "fb" pointer. */
-    old_ptr = pExaScr->SavedCreatePixmap;
     /* create a new upper layer pointer. */
-    wrap(pExaScr, pScreen, CreatePixmap, exaCreatePixmapWithPrepare);
+    wrap_special(pExaScr, pScreen, CreatePixmap, exaCreatePixmapWithPrepare);
 
-    /* save the "fb" pointer. */
-    old_ptr2 = pExaScr->SavedDestroyPixmap;
     /* create a new upper layer pointer. */
-    wrap(pExaScr, pScreen, DestroyPixmap, exaDestroyPixmapWithFinish);
+    wrap_special(pExaScr, pScreen, DestroyPixmap, exaDestroyPixmapWithFinish);
 
     if ((mask & CWBackPixmap) && pWin->backgroundState == BackgroundPixmap) 
 	exaPrepareAccess(&pWin->background.pixmap->drawable, EXA_PREPARE_SRC);
@@ -711,14 +692,10 @@ exaChangeWindowAttributes(WindowPtr pWin, unsigned long mask)
 	exaFinishAccess(&pWin->border.pixmap->drawable, EXA_PREPARE_MASK);
 
     /* switch back to the normal upper layer. */
-    unwrap(pExaScr, pScreen, CreatePixmap);
-    /* restore copy of fb layer pointer. */
-    pExaScr->SavedCreatePixmap = old_ptr;
+    unwrap_special(pExaScr, pScreen, CreatePixmap);
 
     /* switch back to the normal upper layer. */
-    unwrap(pExaScr, pScreen, DestroyPixmap);
-    /* restore copy of fb layer pointer. */
-    pExaScr->SavedDestroyPixmap = old_ptr2;
+    unwrap_special(pExaScr, pScreen, DestroyPixmap);
 
     return ret;
 }
diff --git a/exa/exa_priv.h b/exa/exa_priv.h
index 5b056da..d267c1a 100644
--- a/exa/exa_priv.h
+++ b/exa/exa_priv.h
@@ -152,25 +152,25 @@ typedef struct _ExaMigrationRec {
 typedef void (*EnableDisableFBAccessProcPtr)(int, Bool);
 typedef struct {
     ExaDriverPtr info;
-    ScreenBlockHandlerProcPtr	 SavedBlockHandler;
-    ScreenWakeupHandlerProcPtr	 SavedWakeupHandler;
-    CreateGCProcPtr 		 SavedCreateGC;
-    CloseScreenProcPtr 		 SavedCloseScreen;
-    GetImageProcPtr 		 SavedGetImage;
-    GetSpansProcPtr 		 SavedGetSpans;
-    CreatePixmapProcPtr 	 SavedCreatePixmap;
-    DestroyPixmapProcPtr 	 SavedDestroyPixmap;
-    CopyWindowProcPtr 		 SavedCopyWindow;
-    ChangeWindowAttributesProcPtr SavedChangeWindowAttributes;
-    BitmapToRegionProcPtr        SavedBitmapToRegion;
-    CreateScreenResourcesProcPtr SavedCreateScreenResources;
-    ModifyPixmapHeaderProcPtr    SavedModifyPixmapHeader;
+    ScreenBlockHandlerProcPtr	 SavedBlockHandler[2];
+    ScreenWakeupHandlerProcPtr	 SavedWakeupHandler[2];
+    CreateGCProcPtr 		 SavedCreateGC[2];
+    CloseScreenProcPtr 		 SavedCloseScreen[2];
+    GetImageProcPtr 		 SavedGetImage[2];
+    GetSpansProcPtr 		 SavedGetSpans[2];
+    CreatePixmapProcPtr 	 SavedCreatePixmap[2];
+    DestroyPixmapProcPtr 	 SavedDestroyPixmap[2];
+    CopyWindowProcPtr 		 SavedCopyWindow[2];
+    ChangeWindowAttributesProcPtr SavedChangeWindowAttributes[2];
+    BitmapToRegionProcPtr        SavedBitmapToRegion[2];
+    CreateScreenResourcesProcPtr SavedCreateScreenResources[2];
+    ModifyPixmapHeaderProcPtr    SavedModifyPixmapHeader[2];
 #ifdef RENDER
-    CompositeProcPtr             SavedComposite;
-    TrianglesProcPtr		 SavedTriangles;
-    GlyphsProcPtr                SavedGlyphs;
-    TrapezoidsProcPtr            SavedTrapezoids;
-    AddTrapsProcPtr		 SavedAddTraps;
+    CompositeProcPtr             SavedComposite[2];
+    TrianglesProcPtr		 SavedTriangles[2];
+    GlyphsProcPtr                SavedGlyphs[2];
+    TrapezoidsProcPtr            SavedTrapezoids[2];
+    AddTrapsProcPtr		 SavedAddTraps[2];
 #endif
     void (*do_migration) (ExaMigrationPtr pixmaps, int npixmaps, Bool can_accel);
     Bool (*pixmap_is_offscreen) (PixmapPtr pPixmap);
@@ -224,31 +224,70 @@ extern DevPrivateKey exaGCPrivateKey;
 #define ExaGCPriv(gc) ExaGCPrivPtr pExaGC = ExaGetGCPriv(gc)
 
 /*
+ * Special wrapping macros for overiding top level pointers.
+ * They are needed because without them:
+ * damageDestroyPixmap is swapped with exaDestroyPixmapWithFinish
+ * exaDestroyPixmapWithFinish does it's thing and swaps back to damageDestroyPixmap
+ * exaDestroyPixmap then swaps to exaDestroyPixmapWithFinish, instead of
+ * (w)fbDestroyPixmap, which is bad. So add a 2nd SavedDestroyPixmap to avoid this.
+ */
+
+#define wrap_special(priv, real, mem, func) {\
+    priv->Saved##mem[1] = real->mem; \
+    real->mem = func; \
+}
+
+#define unwrap_special(priv, real, mem) {\
+    real->mem = priv->Saved##mem[1]; \
+}
+
+#define swap_special(priv, real, mem) {\
+    void *tmp = priv->Saved##mem[1]; \
+    priv->Saved##mem[1] = real->mem; \
+    real->mem = tmp; \
+}
+
+/*
  * Some macros to deal with function wrapping.
  */
 #define wrap(priv, real, mem, func) {\
-    priv->Saved##mem = real->mem; \
+    priv->Saved##mem[0] = real->mem; \
     real->mem = func; \
 }
 
 #define unwrap(priv, real, mem) {\
-    real->mem = priv->Saved##mem; \
+    real->mem = priv->Saved##mem[0]; \
 }
 
 #define swap(priv, real, mem) {\
-    void *tmp = priv->Saved##mem; \
-    priv->Saved##mem = real->mem; \
+    void *tmp = priv->Saved##mem[0]; \
+    priv->Saved##mem[0] = real->mem; \
     real->mem = tmp; \
 }
 
 #define EXA_GC_PROLOGUE(_gc_) \
     ExaGCPriv(_gc_); \
-    swap(pExaGC, _gc_, funcs); \
     swap(pExaGC, _gc_, ops);
 
 #define EXA_GC_EPILOGUE(_gc_) \
-    swap(pExaGC, _gc_, funcs); \
-    swap(pExaGC, _gc_, ops);
+    swap(pExaGC, _gc_, ops); \
+
+/* GC functions can be called from fallback GC ops, 
+    ensure that ops are not swapped twice. */
+#define EXA_GC_PROLOGUE_FUNC(_gc_) \
+    ExaGCPriv(_gc_); \
+    Bool restore_gc_ops =  FALSE; \
+    if (_gc_->ops == &exaOps) { \
+        restore_gc_ops = TRUE; \
+        swap(pExaGC, _gc_, ops); \
+    } \
+    swap(pExaGC, _gc_, funcs);
+
+#define EXA_GC_EPILOGUE_FUNC(_gc_) \
+    if (restore_gc_ops) {\
+        swap(pExaGC, _gc_, ops); \
+    } \
+    swap(pExaGC, _gc_, funcs);
 
 /** Align an offset to an arbitrary alignment */
 #define EXA_ALIGN(offset, align) (((offset) + (align) - 1) - \
@@ -313,8 +352,8 @@ typedef struct {
 
 typedef struct {
     /* GC values from the layer below. */
-    GCOps *Savedops;
-    GCFuncs *Savedfuncs;
+    GCOps *Savedops[2];
+    GCFuncs *Savedfuncs[2];
 } ExaGCPrivRec, *ExaGCPrivPtr;
 
 typedef struct {
@@ -552,6 +591,13 @@ exaDoMigration (ExaMigrationPtr pixmaps, int npixmaps, Bool can_accel);
 Bool
 exaPixmapIsPinned (PixmapPtr pPix);
 
+PixmapPtr
+exaCreatePixmapWithPrepare(ScreenPtr pScreen, int w, int h, int depth,
+		unsigned usage_hint);
+
+Bool
+exaDestroyPixmapWithFinish(PixmapPtr pPixmap);
+
 extern const GCFuncs exaGCFuncs;
 
 /* exa_classic.c */
diff --git a/exa/exa_unaccel.c b/exa/exa_unaccel.c
index c8f0172..d7b248b 100644
--- a/exa/exa_unaccel.c
+++ b/exa/exa_unaccel.c
@@ -229,12 +229,21 @@ void
 ExaCheckPolyArc (DrawablePtr pDrawable, GCPtr pGC,
 		int narcs, xArc *pArcs)
 {
+    ScreenPtr pScreen = pDrawable->pScreen;
+    ExaScreenPriv(pScreen);
     EXA_GC_PROLOGUE(pGC);
     EXA_FALLBACK(("to %p (%c)\n", pDrawable, exaDrawableLocation(pDrawable)));
 
     exaPrepareAccess (pDrawable, EXA_PREPARE_DEST);
     exaPrepareAccessGC (pGC);
+    /* Install a new top level pointer that deals with the coming CreatePixmap
+     * call as well as the following DestroyPixmap call.
+     */
+    wrap_special(pExaScr, pScreen, CreatePixmap, exaCreatePixmapWithPrepare);
+    wrap_special(pExaScr, pScreen, DestroyPixmap, exaDestroyPixmapWithFinish);
     pGC->ops->PolyArc (pDrawable, pGC, narcs, pArcs);
+    unwrap_special(pExaScr, pScreen, CreatePixmap);
+    unwrap_special(pExaScr, pScreen, DestroyPixmap);
     exaFinishAccessGC (pGC);
     exaFinishAccess (pDrawable, EXA_PREPARE_DEST);
     EXA_GC_EPILOGUE(pGC);
-- 
1.6.5.1



More information about the xorg-devel mailing list