On Sat, 17 Jul 2010 11:42:54 +0100, Julien Cristau <[email protected]> wrote:
> Xephyr outputs the following at reset for me, on 1.9 rc5: And a lovely bug it is too. I've created four patches to solve this problem, fixing three separate bugs in the process. I would propose that the three patches be merged into a single commit in the server, as the server crashes in the middle of this sequence. I'll just in-line the patches as they're all small. The first adds some asserts to check that everyone is using the Damage API correctly. I'm not sure this should actually be stuck in the server as it might cause crashes sooner than we have today... -------------------------------- From c7a4bdcc76d543a5e25bb9825dce43da48d4d692 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Tue, 20 Jul 2010 09:26:19 -0700 Subject: [PATCH 1/4] Catch misuses of DamageDestroy with some assertions DamageDestroy must not be called on a damage structure that is still registered with a drawable. Could DamageDestroy just call DamageUnregister? All users that I could find would work correctly with this. DamageUnregister should only ever be called using the same drawable passed to the matching DamageRegister call. Should the drawable argument to DamageUnregister be removed? Signed-off-by: Keith Packard <[email protected]> --- miext/damage/damage.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/miext/damage/damage.c b/miext/damage/damage.c index 1cf0513..5ef86fc 100644 --- a/miext/damage/damage.c +++ b/miext/damage/damage.c @@ -2015,6 +2015,7 @@ DamageUnregister (DrawablePtr pDrawable, ScreenPtr pScreen = pDrawable->pScreen; damageScrPriv(pScreen); + assert(pDrawable == pDamage->pDrawable); (*pScrPriv->funcs.Unregister) (pDrawable, pDamage); if (pDrawable->type == DRAWABLE_WINDOW) @@ -2054,6 +2055,7 @@ DamageDestroy (DamagePtr pDamage) ScreenPtr pScreen = pDamage->pScreen; damageScrPriv(pScreen); + assert(!pDamage->pDrawable); if (pDamage->damageDestroy) (*pDamage->damageDestroy) (pDamage, pDamage->closure); (*pScrPriv->funcs.Destroy) (pDamage); -- 1.7.1 -------------------------------------- The second fixes a bug in the misprite code which left a damage object registered after it was destroyed. This would hit one of the above asserts. Without the above asserts, we'd be referencing freed memory when cleaning up the damage bits for the screen pixmap. I'm surprised this hasn't caused problems for anyone, especially on OpenBSD. From f2e7c5e5aa9a94034d773066edb00f11a5e95d9f Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Tue, 20 Jul 2010 09:29:26 -0700 Subject: [PATCH 2/4] Unregister damage object in miSpriteCloseScreen. DamageDestroy requires that the damage object not be registered when destroyed, otherwise the damage object will remain on the list associated with that drawable, continuing to use freed memory. Signed-off-by: Keith Packard <[email protected]> --- mi/misprite.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/mi/misprite.c b/mi/misprite.c index 38a6b93..983775a 100644 --- a/mi/misprite.c +++ b/mi/misprite.c @@ -384,6 +384,7 @@ miSpriteCloseScreen (int i, ScreenPtr pScreen) pScreen->InstallColormap = pScreenPriv->InstallColormap; pScreen->StoreColors = pScreenPriv->StoreColors; + miSpriteDisableDamage(pScreen, pScreenPriv); DamageDestroy (pScreenPriv->pDamage); free(pScreenPriv); -- 1.7.1 -------------------------------------- The third patch cleans up the ephyr damage structure when resetting the server. Otherwise, this would have been leaked. From 2c59327af0ca2647463f426f3200265737266f3f Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Tue, 20 Jul 2010 09:32:17 -0700 Subject: [PATCH 3/4] kdrive/ephyr: Clean up damage object in ephyrScreenFini This destroys the damage object created in ephyrCreateScreenResources. Signed-off-by: Keith Packard <[email protected]> --- hw/kdrive/ephyr/ephyr.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/kdrive/ephyr/ephyr.c b/hw/kdrive/ephyr/ephyr.c index 8096a24..b5b9a74 100644 --- a/hw/kdrive/ephyr/ephyr.c +++ b/hw/kdrive/ephyr/ephyr.c @@ -2,7 +2,7 @@ * Xephyr - A kdrive X server thats runs in a host X window. * Authored by Matthew Allum <[email protected]> * - * Copyright © 2004 Nokia + * Copyright © 2004 Nokia * * Permission to use, copy, modify, distribute, and sell this software and its * documentation for any purpose is hereby granted without fee, provided that @@ -738,7 +738,8 @@ ephyrScreenFini (KdScreenInfo *screen) EphyrScrPriv *scrpriv = screen->driver; if (scrpriv->shadow) { KdShadowFbFree (screen); - } + } else + ephyrUnsetInternalDamage(screen->pScreen); free(screen->driver); screen->driver = NULL; } -- 1.7.1 -------------------------------------- And the final patch makes kdrive free the screen pixmap at CloseScreen time, eliminating the warning message that you found. From 2970cfebf7799e92b73bca6b2345908b2f90a855 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Tue, 20 Jul 2010 09:33:30 -0700 Subject: [PATCH 4/4] kdrive: Destroy screen pixmap at CloseScreen time fbCloseScreen does not destroy the pixmap as the acceleration code may still need to be hooked up to shut down rendering to it. Hence, the driver is responsible for doing this. Signed-off-by: Keith Packard <[email protected]> --- hw/kdrive/src/kdrive.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-) diff --git a/hw/kdrive/src/kdrive.c b/hw/kdrive/src/kdrive.c index 06c3661..8fa266f 100644 --- a/hw/kdrive/src/kdrive.c +++ b/hw/kdrive/src/kdrive.c @@ -736,11 +736,6 @@ KdCloseScreen (int index, ScreenPtr pScreen) Bool ret; pScreenPriv->closed = TRUE; - pScreen->CloseScreen = pScreenPriv->CloseScreen; - if(pScreen->CloseScreen) - ret = (*pScreen->CloseScreen) (index, pScreen); - else - ret = TRUE; if (pScreenPriv->dpmsState != KD_DPMS_NORMAL) (*card->cfuncs->dpms) (pScreen, KD_DPMS_NORMAL); @@ -766,6 +761,15 @@ KdCloseScreen (int index, ScreenPtr pScreen) if(card->cfuncs->scrfini) (*card->cfuncs->scrfini) (screen); + (*pScreen->DestroyPixmap)((PixmapPtr)pScreen->devPrivate); + pScreen->devPrivate = NULL; + + pScreen->CloseScreen = pScreenPriv->CloseScreen; + if(pScreen->CloseScreen) + ret = (*pScreen->CloseScreen) (index, pScreen); + else + ret = TRUE; + /* * Clean up card when last screen is closed, DIX closes them in * reverse order, thus we check for when the first in the list is closed -- 1.7.1 -- [email protected]
pgpRSaQpGvEz2.pgp
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
