Handle Multi-threaded EGL Context Access

[zicklag]

Connections
#1630, bevyengine/bevy#841

Description
Implements the synchronization necessary to use the GL backend from multiple threads. Accomplishes this by using a mutex around the GL context with extra wrapping to bind and unbind the EGL context when locking and unlocking.

Testing
Tested on Ubunty 20.04 with a fork of the Bevy game engine and the WGPU examples ( not that the examples test the multi-threading ).

Remaining Issues

There is only one Bevy example I cannot get to run yet and it's the load_gltf example. It fails with a shader translation error:

Jul 26 20:36:50.949 ERROR naga::back::glsl: Conflicting samplers for _group_3_binding_10    
Jul 26 20:36:50.950  WARN wgpu::backend::direct: Shader translation error for stage FRAGMENT: A image was used with multiple samplers    
Jul 26 20:36:50.950  WARN wgpu::backend::direct: Please report it to https://github.com/gfx-rs/naga    
Jul 26 20:36:50.950 ERROR wgpu::backend::direct: wgpu error: Validation Error

Caused by:
    In Device::create_render_pipeline
    Internal error in FRAGMENT shader: A image was used with multiple samplers

Interestingly, I think the shader in question doesn't have a group(3), binding(10) anywhere that I know of so I'm going to have to drill down a bit more and find out exactly which shader translation is failing more.

This could potentially be fixed in a separate PR. I think the rest of this PR is rather straight-forward and the fix for the error above is probably mostly unrelated to the primary changes made in this PR.

[cwfitzgerald]

I mean, I hate it, but seems mostly reasonable. I want to test this on stuff like rpi and android to see if it works.

[zicklag]

I mean, I hate it, but seems mostly reasonable.

Yeah, according to the EGL spec we can't access the context in multiple threads at a time so essentially we're stuck. 🤷‍♂️

---

Oh, something I forgot. I don't know how to test it, but according to the EGL spec I think the dummy pbuffer that we use as a workaround when surfaceless rendering isn't available won't work in a multi-threaded environment. You're not allowed to have multiple threads that bind the same current pbuffer I don't think, and that's what would happen if we didn't have surfaceless support and you tried to access the context from multiple threads.

We may need to try to detect that and maybe add a downlevel flag or an error message or something.

---

Edit: We may also want to create a multi-threaded example sometime. Currently my only test-case for this is a fork of Bevy updated for WGPU on master.

[cwfitzgerald]

We already do have a multithreaded example, it's cargo test --example hello-compute test_multithreaded_compute. Additionally, the tests are multi-threaded unless --test-threads=1 is passed.

Would having thread-local pbuffers work?

I tested this PR on my intel/haswell machine using the GL tests, and got tons of errors:

[2021-07-27T06:36:21Z ERROR wgpu_hal::gles::egl] EGL 'eglMakeCurrent' code 0x3009: Got an EGLSurface but no EGLContext

[jakobhellermann]

The shader translation error is probably due to gfx-rs/naga#1137.

[kvark]

I like this approach quite a bit.
Previously in gfx, we had a wrapper around GL content that would just make it current without any locks. It was quite painful.
Here, we are locking the context and guaranteeing safe access.

[zicklag]

Would having thread-local pbuffers work?

I think so.

I tested this PR on my intel/haswell machine using the GL tests, and got tons of errors:

Does that machine support surfaceless EGL? If it doesn't, then it's probably the pbfuffer problem. I'll try out thread local pbuffers and then you could test it to see if that fixes it.

cargo test --example hello-compute test_multithreaded_compute

That one fails on master with the expected "DeviceLost" that you get when creating a buffer on a different thread. It still fails with this PR, but with a different message. Not sure why:

---- tests::test_multithreaded_compute stdout ----
thread 'tests::test_multithreaded_compute' panicked at 'UNEXPECTED TEST PASS: BACKEND', wgpu/examples/hello-compute/../../tests/common/mod.rs:299:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[cwfitzgerald]

Does that machine support surfaceless EGL?

It definitely should considering it's running on mesa. Is there a particular extension I should be looking out for?

That one fails on master with the expected "DeviceLost" that you get when creating a buffer on a different thread. It still fails with this PR, but with a different message. Not sure why:

We had a test that used to fail on vulkan but now succeeds but we never marked the test to pass. #1731 fixed it.

[zicklag]

Is there a particular extension I should be looking out for?

Yeah, in the info log you should see EGL context: +surfaceless:

[zicklag]

We had a test that used to fail on vulkan but now succeeds but we never marked the test to pass. #1731 fixed it.

For some reason the test still fails after rebasing.

