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

Jump to bottom

Make Intel Mesa Bug Workaround More Specific #1653

Closed
wants to merge 1 commit into from
Closed

Make Intel Mesa Bug Workaround More Specific #1653

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
30 changes: 23 additions & 7 deletions wgpu-hal/src/gles/egl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,11 +396,6 @@ impl crate::Instance<super::Api> for Instance {
None
};

// Workaround Mesa driver bug on Intel cards by disabling fastclear:
// https://gitlab.freedesktop.org/mesa/mesa/-/issues/2565
// https://github.com/gfx-rs/wgpu/issues/1627#issuecomment-877854185
std::env::set_var("INTEL_DEBUG", "nofc");

let display = if let (Some(library), Some(egl)) =
(wayland_library, egl.upcast::<egl::EGL1_5>())
{
Expand Down Expand Up @@ -601,6 +596,7 @@ impl crate::Instance<super::Api> for Instance {
pbuffer: inner.pbuffer,
wl_window,
swapchain: None,
enable_intel_mesa_fastclear_workaround: None,
})
}
unsafe fn destroy_surface(&self, surface: Surface) {
Expand Down Expand Up @@ -677,6 +673,9 @@ pub struct Surface {
pub(super) presentable: bool,
wl_window: Option<*mut raw::c_void>,
swapchain: Option<Swapchain>,
// If None, the check has not yet been done, if Some(bool) indicates whether or not we
// workaround this bug: https://gitlab.freedesktop.org/mesa/mesa/-/issues/2565
enable_intel_mesa_fastclear_workaround: Option<bool>,
}

unsafe impl Send for Surface {}
Expand Down Expand Up @@ -757,13 +756,30 @@ impl crate::Surface<super::Api> for Surface {
);
}

let format_desc = device.shared.describe_texture_format(config.format);
let gl = &device.shared.context;

// Check renderer version to see if we need to workaround intel mesa bug
if self.enable_intel_mesa_fastclear_workaround.is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

this needs to happen in the expose function, where the renderer string is analyzed. And the result flag should go into PrivateCapabilities ideally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this needs to happen in the expose function,

That's what I wanted to do initially, but a surface can be created without having enumerated adapters, so I didn't know the best way to go about getting to the capability information exposed by the adapter inside the surface, when the adapter could have been exposed at any time after the surface was created.

Any ideas for how to do that?

Copy link
Member

Choose a reason for hiding this comment

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

Would this still hold when #1656 is merged?

let renderer = gl.get_parameter_string(glow::RENDERER);
Copy link
Member

Choose a reason for hiding this comment

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

For string comparisons, we should always make them lower case first.


// Check renderer string for buggy cards
self.enable_intel_mesa_fastclear_workaround = Some(
renderer.contains("Mesa")
&& renderer.contains("Intel")
&& renderer.contains("CML GT2"),
Copy link
Member

Choose a reason for hiding this comment

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

This is going to be way to restrictive. This limits it to Comet Lake and only the GT2 model of that (which is the mid-range gpu in the lineup). https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/4972/diffs?diff_id=75888#22f5d1004713c9bbf857988c7efb81631ab88f99_323_327 comment seems to suggest the issue applies to all skylake based machines which matches with the issue not showing up on my haswell machine.

Here are the PCI ids for all intel cards: https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/include/pci_ids/i965_pci_ids.h.

What we need to do is match on all idents which are "**L". You could write something like:

renderer.split().any(|substr| substr.len() == 3 && substr[2] == 'L') as the model check. You should also point back to this documentation to show why we're making these decisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, great, I was wondering what exactly those model numbers meant. :)

);
}

let format_desc = device.shared.describe_texture_format(config.format);
let renderbuffer = gl.create_renderbuffer().unwrap();
gl.bind_renderbuffer(glow::RENDERBUFFER, Some(renderbuffer));
gl.renderbuffer_storage(
glow::RENDERBUFFER,
format_desc.internal,
if self.enable_intel_mesa_fastclear_workaround.unwrap() {
glow::RGBA8
Copy link
Member

Choose a reason for hiding this comment

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

What end result does this have? Why can we just use a different format?

Copy link
Member

Choose a reason for hiding this comment

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

Does this actually start making the tests pass, considering this is just for the surface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What end result does this have? Why can we just use a different format?

It fixes he pixelated edges on my machine. I don't know why, though. It may be related to how in the mesa issue the original poster said that if he commented out glfwWindowHint(GLFW_SRGB_CAPABLE, 1); it would render fine. It might be an issue specifically with the sRGB formats.

Beats me. 🤷‍♂️

Does this actually start making the tests pass, considering this is just for the surface?

That's a good point. It doesn't fix the tests.

I don't know how it's fixing it for the window display, but if it isn't fixing it for the reftests then this probably isn't going to work.

} else {
format_desc.internal
},
config.extent.width as _,
config.extent.height as _,
);
Expand Down