Discussion:
[PATCH 0/6] glx: Implement GLX_EXT_no_config_context
Add Reply
Adam Jackson
2017-11-14 20:13:00 UTC
Reply
Permalink
Raw Message
The first three here are mostly cleanup, but after that it got
complicated. 4/6 addresses a longstanding bug in the GLX client code
where direct contexts would generate no protocol on MakeCurrent, which
means the server was unable to know what drawables were bound to a
context. We're not presently using that knowledge for anything, but a
DDX might want to react to it - perhaps to change the pixmap's tiling or
memory domain - so it seems worth adding on its own. I have not yet
tried a full piglit run with that change, in particular not with an
older X server.

5/6 depends on that change for the GLX 1.2 case of making a bare Window
current (and forgetting to do glXCreateWindow). Real GLX drawables will
have been created relative to an fbconfig, but for a bare Window we want
to query the server's idea of the config for the drawable. For that to
work, the server has to know that the window has been made current and
applied a config to it, thus the dependency on 4/6.

6/6 itself is unchanged from the last time it was sent.

- ajax
Adam Jackson
2017-11-14 20:13:04 UTC
Reply
Permalink
Raw Message
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 <***@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) {
+ 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)){
+ 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);
+
+ 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);
+ }
+ 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)
{
- 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
--
2.14.3
Ian Romanick
2017-11-14 22:03:32 UTC
Reply
Permalink
Raw Message
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.
Post by Adam Jackson
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.
---
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 }.
Post by Adam Jackson
+ 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 {.
Post by Adam Jackson
+ 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.
Post by Adam Jackson
+
+ 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.
Post by Adam Jackson
+ gc->thread_refcount++;
+ __glXSetCurrentContext(gc);
+ }
}
+ ret = GL_TRUE;
__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. :)
Post by Adam Jackson
- 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
Adam Jackson
2017-11-14 22:18:00 UTC
Reply
Permalink
Raw Message
Post by Ian Romanick
Post by Adam Jackson
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. :)
How do I provoke these warnings in my own build?

- ajax
Ian Romanick
2017-11-14 22:38:20 UTC
Reply
Permalink
Raw Message
Post by Adam Jackson
Post by Ian Romanick
Post by Adam Jackson
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. :)
How do I provoke these warnings in my own build?
I use -Wall -Wextra -Wunsafe-loop-optimizations -Werror=format-security
-Wno-sign-compare, but I think -Wunused (set by -Wall) is sufficient to
provoke this particular warning.
Post by Adam Jackson
- ajax
Adam Jackson
2017-11-14 20:13:01 UTC
Reply
Permalink
Raw Message
The dummy vtable has these slots as NULL already, no need to check for
the dummy context explicitly.

Signed-off-by: Adam Jackson <***@redhat.com>
---
src/glx/glxcmds.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index c707d0cedf..bc93d62510 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -528,7 +528,7 @@ glXWaitGL(void)
{
struct glx_context *gc = __glXGetCurrentContext();

- if (gc != &dummyContext && gc->vtable->wait_gl)
+ if (gc->vtable->wait_gl)
gc->vtable->wait_gl(gc);
}

@@ -541,7 +541,7 @@ glXWaitX(void)
{
struct glx_context *gc = __glXGetCurrentContext();

- if (gc != &dummyContext && gc->vtable->wait_x)
+ if (gc->vtable->wait_x)
gc->vtable->wait_x(gc);
}

@@ -550,7 +550,7 @@ glXUseXFont(Font font, int first, int count, int listBase)
{
struct glx_context *gc = __glXGetCurrentContext();

- if (gc != &dummyContext && gc->vtable->use_x_font)
+ if (gc->vtable->use_x_font)
gc->vtable->use_x_font(gc, font, first, count, listBase);
}

