On 10/02/2013 02:51 PM, Adam Jackson wrote: > I broke this, back in: > > commit a48dadc98a28c969741979b70b7a639f24f4cbbd > Author: Adam Jackson <[email protected]> > Date: Mon Mar 21 11:59:29 2011 -0400 > > glx: Reimplement context tags > > In that, I changed the glx client state to not explicitly track the list > of current contexts for the client (since that was what we were deriving > tags from). The bug was that I removed the code for same from > glxClientCallback without noticing that it had the side effect of > effectively de-currenting those contexts, so that ContextGone could free > them. So, if you had a client exit with a context still current, the > context's memory would leak. Not a huge deal for direct clients, but > viciously bad for indirect, since the swrast context state at the bottom > of Mesa is like 15M. > > Fix this by promoting Bool isCurrent to ClientPtr currentClient, so that > we have a back-pointer to chase when walking the list of contexts when > ClientStateGone happens. > > v2: Explicitly call __glXFreeContext on the ClientStateGone path. Our > current context might be one we got from EXT_import_context and whose > creating client has since died. Without the explicit call, the creating > client's FreeClientResources would not free the context because it's > still current, and the using client's FreeClientResources would not free > the context because it's not an XID it created. This matches the logic > from a48dadc.
This seems like 3 separate changes: - Rename isCurrent to currentClient - Reorder __glXRemoveFromContextList wrt free. - Release all contexts owned by a client on ClientGone. *If* you have to do a v3, you should split it. It doesn't feel worth the effort to do a v3 just for a split. That said, I believe this patch is correct. Reviewed-by: Ian Romanick <[email protected]> > Signed-off-by: Adam Jackson <[email protected]> > --- > glx/createcontext.c | 2 +- > glx/glxcmds.c | 14 +++++++------- > glx/glxcontext.h | 10 +++++----- > glx/glxext.c | 33 ++++++++++++++++++++++----------- > 4 files changed, 35 insertions(+), 24 deletions(-) > > diff --git a/glx/createcontext.c b/glx/createcontext.c > index 13d21cc..78792da 100644 > --- a/glx/createcontext.c > +++ b/glx/createcontext.c > @@ -320,7 +320,7 @@ __glXDisp_CreateContextAttribsARB(__GLXclientState * cl, > GLbyte * pc) > ctx->id = req->context; > ctx->share_id = req->shareList; > ctx->idExists = True; > - ctx->isCurrent = False; > + ctx->currentClient = False; > ctx->isDirect = req->isDirect; > ctx->hasUnflushedCommands = False; > ctx->renderMode = GL_RENDER; > diff --git a/glx/glxcmds.c b/glx/glxcmds.c > index 663448a..288b417 100644 > --- a/glx/glxcmds.c > +++ b/glx/glxcmds.c > @@ -290,7 +290,7 @@ DoCreateContext(__GLXclientState * cl, GLXContextID gcId, > glxc->id = gcId; > glxc->share_id = shareList; > glxc->idExists = GL_TRUE; > - glxc->isCurrent = GL_FALSE; > + glxc->currentClient = NULL; > glxc->isDirect = isDirect; > glxc->hasUnflushedCommands = GL_FALSE; > glxc->renderMode = GL_RENDER; > @@ -398,7 +398,7 @@ __glXDisp_DestroyContext(__GLXclientState * cl, GLbyte * > pc) > return err; > > glxc->idExists = GL_FALSE; > - if (!glxc->isCurrent) > + if (!glxc->currentClient) > FreeResourceByType(req->context, __glXContextRes, FALSE); > > return Success; > @@ -431,7 +431,7 @@ static void > StopUsingContext(__GLXcontext * glxc) > { > if (glxc) { > - glxc->isCurrent = GL_FALSE; > + glxc->currentClient = NULL; > if (!glxc->idExists) { > FreeResourceByType(glxc->id, __glXContextRes, FALSE); > } > @@ -441,7 +441,7 @@ StopUsingContext(__GLXcontext * glxc) > static void > StartUsingContext(__GLXclientState * cl, __GLXcontext * glxc) > { > - glxc->isCurrent = GL_TRUE; > + glxc->currentClient = cl->client; > } > > /** > @@ -571,7 +571,7 @@ DoMakeCurrent(__GLXclientState * cl, > > if (!validGlxContext(client, contextId, DixUseAccess, &glxc, &error)) > return error; > - if ((glxc != prevglxc) && glxc->isCurrent) { > + if ((glxc != prevglxc) && glxc->currentClient) { > /* Context is current to somebody else */ > return BadAccess; > } > @@ -633,7 +633,7 @@ DoMakeCurrent(__GLXclientState * cl, > return __glXError(GLXBadContext); > } > > - glxc->isCurrent = GL_TRUE; > + glxc->currentClient = client; > } > > StopUsingContext(prevglxc); > @@ -854,7 +854,7 @@ __glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc) > /* > ** The destination context must not be current for any client. > */ > - if (dst->isCurrent) { > + if (dst->currentClient) { > client->errorValue = dest; > return BadAccess; > } > diff --git a/glx/glxcontext.h b/glx/glxcontext.h > index 96a4dd2..fd018d0 100644 > --- a/glx/glxcontext.h > +++ b/glx/glxcontext.h > @@ -70,6 +70,11 @@ struct __GLXcontext { > __GLXscreen *pGlxScreen; > > /* > + ** If this context is current for a client, this will be that client > + */ > + ClientPtr currentClient; > + > + /* > ** The XID of this context. > */ > XID id; > @@ -85,11 +90,6 @@ struct __GLXcontext { > GLboolean idExists; > > /* > - ** Whether this context is current for some client. > - */ > - GLboolean isCurrent; > - > - /* > ** Whether this context is a direct rendering context. > */ > GLboolean isDirect; > diff --git a/glx/glxext.c b/glx/glxext.c > index 1bb884f..ff2ceae 100644 > --- a/glx/glxext.c > +++ b/glx/glxext.c > @@ -88,16 +88,15 @@ __glXResetLargeCommandStatus(__GLXclientState * cl) > } > > /* > -** This procedure is called when the client who created the context goes > -** away OR when glXDestroyContext is called. In either case, all we do is > -** flag that the ID is no longer valid, and (maybe) free the context. > -** use. > -*/ > + * This procedure is called when the client who created the context goes away > + * OR when glXDestroyContext is called. In either case, all we do is flag > that > + * the ID is no longer valid, and (maybe) free the context. > + */ > static int > ContextGone(__GLXcontext * cx, XID id) > { > cx->idExists = GL_FALSE; > - if (!cx->isCurrent) { > + if (!cx->currentClient) { > __glXFreeContext(cx); > } > > @@ -131,9 +130,10 @@ DrawableGone(__GLXdrawable * glxPriv, XID xid) > > for (c = glxAllContexts; c; c = next) { > next = c->next; > - if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == > glxPriv)) { > + if (c->currentClient && > + (c->drawPriv == glxPriv || c->readPriv == glxPriv)) { > (*c->loseCurrent) (c); > - c->isCurrent = GL_FALSE; > + c->currentClient = NULL; > } > if (c->drawPriv == glxPriv) > c->drawPriv = NULL; > @@ -187,14 +187,14 @@ __glXRemoveFromContextList(__GLXcontext * cx) > GLboolean > __glXFreeContext(__GLXcontext * cx) > { > - if (cx->idExists || cx->isCurrent) > + if (cx->idExists || cx->currentClient) > return GL_FALSE; > > + __glXRemoveFromContextList(cx); > + > free(cx->feedbackBuf); > free(cx->selectBuf); > > - __glXRemoveFromContextList(cx); > - > /* We can get here through both regular dispatching from > * __glXDispatch() or as a callback from the resource manager. In > * the latter case we need to lift the DRI lock manually. */ > @@ -271,6 +271,7 @@ glxClientCallback(CallbackListPtr *list, pointer closure, > pointer data) > NewClientInfoRec *clientinfo = (NewClientInfoRec *) data; > ClientPtr pClient = clientinfo->client; > __GLXclientState *cl = glxGetClient(pClient); > + __GLXcontext *c, *next; > > switch (pClient->clientState) { > case ClientStateRunning: > @@ -282,6 +283,16 @@ glxClientCallback(CallbackListPtr *list, pointer > closure, pointer data) > break; > > case ClientStateGone: > + /* detach from all current contexts */ > + for (c = glxAllContexts; c; c = next) { > + next = c->next; > + if (c->currentClient == pClient) { > + c->loseCurrent(c); > + c->currentClient = NULL; > + __glXFreeContext(c); > + } > + } > + > free(cl->returnBuf); > free(cl->largeCmdBuf); > free(cl->GLClientextensions); > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
