[Mesa-dev] [PATCH 4/6] glx: Lift sending the MakeCurrent request to top-level code
Ian Romanick
idr at freedesktop.org
Tue Nov 14 22:03:32 UTC 2017
A couple style nits below. At least some of this patch will have to
change pending possible changes to patch 3. Aside from that, I think
this is ok.
On 11/14/2017 12:13 PM, Adam Jackson wrote:
> Somewhat terrifyingly, we never sent this for direct contexts, which
> means the server never knew the context/drawable bindings.
>
> To handle this sanely, pull the request code up out of the indirect
> backend, and rewrite the context switch path to call it as appropriate.
> This attempts to preserve the existing behavior of not calling unbind()
> on the context if its refcount would not drop to zero, which is why the
> diff is a little uglier than I'd like.
>
> Signed-off-by: Adam Jackson <ajax at redhat.com>
> ---
> src/glx/glxcurrent.c | 181 +++++++++++++++++++++++++++++++++++++------------
> src/glx/indirect_glx.c | 125 ++++------------------------------
> 2 files changed, 151 insertions(+), 155 deletions(-)
>
> diff --git a/src/glx/glxcurrent.c b/src/glx/glxcurrent.c
> index 9f8bf7cee1..7ee8a04a60 100644
> --- a/src/glx/glxcurrent.c
> +++ b/src/glx/glxcurrent.c
> @@ -165,17 +165,85 @@ glXGetCurrentDrawable(void)
> return gc->currentDrawable;
> }
>
> -/**
> - * Make a particular context current.
> - *
> - * \note This is in this file so that it can access dummyContext.
> - */
> +static Bool
> +SendMakeCurrentRequest(Display * dpy, GLXContextID gc_id,
> + GLXContextTag gc_tag, GLXDrawable draw,
> + GLXDrawable read, GLXContextTag *out_tag)
> +{
> + xGLXMakeCurrentReply reply;
> + Bool ret;
> + int opcode = __glXSetupForCommand(dpy);
> +
> + LockDisplay(dpy);
> +
> + if (draw == read) {
> + xGLXMakeCurrentReq *req;
> +
> + GetReq(GLXMakeCurrent, req);
> + req->reqType = opcode;
> + req->glxCode = X_GLXMakeCurrent;
> + req->drawable = draw;
> + req->context = gc_id;
> + req->oldContextTag = gc_tag;
> + }
> + else {
> + struct glx_display *priv = __glXInitialize(dpy);
> +
> + if ((priv->majorVersion > 1) || (priv->minorVersion >= 3)) {
> + xGLXMakeContextCurrentReq *req;
> +
> + GetReq(GLXMakeContextCurrent, req);
> + req->reqType = opcode;
> + req->glxCode = X_GLXMakeContextCurrent;
> + req->drawable = draw;
> + req->readdrawable = read;
> + req->context = gc_id;
> + req->oldContextTag = gc_tag;
> + }
> + else {
> + xGLXVendorPrivateWithReplyReq *vpreq;
> + xGLXMakeCurrentReadSGIReq *req;
> +
> + GetReqExtra(GLXVendorPrivateWithReply,
> + sz_xGLXMakeCurrentReadSGIReq -
> + sz_xGLXVendorPrivateWithReplyReq, vpreq);
> + req = (xGLXMakeCurrentReadSGIReq *) vpreq;
> + req->reqType = opcode;
> + req->glxCode = X_GLXVendorPrivateWithReply;
> + req->vendorCode = X_GLXvop_MakeCurrentReadSGI;
> + req->drawable = draw;
> + req->readable = read;
> + req->context = gc_id;
> + req->oldContextTag = gc_tag;
> + }
> + }
> +
> + ret = _XReply(dpy, (xReply *) &reply, 0, False);
> +
> + if (out_tag)
> + *out_tag = reply.contextTag;
> +
> + UnlockDisplay(dpy);
> + SyncHandle();
> +
> + return ret;
> +}
> +
> +static void
> +SetGC(struct glx_context *gc, Display *dpy, GLXDrawable draw, GLXDrawable read)
> +{
> + gc->currentDpy = dpy;
> + gc->currentDrawable = draw;
> + gc->currentReadable = read;
> +}
> +
> static Bool
> MakeContextCurrent(Display * dpy, GLXDrawable draw,
> GLXDrawable read, GLXContext gc_user)
> {
> struct glx_context *gc = (struct glx_context *) gc_user;
> struct glx_context *oldGC = __glXGetCurrentContext();
> + Bool ret = GL_FALSE;
>
> /* Make sure that the new context has a nonzero ID. In the request,
> * a zero context ID is used only to mean that we bind to no current
> @@ -186,59 +254,86 @@ MakeContextCurrent(Display * dpy, GLXDrawable draw,
> }
>
> _glapi_check_multithread();
> -
> __glXLock();
> +
> + /* Same context and drawables: no op, just return */
> if (oldGC == gc &&
> - gc->currentDrawable == draw && gc->currentReadable == read) {
> - __glXUnlock();
> - return True;
> + gc->currentDrawable == draw &&
> + gc->currentReadable == read) {
> + ret = GL_TRUE;
> }
>
> - if (oldGC != &dummyContext) {
> - if (--oldGC->thread_refcount == 0) {
> - oldGC->vtable->unbind(oldGC, gc);
> - oldGC->currentDpy = 0;
> + /* Same context and new drawbles: update drawable bindings */
> + else if (oldGC == gc) {
I'm not a fan of
if () {
...
}
/* comment */
else if () {
...
}
I think the comment should move inside the else and the else should go
back with the }.
> + if (!SendMakeCurrentRequest(dpy, gc->xid, gc->currentContextTag,
> + draw, read, &gc->currentContextTag)) {
> + goto out;
> }
> - }
>
> - if (gc) {
> - /* Attempt to bind the context. We do this before mucking with
> - * gc and __glXSetCurrentContext to properly handle our state in
> - * case of an error.
> - *
> - * If an error occurs, set the Null context since we've already
> - * blown away our old context. The caller is responsible for
> - * figuring out how to handle setting a valid context.
> - */
> - if (gc->vtable->bind(gc, oldGC, draw, read) != Success) {
> + if (gc->vtable->bind(gc, gc, draw, read) != Success) {
> __glXSetCurrentContextNull();
> - __glXUnlock();
> - __glXSendError(dpy, GLXBadContext, None, X_GLXMakeContextCurrent,
> - False);
> - return GL_FALSE;
> + goto out;
> }
> + }
>
> - if (gc->thread_refcount == 0) {
> - gc->currentDpy = dpy;
> - gc->currentDrawable = draw;
> - gc->currentReadable = read;
> + /* Different contexts: release the old, bind the new */
> + else {
> + if (oldGC != &dummyContext) {
> + if (--oldGC->thread_refcount == 0) {
> + if (!SendMakeCurrentRequest(dpy, None, oldGC->currentContextTag,
> + None, None, &oldGC->currentContextTag)){
^
Space before the {.
> + goto out;
> + }
> +
> + oldGC->vtable->unbind(oldGC, gc);
> +
> + if (!oldGC->xid == None) {
> + /* destroyed context, free it */
> + oldGC->vtable->destroy(oldGC);
> + } else {
> + SetGC(oldGC, NULL, None, None);
> + }
> + }
> }
> - gc->thread_refcount++;
> - __glXSetCurrentContext(gc);
> - } else {
> __glXSetCurrentContextNull();
> - }
>
> - if (oldGC->thread_refcount == 0 && oldGC != &dummyContext && oldGC->xid == None) {
> - /* We are switching away from a context that was
> - * previously destroyed, so we need to free the memory
> - * for the old handle. */
> - oldGC->vtable->destroy(oldGC);
> + if (gc) {
> + /*
> + * MESA_multithread_makecurrent makes this complicated. We need to send
> + * the request if the new context is
> + *
> + * a) indirect (may be current to another client), or
> + * b) (direct and) newly being made current, or
> + * c) (direct and) being bound to new drawables
> + */
> + Bool new_drawables = (gc->currentReadable != read) ||
> + (gc->currentDrawable != draw);
I know krh would complain about the (not strictly necessary) parens.
> +
> + if (!gc->isDirect || !gc->thread_refcount || new_drawables) {
> + if (!SendMakeCurrentRequest(dpy, gc->xid, oldGC->currentContextTag,
> + draw, read, &gc->currentContextTag)) {
> + goto out;
> + }
> + }
> +
> + if (gc->vtable->bind(gc, oldGC, draw, read) != Success) {
> + __glXSendError(dpy, GLXBadContext, None, X_GLXMakeContextCurrent,
> + False);
> + goto out;
> + }
> +
> + if (gc->thread_refcount == 0) {
> + SetGC(gc, dpy, draw, read);
> + }
Since the body and the condition are each only one line, the { and } are
optional. This is a fairly recent Mesa style change.
> + gc->thread_refcount++;
> + __glXSetCurrentContext(gc);
> + }
> }
> + ret = GL_TRUE;
>
> +out:
> __glXUnlock();
> -
> - return GL_TRUE;
> + return ret;
> }
>
>
> diff --git a/src/glx/indirect_glx.c b/src/glx/indirect_glx.c
> index b552b5768a..8ac8fd1b47 100644
> --- a/src/glx/indirect_glx.c
> +++ b/src/glx/indirect_glx.c
> @@ -61,129 +61,30 @@ indirect_destroy_context(struct glx_context *gc)
> free((char *) gc);
> }
>
> -static Bool
> -SendMakeCurrentRequest(Display * dpy, GLXContextID gc_id,
> - GLXContextTag gc_tag, GLXDrawable draw,
> - GLXDrawable read, GLXContextTag *out_tag)
> -{
> - xGLXMakeCurrentReply reply;
> - Bool ret;
> - int opcode = __glXSetupForCommand(dpy);
> -
> - LockDisplay(dpy);
> -
> - if (draw == read) {
> - xGLXMakeCurrentReq *req;
> -
> - GetReq(GLXMakeCurrent, req);
> - req->reqType = opcode;
> - req->glxCode = X_GLXMakeCurrent;
> - req->drawable = draw;
> - req->context = gc_id;
> - req->oldContextTag = gc_tag;
> - }
> - else {
> - struct glx_display *priv = __glXInitialize(dpy);
> -
> - /* If the server can support the GLX 1.3 version, we should
> - * perfer that. Not only that, some servers support GLX 1.3 but
> - * not the SGI extension.
> - */
> -
> - if ((priv->majorVersion > 1) || (priv->minorVersion >= 3)) {
> - xGLXMakeContextCurrentReq *req;
> -
> - GetReq(GLXMakeContextCurrent, req);
> - req->reqType = opcode;
> - req->glxCode = X_GLXMakeContextCurrent;
> - req->drawable = draw;
> - req->readdrawable = read;
> - req->context = gc_id;
> - req->oldContextTag = gc_tag;
> - }
> - else {
> - xGLXVendorPrivateWithReplyReq *vpreq;
> - xGLXMakeCurrentReadSGIReq *req;
> -
> - GetReqExtra(GLXVendorPrivateWithReply,
> - sz_xGLXMakeCurrentReadSGIReq -
> - sz_xGLXVendorPrivateWithReplyReq, vpreq);
> - req = (xGLXMakeCurrentReadSGIReq *) vpreq;
> - req->reqType = opcode;
> - req->glxCode = X_GLXVendorPrivateWithReply;
> - req->vendorCode = X_GLXvop_MakeCurrentReadSGI;
> - req->drawable = draw;
> - req->readable = read;
> - req->context = gc_id;
> - req->oldContextTag = gc_tag;
> - }
> - }
> -
> - ret = _XReply(dpy, (xReply *) &reply, 0, False);
> -
> - if (out_tag)
> - *out_tag = reply.contextTag;
> -
> - UnlockDisplay(dpy);
> - SyncHandle();
> -
> - return ret;
> -}
> -
> static int
> indirect_bind_context(struct glx_context *gc, struct glx_context *old,
> GLXDrawable draw, GLXDrawable read)
> {
> - GLXContextTag tag;
> - Display *dpy = gc->psc->dpy;
> - Bool sent;
> -
> - if (old != &dummyContext && !old->isDirect && old->psc->dpy == dpy) {
> - tag = old->currentContextTag;
> - old->currentContextTag = 0;
> - } else {
> - tag = 0;
> - }
> -
> - sent = SendMakeCurrentRequest(dpy, gc->xid, tag, draw, read,
> - &gc->currentContextTag);
> -
> - if (sent) {
> - if (!IndirectAPI)
> - IndirectAPI = __glXNewIndirectAPI();
> - _glapi_set_dispatch(IndirectAPI);
> -
> - /* The indirect vertex array state must to be initialised after we
> - * have setup the context, as it needs to query server attributes.
> - */
> - __GLXattribute *state = gc->client_state_private;
> - if (state && state->array_state == NULL) {
> - glGetString(GL_EXTENSIONS);
> - glGetString(GL_VERSION);
> - __glXInitVertexArrayState(gc);
> - }
> + if (!IndirectAPI)
> + IndirectAPI = __glXNewIndirectAPI();
> + _glapi_set_dispatch(IndirectAPI);
> +
> + /* The indirect vertex array state must to be initialised after we
> + * have setup the context, as it needs to query server attributes.
> + */
> + __GLXattribute *state = gc->client_state_private;
> + if (state && state->array_state == NULL) {
> + glGetString(GL_EXTENSIONS);
> + glGetString(GL_VERSION);
> + __glXInitVertexArrayState(gc);
> }
>
> - return !sent;
> + return state != NULL && state->array_state != NULL;
> }
>
> static void
> indirect_unbind_context(struct glx_context *gc, struct glx_context *new)
> {
Mark the parameters UNUSED so that I don't get extra warnings. :)
> - Display *dpy = gc->psc->dpy;
> -
> - if (gc == new)
> - return;
> -
> - /* We are either switching to no context, away from an indirect
> - * context to a direct context or from one dpy to another and have
> - * to send a request to the dpy to unbind the previous context.
> - */
> - if (!new || new->isDirect || new->psc->dpy != dpy) {
> - SendMakeCurrentRequest(dpy, None, gc->currentContextTag, None, None,
> - NULL);
> - gc->currentContextTag = 0;
> - }
> }
>
> static void
More information about the mesa-dev
mailing list