Discussion:
[Mesa-dev] [PATCH 2/3] glx: Lift sending the MakeCurrent request to top-level code (v2)
Adam Jackson
2017-12-05 21:22:04 UTC
Permalink
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.

Fixes glx-make-context, glx-multi-context-single-window and
glx-query-drawable-glx_fbconfig_id-window with both direct and indirect
contexts. glx-make-glxdrawable-current regresses, but I believe the test
is in error, as the fbconfig it uses to create the context is just
"something double-buffered and rgba" as opposed to "corresponds to the
visual I used to create the windows".

v2: Style fixups (Ian Romanick)

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

diff --git a/src/glx/glxcurrent.c b/src/glx/glxcurrent.c
index 9f8bf7cee1..0ce80670d2 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,87 @@ MakeContextCurrent(Display * dpy, GLXDrawable draw,
}

_glapi_check_multithread();
-
__glXLock();
+
if (oldGC == gc &&
- gc->currentDrawable == draw && gc->currentReadable == read) {
- __glXUnlock();
- return True;
+ gc->currentDrawable == draw &&
+ gc->currentReadable == read) {
+ /* Same context and drawables: no op, just return */
+ ret = GL_TRUE;
}

- if (oldGC != &dummyContext) {
- if (--oldGC->thread_refcount == 0) {
- oldGC->vtable->unbind(oldGC, gc);
- oldGC->currentDpy = 0;
+ else if (oldGC == gc) {
+ /* Same context and new drawbles: update drawable bindings */
+ 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;
+ else {
+ /* Different contexts: release the old, bind the new */
+ 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 5482d768ff..d4b77b3b6e 100644
--- a/src/glx/indirect_glx.c
+++ b/src/glx/indirect_glx.c
@@ -61,135 +61,41 @@ 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;
+ __GLXattribute *state = gc->client_state_private;
+
+ 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.
+ *
+ * At the point this is called gc->currentDpy is not initialized
+ * nor is the thread's current context actually set. Hence the
+ * cleverness before the GetString calls.
+ */
+ if (state && state->array_state == NULL) {
+ gc->currentDpy = gc->psc->dpy;
+ __glXSetCurrentContext(gc);
+ __glXSetCurrentContext(gc);
+ __indirect_glGetString(GL_EXTENSIONS);
+ __indirect_glGetString(GL_VERSION);
+ __glXInitVertexArrayState(gc);
}

- 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.
- *
- * At the point this is called gc->currentDpy is not initialized
- * nor is the thread's current context actually set. Hence the
- * cleverness before the GetString calls.
- */
- __GLXattribute *state = gc->client_state_private;
- if (state && state->array_state == NULL) {
- gc->currentDpy = gc->psc->dpy;
- __glXSetCurrentContext(gc);
- __indirect_glGetString(GL_EXTENSIONS);
- __indirect_glGetString(GL_VERSION);
- __glXInitVertexArrayState(gc);
- }
- }
+ if (state != NULL && state->array_state != NULL)
+ return Success;

- return !sent;
+ return BadAlloc;
}

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
Adam Jackson
2017-12-05 21:22:03 UTC
Permalink
Only relevant for indirect contexts, so let's get that code out of the
common path.

v2: Add the necessary context setup before calling GetString

Signed-off-by: Adam Jackson <***@redhat.com>
---
src/glx/glxcurrent.c | 12 ------------
src/glx/indirect_glx.c | 26 ++++++++++++++++++++++----
2 files changed, 22 insertions(+), 16 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..5482d768ff 100644
--- a/src/glx/indirect_glx.c
+++ b/src/glx/indirect_glx.c
@@ -34,7 +34,7 @@

#include "glapi.h"
#include "glxclient.h"
-
+#include "indirect.h"
#include "util/debug.h"

#ifndef GLX_USE_APPLEGL
@@ -148,9 +148,27 @@ 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.
+ *
+ * At the point this is called gc->currentDpy is not initialized
+ * nor is the thread's current context actually set. Hence the
+ * cleverness before the GetString calls.
+ */
+ __GLXattribute *state = gc->client_state_private;
+ if (state && state->array_state == NULL) {
+ gc->currentDpy = gc->psc->dpy;
+ __glXSetCurrentContext(gc);
+ __indirect_glGetString(GL_EXTENSIONS);
+ __indirect_glGetString(GL_VERSION);
+ __glXInitVertexArrayState(gc);
+ }
+ }

return !sent;
}
--
2.14.3
Emil Velikov
2017-12-06 14:50:11 UTC
Permalink
Hi Adam,
Post by Adam Jackson
Only relevant for indirect contexts, so let's get that code out of the
common path.
v2: Add the necessary context setup before calling GetString
---
src/glx/glxcurrent.c | 12 ------------
src/glx/indirect_glx.c | 26 ++++++++++++++++++++++----
2 files changed, 22 insertions(+), 16 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..5482d768ff 100644
--- a/src/glx/indirect_glx.c
+++ b/src/glx/indirect_glx.c
@@ -34,7 +34,7 @@
#include "glapi.h"
#include "glxclient.h"
-
+#include "indirect.h"
#include "util/debug.h"
#ifndef GLX_USE_APPLEGL
@@ -148,9 +148,27 @@ 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();
Add blank new line.
Post by Adam Jackson
+ _glapi_set_dispatch(IndirectAPI);
+
+ /* The indirect vertex array state must to be initialised after we
s/must to be initialised/must be initialised/
Post by Adam Jackson
+ * have setup the context, as it needs to query server attributes.
+ *
+ * At the point this is called gc->currentDpy is not initialized
+ * nor is the thread's current context actually set. Hence the
+ * cleverness before the GetString calls.
+ */
+ __GLXattribute *state = gc->client_state_private;
+ if (state && state->array_state == NULL) {
+ gc->currentDpy = gc->psc->dpy;
+ __glXSetCurrentContext(gc);
Unless I'm misreading the SendMakeCurrentRequest rework (patch 2/3)
__glXSetCurrentContext() will be called, hence these two lines +
respective comment could be omitted.

-Emil
Adam Jackson
2017-12-06 18:12:12 UTC
Permalink
Post by Emil Velikov
Post by Adam Jackson
+ * have setup the context, as it needs to query server attributes.
+ *
+ * At the point this is called gc->currentDpy is not initialized
+ * nor is the thread's current context actually set. Hence the
+ * cleverness before the GetString calls.
+ */
+ __GLXattribute *state = gc->client_state_private;
+ if (state && state->array_state == NULL) {
+ gc->currentDpy = gc->psc->dpy;
+ __glXSetCurrentContext(gc);
Unless I'm misreading the SendMakeCurrentRequest rework (patch 2/3)
__glXSetCurrentContext() will be called, hence these two lines +
respective comment could be omitted.
Pretty sure you're misreading something. This is the ->bind hook, if it
succeeds then MakeContextCurrent will call __glXSetCurrentContext.
Since we have not yet returned, we have not yet succeeded, and
__glXSetCurrentContext has not yet been called, so we must do it
ourselves.

- ajax
Emil Velikov
2017-12-12 14:22:37 UTC
Permalink
Post by Adam Jackson
Post by Emil Velikov
Post by Adam Jackson
+ * have setup the context, as it needs to query server attributes.
+ *
+ * At the point this is called gc->currentDpy is not initialized
+ * nor is the thread's current context actually set. Hence the
+ * cleverness before the GetString calls.
+ */
+ __GLXattribute *state = gc->client_state_private;
+ if (state && state->array_state == NULL) {
+ gc->currentDpy = gc->psc->dpy;
+ __glXSetCurrentContext(gc);
Unless I'm misreading the SendMakeCurrentRequest rework (patch 2/3)
__glXSetCurrentContext() will be called, hence these two lines +
respective comment could be omitted.
Pretty sure you're misreading something. This is the ->bind hook, if it
succeeds then MakeContextCurrent will call __glXSetCurrentContext.
Since we have not yet returned, we have not yet succeeded, and
__glXSetCurrentContext has not yet been called, so we must do it
ourselves.
Right my bad. Could swear I saw a __glXSetCurrentContext call before
the ->bind() one.

-Emil
Adam Jackson
2017-12-05 21:22:05 UTC
Permalink
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()
v4: Screeching signedness correctness, and disable a now
inappropriate test.

Khronos: https://github.com/KhronosGroup/OpenGL-Registry/pull/102
Reviewed-by: Kenneth Graunke <***@whitecape.org>
Signed-off-by: Adam Jackson <***@redhat.com>
Reviewed-by: Ian Romanick <***@intel.com>
---
src/glx/applegl_glx.c | 3 +++
src/glx/create_context.c | 37 +++++++++++++++++++------------
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/driwindows_glx.c | 2 +-
src/glx/g_glxglvnddispatchfuncs.c | 14 +++++++++++-
src/glx/glxcmds.c | 2 +-
src/glx/glxextensions.c | 1 +
src/glx/glxextensions.h | 1 +
src/glx/tests/create_context_unittest.cpp | 9 --------
12 files changed, 50 insertions(+), 26 deletions(-)

diff --git a/src/glx/applegl_glx.c b/src/glx/applegl_glx.c
index c086e5146a..85f70058c6 100644
--- a/src/glx/applegl_glx.c
+++ b/src/glx/applegl_glx.c
@@ -134,6 +134,9 @@ applegl_create_context(struct glx_screen *psc,
/* TODO: Integrate this with apple_glx_create_context and make
* struct apple_glx_context inherit from struct glx_context. */

+ if (!config)
+ return NULL;
+
gc = calloc(1, sizeof(*gc));
if (gc == NULL)
return NULL;
diff --git a/src/glx/create_context.c b/src/glx/create_context.c
index 38e949ab4c..bc39ffef1d 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,25 @@ glXCreateContextAttribsARB(Display *dpy, GLXFBConfig config,
/* empty */ ;
}

+ if (cfg) {
+ screen = cfg->screen;
+ } else {
+ for (unsigned int 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 (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 +113,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 f280a8cef7..f0560edeb5 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 ab5d6c5bc0..4930ce35a9 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/driwindows_glx.c b/src/glx/driwindows_glx.c
index 85525431bf..b2dc810f8c 100644
--- a/src/glx/driwindows_glx.c
+++ b/src/glx/driwindows_glx.c
@@ -240,7 +240,7 @@ driwindows_create_context_attribs(struct glx_screen *base,
identical values, so far
*/

- if (!psc->base.driScreen)
+ if (!psc->base.driScreen || !config_base)
return NULL;

/* Check the renderType value */
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 638d8bcbbe..8cb4771869 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,
diff --git a/src/glx/tests/create_context_unittest.cpp b/src/glx/tests/create_context_unittest.cpp
index a2590589db..d19271b47e 100644
--- a/src/glx/tests/create_context_unittest.cpp
+++ b/src/glx/tests/create_context_unittest.cpp
@@ -172,15 +172,6 @@ TEST_F(glXCreateContextAttribARB_test, NULL_display_returns_None)
EXPECT_EQ(0, fake_glx_context::contexts_allocated);
}

-TEST_F(glXCreateContextAttribARB_test, NULL_fbconfig_returns_None)
-{
- GLXContext ctx =
- glXCreateContextAttribsARB(this->dpy, NULL, 0, False, NULL);
-
- EXPECT_EQ(None, ctx);
- EXPECT_EQ(0, fake_glx_context::contexts_allocated);
-}
-
TEST_F(glXCreateContextAttribARB_test, NULL_screen_returns_None)
{
delete psc;
--
2.14.3
Emil Velikov
2017-12-06 15:16:53 UTC
Permalink
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()
v4: Screeching signedness correctness, and disable a now
inappropriate test.
Khronos: https://github.com/KhronosGroup/OpenGL-Registry/pull/102
Reviewed-by: Emil Velikov <***@collabora.com>

-Emil
Kyle Brenneman
2017-12-06 21:38:39 UTC
Permalink
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()
v4: Screeching signedness correctness, and disable a now
inappropriate test.
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;
That should just look up the vendor by screen number, not based on the
root window. Calling GetDispatchFromDrawable instead of
(__VND->getDynDispatch(dpy, screen)) requires an extra round trip to the
server.
Emil Velikov
2017-12-06 15:14:29 UTC
Permalink
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.
Fixes glx-make-context, glx-multi-context-single-window and
glx-query-drawable-glx_fbconfig_id-window with both direct and indirect
contexts. glx-make-glxdrawable-current regresses, but I believe the test
is in error, as the fbconfig it uses to create the context is just
"something double-buffered and rgba" as opposed to "corresponds to the
visual I used to create the windows".
v2: Style fixups (Ian Romanick)
Adam, can we make this 1/3 and cc stable?
Post by Adam Jackson
---
src/glx/glxcurrent.c | 182 +++++++++++++++++++++++++++++++++++++------------
src/glx/indirect_glx.c | 140 +++++++------------------------------
2 files changed, 162 insertions(+), 160 deletions(-)
diff --git a/src/glx/glxcurrent.c b/src/glx/glxcurrent.c
index 9f8bf7cee1..0ce80670d2 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)
+{
Nit: I would keep the code move a separate patch, I might be too
pedantic though ;-)
Post by Adam Jackson
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;
Bool is True False. Then again I would drop the variable and
__glXUnlock/return as applicable.
Post by Adam Jackson
- if (gc->vtable->bind(gc, oldGC, draw, read) != Success) {
+ if (gc->vtable->bind(gc, gc, draw, read) != Success) {
__glXSetCurrentContextNull();
This line seems inconsistent/wrong.

The glXMakeCurrent manpage says "If False is returned, the previously
current rendering context and drawable (if any) remain unchanged."
Post by Adam Jackson
+ if (gc->thread_refcount == 0) {
+ SetGC(gc, dpy, draw, read);
+ }
Nit: drop the brackets for the single like if statement.
Post by Adam Jackson
+ if (state != NULL && state->array_state != NULL)
+ return Success;
- return !sent;
+ return BadAlloc;
Nit: diverge the exception (error path) keeping the main codeflow straight.

if (state == NULL || state->array_state == NULL)
return BadAlloc;

return Success;
Post by Adam Jackson
}
static void
indirect_unbind_context(struct glx_context *gc, struct glx_context *new)
{
Nit: add a comment to link "nothing to do here - MakeContextCurrent
sends the required protocol as applicable"

-Emil
Adam Jackson
2017-12-06 19:25:55 UTC
Permalink
Post by Emil Velikov
Post by Adam Jackson
- if (gc->vtable->bind(gc, oldGC, draw, read) != Success) {
+ if (gc->vtable->bind(gc, gc, draw, read) != Success) {
__glXSetCurrentContextNull();
This line seems inconsistent/wrong.
The glXMakeCurrent manpage says "If False is returned, the previously
current rendering context and drawable (if any) remain unchanged."
Ugh. That's not really possible to get perfectly right, there are
unrecoverable states (think MakeCurrent away from a context that's been
deleted, or whose current drawable is a destroyed window). Still, I
suppose we should try at least a little.

- ajax
Emil Velikov
2017-12-12 14:31:16 UTC
Permalink
Post by Adam Jackson
Post by Emil Velikov
Post by Adam Jackson
- if (gc->vtable->bind(gc, oldGC, draw, read) != Success) {
+ if (gc->vtable->bind(gc, gc, draw, read) != Success) {
__glXSetCurrentContextNull();
This line seems inconsistent/wrong.
The glXMakeCurrent manpage says "If False is returned, the previously
current rendering context and drawable (if any) remain unchanged."
Ugh. That's not really possible to get perfectly right, there are
unrecoverable states (think MakeCurrent away from a context that's been
deleted, or whose current drawable is a destroyed window). Still, I
suppose we should try at least a little.
From a quick look delaying the unbind call should address that. I'd
drop the __glXSetCurrentContextNull call for now and keep the rest of
the yak shaving at a later stage.

Just noticed some copy/paste damage

indirect_bind_context(struct glx_context *gc, struct glx_context *old,
GLXDrawable draw, GLXDrawable read)
{

+ * cleverness before the GetString calls.
+ */
+ if (state && state->array_state == NULL) {
+ gc->currentDpy = gc->psc->dpy;
+ __glXSetCurrentContext(gc);
+ __glXSetCurrentContext(gc);

__glXSetCurrentContext should be only once.

Thanks
Emil

Loading...