[Mesa-dev] [PATCH] glx: conditionaly build dri3 and present loader

Fri Nov 8 14:04:27 PST 2013

On 11/08/2013 01:38 PM, Armin K wrote:
> This patch makes it possible to disable dri3 if desired.
> 
> I am not sure if first set of changes for src/glx/glxext.c
> was necessary. Feel free to modify it as you may please.
> 
> Tested with:
> 
> ./configure --disable-dri3 --with-dri-drivers=i965 \
> --with-gallium-drivers= --disable-vdpau --disable-egl \
> --disable-gbm --disable-xvmc

I'm generally not thrilled about conditional support for features like
this.  The problem is that we end up with a bunch of combinations of
builds, and very few of them actually get tested.

In spite of the great efforts by Matt and others, Mesa's build system is
still... rough around the edges.  It's easy to get into a situation
where libGL is built one way and drivers are built another.  This often
leads to hours wasted debugging non-problems.

As I point out in a couple places below, it also adds clutter to the code.

> ---
>  configure.ac        | 21 ++++++++++++++++++---
>  src/glx/Makefile.am |  8 ++++++--
>  src/glx/glxclient.h |  4 ++++
>  src/glx/glxext.c    |  6 ++++++
>  4 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 8fb5e0d..7ed9507 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -536,6 +536,11 @@ AC_ARG_ENABLE([dri],
>          [enable DRI modules @<:@default=enabled@:>@])],
>      [enable_dri="$enableval"],
>      [enable_dri=yes])
> +AC_ARG_ENABLE([dri3],
> +    [AS_HELP_STRING([--enable-dri3],
> +        [enable DRI3 @<:@default=enabled@:>@])],
> +    [enable_dri3="$enableval"],
> +    [enable_dri3=yes])
>  AC_ARG_ENABLE([glx],
>      [AS_HELP_STRING([--enable-glx],
>          [enable GLX library @<:@default=enabled@:>@])],
> @@ -702,6 +707,7 @@ fi
>  AM_CONDITIONAL(HAVE_DRI_GLX, test "x$enable_glx" = xyes -a \
>                                    "x$enable_dri" = xyes)
>  AM_CONDITIONAL(HAVE_DRI, test "x$enable_dri" = xyes)
> +AM_CONDITIONAL(HAVE_DRI3, test "x$enable_dri3" = xyes)
>  
>  AC_ARG_ENABLE([shared-glapi],
>      [AS_HELP_STRING([--enable-shared-glapi],
> @@ -811,13 +817,19 @@ xyesno)
>          fi
>          PKG_CHECK_MODULES([DRI2PROTO], [dri2proto >= $DRI2PROTO_REQUIRED])
>          GL_PC_REQ_PRIV="$GL_PC_REQ_PRIV libdrm >= $LIBDRM_REQUIRED"
> -        PKG_CHECK_MODULES([DRI3PROTO], [dri3proto >= $DRI3PROTO_REQUIRED])
> -        PKG_CHECK_MODULES([PRESENTPROTO], [presentproto >= $PRESENTPROTO_REQUIRED])
>          PKG_CHECK_MODULES([LIBUDEV], [libudev >= $LIBUDEV_REQUIRED])
> +        if test x"$enable_dri3" = xyes; then
> +            PKG_CHECK_MODULES([DRI3PROTO], [dri3proto >= $DRI3PROTO_REQUIRED])
> +            PKG_CHECK_MODULES([PRESENTPROTO], [presentproto >= $PRESENTPROTO_REQUIRED])
> +        fi
>      fi
>  
>      # find the DRI deps for libGL
> -    dri_modules="x11 xext xdamage xfixes x11-xcb xcb-glx >= 1.8.1 xcb-dri2 >= 1.8 xcb-dri3 xcb-present xcb-sync xshmfence"
> +    dri_modules="x11 xext xdamage xfixes x11-xcb xcb-glx >= 1.8.1 xcb-dri2 >= 1.8 xcb-dri3 xcb-present xcb-sync"
> +
> +    if test x"$enable_dri3" = xyes; then
> +        dri_modules="$dri_modules xshmfence"
> +    fi
>  
>      # add xf86vidmode if available
>      PKG_CHECK_MODULES([XF86VIDMODE], [xxf86vm], HAVE_XF86VIDMODE=yes, HAVE_XF86VIDMODE=no)
> @@ -947,6 +959,9 @@ if test "x$enable_dri" = xyes; then
>      linux*)
>          DEFINES="$DEFINES -DUSE_EXTERNAL_DXTN_LIB=1"
>          DEFINES="$DEFINES -DHAVE_ALIAS"
> +        if test "x$enable_dri3" = xyes; then
> +            DEFINES="$DEFINES -DHAVE_DRI3"
> +        fi
>  
>          case "$host_cpu" in
>          x86_64|amd64)
> diff --git a/src/glx/Makefile.am b/src/glx/Makefile.am
> index ae296b9..0aec2aa 100644
> --- a/src/glx/Makefile.am
> +++ b/src/glx/Makefile.am
> @@ -94,10 +94,14 @@ libglx_la_SOURCES = \
>  	  dri2_glx.c \
>  	  dri2.c \
>  	  dri2_query_renderer.c \
> -          dri3_glx.c \
> -          dri3_common.c \
>  	  applegl_glx.c
>  
> +if HAVE_DRI3
> +libglx_la_SOURCES += \
> +          dri3_glx.c \
> +          dri3_common.c
> +endif
> +
>  GL_LIBS = \
>  	libglx.la \
>  	$(SHARED_GLAPI_LIBS) \
> diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h
> index e26a83e..8ccc843 100644
> --- a/src/glx/glxclient.h
> +++ b/src/glx/glxclient.h
> @@ -150,7 +150,9 @@ extern __GLXDRIdisplay *dri2CreateDisplay(Display * dpy);
>  extern void dri2InvalidateBuffers(Display *dpy, XID drawable);
>  extern unsigned dri2GetSwapEventType(Display *dpy, XID drawable);
>  
> +#if defined(HAVE_DRI3)
>  extern __GLXDRIdisplay *dri3_create_display(Display * dpy);
> +#endif

I don't think this change is strictly necessary, is it?  The extra
prototype should be harmless.  If code accidentally calls
dri3_create_display, we'll get a link failure instead of a compile
failure.  Right?

>  /*
>  ** Functions to obtain driver configuration information from a direct
> @@ -588,8 +590,10 @@ struct glx_display
>     __GLXDRIdisplay *driswDisplay;
>     __GLXDRIdisplay *driDisplay;
>     __GLXDRIdisplay *dri2Display;
> +#if defined(HAVE_DRI3)
>     __GLXDRIdisplay *dri3Display;
>  #endif
> +#endif

I don't think this change is strictly necessary either.  The extra field
should be harmless.

>  };
>  
>  struct glx_drawable {
> diff --git a/src/glx/glxext.c b/src/glx/glxext.c
> index c6e4d9f..bcf3e9c 100644
> --- a/src/glx/glxext.c
> +++ b/src/glx/glxext.c
> @@ -770,9 +770,13 @@ AllocAndFetchScreenConfigs(Display * dpy, struct glx_display * priv)
>     for (i = 0; i < screens; i++, psc++) {
>        psc = NULL;
>  #if defined(GLX_DIRECT_RENDERING) && !defined(GLX_USE_APPLEGL)
> +#if defined(HAVE_DRI3)
>        if (priv->dri3Display)
>           psc = (*priv->dri3Display->createScreen) (i, priv);
>        if (psc == NULL && priv->dri2Display)
> +#else
> +      if (priv->dri2Display)
> +#endif

Since psc is previously set to NULL, I think it's easier to just #if
around the "if (priv->dri3Display) psc =
(*priv->dri3Display->createScreen) (i, priv);" block.  It makes the code
a little easier to read.

Thinking about it more... if we don't remove the dri3Display field, we
shouldn't need this hunk at all because dri3Display will always be NULL.

>  	 psc = (*priv->dri2Display->createScreen) (i, priv);
>        if (psc == NULL && priv->driDisplay)
>  	 psc = (*priv->driDisplay->createScreen) (i, priv);
> @@ -865,8 +869,10 @@ __glXInitialize(Display * dpy)
>      ** (e.g., those called in AllocAndFetchScreenConfigs).
>      */
>     if (glx_direct && glx_accel) {
> +#if defined(HAVE_DRI3)
>        if (!getenv("LIBGL_DRI3_DISABLE"))
>           dpyPriv->dri3Display = dri3_create_display(dpy);
> +#endif
>        dpyPriv->dri2Display = dri2CreateDisplay(dpy);
>        dpyPriv->driDisplay = driCreateDisplay(dpy);
>     }
> 