@@ -2431,7 +2431,7 @@ __glXBindTexImageEXT(Display * dpy,
{
struct glx_context *gc = __glXGetCurrentContext();

- if (gc == &dummyContext || gc->vtable->bind_tex_image == NULL)
+ if (gc->vtable->bind_tex_image == NULL)
return;

gc->vtable->bind_tex_image(dpy, drawable, buffer, attrib_list);
@@ -2442,7 +2442,7 @@ __glXReleaseTexImageEXT(Display * dpy, GLXDrawable drawable, int buffer)
{
struct glx_context *gc = __glXGetCurrentContext();

- if (gc == &dummyContext || gc->vtable->release_tex_image == NULL)
+ if (gc->vtable->release_tex_image == NULL)
return;

gc->vtable->release_tex_image(dpy, drawable, buffer);
--
2.14.3
Emil Velikov
2017-11-30 15:50:47 UTC
Reply
Permalink
Raw Message
Post by Adam Jackson
The dummy vtable has these slots as NULL already, no need to check for
the dummy context explicitly.
Looks good, but make sure that `make check' still passes. With that
Reviewed-by: Emil Velikov <***@collabora.com>

-Emil
Adam Jackson
2017-11-14 20:13:06 UTC
Reply
Permalink
Raw Message
This more or less ports EGL_KHR_no_config_context to GLX.

v2: Enable the extension only for those backends that support it.
v3: Fix glvnd path and dri2_convert_glx_attribs()

Khronos: https://github.com/KhronosGroup/OpenGL-Registry/pull/102
Reviewed-by: Kenneth Graunke <***@whitecape.org>
Signed-off-by: Adam Jackson <***@redhat.com>
---
src/glx/create_context.c | 41 ++++++++++++++++++++++++++-------------
src/glx/dri2_glx.c | 1 +
src/glx/dri3_glx.c | 1 +
src/glx/dri_common.c | 4 ++++
src/glx/drisw_glx.c | 1 +
src/glx/g_glxglvnddispatchfuncs.c | 14 ++++++++++++-
src/glx/glxcmds.c | 2 +-
src/glx/glxextensions.c | 1 +
src/glx/glxextensions.h | 1 +
9 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/src/glx/create_context.c b/src/glx/create_context.c
index 38e949ab4c..eab6511ad8 100644
--- a/src/glx/create_context.c
+++ b/src/glx/create_context.c
@@ -47,21 +47,11 @@ glXCreateContextAttribsARB(Display *dpy, GLXFBConfig config,
xcb_generic_error_t *err;
xcb_void_cookie_t cookie;
unsigned dummy_err = 0;
+ int screen = -1;

-
- if (dpy == NULL || cfg == NULL)
- return NULL;
-
- /* This means that either the caller passed the wrong display pointer or
- * one of the internal GLX data structures (probably the fbconfig) has an
- * error. There is nothing sensible to do, so return an error.
- */
- psc = GetGLXScreenConfigs(dpy, cfg->screen);
- if (psc == NULL)
+ if (dpy == NULL)
return NULL;

- assert(cfg->screen == psc->scr);
-
/* Count the number of attributes specified by the application. All
* attributes appear in pairs, except the terminating None.
*/
@@ -70,6 +60,29 @@ glXCreateContextAttribsARB(Display *dpy, GLXFBConfig config,
/* empty */ ;
}

+ if (cfg) {
+ screen = cfg->screen;
+ } else {
+ int i;
+ for (i = 0; i < num_attribs; i++) {
+ if (attrib_list[i * 2] == GLX_SCREEN)
+ screen = attrib_list[i * 2 + 1];
+ }
+ }
+
+ /* This means that either the caller passed the wrong display pointer or
+ * one of the internal GLX data structures (probably the fbconfig) has an
+ * error. There is nothing sensible to do, so return an error.
+ */
+ psc = GetGLXScreenConfigs(dpy, screen);
+ if (psc == NULL)
+ return NULL;
+
+ assert(screen == psc->scr);
+
+ if (!cfg && !__glXExtensionBitIsEnabled(psc, EXT_no_config_context_bit))
+ return NULL;
+
if (direct && psc->vtable->create_context_attribs) {
/* GLX drops the error returned by the driver. The expectation is that
* an error will also be returned by the server. The server's error
@@ -104,8 +117,8 @@ glXCreateContextAttribsARB(Display *dpy, GLXFBConfig config,
cookie =
xcb_glx_create_context_attribs_arb_checked(c,
gc->xid,
- cfg->fbconfigID,
- cfg->screen,
+ cfg ? cfg->fbconfigID : 0,
+ screen,
gc->share_xid,
gc->isDirect,
num_attribs,
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 0f44635725..eeec4f0d60 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -1129,6 +1129,7 @@ dri2BindExtensions(struct dri2_screen *psc, struct glx_display * priv,

__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context");
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context_profile");
+ __glXEnableDirectExtension(&psc->base, "GLX_EXT_no_config_context");

if ((mask & ((1 << __DRI_API_GLES) |
(1 << __DRI_API_GLES2) |
diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
index a10306fe32..ac2e106a7e 100644
--- a/src/glx/dri3_glx.c
+++ b/src/glx/dri3_glx.c
@@ -720,6 +720,7 @@ dri3_bind_extensions(struct dri3_screen *psc, struct glx_display * priv,

__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context");
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context_profile");
+ __glXEnableDirectExtension(&psc->base, "GLX_EXT_no_config_context");

if ((mask & ((1 << __DRI_API_GLES) |
(1 << __DRI_API_GLES2) |
diff --git a/src/glx/dri_common.c b/src/glx/dri_common.c
index 0d0b2d997b..2e0ecaaf8d 100644
--- a/src/glx/dri_common.c
+++ b/src/glx/dri_common.c
@@ -562,6 +562,10 @@ dri2_convert_glx_attribs(unsigned num_attribs, const uint32_t *attribs,
return false;
}
break;
+ case GLX_SCREEN:
+ /* Implies GLX_EXT_no_config_context */
+ *render_type = GLX_DONT_CARE;
+ break;
default:
/* If an unknown attribute is received, fail.
*/
diff --git a/src/glx/drisw_glx.c b/src/glx/drisw_glx.c
index df2467a5c2..e33ac4f2a9 100644
--- a/src/glx/drisw_glx.c
+++ b/src/glx/drisw_glx.c
@@ -628,6 +628,7 @@ driswBindExtensions(struct drisw_screen *psc, const __DRIextension **extensions)
if (psc->swrast->base.version >= 3) {
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context");
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context_profile");
+ __glXEnableDirectExtension(&psc->base, "GLX_EXT_no_config_context");

/* DRISW version >= 2 implies support for OpenGL ES.
*/
diff --git a/src/glx/g_glxglvnddispatchfuncs.c b/src/glx/g_glxglvnddispatchfuncs.c
index 56d894eda7..04f6d8263a 100644
--- a/src/glx/g_glxglvnddispatchfuncs.c
+++ b/src/glx/g_glxglvnddispatchfuncs.c
@@ -164,7 +164,19 @@ static GLXContext dispatch_CreateContextAttribsARB(Display *dpy,
__GLXvendorInfo *dd;
GLXContext ret;

- dd = GetDispatchFromFBConfig(dpy, config);
+ if (config) {
+ dd = GetDispatchFromFBConfig(dpy, config);
+ } else if (attrib_list) {
+ int i, screen;
+
+ for (i = 0; attrib_list[i * 2] != None; i++) {
+ if (attrib_list[i * 2] == GLX_SCREEN) {
+ screen = attrib_list[i * 2 + 1];
+ dd = GetDispatchFromDrawable(dpy, RootWindow(dpy, screen));
+ break;
+ }
+ }
+ }
if (dd == NULL)
return None;

diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index eee45d962d..bc5333588b 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -237,7 +237,7 @@ validate_renderType_against_config(const struct glx_config *config,
{
/* GLX_EXT_no_config_context supports any render type */
if (!config)
- return True;
+ return renderType == GLX_DONT_CARE;

switch (renderType) {
case GLX_RGBA_TYPE:
diff --git a/src/glx/glxextensions.c b/src/glx/glxextensions.c
index af6ffbf660..4853ad534e 100644
--- a/src/glx/glxextensions.c
+++ b/src/glx/glxextensions.c
@@ -146,6 +146,7 @@ static const struct extension_info known_glx_extensions[] = {
{ GLX(EXT_fbconfig_packed_float), VER(0,0), Y, Y, N, N },
{ GLX(EXT_framebuffer_sRGB), VER(0,0), Y, Y, N, N },
{ GLX(EXT_import_context), VER(0,0), Y, Y, N, N },
+ { GLX(EXT_no_config_context), VER(0,0), Y, N, N, N },
{ GLX(EXT_texture_from_pixmap), VER(0,0), Y, N, N, N },
{ GLX(EXT_visual_info), VER(0,0), Y, Y, N, N },
{ GLX(EXT_visual_rating), VER(0,0), Y, Y, N, N },
diff --git a/src/glx/glxextensions.h b/src/glx/glxextensions.h
index d73128bd0e..07cd3af0ff 100644
--- a/src/glx/glxextensions.h
+++ b/src/glx/glxextensions.h
@@ -50,6 +50,7 @@ enum
EXT_fbconfig_packed_float_bit,
EXT_framebuffer_sRGB_bit,
EXT_import_context_bit,
+ EXT_no_config_context_bit,
EXT_texture_from_pixmap_bit,
EXT_visual_info_bit,
EXT_visual_rating_bit,
--
2.14.3
Ian Romanick
2017-11-14 22:08:00 UTC
Reply
Permalink
Raw Message
Post by Adam Jackson
This more or less ports EGL_KHR_no_config_context to GLX.
v2: Enable the extension only for those backends that support it.
v3: Fix glvnd path and dri2_convert_glx_attribs()
Khronos: https://github.com/KhronosGroup/OpenGL-Registry/pull/102
---
src/glx/create_context.c | 41 ++++++++++++++++++++++++++-------------
src/glx/dri2_glx.c | 1 +
src/glx/dri3_glx.c | 1 +
src/glx/dri_common.c | 4 ++++
src/glx/drisw_glx.c | 1 +
src/glx/g_glxglvnddispatchfuncs.c | 14 ++++++++++++-
src/glx/glxcmds.c | 2 +-
src/glx/glxextensions.c | 1 +
src/glx/glxextensions.h | 1 +
9 files changed, 50 insertions(+), 16 deletions(-)
diff --git a/src/glx/create_context.c b/src/glx/create_context.c
index 38e949ab4c..eab6511ad8 100644
--- a/src/glx/create_context.c
+++ b/src/glx/create_context.c
@@ -47,21 +47,11 @@ glXCreateContextAttribsARB(Display *dpy, GLXFBConfig config,
xcb_generic_error_t *err;
xcb_void_cookie_t cookie;
unsigned dummy_err = 0;
+ int screen = -1;
-
- if (dpy == NULL || cfg == NULL)
- return NULL;
-
- /* This means that either the caller passed the wrong display pointer or
- * one of the internal GLX data structures (probably the fbconfig) has an
- * error. There is nothing sensible to do, so return an error.
- */
- psc = GetGLXScreenConfigs(dpy, cfg->screen);
- if (psc == NULL)
+ if (dpy == NULL)
return NULL;
- assert(cfg->screen == psc->scr);
-
/* Count the number of attributes specified by the application. All
* attributes appear in pairs, except the terminating None.
*/
@@ -70,6 +60,29 @@ glXCreateContextAttribsARB(Display *dpy, GLXFBConfig config,
/* empty */ ;
}
+ if (cfg) {
+ screen = cfg->screen;
+ } else {
+ int i;
unsigned to prevent dumb GCC warnings. Also, I'm pretty sure this code
can use C99 declarations in the for-loop.

Aside from that, this patch is
Post by Adam Jackson
+ for (i = 0; i < num_attribs; i++) {
+ if (attrib_list[i * 2] == GLX_SCREEN)
+ screen = attrib_list[i * 2 + 1];
+ }
+ }
+
+ /* This means that either the caller passed the wrong display pointer or
+ * one of the internal GLX data structures (probably the fbconfig) has an
+ * error. There is nothing sensible to do, so return an error.
+ */
+ psc = GetGLXScreenConfigs(dpy, screen);
+ if (psc == NULL)
+ return NULL;
+
+ assert(screen == psc->scr);
+
+ if (!cfg && !__glXExtensionBitIsEnabled(psc, EXT_no_config_context_bit))
+ return NULL;
+
if (direct && psc->vtable->create_context_attribs) {
/* GLX drops the error returned by the driver. The expectation is that
* an error will also be returned by the server. The server's error
@@ -104,8 +117,8 @@ glXCreateContextAttribsARB(Display *dpy, GLXFBConfig config,
cookie =
xcb_glx_create_context_attribs_arb_checked(c,
gc->xid,
- cfg->fbconfigID,
- cfg->screen,
+ cfg ? cfg->fbconfigID : 0,
+ screen,
gc->share_xid,
gc->isDirect,
num_attribs,
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 0f44635725..eeec4f0d60 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -1129,6 +1129,7 @@ dri2BindExtensions(struct dri2_screen *psc, struct glx_display * priv,
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context");
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context_profile");
+ __glXEnableDirectExtension(&psc->base, "GLX_EXT_no_config_context");
if ((mask & ((1 << __DRI_API_GLES) |
(1 << __DRI_API_GLES2) |
diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
index a10306fe32..ac2e106a7e 100644
--- a/src/glx/dri3_glx.c
+++ b/src/glx/dri3_glx.c
@@ -720,6 +720,7 @@ dri3_bind_extensions(struct dri3_screen *psc, struct glx_display * priv,
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context");
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context_profile");
+ __glXEnableDirectExtension(&psc->base, "GLX_EXT_no_config_context");
if ((mask & ((1 << __DRI_API_GLES) |
(1 << __DRI_API_GLES2) |
diff --git a/src/glx/dri_common.c b/src/glx/dri_common.c
index 0d0b2d997b..2e0ecaaf8d 100644
--- a/src/glx/dri_common.c
+++ b/src/glx/dri_common.c
@@ -562,6 +562,10 @@ dri2_convert_glx_attribs(unsigned num_attribs, const uint32_t *attribs,
return false;
}
break;
+ /* Implies GLX_EXT_no_config_context */
+ *render_type = GLX_DONT_CARE;
+ break;
/* If an unknown attribute is received, fail.
*/
diff --git a/src/glx/drisw_glx.c b/src/glx/drisw_glx.c
index df2467a5c2..e33ac4f2a9 100644
--- a/src/glx/drisw_glx.c
+++ b/src/glx/drisw_glx.c
@@ -628,6 +628,7 @@ driswBindExtensions(struct drisw_screen *psc, const __DRIextension **extensions)
if (psc->swrast->base.version >= 3) {
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context");
__glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context_profile");
+ __glXEnableDirectExtension(&psc->base, "GLX_EXT_no_config_context");
/* DRISW version >= 2 implies support for OpenGL ES.
*/
diff --git a/src/glx/g_glxglvnddispatchfuncs.c b/src/glx/g_glxglvnddispatchfuncs.c
index 56d894eda7..04f6d8263a 100644
--- a/src/glx/g_glxglvnddispatchfuncs.c
+++ b/src/glx/g_glxglvnddispatchfuncs.c
@@ -164,7 +164,19 @@ static GLXContext dispatch_CreateContextAttribsARB(Display *dpy,
__GLXvendorInfo *dd;
GLXContext ret;
- dd = GetDispatchFromFBConfig(dpy, config);
+ if (config) {
+ dd = GetDispatchFromFBConfig(dpy, config);
+ } else if (attrib_list) {
+ int i, screen;
+
+ for (i = 0; attrib_list[i * 2] != None; i++) {
+ if (attrib_list[i * 2] == GLX_SCREEN) {
+ screen = attrib_list[i * 2 + 1];
+ dd = GetDispatchFromDrawable(dpy, RootWindow(dpy, screen));
+ break;
+ }
+ }
+ }
if (dd == NULL)
return None;
diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index eee45d962d..bc5333588b 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -237,7 +237,7 @@ validate_renderType_against_config(const struct glx_config *config,
{
/* GLX_EXT_no_config_context supports any render type */
if (!config)
- return True;
+ return renderType == GLX_DONT_CARE;
switch (renderType) {
diff --git a/src/glx/glxextensions.c b/src/glx/glxextensions.c
index af6ffbf660..4853ad534e 100644
--- a/src/glx/glxextensions.c
+++ b/src/glx/glxextensions.c
@@ -146,6 +146,7 @@ static const struct extension_info known_glx_extensions[] = {
{ GLX(EXT_fbconfig_packed_float), VER(0,0), Y, Y, N, N },
{ GLX(EXT_framebuffer_sRGB), VER(0,0), Y, Y, N, N },
{ GLX(EXT_import_context), VER(0,0), Y, Y, N, N },
+ { GLX(EXT_no_config_context), VER(0,0), Y, N, N, N },
{ GLX(EXT_texture_from_pixmap), VER(0,0), Y, N, N, N },
{ GLX(EXT_visual_info), VER(0,0), Y, Y, N, N },
{ GLX(EXT_visual_rating), VER(0,0), Y, Y, N, N },
diff --git a/src/glx/glxextensions.h b/src/glx/glxextensions.h
index d73128bd0e..07cd3af0ff 100644
--- a/src/glx/glxextensions.h
+++ b/src/glx/glxextensions.h
@@ -50,6 +50,7 @@ enum
EXT_fbconfig_packed_float_bit,
EXT_framebuffer_sRGB_bit,
EXT_import_context_bit,
+ EXT_no_config_context_bit,
EXT_texture_from_pixmap_bit,
EXT_visual_info_bit,
EXT_visual_rating_bit,
Emil Velikov
2017-11-30 16:10:34 UTC
Reply
Permalink
Raw Message
Post by Adam Jackson
@@ -562,6 +562,10 @@ dri2_convert_glx_attribs(unsigned num_attribs, const uint32_t *attribs,
return false;
}
break;
+ /* Implies GLX_EXT_no_config_context */
+ *render_type = GLX_DONT_CARE;
+ break;
We should fall-through (and fail) when GLX_SCREEN is set but the
extension is missing.
Post by Adam Jackson
/* If an unknown attribute is received, fail.
*/
Emil
Adam Jackson
2017-12-05 20:03:40 UTC
Reply
Permalink
Raw Message
Post by Emil Velikov
Post by Adam Jackson
@@ -562,6 +562,10 @@ dri2_convert_glx_attribs(unsigned num_attribs,
const uint32_t *attribs,
return false;
}
break;
+ /* Implies GLX_EXT_no_config_context */
+ *render_type = GLX_DONT_CARE;
+ break;
We should fall-through (and fail) when GLX_SCREEN is set but the
extension is missing.
Nah. This function does not make reference to the screen we're trying
to create a context for. And, for direct contexts (that call this
conversion helper) this extension is now always present anyway.

- ajax
Emil Velikov
2017-12-06 13:36:06 UTC
Reply
Permalink
Raw Message
Post by Adam Jackson
Post by Emil Velikov
Post by Adam Jackson
@@ -562,6 +562,10 @@ dri2_convert_glx_attribs(unsigned num_attribs,
const uint32_t *attribs,
return false;
}
break;
+ /* Implies GLX_EXT_no_config_context */
+ *render_type = GLX_DONT_CARE;
+ break;
We should fall-through (and fail) when GLX_SCREEN is set but the
extension is missing.
Nah. This function does not make reference to the screen we're trying
to create a context for. And, for direct contexts (that call this
conversion helper) this extension is now always present anyway.
Hmm you're right - having adding check is pain in the rear and an overkill.

-Emil

Adam Jackson
2017-11-14 20:13:05 UTC
Reply
Permalink
Raw Message
When we look up the DRI drawable state we need to associate an fbconfig
with the drawable. With GLX_EXT_no_config_context we can no longer infer
that from the context and must instead query the server.

Signed-off-by: Adam Jackson <***@redhat.com>
---
src/glx/dri_common.c | 22 ++++++++++++++++++++--
src/glx/glx_pbuffer.c | 2 +-
src/glx/glxclient.h | 4 ++++
3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/src/glx/dri_common.c b/src/glx/dri_common.c
index 3b82309fa2..0d0b2d997b 100644
--- a/src/glx/dri_common.c
+++ b/src/glx/dri_common.c
@@ -396,12 +396,25 @@ driDestroyConfigs(const __DRIconfig **configs)
free(configs);
}

+static struct glx_config *
+driInferDrawableConfig(struct glx_screen *psc, GLXDrawable draw)
+{
+ unsigned int fbconfig = 0;
+
+ if (GetDrawableAttribute(psc->dpy, draw, GLX_FBCONFIG_ID, &fbconfig)) {
+ return glx_config_find_fbconfig(psc->configs, fbconfig);
+ }
+
+ return NULL;
+}
+
_X_HIDDEN __GLXDRIdrawable *
driFetchDrawable(struct glx_context *gc, GLXDrawable glxDrawable)
{
struct glx_display *const priv = __glXInitialize(gc->psc->dpy);
__GLXDRIdrawable *pdraw;
struct glx_screen *psc;
+ struct glx_config *config = gc->config;

if (priv == NULL)
return NULL;
@@ -418,8 +431,13 @@ driFetchDrawable(struct glx_context *gc, GLXDrawable glxDrawable)
return pdraw;
}

- pdraw = psc->driScreen->createDrawable(psc, glxDrawable,
- glxDrawable, gc->config);
+ if (config == NULL)
+ config = driInferDrawableConfig(gc->psc, glxDrawable);
+ if (config == NULL)
+ return NULL;
+
+ pdraw = psc->driScreen->createDrawable(psc, glxDrawable, glxDrawable,
+ config);

if (pdraw == NULL) {
ErrorMessageF("failed to create drawable\n");
diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
index 933b5d9ecd..42e7996e37 100644
--- a/src/glx/glx_pbuffer.c
+++ b/src/glx/glx_pbuffer.c
@@ -272,7 +272,7 @@ DestroyDRIDrawable(Display *dpy, GLXDrawable drawable, int destroy_xdrawable)
* 10. Given that, this routine should try to use an array on the stack to
* capture the reply rather than always calling Xmalloc.
*/
-static int
+int
GetDrawableAttribute(Display * dpy, GLXDrawable drawable,
int attribute, unsigned int *value)
{
diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h
index 0d29e5635e..a448c4c000 100644
--- a/src/glx/glxclient.h
+++ b/src/glx/glxclient.h
@@ -841,6 +841,10 @@ indirect_create_context_attribs(struct glx_screen *base,
const uint32_t *attribs,
unsigned *error);

+
+extern int GetDrawableAttribute(Display * dpy, GLXDrawable drawable,
+ int attribute, unsigned int *value);
+
#ifdef __cplusplus
}
#endif
--
2.14.3
Ian Romanick
2017-11-14 22:03:51 UTC
Reply
Permalink
Raw Message
Post by Adam Jackson
When we look up the DRI drawable state we need to associate an fbconfig
with the drawable. With GLX_EXT_no_config_context we can no longer infer
that from the context and must instead query the server.
---
src/glx/dri_common.c | 22 ++++++++++++++++++++--
src/glx/glx_pbuffer.c | 2 +-
src/glx/glxclient.h | 4 ++++
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/src/glx/dri_common.c b/src/glx/dri_common.c
index 3b82309fa2..0d0b2d997b 100644
--- a/src/glx/dri_common.c
+++ b/src/glx/dri_common.c
@@ -396,12 +396,25 @@ driDestroyConfigs(const __DRIconfig **configs)
free(configs);
}
+static struct glx_config *
+driInferDrawableConfig(struct glx_screen *psc, GLXDrawable draw)
+{
+ unsigned int fbconfig = 0;
+
+ if (GetDrawableAttribute(psc->dpy, draw, GLX_FBCONFIG_ID, &fbconfig)) {
+ return glx_config_find_fbconfig(psc->configs, fbconfig);
+ }
+
+ return NULL;
+}
+
_X_HIDDEN __GLXDRIdrawable *
driFetchDrawable(struct glx_context *gc, GLXDrawable glxDrawable)
{
struct glx_display *const priv = __glXInitialize(gc->psc->dpy);
__GLXDRIdrawable *pdraw;
struct glx_screen *psc;
+ struct glx_config *config = gc->config;
if (priv == NULL)
return NULL;
@@ -418,8 +431,13 @@ driFetchDrawable(struct glx_context *gc, GLXDrawable glxDrawable)
return pdraw;
}
- pdraw = psc->driScreen->createDrawable(psc, glxDrawable,
- glxDrawable, gc->config);
+ if (config == NULL)
+ config = driInferDrawableConfig(gc->psc, glxDrawable);
+ if (config == NULL)
+ return NULL;
+
+ pdraw = psc->driScreen->createDrawable(psc, glxDrawable, glxDrawable,
+ config);
if (pdraw == NULL) {
ErrorMessageF("failed to create drawable\n");
diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
index 933b5d9ecd..42e7996e37 100644
--- a/src/glx/glx_pbuffer.c
+++ b/src/glx/glx_pbuffer.c
@@ -272,7 +272,7 @@ DestroyDRIDrawable(Display *dpy, GLXDrawable drawable, int destroy_xdrawable)
* 10. Given that, this routine should try to use an array on the stack to
* capture the reply rather than always calling Xmalloc.
*/
-static int
+int
GetDrawableAttribute(Display * dpy, GLXDrawable drawable,
I'm trying to decide whether or not this needs a __glX prefix now. Yes?

Other than that, this patch is
Post by Adam Jackson
int attribute, unsigned int *value)
{
diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h
index 0d29e5635e..a448c4c000 100644
--- a/src/glx/glxclient.h
+++ b/src/glx/glxclient.h
@@ -841,6 +841,10 @@ indirect_create_context_attribs(struct glx_screen *base,
const uint32_t *attribs,
unsigned *error);
+
+extern int GetDrawableAttribute(Display * dpy, GLXDrawable drawable,
+ int attribute, unsigned int *value);
+
#ifdef __cplusplus
}
#endif
Adam Jackson
2017-12-01 21:02:24 UTC
Reply
Permalink
Raw Message
Post by Ian Romanick
Post by Adam Jackson
diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
index 933b5d9ecd..42e7996e37 100644
--- a/src/glx/glx_pbuffer.c
+++ b/src/glx/glx_pbuffer.c
@@ -272,7 +272,7 @@ DestroyDRIDrawable(Display *dpy, GLXDrawable drawable, int destroy_xdrawable)
* 10. Given that, this routine should try to use an array on the stack to
* capture the reply rather than always calling Xmalloc.
*/
-static int
+int
GetDrawableAttribute(Display * dpy, GLXDrawable drawable,
I'm trying to decide whether or not this needs a __glX prefix now. Yes?
Eh, sure. Anyone whose toolchain still doesn't have visibility control
is losing pretty hard, but we may as well be consistent.
Post by Ian Romanick
Other than that, this patch is
Merged 1 2 and 5, will respin the rest.

- ajax
Adam Jackson
2017-11-14 20:13:03 UTC
Reply
Permalink
Raw Message
Only relevant for indirect contexts, so let's get that code out of the
common path.

Signed-off-by: Adam Jackson <***@redhat.com>
---
src/glx/glxcurrent.c | 12 ------------
src/glx/indirect_glx.c | 18 +++++++++++++++---
2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/glx/glxcurrent.c b/src/glx/glxcurrent.c
index fd04929b89..9f8bf7cee1 100644
--- a/src/glx/glxcurrent.c
+++ b/src/glx/glxcurrent.c
@@ -238,18 +238,6 @@ MakeContextCurrent(Display * dpy, GLXDrawable draw,

__glXUnlock();

- /* The indirect vertex array state must to be initialised after we
- * have setup the context, as it needs to query server attributes.
- */
- if (gc && !gc->isDirect) {
- __GLXattribute *state = gc->client_state_private;
- if (state && state->array_state == NULL) {
- glGetString(GL_EXTENSIONS);
- glGetString(GL_VERSION);
- __glXInitVertexArrayState(gc);
- }
- }
-
return GL_TRUE;
}

diff --git a/src/glx/indirect_glx.c b/src/glx/indirect_glx.c
index cfae12f6c0..b552b5768a 100644
--- a/src/glx/indirect_glx.c
+++ b/src/glx/indirect_glx.c
@@ -148,9 +148,21 @@ indirect_bind_context(struct glx_context *gc, struct glx_context *old,
sent = SendMakeCurrentRequest(dpy, gc->xid, tag, draw, read,
&gc->currentContextTag);

- if (!IndirectAPI)
- IndirectAPI = __glXNewIndirectAPI();
- _glapi_set_dispatch(IndirectAPI);
+ 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);
+ }
+ }

return !sent;
}
--
2.14.3
Ian Romanick
2017-11-14 21:15:45 UTC
Reply
Permalink
Raw Message
Post by Adam Jackson
Only relevant for indirect contexts, so let's get that code out of the
common path.
---
src/glx/glxcurrent.c | 12 ------------
src/glx/indirect_glx.c | 18 +++++++++++++++---
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/glx/glxcurrent.c b/src/glx/glxcurrent.c
index fd04929b89..9f8bf7cee1 100644
--- a/src/glx/glxcurrent.c
+++ b/src/glx/glxcurrent.c
@@ -238,18 +238,6 @@ MakeContextCurrent(Display * dpy, GLXDrawable draw,
__glXUnlock();
- /* The indirect vertex array state must to be initialised after we
- * have setup the context, as it needs to query server attributes.
- */
- if (gc && !gc->isDirect) {
- __GLXattribute *state = gc->client_state_private;
- if (state && state->array_state == NULL) {
- glGetString(GL_EXTENSIONS);
- glGetString(GL_VERSION);
- __glXInitVertexArrayState(gc);
- }
- }
-
return GL_TRUE;
}
diff --git a/src/glx/indirect_glx.c b/src/glx/indirect_glx.c
index cfae12f6c0..b552b5768a 100644
--- a/src/glx/indirect_glx.c
+++ b/src/glx/indirect_glx.c
@@ -148,9 +148,21 @@ indirect_bind_context(struct glx_context *gc, struct glx_context *old,
sent = SendMakeCurrentRequest(dpy, gc->xid, tag, draw, read,
&gc->currentContextTag);
- if (!IndirectAPI)
- IndirectAPI = __glXNewIndirectAPI();
- _glapi_set_dispatch(IndirectAPI);
+ 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);
+ }
+ }
This is where this code used to be, but commit d57c85c1 moved it. Does
this not re-regress things? I guess wrapping it in 'if (sent)' seems
like it ought to be enough to prevent the original problem.
Post by Adam Jackson
return !sent;
}
Adam Jackson
2017-11-14 21:30:35 UTC
Reply
Permalink
Raw Message
Post by Ian Romanick
Post by Adam Jackson
diff --git a/src/glx/indirect_glx.c b/src/glx/indirect_glx.c
index cfae12f6c0..b552b5768a 100644
--- a/src/glx/indirect_glx.c
+++ b/src/glx/indirect_glx.c
@@ -148,9 +148,21 @@ indirect_bind_context(struct glx_context *gc, struct glx_context *old,
sent = SendMakeCurrentRequest(dpy, gc->xid, tag, draw, read,
&gc->currentContextTag);
- if (!IndirectAPI)
- IndirectAPI = __glXNewIndirectAPI();
- _glapi_set_dispatch(IndirectAPI);
+ 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);
+ }
+ }
This is where this code used to be, but commit d57c85c1 moved it. Does
this not re-regress things? I guess wrapping it in 'if (sent)' seems
like it ought to be enough to prevent the original problem.
Eep, good catch. I think this would indeed re-regress things. We're
calling ::bind before __glXSetCurrentContext - in fact with the dummy
context bound - which means those glGetStrings would probably fizzle.

I'll dig into this a bit further, hopefully a real piglit run will
shake out this and anything else remaining.

- ajax
Adam Jackson
2017-11-14 20:13:02 UTC
Reply
Permalink
Raw Message
This also fixes a bug, the error path through MakeCurrent didn't
translate the error code by the extension's error base.

Signed-off-by: Adam Jackson <***@redhat.com>
---
src/glx/glxcmds.c | 10 +---------
src/glx/glxcurrent.c | 20 +++-----------------
2 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index bc93d62510..eee45d962d 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -392,15 +392,7 @@ glXCreateContext(Display * dpy, XVisualInfo * vis,
config = glx_config_find_visual(psc->visuals, vis->visualid);

if (config == NULL) {
- xError error;
-
- error.errorCode = BadValue;
- error.resourceID = vis->visualid;
- error.sequenceNumber = dpy->request;
- error.type = X_Error;
- error.majorCode = __glXSetupForCommand(dpy);
- error.minorCode = X_GLXCreateContext;
- _XError(dpy, &error);
+ __glXSendError(dpy, BadValue, vis->visualid, X_GLXCreateContext, True);
return None;
}

diff --git a/src/glx/glxcurrent.c b/src/glx/glxcurrent.c
index d1193265f9..fd04929b89 100644
--- a/src/glx/glxcurrent.c
+++ b/src/glx/glxcurrent.c
@@ -36,8 +36,8 @@
#include <pthread.h>

#include "glxclient.h"
-
#include "glapi.h"
+#include "glx_error.h"

/*
** We setup some dummy structures here so that the API can be used
@@ -165,21 +165,6 @@ glXGetCurrentDrawable(void)
return gc->currentDrawable;
}

-static void
-__glXGenerateError(Display * dpy, XID resource,
- BYTE errorCode, CARD16 minorCode)
-{
- xError error;
-
- error.errorCode = errorCode;
- error.resourceID = resource;
- error.sequenceNumber = dpy->request;
- error.type = X_Error;
- error.majorCode = __glXSetupForCommand(dpy);
- error.minorCode = minorCode;
- _XError(dpy, &error);
-}
-
/**
* Make a particular context current.
*
@@ -228,7 +213,8 @@ MakeContextCurrent(Display * dpy, GLXDrawable draw,
if (gc->vtable->bind(gc, oldGC, draw, read) != Success) {
__glXSetCurrentContextNull();
__glXUnlock();
- __glXGenerateError(dpy, None, GLXBadContext, X_GLXMakeContextCurrent);
+ __glXSendError(dpy, GLXBadContext, None, X_GLXMakeContextCurrent,
+ False);
return GL_FALSE;
}
--
2.14.3
Ian Romanick
2017-11-14 21:10:32 UTC
Reply
Permalink
Raw Message
This patch is
Post by Adam Jackson
This also fixes a bug, the error path through MakeCurrent didn't
translate the error code by the extension's error base.
---
src/glx/glxcmds.c | 10 +---------
src/glx/glxcurrent.c | 20 +++-----------------
2 files changed, 4 insertions(+), 26 deletions(-)
diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index bc93d62510..eee45d962d 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -392,15 +392,7 @@ glXCreateContext(Display * dpy, XVisualInfo * vis,
config = glx_config_find_visual(psc->visuals, vis->visualid);
if (config == NULL) {
- xError error;
-
- error.errorCode = BadValue;
- error.resourceID = vis->visualid;
- error.sequenceNumber = dpy->request;
- error.type = X_Error;
- error.majorCode = __glXSetupForCommand(dpy);
- error.minorCode = X_GLXCreateContext;
- _XError(dpy, &error);
+ __glXSendError(dpy, BadValue, vis->visualid, X_GLXCreateContext, True);
return None;
}
diff --git a/src/glx/glxcurrent.c b/src/glx/glxcurrent.c
index d1193265f9..fd04929b89 100644
--- a/src/glx/glxcurrent.c
+++ b/src/glx/glxcurrent.c
@@ -36,8 +36,8 @@
#include <pthread.h>
#include "glxclient.h"
-
#include "glapi.h"
+#include "glx_error.h"
/*
** We setup some dummy structures here so that the API can be used
@@ -165,21 +165,6 @@ glXGetCurrentDrawable(void)
return gc->currentDrawable;
}
-static void
-__glXGenerateError(Display * dpy, XID resource,
- BYTE errorCode, CARD16 minorCode)
-{
- xError error;
-
- error.errorCode = errorCode;
- error.resourceID = resource;
- error.sequenceNumber = dpy->request;
- error.type = X_Error;
- error.majorCode = __glXSetupForCommand(dpy);
- error.minorCode = minorCode;
- _XError(dpy, &error);
-}
-
/**
* Make a particular context current.
*
@@ -228,7 +213,8 @@ MakeContextCurrent(Display * dpy, GLXDrawable draw,
if (gc->vtable->bind(gc, oldGC, draw, read) != Success) {
__glXSetCurrentContextNull();
__glXUnlock();
- __glXGenerateError(dpy, None, GLXBadContext, X_GLXMakeContextCurrent);
+ __glXSendError(dpy, GLXBadContext, None, X_GLXMakeContextCurrent,
+ False);
return GL_FALSE;
}
Emil Velikov
2017-11-30 15:46:43 UTC
Reply
Permalink
Raw Message
Post by Adam Jackson
This also fixes a bug, the error path through MakeCurrent didn't
translate the error code by the extension's error base.
May I suggest splitting the MakeContextCurrent bugfix from the refactor?
Reason being, that the refactor is not a no-op.

Namely: __glXSendError does not call
__glXSetupForCommand/__glXFlushRenderBuffer.

-Emil
Loading...