glx: Demand success from CreateContext requests

Submitted by Adam Jackson on Aug. 3, 2018, 5:44 p.m.

Details

Message ID 20180803174429.13762-1-ajax@redhat.com
State New
Headers show
Series "glx: Demand success from CreateContext requests" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Adam Jackson Aug. 3, 2018, 5:44 p.m.
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 <ajax@redhat.com>
---
 src/glx/glxcmds.c | 92 +++++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 38 deletions(-)

Patch hide | download patch | download mbox

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 dpy        Display 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), &err);
+   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, &error);
+
+      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 dpy        Display 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), &err);
-   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);

Comments

Hey

On Fri, Aug 3, 2018 at 7:44 PM Adam Jackson <ajax@redhat.com> 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 <ajax@redhat.com>
> ---
>  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 dpy        Display 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), &err);
> +   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, &error);
> +
> +      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 dpy        Display 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), &err);
> -   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 above, it works in my test case and avoids
the [xcb] “Unknown sequence number while processing queue” when trying
to catch the XError is IsDirect().


Cheers,
Olivier