glamor and the sync extension

Currently glamor hits an assertion on systems that don't have
xshmfence.  This happens when the glamor code calls
miSyncGetScreenFuncs() because the miSyncScreenPrivateKey has not been
set up.  For systems with xshmfence, this happens when
miSyncShmScreenInit() gets called, but that code is wrapped within
#ifdef HAVE_XSHMFENCE.  The diff below simply calls miSyncSetup()
instead if HAVE_XSHMFENCE is not defined.  This makes things work, but
I'm not sure if this is the right approach.

Thoughts?

Should we just unconditionally enable xshmfence? Are there any OSes we
care about that can't implement a fence primitive?

On Sun, Nov 15, 2015 at 8:59 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> Currently glamor hits an assertion on systems that don't have
> xshmfence.  This happens when the glamor code calls
> miSyncGetScreenFuncs() because the miSyncScreenPrivateKey has not been
> set up.  For systems with xshmfence, this happens when
> miSyncShmScreenInit() gets called, but that code is wrapped within
> #ifdef HAVE_XSHMFENCE.  The diff below simply calls miSyncSetup()
> instead if HAVE_XSHMFENCE is not defined.  This makes things work, but
> I'm not sure if this is the right approach.
>
> Thoughts?
>
>
> Index: glamor/glamor_sync.c
> ===================================================================
> RCS file: /cvs/xenocara/xserver/glamor/glamor_sync.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 glamor_sync.c
> --- glamor/glamor_sync.c        16 Sep 2015 19:10:21 -0000      1.1
> +++ glamor/glamor_sync.c        15 Nov 2015 13:02:31 -0000
> @@ -97,6 +97,9 @@ glamor_sync_init(ScreenPtr screen)
>  #ifdef HAVE_XSHMFENCE
>         if (!miSyncShmScreenInit(screen))
>                 return FALSE;
> +#else
> +       if (!miSyncSetup(screen))
> +               return FALSE;
>  #endif
>
>         screen_funcs = miSyncGetScreenFuncs(screen);
>
> _______________________________________________
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel

> Date: Sun, 15 Nov 2015 09:53:57 -0800
> From: "Jasper St. Pierre" <jstpierre@mecheye.net>
> 
> Should we just unconditionally enable xshmfence? Are there any OSes
> we care about that can't implement a fence primitive?

There currently is no implementation on OpenBSD, and that's the
platform I care about ;).

The futex implementation supportsLinux and FreeBSD.  The pthreads
implementation should in theory supports many other OSes.  But in
practice support for process-shared mutexes and condition variables
isn't very widespread.  It probably works on Solaris, but not much
else.

An important reason why I haven't pushed for an xshmfence
implementation on OpenBSD is that I have some security concerns.
Especially with the pthreads implementation.

It is trivial to block any application that calls xshmfence_await() by
simply never triggering the fence.  I guess that isn't a major concern
as long the xserver never waits on a fence and only triggers them.
I'm not sure that's the case though with dri3-enable glamor.

However, with the pthreads implementation, even triggering a fence
becomes dangerous since it needs to acquire a shared mutex before
broadcasting the condition.  So a client could trivially DOS the
server by locking the mutex and never unlocking it.  Doesn't even have
to be mailicious.  A buggy client could simply crash after locking the
mutex and the server would be toast.  So in my view the pthreads
implementation is not a robust xshmfence implementationand should
probably be avoided.

> On Sun, Nov 15, 2015 at 8:59 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > Currently glamor hits an assertion on systems that don't have
> > xshmfence.  This happens when the glamor code calls
> > miSyncGetScreenFuncs() because the miSyncScreenPrivateKey has not been
> > set up.  For systems with xshmfence, this happens when
> > miSyncShmScreenInit() gets called, but that code is wrapped within
> > #ifdef HAVE_XSHMFENCE.  The diff below simply calls miSyncSetup()
> > instead if HAVE_XSHMFENCE is not defined.  This makes things work, but
> > I'm not sure if this is the right approach.
> >
> > Thoughts?
> >
> >
> > Index: glamor/glamor_sync.c
> > ===================================================================
> > RCS file: /cvs/xenocara/xserver/glamor/glamor_sync.c,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 glamor_sync.c
> > --- glamor/glamor_sync.c        16 Sep 2015 19:10:21 -0000      1.1
> > +++ glamor/glamor_sync.c        15 Nov 2015 13:02:31 -0000
> > @@ -97,6 +97,9 @@ glamor_sync_init(ScreenPtr screen)
> >  #ifdef HAVE_XSHMFENCE
> >         if (!miSyncShmScreenInit(screen))
> >                 return FALSE;
> > +#else
> > +       if (!miSyncSetup(screen))
> > +               return FALSE;
> >  #endif
> >
> >         screen_funcs = miSyncGetScreenFuncs(screen);
> >

Mark Kettenis <mark.kettenis@xs4all.nl> writes:

> Currently glamor hits an assertion on systems that don't have
> xshmfence.  This happens when the glamor code calls
> miSyncGetScreenFuncs() because the miSyncScreenPrivateKey has not been
> set up.  For systems with xshmfence, this happens when
> miSyncShmScreenInit() gets called, but that code is wrapped within
> #ifdef HAVE_XSHMFENCE.  The diff below simply calls miSyncSetup()
> instead if HAVE_XSHMFENCE is not defined.  This makes things work, but
> I'm not sure if this is the right approach.

If you don't have xshmfence working, then you can't use DRI3, but then
the DRI3 support in Mesa unconditionally requires xshmfence currently.

You will still get the sync fences support, which might be useful if you
want to use Present without DRI3. So, yes, I think your patch is a fine
idea.