[PATCH] xfree86: vgaarb: make arbiter optional at build time (v2)

[Tiago Vignatti]

On Fri, Sep 11, 2009 at 07:43:43PM +0200, ext Dan Nicholson wrote:
> On Fri, Sep 11, 2009 at 9:04 AM, Tiago Vignatti
> <tiago.vignatti at nokia.com> wrote:
> > This patch implements the option of a server without support for vga
> > arbitration, using dummy functions. It introduces --{enable, disable}-vgaarb
> > to autoconf (enabled by default).
> >
> > Note that the server now requires libpciaccess >= 0.10.8.
> >
> > Signed-off-by: Tiago Vignatti <tiago.vignatti at nokia.com>
> > ---
> >
> > What about this one, Dave? I don't know if this way is how you thought. Well,
> > here are the stats:
> >
> > - with vgaarbiter
> >   text    data     bss     dec     hex filename
> > 1658395   48164   43756 1750315  1ab52b /opt/master/bin/Xorg
> >
> > - without vgaarbiter and with dummy functions we got .7411 % of text removal:
> >   text    data     bss     dec     hex filename
> > 1646105   48008   43756 1737869  1a848d /opt/master/bin/Xorg
> >
> > - without vgaarbiter with ugly macros (previous patch) all over the code we
> >  got .7657 % of text removal:
> >   text    data     bss     dec     hex filename
> > 1645698   47976   43756 1737430  1a82d6 /opt/master/bin/Xorg
> >
> > So it's far from being an extraordinary removal but IMHO already justifies.
> >
> >
> >
> >  configure.ac                       |   10 ++++++++--
> >  hw/xfree86/common/Makefile.am      |   10 +++++++---
> >  hw/xfree86/common/xf86VGAarbiter.c |    5 ++---
> >  hw/xfree86/common/xf86VGAarbiter.h |    2 +-
> >  hw/xfree86/common/xf86str.h        |    6 ++++++
> >  hw/xfree86/dri/dri.c               |    2 +-
> >  include/xorg-config.h.in           |    6 +++---
> >  7 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 6345fd9..28e5a59 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -591,6 +591,7 @@ AC_ARG_ENABLE(xaa,               AS_HELP_STRING([--enable-xaa], [Build XAA (defa
> >  AC_ARG_ENABLE(vgahw,          AS_HELP_STRING([--enable-vgahw], [Build Xorg with vga access (default: enabled)]), [VGAHW=$enableval], [VGAHW=yes])
> >  AC_ARG_ENABLE(vbe,            AS_HELP_STRING([--enable-vbe], [Build Xorg with VBE module (default: enabled)]), [VBE=$enableval], [VBE=yes])
> >  AC_ARG_ENABLE(int10-module,     AS_HELP_STRING([--enable-int10-module], [Build Xorg with int10 module (default: enabled)]), [INT10MODULE=$enableval], [INT10MODULE=yes])
> > +AC_ARG_ENABLE(vgaarb,         AS_HELP_STRING([--enable-vgaarb], [Build Xorg with VGA arbitration support (default: enabled)]), [VGAARBITER=$enableval], [VGAARBITER=yes])
> >
> >  dnl DDXes.
> >  AC_ARG_ENABLE(xorg,                  AS_HELP_STRING([--enable-xorg], [Build Xorg server (default: auto)]), [XORG=$enableval], [XORG=auto])
> > @@ -1381,7 +1382,7 @@ if test "x$XORG" = xyes; then
> >        AC_SUBST([symbol_visibility])
> >        dnl ===================================================================
> >
> > -       PKG_CHECK_MODULES([PCIACCESS], [pciaccess >= 0.8.0])
> > +       PKG_CHECK_MODULES([PCIACCESS], [pciaccess >= 0.10.8])
> >        SAVE_LIBS=$LIBS
> >        SAVE_CFLAGS=$CFLAGS
> >        CFLAGS=$PCIACCESS_CFLAGS
> > @@ -1389,7 +1390,11 @@ if test "x$XORG" = xyes; then
> >        AC_CHECK_FUNCS([pci_system_init_dev_mem])
> >        AC_CHECK_FUNCS([pci_device_enable])
> >        AC_CHECK_FUNCS([pci_device_is_boot_vga])
> > -       AC_CHECK_FUNCS([pci_device_vgaarb_init])
> > +
> > +       if test "x$VGAARBITER" == xyes; then
> > +           AC_DEFINE(VGAARB, 1, [Want VGA arbitration.])
> > +       fi
> 
> Don't use == for the test; that's not portable sh. Just = does the same thing.
> 
> > +
> >        LIBS=$SAVE_LIBS
> >        CFLAGS=$SAVE_CFLAGS
> >        XORG_SYS_LIBS="$XORG_SYS_LIBS $PCIACCESS_LIBS $DLOPEN_LIBS $GLX_SYS_LIBS $SELINUX_LIB"
> > @@ -1609,6 +1614,7 @@ AM_CONDITIONAL([SOLARIS_ASM_INLINE], [test "x$solaris_asm_inline" = xyes])
> >  AM_CONDITIONAL([SOLARIS_VT], [test "x$solaris_vt" = xyes])
> >  AM_CONDITIONAL([DGA], [test "x$DGA" = xyes])
> >  AM_CONDITIONAL([XF86VIDMODE], [test "x$XF86VIDMODE" = xyes])
> > +AM_CONDITIONAL([VGAARBITER], [test "x$VGAARBITER" = xyes])
> >
> >  dnl XWin DDX
> >
> > diff --git a/hw/xfree86/common/Makefile.am b/hw/xfree86/common/Makefile.am
> > index ad27210..76e4bdd 100644
> > --- a/hw/xfree86/common/Makefile.am
> > +++ b/hw/xfree86/common/Makefile.am
> > @@ -17,6 +17,10 @@ if DGA
> >  DGASOURCES = xf86DGA.c
> >  endif
> >
> > +if VGAARBITER
> > +VGAARB_EXTRADIST = xf86VGAarbiterPriv.h
> > +endif
> > +
> >  XISOURCES = xf86Xinput.c xisb.c
> >  XISDKINCS = xf86Xinput.h xisb.h
> >  RANDRSOURCES = xf86RandR.c
> > @@ -83,9 +87,9 @@ EXTRA_DIST = \
> >        xorgVersion.h \
> >        $(MODEDEFSOURCES) \
> >        modeline2c.awk \
> > -       xf86VGAarbiter.h \
> > -       xf86VGAarbiterPriv.h \
> > -        $(DISTKBDSOURCES)
> > +       xf86VGAarbiter.h
> > +       $(VGAARB_EXTRADIST) \
> > +       $(DISTKBDSOURCES)
> 
> You always want to distribute xf86VGAarbiterPriv.h regardless of
> whether the person generating the tarball is using vgaarb or not. So,
> this part of the patch should be removed. You also removed a trailing
> \ after xf86VGAarbiter.h which would break things.

Cool. Thanks for the review Dan.

            Tiago