Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for VirtuaGL. #499

Merged
merged 1 commit into from
Oct 15, 2015
+9 −5
Merged

Fixes for VirtuaGL. #499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitsubprojects
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
git_subproject(vmmlib https://github.com/Eyescale/vmmlib.git 2bec113)
git_subproject(Lunchbox https://github.com/Eyescale/Lunchbox.git 33546f1)
git_subproject(Pression https://github.com/Eyescale/Pression.git 93883cf)
git_subproject(hwsd https://github.com/Eyescale/hwsd.git 0ecf64a)
git_subproject(hwsd https://github.com/Eyescale/hwsd.git f20749f)
git_subproject(Collage https://github.com/Eyescale/Collage.git 182698e)
git_subproject(GLStats https://github.com/Eyescale/GLStats.git 3c76246)
git_subproject(Deflect https://github.com/BlueBrain/Deflect.git 8556bc3)
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ include(GitExternal)

set(VERSION_MAJOR "1")
set(VERSION_MINOR "10")
set(VERSION_PATCH "0")
set(VERSION_PATCH "1")
set(VERSION_ABI 191)

set(EQUALIZER_INCLUDE_NAME eq)
Expand Down
10 changes: 7 additions & 3 deletions eq/server/config/resources.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,19 +315,23 @@ bool Resources::discover( ServerPtr server, Config* config,
std::stringstream name;
if( info.device == LB_UNDEFINED_UINT32 &&
// VirtualGL display redirects to local GPU (see continue above)
!(info.flags & hwsd::GPUInfo::FLAG_VIRTUALGL) )
!(info.flags & hwsd::GPUInfo::FLAG_VIRTUALGL ))
{
name << "display";
}
else
{
name << "GPU" << ++gpuCounter;
if( info.flags & hwsd::GPUInfo::FLAG_VIRTUALGL &&
if( // When running under VirtualGL, GPUs that are not VNC virtual
// devices mustn't be interposed.
( info.flags & ( hwsd::GPUInfo::FLAG_VIRTUALGL |
hwsd::GPUInfo::FLAG_VNC )) ==
hwsd::GPUInfo::FLAG_VIRTUALGL &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! ;)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

info.device != LB_UNDEFINED_UINT32 )
{
std::ostringstream displayString;
displayString << ":" << info.port << "." << info.device;
excludedDisplays += displayString.str() + ";";
excludedDisplays += displayString.str() + ",";
}
}

Expand Down