[PATCH] DRI2 resource management fixes.
Kristian Høgsberg
krh at bitplanet.net
Mon Aug 3 08:07:10 PDT 2009
Hi Michel,
I spent a good deal of time avoiding using resources in dri2.c since
we should be able to just use pointers directly from aiglx clients.
There should be a simpler way to fix this that doesn't use resources
or clients in dri2.c. Keeping drawable resource tracking out of
dri2.c was a deliberate choice and I don't want to see that reverted
unless there is no other way. The problem is that the DRI2Drawable is
a client side resource for direct rendering clients (they get to it
through dri2ext.c) but for aiglx clients, it's a server internal
object that is not exposed as a client resource.
cheers,
Kristian
2009/8/2 Michel Dänzer <michel at daenzer.net>:
> From: Michel Dänzer <daenzer at vmware.com>
>
> * Add new variants of DRI2CreateDrawable, DRI2DestroyDrawable and
> DRI2CopyRegion which take a ClientPtr and an XID instead of a DrawablePtr.
> Use these in dri2ext.c always and in glxdri2.c for windows (not all pixmaps
> have an associated XID, and we can increase their reference count to prevent
> them from disappearing beneath us) to avoid derefencing a DrawablePtr for a
> window which has been destroyed. Fixes crashes when quitting MacSlow's
> rgba-glx demo.
> * Move DRI2 drawable resource tracking from dri2ext.c to dri2.c, and add screen
> DestroyPixmap/Window hooks to prevent DRI2 drawable leaks under any
> circumstances.
> ---
> glx/glxcmds.c | 26 ++++-----
> glx/glxdrawable.h | 1 +
> glx/glxdri2.c | 39 ++++++++----
> hw/xfree86/dri2/dri2.c | 145 +++++++++++++++++++++++++++++++++++++++++++--
> hw/xfree86/dri2/dri2.h | 8 +++
> hw/xfree86/dri2/dri2ext.c | 32 ++--------
> 6 files changed, 191 insertions(+), 60 deletions(-)
>
> diff --git a/glx/glxcmds.c b/glx/glxcmds.c
> index f5632d1..5112303 100644
> --- a/glx/glxcmds.c
> +++ b/glx/glxcmds.c
> @@ -1085,7 +1085,8 @@ __glXDrawableInit(__GLXdrawable *drawable,
> __GLXscreen *screen, DrawablePtr pDraw, int type,
> XID drawId, __GLXconfig *config)
> {
> - drawable->pDraw = pDraw;
> + drawable->pScreen = pDraw->pScreen;
> + drawable->pDraw = pDraw->type == DRAWABLE_PIXMAP ? pDraw : NULL;
> drawable->type = type;
> drawable->drawId = drawId;
> drawable->config = config;
> @@ -1097,19 +1098,14 @@ __glXDrawableInit(__GLXdrawable *drawable,
> void
> __glXDrawableRelease(__GLXdrawable *drawable)
> {
> - ScreenPtr pScreen = drawable->pDraw->pScreen;
> -
> - switch (drawable->type) {
> - case GLX_DRAWABLE_PIXMAP:
> - case GLX_DRAWABLE_PBUFFER:
> - (*pScreen->DestroyPixmap)((PixmapPtr) drawable->pDraw);
> - break;
> - }
> + /* We only store the DrawablePtr for pixmaps */
> + if (drawable->pDraw)
> + (*drawable->pScreen->DestroyPixmap)((PixmapPtr) drawable->pDraw);
> }
>
> static int
> DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, __GLXconfig *config,
> - DrawablePtr pDraw, XID glxDrawableId, int type)
> + DrawablePtr pDraw, XID drawableId, XID glxDrawableId, int type)
> {
> __GLXdrawable *pGlxDraw;
>
> @@ -1119,7 +1115,7 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, __GLXconfig *conf
> return BadMatch;
>
> pGlxDraw = pGlxScreen->createDrawable(pGlxScreen, pDraw, type,
> - glxDrawableId, config);
> + drawableId, config);
> if (pGlxDraw == NULL)
> return BadAlloc;
>
> @@ -1148,7 +1144,7 @@ DoCreateGLXPixmap(ClientPtr client, __GLXscreen *pGlxScreen, __GLXconfig *config
> return BadPixmap;
> }
>
> - err = DoCreateGLXDrawable(client, pGlxScreen, config, pDraw,
> + err = DoCreateGLXDrawable(client, pGlxScreen, config, pDraw, drawableId,
> glxDrawableId, GLX_DRAWABLE_PIXMAP);
>
> if (err == Success)
> @@ -1305,7 +1301,7 @@ DoCreatePbuffer(ClientPtr client, int screenNum, XID fbconfigId,
> __glXleaveServer(GL_FALSE);
>
> return DoCreateGLXDrawable(client, pGlxScreen, config, &pPixmap->drawable,
> - glxDrawableId, GLX_DRAWABLE_PBUFFER);
> + glxDrawableId, glxDrawableId, GLX_DRAWABLE_PBUFFER);
> }
>
> int __glXDisp_CreatePbuffer(__GLXclientState *cl, GLbyte *pc)
> @@ -1425,8 +1421,8 @@ int __glXDisp_CreateWindow(__GLXclientState *cl, GLbyte *pc)
> if (!validGlxFBConfigForWindow(client, config, pDraw, &err))
> return err;
>
> - return DoCreateGLXDrawable(client, pGlxScreen, config,
> - pDraw, req->glxwindow, GLX_DRAWABLE_WINDOW);
> + return DoCreateGLXDrawable(client, pGlxScreen, config, pDraw,
> + req->window, req->glxwindow, GLX_DRAWABLE_WINDOW);
> }
>
> int __glXDisp_DestroyWindow(__GLXclientState *cl, GLbyte *pc)
> diff --git a/glx/glxdrawable.h b/glx/glxdrawable.h
> index 3f165ed..922eefd 100644
> --- a/glx/glxdrawable.h
> +++ b/glx/glxdrawable.h
> @@ -51,6 +51,7 @@ struct __GLXdrawable {
> void (*waitX)(__GLXdrawable *);
> void (*waitGL)(__GLXdrawable *);
>
> + ScreenPtr pScreen;
> DrawablePtr pDraw;
> XID drawId;
>
> diff --git a/glx/glxdri2.c b/glx/glxdri2.c
> index ed7fb4c..e8fde0d 100644
> --- a/glx/glxdri2.c
> +++ b/glx/glxdri2.c
> @@ -104,10 +104,10 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
>
> (*core->destroyDrawable)(private->driDrawable);
>
> - /* If the X window was destroyed, the dri DestroyWindow hook will
> - * aready have taken care of this, so only call if pDraw isn't NULL. */
> - if (drawable->pDraw != NULL)
> + if (drawable->pDraw)
> DRI2DestroyDrawable(drawable->pDraw);
> + else
> + DRI2DestroyDrawableWithId(__pGlxClient, drawable->drawId);
>
> __glXDrawableRelease(drawable);
>
> @@ -126,10 +126,14 @@ __glXDRIdrawableCopySubBuffer(__GLXdrawable *drawable,
> box.y1 = private->height - y - h;
> box.x2 = x + w;
> box.y2 = private->height - y;
> - REGION_INIT(drawable->pDraw->pScreen, ®ion, &box, 0);
> + REGION_INIT(drawable->pScreen, ®ion, &box, 0);
>
> - DRI2CopyRegion(drawable->pDraw, ®ion,
> - DRI2BufferFrontLeft, DRI2BufferBackLeft);
> + if (drawable->pDraw)
> + DRI2CopyRegion(drawable->pDraw, ®ion,
> + DRI2BufferFrontLeft, DRI2BufferBackLeft);
> + else
> + DRI2CopyRegionWithId(__pGlxClient, drawable->drawId, ®ion,
> + DRI2BufferFrontLeft, DRI2BufferBackLeft);
> }
>
> static GLboolean
> @@ -154,10 +158,14 @@ __glXDRIdrawableWaitX(__GLXdrawable *drawable)
> box.y1 = 0;
> box.x2 = private->width;
> box.y2 = private->height;
> - REGION_INIT(drawable->pDraw->pScreen, ®ion, &box, 0);
> + REGION_INIT(drawable->pScreen, ®ion, &box, 0);
>
> - DRI2CopyRegion(drawable->pDraw, ®ion,
> - DRI2BufferFakeFrontLeft, DRI2BufferFrontLeft);
> + if (drawable->pDraw)
> + DRI2CopyRegion(drawable->pDraw, ®ion,
> + DRI2BufferFakeFrontLeft, DRI2BufferFrontLeft);
> + else
> + DRI2CopyRegionWithId(__pGlxClient, drawable->drawId, ®ion,
> + DRI2BufferFakeFrontLeft, DRI2BufferFrontLeft);
> }
>
> static void
> @@ -171,10 +179,14 @@ __glXDRIdrawableWaitGL(__GLXdrawable *drawable)
> box.y1 = 0;
> box.x2 = private->width;
> box.y2 = private->height;
> - REGION_INIT(drawable->pDraw->pScreen, ®ion, &box, 0);
> + REGION_INIT(drawable->pScreen, ®ion, &box, 0);
>
> - DRI2CopyRegion(drawable->pDraw, ®ion,
> - DRI2BufferFrontLeft, DRI2BufferFakeFrontLeft);
> + if (drawable->pDraw)
> + DRI2CopyRegion(drawable->pDraw, ®ion,
> + DRI2BufferFrontLeft, DRI2BufferFakeFrontLeft);
> + else
> + DRI2CopyRegionWithId(__pGlxClient, drawable->drawId, ®ion,
> + DRI2BufferFrontLeft, DRI2BufferFakeFrontLeft);
> }
>
> static int
> @@ -387,7 +399,8 @@ __glXDRIscreenCreateDrawable(__GLXscreen *screen,
> private->base.waitGL = __glXDRIdrawableWaitGL;
> private->base.waitX = __glXDRIdrawableWaitX;
>
> - if (DRI2CreateDrawable(pDraw)) {
> + if (private->base.pDraw ? DRI2CreateDrawable(private->base.pDraw) :
> + DRI2CreateDrawableWithId(__pGlxClient, private->base.drawId)) {
> xfree(private);
> return NULL;
> }
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index 8795cd1..d054b8a 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -42,6 +42,18 @@
>
> #include "xf86.h"
>
> +extern RESTYPE dri2DrawableRes;
> +
> +static Bool
> +validDrawable(ClientPtr client, XID drawable, DrawablePtr *pDrawable, int *status)
> +{
> + *status = dixLookupDrawable(pDrawable, drawable, client, 0, DixReadAccess);
> + if (*status != Success)
> + return FALSE;
> +
> + return TRUE;
> +}
> +
> static int dri2ScreenPrivateKeyIndex;
> static DevPrivateKey dri2ScreenPrivateKey = &dri2ScreenPrivateKeyIndex;
> static int dri2WindowPrivateKeyIndex;
> @@ -68,6 +80,8 @@ typedef struct _DRI2Screen {
> DRI2DestroyBufferProcPtr DestroyBuffer;
> DRI2CopyRegionProcPtr CopyRegion;
>
> + DestroyPixmapProcPtr DestroyPixmap;
> + DestroyWindowProcPtr DestroyWindow;
> HandleExposuresProcPtr HandleExposures;
> } DRI2ScreenRec, *DRI2ScreenPtr;
>
> @@ -133,6 +147,27 @@ DRI2CreateDrawable(DrawablePtr pDraw)
> return Success;
> }
>
> +int
> +DRI2CreateDrawableWithId(ClientPtr client, XID xDrawable)
> +{
> + DrawablePtr pDraw;
> + int status;
> +
> + if (!validDrawable(client, xDrawable, &pDraw, &status))
> + return status;
> +
> + status = DRI2CreateDrawable(pDraw);
> + if (status != Success)
> + return status;
> +
> + if (!AddResource(xDrawable, dri2DrawableRes, client)) {
> + DRI2DestroyDrawable(pDraw);
> + return BadAlloc;
> + }
> +
> + return Success;
> +}
> +
> static int
> find_attachment(DRI2DrawablePtr pPriv, unsigned attachment)
> {
> @@ -337,8 +372,21 @@ DRI2CopyRegion(DrawablePtr pDraw, RegionPtr pRegion,
> return Success;
> }
>
> -void
> -DRI2DestroyDrawable(DrawablePtr pDraw)
> +int
> +DRI2CopyRegionWithId(ClientPtr client, XID xDrawable, RegionPtr pRegion,
> + unsigned int dest, unsigned int src)
> +{
> + DrawablePtr pDraw;
> + int status;
> +
> + if (!validDrawable(client, xDrawable, &pDraw, &status))
> + return status;
> +
> + return DRI2CopyRegion(pDraw, pRegion, dest, src);
> +}
> +
> +static void
> +DRI2DoDestroyDrawable(DrawablePtr pDraw)
> {
> DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen);
> DRI2DrawablePtr pPriv;
> @@ -349,10 +397,6 @@ DRI2DestroyDrawable(DrawablePtr pDraw)
> if (pPriv == NULL)
> return;
>
> - pPriv->refCount--;
> - if (pPriv->refCount > 0)
> - return;
> -
> if (pPriv->buffers != NULL) {
> int i;
>
> @@ -376,6 +420,87 @@ DRI2DestroyDrawable(DrawablePtr pDraw)
> }
> }
>
> +void
> +DRI2DestroyDrawable(DrawablePtr pDraw)
> +{
> + DRI2DrawablePtr pPriv;
> +
> + pPriv = DRI2GetDrawable(pDraw);
> + if (pPriv == NULL)
> + return;
> +
> + pPriv->refCount--;
> + if (pPriv->refCount > 0)
> + return;
> +
> + DRI2DoDestroyDrawable(pDraw);
> +}
> +
> +int
> +DRI2DestroyDrawableWithId(ClientPtr client, XID xDrawable)
> +{
> + DrawablePtr pDraw;
> + int status;
> +
> + if (!validDrawable(client, xDrawable, &pDraw, &status))
> + return status;
> +
> + DRI2DestroyDrawable(pDraw);
> +
> + return Success;
> +}
> +
> +static Bool
> +DRI2DestroyPixmap (PixmapPtr pPixmap)
> +{
> + ScreenPtr pScreen = pPixmap->drawable.pScreen;
> + DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
> + Bool retval = TRUE;
> +
> + if (pPixmap->refcnt == 1)
> + DRI2DoDestroyDrawable(&pPixmap->drawable);
> +
> + /* call lower wrapped functions */
> + if (ds->DestroyPixmap) {
> + /* unwrap */
> + pScreen->DestroyPixmap = ds->DestroyPixmap;
> +
> + /* call lower layers */
> + retval = (*pScreen->DestroyPixmap)(pPixmap);
> +
> + /* rewrap */
> + ds->DestroyPixmap = pScreen->DestroyPixmap;
> + pScreen->DestroyPixmap = DRI2DestroyPixmap;
> + }
> +
> + return retval;
> +}
> +
> +static Bool
> +DRI2DestroyWindow(WindowPtr pWin)
> +{
> + ScreenPtr pScreen = pWin->drawable.pScreen;
> + DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
> + Bool retval = TRUE;
> +
> + DRI2DoDestroyDrawable(&pWin->drawable);
> +
> + /* call lower wrapped functions */
> + if (ds->DestroyWindow) {
> + /* unwrap */
> + pScreen->DestroyWindow = ds->DestroyWindow;
> +
> + /* call lower layers */
> + retval = (*pScreen->DestroyWindow)(pWin);
> +
> + /* rewrap */
> + ds->DestroyWindow = pScreen->DestroyWindow;
> + pScreen->DestroyWindow = DRI2DestroyWindow;
> + }
> +
> + return retval;
> +}
> +
> Bool
> DRI2Connect(ScreenPtr pScreen, unsigned int driverType, int *fd,
> const char **driverName, const char **deviceName)
> @@ -426,6 +551,11 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
> ds->DestroyBuffer = info->DestroyBuffer;
> ds->CopyRegion = info->CopyRegion;
>
> + ds->DestroyPixmap = pScreen->DestroyPixmap;
> + pScreen->DestroyPixmap = DRI2DestroyPixmap;
> + ds->DestroyWindow = pScreen->DestroyWindow;
> + pScreen->DestroyWindow = DRI2DestroyWindow;
> +
> dixSetPrivate(&pScreen->devPrivates, dri2ScreenPrivateKey, ds);
>
> xf86DrvMsg(pScreen->myNum, X_INFO, "[DRI2] Setup complete\n");
> @@ -438,6 +568,9 @@ DRI2CloseScreen(ScreenPtr pScreen)
> {
> DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
>
> + pScreen->DestroyPixmap = ds->DestroyPixmap;
> + pScreen->DestroyWindow = ds->DestroyWindow;
> +
> xfree(ds);
> dixSetPrivate(&pScreen->devPrivates, dri2ScreenPrivateKey, NULL);
> }
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index 175471a..cc4bbcb 100644
> --- a/hw/xfree86/dri2/dri2.h
> +++ b/hw/xfree86/dri2/dri2.h
> @@ -100,8 +100,10 @@ extern _X_EXPORT Bool DRI2Connect(ScreenPtr pScreen,
> extern _X_EXPORT Bool DRI2Authenticate(ScreenPtr pScreen, drm_magic_t magic);
>
> extern _X_EXPORT int DRI2CreateDrawable(DrawablePtr pDraw);
> +extern _X_EXPORT int DRI2CreateDrawableWithId(ClientPtr client, XID drawable);
>
> extern _X_EXPORT void DRI2DestroyDrawable(DrawablePtr pDraw);
> +extern _X_EXPORT int DRI2DestroyDrawableWithId(ClientPtr client, XID drawable);
>
> extern _X_EXPORT DRI2BufferPtr *DRI2GetBuffers(DrawablePtr pDraw,
> int *width,
> @@ -115,6 +117,12 @@ extern _X_EXPORT int DRI2CopyRegion(DrawablePtr pDraw,
> unsigned int dest,
> unsigned int src);
>
> +extern _X_EXPORT int DRI2CopyRegionWithId(ClientPtr client,
> + XID xDrawable,
> + RegionPtr pRegion,
> + unsigned int dest,
> + unsigned int src);
> +
> /**
> * Determine the major and minor version of the DRI2 extension.
> *
> diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c
> index 029dce8..fe5de7d 100644
> --- a/hw/xfree86/dri2/dri2ext.c
> +++ b/hw/xfree86/dri2/dri2ext.c
> @@ -50,7 +50,7 @@
> #include "xf86Module.h"
>
> static ExtensionEntry *dri2Extension;
> -static RESTYPE dri2DrawableRes;
> +RESTYPE dri2DrawableRes;
>
> static Bool
> validDrawable(ClientPtr client, XID drawable,
> @@ -156,23 +156,14 @@ static int
> ProcDRI2CreateDrawable(ClientPtr client)
> {
> REQUEST(xDRI2CreateDrawableReq);
> - DrawablePtr pDrawable;
> int status;
>
> REQUEST_SIZE_MATCH(xDRI2CreateDrawableReq);
>
> - if (!validDrawable(client, stuff->drawable, &pDrawable, &status))
> - return status;
> -
> - status = DRI2CreateDrawable(pDrawable);
> + status = DRI2CreateDrawableWithId(client, stuff->drawable);
> if (status != Success)
> return status;
>
> - if (!AddResource(stuff->drawable, dri2DrawableRes, pDrawable)) {
> - DRI2DestroyDrawable(pDrawable);
> - return BadAlloc;
> - }
> -
> return client->noClientException;
> }
>
> @@ -180,16 +171,10 @@ static int
> ProcDRI2DestroyDrawable(ClientPtr client)
> {
> REQUEST(xDRI2DestroyDrawableReq);
> - DrawablePtr pDrawable;
> - int status;
>
> REQUEST_SIZE_MATCH(xDRI2DestroyDrawableReq);
> - if (!validDrawable(client, stuff->drawable, &pDrawable, &status))
> - return status;
> -
> - FreeResourceByType(stuff->drawable, dri2DrawableRes, FALSE);
>
> - return client->noClientException;
> + return DRI2DestroyDrawableWithId(client, stuff->drawable);
> }
>
>
> @@ -290,18 +275,15 @@ ProcDRI2CopyRegion(ClientPtr client)
> {
> REQUEST(xDRI2CopyRegionReq);
> xDRI2CopyRegionReply rep;
> - DrawablePtr pDrawable;
> int status;
> RegionPtr pRegion;
>
> REQUEST_SIZE_MATCH(xDRI2CopyRegionReq);
>
> - if (!validDrawable(client, stuff->drawable, &pDrawable, &status))
> - return status;
> -
> VERIFY_REGION(pRegion, stuff->region, client, DixReadAccess);
>
> - status = DRI2CopyRegion(pDrawable, pRegion, stuff->dest, stuff->src);
> + status = DRI2CopyRegionWithId(client, stuff->drawable, pRegion, stuff->dest,
> + stuff->src);
> if (status != Success)
> return status;
>
> @@ -398,9 +380,7 @@ SProcDRI2Dispatch (ClientPtr client)
>
> static int DRI2DrawableGone(pointer p, XID id)
> {
> - DrawablePtr pDrawable = p;
> -
> - DRI2DestroyDrawable(pDrawable);
> + DRI2DestroyDrawableWithId(p, id);
>
> return Success;
> }
> --
> 1.6.3.3
>
> _______________________________________________
> xorg-devel mailing list
> xorg-devel at lists.x.org
> http://lists.x.org/mailman/listinfo/xorg-devel
>
More information about the xorg-devel
mailing list