[Mesa-dev] [PATCH 2/3] glx: Lift sending the MakeCurrent request to top-level code (v2)

[Emil Velikov]

On 5 December 2017 at 21:22, Adam Jackson <ajax at redhat.com> wrote:
> Somewhat terrifyingly, we never sent this for direct contexts, which
> means the server never knew the context/drawable bindings.
>
> To handle this sanely, pull the request code up out of the indirect
> backend, and rewrite the context switch path to call it as appropriate.
> This attempts to preserve the existing behavior of not calling unbind()
> on the context if its refcount would not drop to zero, which is why the
> diff is a little uglier than I'd like.
>
> 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?

> Signed-off-by: Adam Jackson <ajax at 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)
> +{
Nit: I would keep the code move a separate patch, I might be too
pedantic though ;-)


>  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.


> -      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."


> +         if (gc->thread_refcount == 0) {
> +            SetGC(gc, dpy, draw, read);
> +         }
Nit: drop the brackets for the single like if statement.


> +   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;

>  }
>
>  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