$ WGPU_BACKEND=gl cargo test --example hello-compute test_multithreaded_compute
running 1 test
test tests::test_multithreaded_compute ... FAILED

failures:

---- tests::test_multithreaded_compute stdout ----
thread 'tests::test_multithreaded_compute' panicked at 'UNEXPECTED TEST PASS: BACKEND', wgpu/examples/hello-compute/../../tests/common/mod.rs:299:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::test_multithreaded_compute

[cwfitzgerald]

Oh I misread what was causing it. This is because it now passes on your machine but is still marked to fail on OpenGL. You should got to the test definition wgpu/example/hello-compute/tests.rs and un-mark it as failing.

EGL context: +surfaceless

Yeah this doesn't support that, it only supports EGL 1.4. In fact all my GL devices are only EGL 1.4. That being said, it does support EGL_KHR_surfaceless_context, so we might need to use that.

[zicklag]

Oh I misread what was causing it. This is because it now passes on your machine but is still marked to fail on OpenGL. You should got to the test definition wgpu/example/hello-compute/tests.rs and un-mark it as failing.

Ah, I get it now. 👍

Yeah this doesn't support that, it only supports EGL 1.4. In fact all my GL devices are only EGL 1.4. That being said, it does support EGL_KHR_surfaceless_context, so we might need to use that.

Well if it supports the extension then we shouldn't have a problem. Are you not seeing the +surfaceless in the logs? Maybe there's something wrong with our check for the extension?

wgpu/wgpu-hal/src/gles/egl.rs

Lines 331 to 343 in 9ce884c

	let pbuffer =
	if version < (1, 5) || !display_extensions.contains("EGL_KHR_surfaceless_context") {
	let attributes = [egl::WIDTH, 1, egl::HEIGHT, 1, egl::NONE];
	egl.create_pbuffer_surface(display, config, &attributes)
	.map(Some)
	.map_err(|e| {
	log::warn!("Error in create_pbuffer_surface: {:?}", e);
	crate::InstanceError
	})?
	} else {
	log::info!("\tEGL context: +surfaceless");
	None
	};

---

The shader translation error is probably due to gfx-rs/naga#1137.

That fixed it. Now all the Bevy examples work!

[cwfitzgerald]

Yeah the check is wrong. that should be && not ||.

[zicklag]

Oh, duh. 🤦‍♂️ Well, it looks like we found a way to test the pbuffer workaround. :)

I'll fix that check and push an update, then it should work for you on your EGL 1.4 devices.

Then I'll also experiment to see if I can get the pbuffer workaround working by adding || true to that check so I can pretend I don't have the required version/extensions on my machine.

[zicklag]

OK I fixed the surfaceless check and I also fixed the error you were getting earlier with "Got an EGLSurface but no EGLContext". When I made the context not current I was not supposed to bind the pbuffer as the current surface.

Also the pbuffer workaround works totally fine with multi-threading so we won't need thread-local pbuffers or anything. I was misinterpreting when we would need to bind the pbuffer. Since we only bind the pbuffer when we bind the context, everything is fine and it is only ever bound by one thread.

If this works on your hardware @cwfitzgerald, I think this should be ready ( pending re-review ).

[zicklag]

Pushed an update!

[kvark]

just pending on @cwfitzgerald

[cwfitzgerald]

Alright, so all tests pass as long as I pass in --test-threads=1. I think the problem may be stemming from having multiple adapters open at once. I need to test my raspberry pi, but I think that is an issue we can deal with in a follow up PR.

[zicklag]

Alright, so all tests pass as long as I pass in --test-threads=1.

Interesting. All the tests pass for me, even without the --test-threads=1. I wonder what the difference is.

I need to test my raspberry pi, but I think that is an issue we can deal with in a follow up PR.

👍

[kvark]

uh, isn't this a bit worrying? The whole point of the PR is to support more than one thread.
Would you have energy to investigate? My time is getting low now since the move is nigh...

[cwfitzgerald]

The whole point of the PR is to support more than one thread.

It does, the multi-threaded example works perfectly fine, it's juggling multiple adapters which is the issue (which is what the test harness is doing without --test-threads=1). So I'm comfortable trying to deal with this in a follow up.

Would you have energy to investigate?

Yeah ofc!

[kvark]

ok, sounds reasonable
bors r=cwfitzgerald,kvark

[bors]

Build succeeded:

• Android aarch64
• Format
• IOS aarch64
• Linux x86_64
• MacOS x86_64
• WebAssembly
• Windows x86_64

[zicklag]

Awesome! Thanks guys! I'm super excited to actually have gotten Bevy running on OpenGL. 😃

[cwfitzgerald]

Thank you for all the help getting it working!