[PATCH] Initialize dev privates before using any
Keith Packard
keithp at keithp.com
Tue Jul 20 09:43:52 PDT 2010
On Sat, 17 Jul 2010 11:42:54 +0100, Julien Cristau <jcristau at debian.org> 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 <keithp at keithp.com>
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 <keithp at keithp.com>
---
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 <keithp at keithp.com>
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 <keithp at keithp.com>
---
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 <keithp at keithp.com>
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 <keithp at keithp.com>
---
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 <mallum at openedhand.com>
*
- * 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 <keithp at keithp.com>
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 <keithp at keithp.com>
---
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
--
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20100720/26214bee/attachment.pgp>
More information about the xorg-devel
mailing list