From 6d7ba5e0fcb5d1bce6bb213dec009f3a0f802d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristian=20H=C3=B8gsberg?= Date: Sat, 1 May 2010 13:07:46 -0400 Subject: [PATCH 1/4] dix: Update element count in FreeResource*() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FreeResource() keeps clientTable[cid].elements up to date with the number of resources allocated to the client. The other free resource functions (FreeResourceByType(), FreeClientNeverRetainResources() and FreeClientResources()) don't maintain this invariant. Typically, the only consequence is that the element count is too high and we end up allocating the hash table bigger than necessary. However, FreeResource() also relies on the element count to restart the search if the list of resources has been changed during a resource destruction callback. Since FreeResourceByType() doesn't update the count, if we call that from a resource destruction callback from FreeResource(), the loop isn't restarted and we end up following an invalid next pointer. Furthermore, LookupClientResourceComplex() and FreeClientNeverRetainResources() don't use the element count to detect if a callback deleted a resource and may end up following an invalid next pointer if the resource system is called into recursively. Signed-off-by: Kristian Høgsberg Reviewed-by: Keith Packard --- dix/resource.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dix/resource.c b/dix/resource.c index 91d0cfb1c..ab3762eb5 100644 --- a/dix/resource.c +++ b/dix/resource.c @@ -589,6 +589,7 @@ FreeResourceByType(XID id, RESTYPE type, Bool skipFree) res->value, TypeNameString(res->type)); #endif *prev = res->next; + clientTable[cid].elements--; CallResourceStateCallback(ResourceStateFreeing, res); @@ -734,12 +735,14 @@ FreeClientNeverRetainResources(ClientPtr client) ResourcePtr *resources; ResourcePtr this; ResourcePtr *prev; - int j; + int j, elements; + int *eltptr; if (!client) return; resources = clientTable[client->index].resources; + eltptr = &clientTable[client->index].elements; for (j=0; j < clientTable[client->index].buckets; j++) { prev = &resources[j]; @@ -753,11 +756,15 @@ FreeClientNeverRetainResources(ClientPtr client) this->value, TypeNameString(this->type)); #endif *prev = this->next; + clientTable[client->index].elements--; CallResourceStateCallback(ResourceStateFreeing, this); + elements = *eltptr; (*DeleteFuncs[rtype & TypeMask])(this->value, this->id); xfree(this); + if (*eltptr != elements) + prev = &resources[j]; /* prev may no longer be valid */ } else prev = &this->next; @@ -804,6 +811,7 @@ FreeClientResources(ClientPtr client) this->value, TypeNameString(this->type)); #endif *head = this->next; + clientTable[client->index].elements--; CallResourceStateCallback(ResourceStateFreeing, this); From 4a8a615d01b9ed18c272414bd11dc2fc661727e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristian=20H=C3=B8gsberg?= Date: Sat, 1 May 2010 13:13:54 -0400 Subject: [PATCH 2/4] glxdri2: Hard-code the extension version we need MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we use the #define'd version from dri_interface.h, the server will require at least that version of the extension. If we're compiling against a dri_interface.h with a newer version we don't really require, glxdri2 will require a too high version of the extension. The right approach is to just hard-code the version we need instead of using the #defines. Signed-off-by: Kristian Høgsberg Reviewed-by: Ian Romanick Reviewed-by: Adam Jackson --- glx/glxdri2.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/glx/glxdri2.c b/glx/glxdri2.c index 74d6ebc5d..c34e29a5a 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -653,7 +653,7 @@ initializeExtensions(__GLXDRIscreen *screen) #ifdef __DRI2_FLUSH if (strcmp(extensions[i]->name, __DRI2_FLUSH) == 0 && - extensions[i]->version >= __DRI2_FLUSH_VERSION) { + extensions[i]->version >= 3) { screen->flush = (__DRI2flushExtension *) extensions[i]; } #endif @@ -713,11 +713,11 @@ __glXDRIscreenProbe(ScreenPtr pScreen) for (i = 0; extensions[i]; i++) { if (strcmp(extensions[i]->name, __DRI_CORE) == 0 && - extensions[i]->version >= __DRI_CORE_VERSION) { + extensions[i]->version >= 1) { screen->core = (const __DRIcoreExtension *) extensions[i]; } if (strcmp(extensions[i]->name, __DRI_DRI2) == 0 && - extensions[i]->version >= __DRI_DRI2_VERSION) { + extensions[i]->version >= 1) { screen->dri2 = (const __DRIdri2Extension *) extensions[i]; } } From 32381363cd8f43aeb741bad70bcf96a287dac0c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristian=20H=C3=B8gsberg?= Date: Sat, 1 May 2010 13:15:00 -0400 Subject: [PATCH 3/4] list.h: Add list_for_each_entry_safe() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Kristian Høgsberg Reviewed-by: Adam Jackson --- include/list.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/list.h b/include/list.h index a126a652d..89dc29dd0 100644 --- a/include/list.h +++ b/include/list.h @@ -94,4 +94,10 @@ list_is_empty(struct list *head) &pos->member != (head); \ pos = __container_of(pos->member.next, pos, member)) +#define list_for_each_entry_safe(pos, next, head, member) \ + for (pos = __container_of((head)->next, pos, member), \ + next = __container_of(pos->member.next, pos, member); \ + &pos->member != (head); \ + pos = next, next = __container_of(next->member.next, next, member)) + #endif From 9de0e31746d5f0d9d39d11c94ec3cbc04a9935fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristian=20H=C3=B8gsberg?= Date: Thu, 29 Apr 2010 16:36:10 -0400 Subject: [PATCH 4/4] dri2: Take an XID for tracking the DRI2 drawable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some pixmaps (window pixmaps and scratch pixmaps) don't have the drawable->id set and thus DRI2 gets confused when using that field for looking up the DRI2 drawable. Go back to using privates for getting at the DRI2 drawable from a DrawablePtr. We need to keep the resource tracking in place so we can remove the DRI2 drawable when the X resource it was created for goes away. Additionally, we also now track the DRI2 drawable using a client XID so we can reclaim the DRI2 drawable even if the client goes before the drawable and doesn't destroy the DRI2 drawable. Tested-by: Owen W. Taylor Signed-off-by: Kristian Høgsberg --- glx/glxcmds.c | 23 +++--- glx/glxdri.c | 8 ++- glx/glxdri2.c | 10 +-- glx/glxdriswrast.c | 8 ++- glx/glxscreens.h | 6 +- hw/xfree86/dri2/dri2.c | 148 ++++++++++++++++++++++++++++++++------ hw/xfree86/dri2/dri2.h | 3 +- hw/xfree86/dri2/dri2ext.c | 2 +- 8 files changed, 164 insertions(+), 44 deletions(-) diff --git a/glx/glxcmds.c b/glx/glxcmds.c index 087d52ec2..ec3bbe6a3 100644 --- a/glx/glxcmds.c +++ b/glx/glxcmds.c @@ -512,8 +512,9 @@ __glXGetDrawable(__GLXcontext *glxc, GLXDrawable drawId, ClientPtr client, if (!validGlxFBConfigForWindow(client, glxc->config, pDraw, error)) return NULL; - pGlxDraw = glxc->pGlxScreen->createDrawable(glxc->pGlxScreen, - pDraw, GLX_DRAWABLE_WINDOW, + pGlxDraw = glxc->pGlxScreen->createDrawable(client, glxc->pGlxScreen, + pDraw, drawId, + GLX_DRAWABLE_WINDOW, drawId, glxc->config); /* since we are creating the drawablePrivate, drawId should be new */ @@ -1104,15 +1105,17 @@ __glXDrawableRelease(__GLXdrawable *drawable) } static int -DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, __GLXconfig *config, - DrawablePtr pDraw, XID glxDrawableId, int type) +DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, + __GLXconfig *config, DrawablePtr pDraw, XID drawableId, + XID glxDrawableId, int type) { __GLXdrawable *pGlxDraw; if (pGlxScreen->pScreen != pDraw->pScreen) return BadMatch; - pGlxDraw = pGlxScreen->createDrawable(pGlxScreen, pDraw, type, + pGlxDraw = pGlxScreen->createDrawable(client, pGlxScreen, pDraw, + drawableId, type, glxDrawableId, config); if (pGlxDraw == NULL) return BadAlloc; @@ -1125,7 +1128,7 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, __GLXconfig *conf /* Add the glx drawable under the XID of the underlying X drawable * too. That way we'll get a callback in DrawableGone and can * clean up properly when the drawable is destroyed. */ - if (pDraw->id != glxDrawableId && + if (drawableId != glxDrawableId && !AddResource(pDraw->id, __glXDrawableRes, pGlxDraw)) { pGlxDraw->destroy (pGlxDraw); return BadAlloc; @@ -1153,7 +1156,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); return err; @@ -1316,7 +1319,8 @@ DoCreatePbuffer(ClientPtr client, int screenNum, XID fbconfigId, return BadAlloc; return DoCreateGLXDrawable(client, pGlxScreen, config, &pPixmap->drawable, - glxDrawableId, GLX_DRAWABLE_PBUFFER); + glxDrawableId, glxDrawableId, + GLX_DRAWABLE_PBUFFER); } int __glXDisp_CreatePbuffer(__GLXclientState *cl, GLbyte *pc) @@ -1439,7 +1443,8 @@ int __glXDisp_CreateWindow(__GLXclientState *cl, GLbyte *pc) return err; return DoCreateGLXDrawable(client, pGlxScreen, config, - pDraw, req->glxwindow, GLX_DRAWABLE_WINDOW); + pDraw, req->window, + req->glxwindow, GLX_DRAWABLE_WINDOW); } int __glXDisp_DestroyWindow(__GLXclientState *cl, GLbyte *pc) diff --git a/glx/glxdri.c b/glx/glxdri.c index 9810a73a7..1d8c902c4 100644 --- a/glx/glxdri.c +++ b/glx/glxdri.c @@ -683,10 +683,12 @@ __glXDRIscreenCreateContext(__GLXscreen *baseScreen, } static __GLXdrawable * -__glXDRIscreenCreateDrawable(__GLXscreen *screen, +__glXDRIscreenCreateDrawable(ClientPtr client, + __GLXscreen *screen, DrawablePtr pDraw, - int type, XID drawId, + int type, + XID glxDrawId, __GLXconfig *glxConfig) { __GLXDRIscreen *driScreen = (__GLXDRIscreen *) screen; @@ -700,7 +702,7 @@ __glXDRIscreenCreateDrawable(__GLXscreen *screen, return NULL; if (!__glXDrawableInit(&private->base, screen, - pDraw, type, drawId, glxConfig)) { + pDraw, type, glxDrawId, glxConfig)) { xfree(private); return NULL; } diff --git a/glx/glxdri2.c b/glx/glxdri2.c index c34e29a5a..bad451658 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -430,10 +430,12 @@ __glXDRIscreenCreateContext(__GLXscreen *baseScreen, } static __GLXdrawable * -__glXDRIscreenCreateDrawable(__GLXscreen *screen, +__glXDRIscreenCreateDrawable(ClientPtr client, + __GLXscreen *screen, DrawablePtr pDraw, - int type, XID drawId, + int type, + XID glxDrawId, __GLXconfig *glxConfig) { __GLXDRIscreen *driScreen = (__GLXDRIscreen *) screen; @@ -446,7 +448,7 @@ __glXDRIscreenCreateDrawable(__GLXscreen *screen, private->screen = driScreen; if (!__glXDrawableInit(&private->base, screen, - pDraw, type, drawId, glxConfig)) { + pDraw, type, glxDrawId, glxConfig)) { xfree(private); return NULL; } @@ -457,7 +459,7 @@ __glXDRIscreenCreateDrawable(__GLXscreen *screen, private->base.waitGL = __glXDRIdrawableWaitGL; private->base.waitX = __glXDRIdrawableWaitX; - if (DRI2CreateDrawable(pDraw)) { + if (DRI2CreateDrawable(client, pDraw, drawId)) { xfree(private); return NULL; } diff --git a/glx/glxdriswrast.c b/glx/glxdriswrast.c index 918383cf0..4ba448afc 100644 --- a/glx/glxdriswrast.c +++ b/glx/glxdriswrast.c @@ -301,10 +301,12 @@ glxChangeGC(GCPtr gc, BITS32 mask, CARD32 val) } static __GLXdrawable * -__glXDRIscreenCreateDrawable(__GLXscreen *screen, +__glXDRIscreenCreateDrawable(ClientPtr client, + __GLXscreen *screen, DrawablePtr pDraw, - int type, XID drawId, + int type, + XID glxDrawId, __GLXconfig *glxConfig) { __GLXDRIscreen *driScreen = (__GLXDRIscreen *) screen; @@ -319,7 +321,7 @@ __glXDRIscreenCreateDrawable(__GLXscreen *screen, private->screen = driScreen; if (!__glXDrawableInit(&private->base, screen, - pDraw, type, drawId, glxConfig)) { + pDraw, type, glxDrawId, glxConfig)) { xfree(private); return NULL; } diff --git a/glx/glxscreens.h b/glx/glxscreens.h index d52099fc2..861e03ce8 100644 --- a/glx/glxscreens.h +++ b/glx/glxscreens.h @@ -134,10 +134,12 @@ struct __GLXscreen { __GLXconfig *modes, __GLXcontext *shareContext); - __GLXdrawable *(*createDrawable)(__GLXscreen *context, + __GLXdrawable *(*createDrawable)(ClientPtr client, + __GLXscreen *context, DrawablePtr pDraw, - int type, XID drawId, + int type, + XID glxDrawId, __GLXconfig *modes); int (*swapInterval) (__GLXdrawable *drawable, int interval); diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 6c4dabc41..11442d092 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -37,6 +37,7 @@ #include #include #include "xf86Module.h" +#include "list.h" #include "scrnintstr.h" #include "windowstr.h" #include "dixstruct.h" @@ -50,12 +51,18 @@ CARD8 dri2_minor; static int dri2ScreenPrivateKeyIndex; static DevPrivateKey dri2ScreenPrivateKey = &dri2ScreenPrivateKeyIndex; +static int dri2WindowPrivateKeyIndex; +static DevPrivateKey dri2WindowPrivateKey = &dri2WindowPrivateKeyIndex; +static int dri2PixmapPrivateKeyIndex; +static DevPrivateKey dri2PixmapPrivateKey = &dri2PixmapPrivateKeyIndex; static RESTYPE dri2DrawableRes; typedef struct _DRI2Screen *DRI2ScreenPtr; typedef struct _DRI2Drawable { DRI2ScreenPtr dri2_screen; + DrawablePtr drawable; + struct list reference_list; int width; int height; DRI2BufferPtr *buffers; @@ -74,6 +81,7 @@ typedef struct _DRI2Drawable { typedef struct _DRI2Screen { ScreenPtr screen; + int refcnt; unsigned int numDrivers; const char **driverNames; const char *deviceName; @@ -99,35 +107,33 @@ DRI2GetScreen(ScreenPtr pScreen) static DRI2DrawablePtr DRI2GetDrawable(DrawablePtr pDraw) { - DRI2DrawablePtr pPriv; - int rc; + WindowPtr pWin; + PixmapPtr pPixmap; - rc = dixLookupResourceByType((pointer *) &pPriv, pDraw->id, - dri2DrawableRes, NULL, DixReadAccess); - if (rc != Success) - return NULL; - - return pPriv; + if (pDraw->type == DRAWABLE_WINDOW) { + pWin = (WindowPtr) pDraw; + return dixLookupPrivate(&pWin->devPrivates, dri2WindowPrivateKey); + } else { + pPixmap = (PixmapPtr) pDraw; + return dixLookupPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey); + } } -int -DRI2CreateDrawable(DrawablePtr pDraw) +static DRI2DrawablePtr +DRI2AllocateDrawable(DrawablePtr pDraw) { DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen); DRI2DrawablePtr pPriv; CARD64 ust; - int rc; - - rc = dixLookupResourceByType((pointer *) &pPriv, pDraw->id, - dri2DrawableRes, NULL, DixReadAccess); - if (rc == Success || rc != BadValue) - return rc; + WindowPtr pWin; + PixmapPtr pPixmap; pPriv = xalloc(sizeof *pPriv); if (pPriv == NULL) - return BadAlloc; + return NULL; pPriv->dri2_screen = ds; + pPriv->drawable = pDraw; pPriv->width = pDraw->width; pPriv->height = pDraw->height; pPriv->buffers = NULL; @@ -145,9 +151,77 @@ DRI2CreateDrawable(DrawablePtr pDraw) pPriv->swap_limit = 1; /* default to double buffering */ pPriv->last_swap_msc = 0; pPriv->last_swap_ust = 0; + list_init(&pPriv->reference_list); - if (!AddResource(pDraw->id, dri2DrawableRes, pPriv)) + if (pDraw->type == DRAWABLE_WINDOW) { + pWin = (WindowPtr) pDraw; + dixSetPrivate(&pWin->devPrivates, dri2WindowPrivateKey, pPriv); + } else { + pPixmap = (PixmapPtr) pDraw; + dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, pPriv); + } + + return pPriv; +} + +typedef struct DRI2DrawableRefRec { + XID id; + XID dri2_id; + struct list link; +} DRI2DrawableRefRec, *DRI2DrawableRefPtr; + +static DRI2DrawableRefPtr +DRI2LookupDrawableRef(DRI2DrawablePtr pPriv, XID id) +{ + DRI2DrawableRefPtr ref; + + list_for_each_entry(ref, &pPriv->reference_list, link) { + if (ref->id == id) + return ref; + } + + return NULL; +} + +static int +DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID dri2_id) +{ + DRI2DrawableRefPtr ref; + + ref = malloc(sizeof *ref); + if (ref == NULL) return BadAlloc; + + if (!AddResource(dri2_id, dri2DrawableRes, pPriv)) + return BadAlloc; + if (!DRI2LookupDrawableRef(pPriv, id)) + if (!AddResource(id, dri2DrawableRes, pPriv)) + return BadAlloc; + + ref->id = id; + ref->dri2_id = dri2_id; + list_add(&ref->link, &pPriv->reference_list); + + return Success; +} + +int +DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID id) +{ + DRI2DrawablePtr pPriv; + XID dri2_id; + int rc; + + pPriv = DRI2GetDrawable(pDraw); + if (pPriv == NULL) + pPriv = DRI2AllocateDrawable(pDraw); + if (pPriv == NULL) + return BadAlloc; + + dri2_id = FakeClientID(client->index); + rc = DRI2AddDrawableRef(pPriv, id, dri2_id); + if (rc != Success) + return rc; return Success; } @@ -156,13 +230,45 @@ static int DRI2DrawableGone(pointer p, XID id) { DRI2DrawablePtr pPriv = p; DRI2ScreenPtr ds = pPriv->dri2_screen; - DrawablePtr root; + DRI2DrawableRefPtr ref, next; + WindowPtr pWin; + PixmapPtr pPixmap; + DrawablePtr pDraw; int i; - root = &WindowTable[ds->screen->myNum]->drawable; + list_for_each_entry_safe(ref, next, &pPriv->reference_list, link) { + if (ref->dri2_id == id) { + list_del(&ref->link); + /* If this was the last ref under this X drawable XID, + * unregister the X drawable resource. */ + if (!DRI2LookupDrawableRef(pPriv, ref->id)) + FreeResourceByType(ref->id, dri2DrawableRes, TRUE); + free(ref); + break; + } + + if (ref->id == id) { + list_del(&ref->link); + FreeResourceByType(ref->dri2_id, dri2DrawableRes, TRUE); + free(ref); + } + } + + if (!list_is_empty(&pPriv->reference_list)) + return Success; + + pDraw = pPriv->drawable; + if (pDraw->type == DRAWABLE_WINDOW) { + pWin = (WindowPtr) pDraw; + dixSetPrivate(&pWin->devPrivates, dri2WindowPrivateKey, NULL); + } else { + pPixmap = (PixmapPtr) pDraw; + dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, NULL); + } + if (pPriv->buffers != NULL) { for (i = 0; i < pPriv->bufferCount; i++) - (*ds->DestroyBuffer)(root, pPriv->buffers[i]); + (*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]); xfree(pPriv->buffers); } diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h index ce8a5df41..5415a0b4a 100644 --- a/hw/xfree86/dri2/dri2.h +++ b/hw/xfree86/dri2/dri2.h @@ -198,7 +198,8 @@ 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 DRI2CreateDrawable(ClientPtr client, + DrawablePtr pDraw, XID id); extern _X_EXPORT void DRI2DestroyDrawable(DrawablePtr pDraw); diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c index 17df1304e..58eaa1086 100644 --- a/hw/xfree86/dri2/dri2ext.c +++ b/hw/xfree86/dri2/dri2ext.c @@ -167,7 +167,7 @@ ProcDRI2CreateDrawable(ClientPtr client) &pDrawable, &status)) return status; - status = DRI2CreateDrawable(pDrawable); + status = DRI2CreateDrawable(client, pDrawable, stuff->drawable); if (status != Success) return status;