[PATCH xserver 7/7] glx: Conditionalize building indirect GLX support

[Emil Velikov]

Hi Adam,

As some people (me alone?) finds all the ifdefined a bit annoying here
are some ideas of how to resolve it. Other than that, there's a couple
of serious suggestions inline, listing them here to just in case:
- One should AC_ERROR when using --disable-glx --enable-iglx ?
- How do we manage {+,-}iglx command line options, when built without
IGLX ? Warn, error or hide the options ?

On 1 April 2016 at 17:53, Adam Jackson <ajax at redhat.com> wrote:
> This nerfs:
>
> - Render and RenderLarge
> - UseXFont
> - CopyContext
> - vendor private GL requests
> - vendor private GLX requests where GLX protocol is only generated for
>   indirect contexts, namely SGI_swap_control, MESA_copy_sub_buffer, and
>   EXT_texture_from_pixmap
>
> What's left is enough GLX to support direct contexts only, and the
> resulting libglx does not need to link against libGL.
>
> Signed-off-by: Adam Jackson <ajax at redhat.com>
> ---
>  configure.ac            | 12 +++++--
>  glx/Makefile.am         | 39 +++++++++++---------
>  glx/glxcmds.c           | 50 +++++++++++++++++++++++++-
>  glx/glxcmdsswap.c       | 34 ++++++++++++++++++
>  glx/glxdri2.c           |  6 ++++
>  glx/glxdriswrast.c      |  7 +++-
>  glx/glxext.c            | 96 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  glx/glxserver.h         |  4 +++
>  include/dix-config.h.in |  3 ++
>  9 files changed, 229 insertions(+), 22 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index dff06ef..9ef0f39 100644
> --- a/configure.ac
> +++ b/configure.ac

> +if test "x$IGLX" = xyes; then
> +    AC_DEFINE(IGLX, 1, [Build indirect GLX support])
> +fi
> +AM_CONDITIONAL(IGLX, test "x$IGLX" = xyes)
> +
Afaics IGLX depends on GLX, thus we should error here ?

That is unless we have have a volunteer to refactor things to allow
IGLX without GLX.


> @@ -52,11 +72,8 @@ libglx_la_SOURCES = \
>         createcontext.c \
>         extension_string.c \
>         extension_string.h \
> -       indirect_util.c \
>         indirect_util.h \
> -       indirect_program.c \
>         indirect_table.h \
Worth moving these alongside their indirect brothers/sisters above,
even though some/most of these are included by the dri* code. Ideally
that'll get untangled at some point.


> --- a/glx/glxcmds.c
> +++ b/glx/glxcmds.c
> @@ -289,7 +289,10 @@ DoCreateContext(__GLXclientState * cl, GLXContextID gcId,
>           * it's a massive attack surface for buffer overflow type
>           * errors.
>           */
> -        if (!enableIndirectGLX) {
> +#if defined(IGLX)
> +        if (!enableIndirectGLX)
> +#endif
I would move the enableIndirectGLX management to inline wrappers in
the header. As is, if xserver is built without IGLX, yet the user
attempts to enable it via the cmd line, they won't get any feedback.
Thus as they try using it there will be a lot of confusion.

I'm thinking about something like

cat some_header.h

static inline void
toggleIndirectGLX(Bool foo)
{
#if defined(IGLX)
   enableIndirectGLX = foo;
#else
   warn/error(xserver was built without support for IGLX)
#endif
}

static inline Bool
isEnabledIndirectGLX(void)
{
#if defined(IGLX)
   return enableIndirectGLX;
#else
   /* warn_once ? */
   return False;
#endif
}

Speaking of which ... enableIndirectGLX should be hidden (kill off the
_X_EXPORT), shouldn't it ?

> +        {
>              client->errorValue = isDirect;
>              return BadValue;
>          }
> @@ -863,6 +866,7 @@ __glXDisp_WaitX(__GLXclientState * cl, GLbyte * pc)
>  int
>  __glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc)
>  {
> +#if defined(IGLX)
>      ClientPtr client = cl->client;
>      xGLXCopyContextReq *req = (xGLXCopyContextReq *) pc;
>      GLXContextID source;
> @@ -937,6 +941,10 @@ __glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc)
>          return BadValue;
>      }
>      return Success;
> +#else
> +    /* we know we never have indirect contexts */
> +    return __glXError(GLXBadContext);
> +#endif
>  }
>
This and the remaining ifdef IGLX could be moved with headers
providing handly stubs. Namely

$ git grep __glXDisp_CopyContext -- glx/indirect_dispatch.h

#if defined(IGLX)
// Function implementation/definition is in indirect_something.c file
int
__glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc);
#else
static inline int
__glXDisp_CopyContext(__GLXclientState * cl, GLbyte * pc)
{
    /* we know we never have indirect contexts */
    return __glXError(GLXBadContext);
}
#endif


One cold even move the generic (non IGLX specific) function
declarations to glx{,_}dispatch.h or alike.


> +#if !defined(IGLX)
> +
> +#define proc(s) \
> +    if (vendorcode == X_GLXvop_ ## s) \
> +        return __glXDisp_ ## s;
> +
Move the define where it's used (within the function) and undef when done ?


> @@ -2359,9 +2399,13 @@ __glXDisp_VendorPrivate(__GLXclientState * cl, GLbyte * pc)
>
>      REQUEST_AT_LEAST_SIZE(xGLXVendorPrivateReq);
>
> +#if defined(IGLX)
>      proc = (__GLXdispatchVendorPrivProcPtr)
>          __glXGetProtocolDecodeFunction(&VendorPriv_dispatch_info,
>                                         vendorcode, 0);
> +#else
> +    proc = __glXGetVendorPrivProc(vendorcode);
> +#endif
Here (and the other 3 cases) one could use something like

proc = __glXGetVendorPrivProc(vendorcode, swapped);

... if one implements an indirect version as a wrapper around
glXGetProtocolDecodeFunction. If the code is in indirect_table.c one
can even make VendorPriv_dispatch_info static ;-)


> @@ -567,6 +568,89 @@ void *__glGetProcAddress(const char *proc)
>
>      return ret ? ret : (void *) NoopDDA;
Unrelated: using NoopDDA on Windows is going to cause stack corruption
due to the different calling conventions - cdecl vs stdcall. Unless
Xwin/Xming does something to override that ?


> @@ -605,8 +689,18 @@ __glXDispatch(ClientPtr client)
>      /*
>       ** Use the opcode to index into the procedure table.
>       */
> +#if defined(IGLX)
>      proc = __glXGetProtocolDecodeFunction(&Single_dispatch_info, opcode,
>                                            client->swapped);
> +#else
> +    if (opcode <= X_GLXSetConfigInfo2ARB) {
> +        if (client->swapped)
> +            proc = SGLXProcVector[opcode];
> +        else
> +            proc = GLXProcVector[opcode];
> +    }
> +#endif
> +
Rework based on the __glXGetVendorPrivProc suggestion above ? Perhaps
even the Render_dispatch_info ones.

Again, please don't read too much into these suggestions, just some
polish that one can do when feeling bored ;-)

Regards,
Emil