Fixes for VirtuaGL.

[hernando]

Correction of VGL_EXCLUDE variable generation to use , instead of ;. This
is the separator finally adopted by the upstream version 2.4.80.

VNC displays must be interposed when running under vglrun, but hwsd was not
providing enough information to detect a VNC display. This is also fixed
in this commit.

[bbpbuildbot]

Refer to this link for build results (access rights to CI server needed):
https://bbpcode.epfl.ch/ci//job/oss.Equalizer.github/241/

[eile]

So you are ignoring vnc servers since they are not glx capable?

[hernando]

No. It's true that the VNC server we use is not GLX capable, but even if it was, it's a virtual device, so using VirtualGL for OpenGL can't harm.
With this change, displays that refer to a VNC server are not included in the exclude list for VirtualGL. That doesn't mean that they are ignored (that's is done only for the VGL_DISPLAY at line 250) , it means that they must be interposed by VirtualGL. Without this change Equalizer will try to use the VNC display directly and will crash later on.

[eile]

Ok, got it.

That happens if one rung an Eq app without vglrun, or also when run with vglrun from the vnc X server?

So the problem is rather that Eq is trying to use a display which is not GL(X) capable? Is there not a more generic way of filtering this display?

[eile]

The reason I'm so picky about it is that this code already was too complex for my liking. The magic is hard to control! ;)

[hernando]

I agree on that last comment.

The crash happens only when running with vglrun. As a matter of fact, Equalizer does filter non GLX capable displays, because hwsd discovery doesn't report them (gpu/glx/module.cpp:92) (unless you use vglrun as I explain below), so if you run eqPly on the VNC server without VirtualGL it will use the devices from :0 and ignore the VNC display (let's say :1), and you can't see anything.
Now, when vglrun is used, VirtualGL will report :1 as having the GLX extension (this is "works as design"). However, if the display is added to exclude list, it will happen that in fact, it doesn't have it, making Equalizer crash later on. Notice that VirtualGL isn't lying, because the extension is queried before the display is added to the exclude list.

So my answer is that it can be made clearer but not simpler. I was thinking of moving the condition to a separate function, but I'm not fully sure about the logic behind the comparison of info.device against LB_UNDEFINED_UINT32 (which works for the virtual server created by ssh -X or vglconnect in a way that I don't understand because hwsd reports the :10.0 or whatever it is), for that reason I preferred waiting for your comments.

[eile]

So the vnc display under vglrun is detected as a vgl display?
if yes: Why is it added to the exclude list
if no: Why not?

[hernando]

All (local) displays have FLAG_VIRTUALGL when running under vglrun. The reason is simple: VirtualGL is interposing all displays by default, so when hwsd calls glXGetClientString( display, GLX_VENDOR ) the answer is always VirtualGL. This is correct, because as I said, VirtualGL will interpose the display if not added to the exclude list.

Right now, the VNC display is added to the exclude list because the code works that way. Don't ask me why because I don't understand why LB_UNDEFINED_UINT32 is used to identify the server provided by ssh -X.
With VNC there's no way the code below can distinguish between the VNC display and any other local display, so it decides to add it to the exclude list (note that the VGL_DISPLAY device has already been filtered out in line 250 because it mustn't be used in the configuration).

   if( info.device == LB_UNDEFINED_UINT32 &&
        // VirtualGL display redirects to local GPU (see continue above)
        !(info.flags & hwsd::GPUInfo::FLAG_VIRTUALGL) )
    {
        name << "display";
    }
    else
    {
        name << "GPU" << ++gpuCounter;
        if( info.flags & hwsd::GPUInfo::FLAG_VIRTUALGL &&
            info.device != LB_UNDEFINED_UINT32 )
        {
              ... // exclude
        }
    }

If you want we can talk on Skype, because I don't get why are you still so hesitant about this change.

[hernando]

This PR depends on Eyescale/hwsd#47

[hernando]

retest this please

[hernando]

retest this please

[hernando]

Now it builds. Ready to merge?