Re: [Mesa-dev] [PATCH] glx: Demand success from CreateContext requests (v2)
On 2018-08-07 10:55 PM, Adam Jackson wrote:
> GLXCreate{,New}Context, like most X resource creation requests, does not
> emit a reply and therefore is emitted into the X stream asynchronously.
> However, unlike most resource creation requests, the GLXContext we
> return is a handle to library state instead of an XID. So if context
> creation fails for any reason - say, the server doesn't support indirect
> contexts - then we will fail in strange places for strange reasons.
>
> We could make every GLX entrypoint robust against half-created contexts,
> or we could just verify that context creation worked. Reuse the
> __glXIsDirect code to do this, as a cheap way of verifying that the
> XID is real.
>
> glXCreateContextAttribsARB solves this by using the _checked version of
> the xcb command, so effectively this change makes the classic context
> creation paths as robust as CreateContextAttribs.
>
> v2: Better use of Bool, check that error != NULL first (Olivier Fourdan)
>
> Signed-off-by: Adam Jackson
Reviewed-by: Michel Dänzer
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glx: Demand success from CreateContext requests (v2)
GLXCreate{,New}Context, like most X resource creation requests, does not
emit a reply and therefore is emitted into the X stream asynchronously.
However, unlike most resource creation requests, the GLXContext we
return is a handle to library state instead of an XID. So if context
creation fails for any reason - say, the server doesn't support indirect
contexts - then we will fail in strange places for strange reasons.
We could make every GLX entrypoint robust against half-created contexts,
or we could just verify that context creation worked. Reuse the
__glXIsDirect code to do this, as a cheap way of verifying that the
XID is real.
glXCreateContextAttribsARB solves this by using the _checked version of
the xcb command, so effectively this change makes the classic context
creation paths as robust as CreateContextAttribs.
v2: Better use of Bool, check that error != NULL first (Olivier Fourdan)
Signed-off-by: Adam Jackson
---
src/glx/glxcmds.c | 93 ---
1 file changed, 55 insertions(+), 38 deletions(-)
diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index 4db0228eaba..3ed960fbf3c 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -272,6 +272,44 @@ glx_context_init(struct glx_context *gc,
return True;
}
+/**
+ * Determine if a context uses direct rendering.
+ *
+ * \param dpyDisplay where the context was created.
+ * \param contextID ID of the context to be tested.
+ * \param error Out parameter, set to True on error if not NULL
+ *
+ * \returns \c True if the context is direct rendering or not.
+ */
+static Bool
+__glXIsDirect(Display * dpy, GLXContextID contextID, Bool *error)
+{
+ CARD8 opcode;
+ xcb_connection_t *c;
+ xcb_generic_error_t *err;
+ xcb_glx_is_direct_reply_t *reply;
+ Bool is_direct;
+
+ opcode = __glXSetupForCommand(dpy);
+ if (!opcode) {
+ return False;
+ }
+
+ c = XGetXCBConnection(dpy);
+ reply = xcb_glx_is_direct_reply(c, xcb_glx_is_direct(c, contextID), );
+ is_direct = (reply != NULL && reply->is_direct) ? True : False;
+
+ if (err != NULL) {
+ if (error)
+ *error = True;
+ __glXSendErrorForXcb(dpy, err);
+ free(err);
+ }
+
+ free(reply);
+
+ return is_direct;
+}
/**
* Create a new context.
@@ -376,6 +414,21 @@ CreateContext(Display *dpy, int generic_id, struct
glx_config *config,
gc->share_xid = shareList ? shareList->xid : None;
gc->imported = GL_FALSE;
+ /* Unlike most X resource creation requests, we're about to return a handle
+* with client-side state, not just an XID. To simplify error handling
+* elsewhere in libGL, force a round-trip here to ensure the CreateContext
+* request above succeeded.
+*/
+ {
+ Bool error = False;
+ int isDirect = __glXIsDirect(dpy, gc->xid, );
+
+ if (error != False || isDirect != gc->isDirect) {
+ gc->vtable->destroy(gc);
+ gc = NULL;
+ }
+ }
+
return (GLXContext) gc;
}
@@ -612,42 +665,6 @@ glXCopyContext(Display * dpy, GLXContext source_user,
}
-/**
- * Determine if a context uses direct rendering.
- *
- * \param dpyDisplay where the context was created.
- * \param contextID ID of the context to be tested.
- *
- * \returns \c True if the context is direct rendering or not.
- */
-static Bool
-__glXIsDirect(Display * dpy, GLXContextID contextID)
-{
- CARD8 opcode;
- xcb_connection_t *c;
- xcb_generic_error_t *err;
- xcb_glx_is_direct_reply_t *reply;
- Bool is_direct;
-
- opcode = __glXSetupForCommand(dpy);
- if (!opcode) {
- return False;
- }
-
- c = XGetXCBConnection(dpy);
- reply = xcb_glx_is_direct_reply(c, xcb_glx_is_direct(c, contextID), );
- is_direct = (reply != NULL && reply->is_direct) ? True : False;
-
- if (err != NULL) {
- __glXSendErrorForXcb(dpy, err);
- free(err);
- }
-
- free(reply);
-
- return is_direct;
-}
-
/**
* \todo
* Shouldn't this function \b always return \c False when
@@ -668,7 +685,7 @@ glXIsDirect(Display * dpy, GLXContext gc_user)
#ifdef GLX_USE_APPLEGL /* TODO: indirect on darwin */
return False;
#else
- return __glXIsDirect(dpy, gc->xid);
+ return __glXIsDirect(dpy, gc->xid, NULL);
#endif
}
@@ -1428,7 +1445,7 @@ glXImportContextEXT(Display *dpy, GLXContextID contextID)
return NULL;
}
- if (__glXIsDirect(dpy, contextID))
+ if (__glXIsDirect(dpy, contextID, NULL))
return NULL;
opcode = __glXSetupForCommand(dpy);
--
2.17.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glx: Demand success from CreateContext requests
Hey
On Fri, Aug 3, 2018 at 7:44 PM Adam Jackson wrote:
>
> GLXCreate{,New}Context, like most X resource creation requests, does not
> emit a reply and therefore is emitted into the X stream asynchronously.
> However, unlike most resource creation requests, the GLXContext we
> return is a handle to library state instead of an XID. So if context
> creation fails for any reason - say, the server doesn't support indirect
> contexts - then we will fail in strange places for strange reasons.
>
> We could make every GLX entrypoint robust against half-created contexts,
> or we could just verify that context creation worked. Reuse the
> __glXIsDirect code to do this, as a cheap way of verifying that the
> XID is real.
>
> glXCreateContextAttribsARB solves this by using the _checked version of
> the xcb command, so effectively this change makes the classic context
> creation paths as robust as CreateContextAttribs.
>
> Signed-off-by: Adam Jackson
> ---
> src/glx/glxcmds.c | 92 +++
> 1 file changed, 54 insertions(+), 38 deletions(-)
>
> diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
> index 4db0228eaba..3789f55d038 100644
> --- a/src/glx/glxcmds.c
> +++ b/src/glx/glxcmds.c
> @@ -272,6 +272,43 @@ glx_context_init(struct glx_context *gc,
> return True;
> }
>
> +/**
> + * Determine if a context uses direct rendering.
> + *
> + * \param dpyDisplay where the context was created.
> + * \param contextID ID of the context to be tested.
> + * \param error Out parameter, set to True on error if not NULL
> + *
> + * \returns \c True if the context is direct rendering or not.
> + */
> +static Bool
> +__glXIsDirect(Display * dpy, GLXContextID contextID, int *error)
Nitpicking, maybe a Bool would be more explicit here (even if it's the
same), it's set to “None” and later set to “True”.
> +{
> + CARD8 opcode;
> + xcb_connection_t *c;
> + xcb_generic_error_t *err;
> + xcb_glx_is_direct_reply_t *reply;
> + Bool is_direct;
> +
> + opcode = __glXSetupForCommand(dpy);
> + if (!opcode) {
> + return False;
> + }
> +
> + c = XGetXCBConnection(dpy);
> + reply = xcb_glx_is_direct_reply(c, xcb_glx_is_direct(c, contextID), );
> + is_direct = (reply != NULL && reply->is_direct) ? True : False;
> +
> + if (err != NULL) {
> + *error = True;
glXIsDirect() passes “NULL” as “error”, but it's set unconditionally here.
> + __glXSendErrorForXcb(dpy, err);
> + free(err);
> + }
> +
> + free(reply);
> +
> + return is_direct;
> +}
>
> /**
> * Create a new context.
> @@ -376,6 +413,21 @@ CreateContext(Display *dpy, int generic_id, struct
> glx_config *config,
> gc->share_xid = shareList ? shareList->xid : None;
> gc->imported = GL_FALSE;
>
> + /* Unlike most X resource creation requests, we're about to return a
> handle
> +* with client-side state, not just an XID. To simplify error handling
> +* elsewhere in libGL, force a round-trip here to ensure the CreateContext
> +* request above succeeded.
> +*/
> + {
> + int error = None;
> + int isDirect = __glXIsDirect(dpy, gc->xid, );
> +
> + if (error != None || isDirect != gc->isDirect) {
> + gc->vtable->destroy(gc);
> + gc = NULL;
> + }
> + }
> +
> return (GLXContext) gc;
> }
>
> @@ -612,42 +664,6 @@ glXCopyContext(Display * dpy, GLXContext source_user,
> }
>
>
> -/**
> - * Determine if a context uses direct rendering.
> - *
> - * \param dpyDisplay where the context was created.
> - * \param contextID ID of the context to be tested.
> - *
> - * \returns \c True if the context is direct rendering or not.
> - */
> -static Bool
> -__glXIsDirect(Display * dpy, GLXContextID contextID)
> -{
> - CARD8 opcode;
> - xcb_connection_t *c;
> - xcb_generic_error_t *err;
> - xcb_glx_is_direct_reply_t *reply;
> - Bool is_direct;
> -
> - opcode = __glXSetupForCommand(dpy);
> - if (!opcode) {
> - return False;
> - }
> -
> - c = XGetXCBConnection(dpy);
> - reply = xcb_glx_is_direct_reply(c, xcb_glx_is_direct(c, contextID), );
> - is_direct = (reply != NULL && reply->is_direct) ? True : False;
> -
> - if (err != NULL) {
> - __glXSendErrorForXcb(dpy, err);
> - free(err);
> - }
> -
> - free(reply);
> -
> - return is_direct;
> -}
> -
> /**
> * \todo
> * Shouldn't this function \b always return \c False when
> @@ -668,7 +684,7 @@ glXIsDirect(Display * dpy, GLXContext gc_user)
> #ifdef GLX_USE_APPLEGL /* TODO: indirect on darwin */
> return False;
> #else
> - return __glXIsDirect(dpy, gc->xid);
> + return __glXIsDirect(dpy, gc->xid, NULL);
> #endif
> }
>
> @@ -1428,7 +1444,7 @@ glXImportContextEXT(Display *dpy, GLXContextID
> contextID)
>return NULL;
> }
>
> - if (__glXIsDirect(dpy, contextID))
> + if (__glXIsDirect(dpy, contextID, NULL))
>return NULL;
>
> opcode = __glXSetupForCommand(dpy);
> --
> 2.17.0
OIther than the two points
[Mesa-dev] [PATCH] glx: Demand success from CreateContext requests
GLXCreate{,New}Context, like most X resource creation requests, does not
emit a reply and therefore is emitted into the X stream asynchronously.
However, unlike most resource creation requests, the GLXContext we
return is a handle to library state instead of an XID. So if context
creation fails for any reason - say, the server doesn't support indirect
contexts - then we will fail in strange places for strange reasons.
We could make every GLX entrypoint robust against half-created contexts,
or we could just verify that context creation worked. Reuse the
__glXIsDirect code to do this, as a cheap way of verifying that the
XID is real.
glXCreateContextAttribsARB solves this by using the _checked version of
the xcb command, so effectively this change makes the classic context
creation paths as robust as CreateContextAttribs.
Signed-off-by: Adam Jackson
---
src/glx/glxcmds.c | 92 +++
1 file changed, 54 insertions(+), 38 deletions(-)
diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index 4db0228eaba..3789f55d038 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -272,6 +272,43 @@ glx_context_init(struct glx_context *gc,
return True;
}
+/**
+ * Determine if a context uses direct rendering.
+ *
+ * \param dpyDisplay where the context was created.
+ * \param contextID ID of the context to be tested.
+ * \param error Out parameter, set to True on error if not NULL
+ *
+ * \returns \c True if the context is direct rendering or not.
+ */
+static Bool
+__glXIsDirect(Display * dpy, GLXContextID contextID, int *error)
+{
+ CARD8 opcode;
+ xcb_connection_t *c;
+ xcb_generic_error_t *err;
+ xcb_glx_is_direct_reply_t *reply;
+ Bool is_direct;
+
+ opcode = __glXSetupForCommand(dpy);
+ if (!opcode) {
+ return False;
+ }
+
+ c = XGetXCBConnection(dpy);
+ reply = xcb_glx_is_direct_reply(c, xcb_glx_is_direct(c, contextID), );
+ is_direct = (reply != NULL && reply->is_direct) ? True : False;
+
+ if (err != NULL) {
+ *error = True;
+ __glXSendErrorForXcb(dpy, err);
+ free(err);
+ }
+
+ free(reply);
+
+ return is_direct;
+}
/**
* Create a new context.
@@ -376,6 +413,21 @@ CreateContext(Display *dpy, int generic_id, struct
glx_config *config,
gc->share_xid = shareList ? shareList->xid : None;
gc->imported = GL_FALSE;
+ /* Unlike most X resource creation requests, we're about to return a handle
+* with client-side state, not just an XID. To simplify error handling
+* elsewhere in libGL, force a round-trip here to ensure the CreateContext
+* request above succeeded.
+*/
+ {
+ int error = None;
+ int isDirect = __glXIsDirect(dpy, gc->xid, );
+
+ if (error != None || isDirect != gc->isDirect) {
+ gc->vtable->destroy(gc);
+ gc = NULL;
+ }
+ }
+
return (GLXContext) gc;
}
@@ -612,42 +664,6 @@ glXCopyContext(Display * dpy, GLXContext source_user,
}
-/**
- * Determine if a context uses direct rendering.
- *
- * \param dpyDisplay where the context was created.
- * \param contextID ID of the context to be tested.
- *
- * \returns \c True if the context is direct rendering or not.
- */
-static Bool
-__glXIsDirect(Display * dpy, GLXContextID contextID)
-{
- CARD8 opcode;
- xcb_connection_t *c;
- xcb_generic_error_t *err;
- xcb_glx_is_direct_reply_t *reply;
- Bool is_direct;
-
- opcode = __glXSetupForCommand(dpy);
- if (!opcode) {
- return False;
- }
-
- c = XGetXCBConnection(dpy);
- reply = xcb_glx_is_direct_reply(c, xcb_glx_is_direct(c, contextID), );
- is_direct = (reply != NULL && reply->is_direct) ? True : False;
-
- if (err != NULL) {
- __glXSendErrorForXcb(dpy, err);
- free(err);
- }
-
- free(reply);
-
- return is_direct;
-}
-
/**
* \todo
* Shouldn't this function \b always return \c False when
@@ -668,7 +684,7 @@ glXIsDirect(Display * dpy, GLXContext gc_user)
#ifdef GLX_USE_APPLEGL /* TODO: indirect on darwin */
return False;
#else
- return __glXIsDirect(dpy, gc->xid);
+ return __glXIsDirect(dpy, gc->xid, NULL);
#endif
}
@@ -1428,7 +1444,7 @@ glXImportContextEXT(Display *dpy, GLXContextID contextID)
return NULL;
}
- if (__glXIsDirect(dpy, contextID))
+ if (__glXIsDirect(dpy, contextID, NULL))
return NULL;
opcode = __glXSetupForCommand(dpy);
--
2.17.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
