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

Work Around Fastclear Bug for Web and Native GL #1717

Merged
merged 1 commit into from
Jul 26, 2021
Merged

Work Around Fastclear Bug for Web and Native GL #1717

merged 1 commit into from
Jul 26, 2021

Conversation

zicklag
Copy link
Contributor

@zicklag zicklag commented Jul 23, 2021

Connections
Resolves #1627

Description
Works around Mesa fastclear bug by doing a manual shader clear on effected platforms

Testing
Tested on Mesa Intel(R) UHD Graphics (CML GT2) (Gl)

@zicklag zicklag mentioned this pull request Jul 23, 2021
Closed
cwfitzgerald
cwfitzgerald requested changes Jul 23, 2021
View reviewed changes
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.

Some comments, but in general looks good

wgpu-hal/src/gles/queue.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/queue.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/device.rs Outdated Show resolved Hide resolved
kvark
kvark requested changes Jul 23, 2021
View reviewed changes
wgpu-hal/src/gles/adapter.rs Show resolved Hide resolved
wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
@kvark kvark mentioned this pull request Jul 23, 2021
Merged
7 tasks
@zicklag zicklag force-pushed the fastclear-fix-3 branch 4 times, most recently from 7fb4f78 to ca705d4 Compare July 23, 2021 17:37
@kvark
Copy link
Member

kvark commented Jul 23, 2021

I think we don't actually need to track the GL state here. Correct me if that's wrong.
We may want to do it anyway at some point later, but right now it's not the time and place to do. We need correctness first, complexity later.

The reason why I don't think it's needed is because the clears are only done at the beginning of a render pass. This is where all the state is reset from WebGPU point of view, so the GL backend doesn't need to worry about restoring any states if it does the draw.

@zicklag zicklag force-pushed the fastclear-fix-3 branch 3 times, most recently from 4ba001c to 3523c45 Compare July 23, 2021 19:04
@zicklag
Copy link
Contributor Author

zicklag commented Jul 23, 2021

OK, I think I addressed all of the review points with the latest push.

cwfitzgerald
cwfitzgerald requested changes Jul 23, 2021
View reviewed changes
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.

A few more minor things, but this looks good.

wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/mod.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/mod.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/mod.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/shader_clear.vert Outdated Show resolved Hide resolved
wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
@zicklag
Copy link
Contributor Author

zicklag commented Jul 23, 2021

There you go! I went with the most specific PrivateCapabilities flag name: WORKAROUND_MESA_I915_SRGB_SHADER_CLEAR. I don't care what we go with final, just let me know if you want to change it.

@zicklag
Copy link
Contributor Author

zicklag commented Jul 23, 2021

Oh, wait! The shadow example is only clearing half the screen for some reason. Got to check that out.

@zicklag
Copy link
Contributor Author

zicklag commented Jul 23, 2021

OK, fixed it, had to set the viewport and scissor before clearing.

cwfitzgerald
cwfitzgerald approved these changes Jul 23, 2021
View reviewed changes
wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
kvark
kvark requested changes Jul 23, 2021
View reviewed changes
wgpu-hal/src/gles/adapter.rs Outdated
};
#[cfg(target_arch = "wasm32")]
let (vendor, renderer) = {
let vendor_const = if extensions.contains("WEBGL_debug_renderer_info") {
Copy link
Member

Choose a reason for hiding this comment

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

can we combine these two? i.e. let (vendor_const, renderer_const) = ...;

Copy link
Member

Choose a reason for hiding this comment

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

I think what he meant was

let (vendor, renderer) = if extensions.contains("WEBGL_debug_renderer_info") {
     (GL_UNMASKED_VENDOR_WEBGL, GL_UNMASKED_RENDERER_WEBGL)
else {
     (glow::VENDOR, glow::RENDERER)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that makes more sense. :) Pushed an update.

wgpu-hal/src/gles/adapter.rs Show resolved Hide resolved
wgpu-hal/src/gles/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/command.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/queue.rs Show resolved Hide resolved
@zicklag
Copy link
Contributor Author

zicklag commented Jul 23, 2021

Pushed another update. 👍

cwfitzgerald
cwfitzgerald requested changes Jul 24, 2021
View reviewed changes
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.

This has had a lot of iterations, thanks for sticking with us! We're getting close.

wgpu-hal/src/gles/mod.rs Outdated
// This comment
// (https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/4972/diffs?diff_id=75888#22f5d1004713c9bbf857988c7efb81631ab88f99_323_327)
// seems to indicate all skylake models are effected.
const WORKAROUND_MESA_I915_SRGB_SHADER_CLEAR = 1 << 0;
Copy link
Member

Choose a reason for hiding this comment

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

Now that this is in a workarounds struct, the individual const shouldn't be prefixed with WORKAROUND

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. Fixed!

@zicklag zicklag force-pushed the fastclear-fix-3 branch 2 times, most recently from d6bc195 to e0762d3 Compare July 24, 2021 20:09
cwfitzgerald
cwfitzgerald approved these changes Jul 24, 2021
View reviewed changes
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.

Looks good from my end. Just want kvark to take one last look. Does this make all the GL tests pass on your machine?

@zicklag
Copy link
Contributor Author

zicklag commented Jul 24, 2021

One of the skybox tests is the only failing one, and that one fails on master so I think it's unrelated.

@cwfitzgerald
Copy link
Member

Alright, sounds fine.

@cwfitzgerald cwfitzgerald requested a review from kvark July 25, 2021 16:50
@zicklag zicklag mentioned this pull request Jul 25, 2021
Closed
5 tasks
@kvark
Copy link
Member

kvark commented Jul 26, 2021

bors r=cwfitzgerald,kvark

@bors
Copy link
Contributor

bors bot commented Jul 26, 2021

Build succeeded:

@bors bors bot merged commit ee9e145 into gfx-rs:master Jul 26, 2021
@zicklag zicklag deleted the fastclear-fix-3 branch July 26, 2021 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwfitzgerald cwfitzgerald cwfitzgerald approved these changes

@kvark kvark kvark approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fastclear Bug With Intel Mesa Adapters on the GL Backend
3 participants
@zicklag @kvark @cwfitzgerald