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.

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

Make sure to unset current context in wgl Surface::configure/present #5087

Merged
merged 2 commits into from
Jan 21, 2024

Conversation

Imberflur
Copy link
Contributor

@Imberflur Imberflur commented Jan 18, 2024

Connections
None known

Description
When re-building pipelines in background threads on the wgl backend we encounted this panic:

called `Result::unwrap()` on an `Err` value: Os { code: 170, kind: ResourceBusy, message: "The requested resource is in use." }

which is produced by the unwrap when making a context current fails here

inner.context.make_current(inner.device.dc).unwrap();

I tracked this down to Surface::configure and Surface::present not unsetting the current context after setting it even though they release the associated lock used for synchronizing this.

To fix this I took advantage of the existing guard type and added a new locking + make current method (called lock_with_dc that allows passing in the DC that Surface::configure/Surface::present were previously passing directly to make_current.

Note that with this guard type, if unmake_current fails it will panic. I wasn't completely sure if this was appropriate for Surface::configure/present since they return errors instead of panicking when make_current fails. However, if unmake_current fails it seems like later calls to make_current (if called on a different thread) would also fail causing AdapterContext::lock to panic anyway? I'm not really familiar with what would cause unmake_current to fail though and what the behavior would be in terms of whether make_current can succeed later after such a failure.

Note, another small behavior change is that this uses the same .try_lock_for(Duration::from_secs(CONTEXT_LOCK_TIMEOUT_SECS)) pattern instead of just a .lock().

Testing
To reproduce this I spawned an extra thread in the examples framework.rs that called Adapter::get_texture_format_features in a loop. This consistently produced panics when trying to make the gl context current to that thread. After this change, no panics were observed with this testing setup. Additionally, the original issue when re-building pipelines seems to be resolved.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@Imberflur Imberflur requested a review from a team as a code owner January 18, 2024 06:07
…esent and Surface::configure of wgl backend. So that we don't fail to set the context on other threads.
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Very nice!

wgpu-hal/src/gles/egl.rs Show resolved Hide resolved
@cwfitzgerald cwfitzgerald merged commit 6c7c6fb into gfx-rs:trunk Jan 21, 2024
27 checks passed
@cwfitzgerald cwfitzgerald added the PR: needs back-porting PR with a fix that needs to land on crates label Jan 21, 2024
@Imberflur Imberflur deleted the wgl-fix-current branch January 21, 2024 15:24
@cwfitzgerald cwfitzgerald removed the PR: needs back-porting PR with a fix that needs to land on crates label Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants