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

[0.2] First bits of draw call validation, better submission tracking #114

Merged
merged 8 commits into from
Mar 31, 2019

Conversation

kvark
Copy link
Member

@kvark kvark commented Mar 29, 2019

Fixes #113

@kvark kvark requested a review from grovesNL March 29, 2019 03:55
@kvark kvark changed the title [0.2/WIP] Blend color and the first bits of validation [0.2/WIP] First bits of validation Mar 29, 2019
@kvark kvark changed the title [0.2/WIP] First bits of validation [0.2/WIP] First bits of draw call validation Mar 29, 2019
@kvark
Copy link
Member Author

kvark commented Mar 29, 2019

Interestingly, eager binding of descriptor sets is more complicated than I'd hope. We can't bind set N+1 if the currently bound set N is incompatible (in gfx-rs or Vulkan). At the same time WebGPU allows doing so. Therefore, the implementation needs to remember the descriptor sets that are waiting to be bound when the previous ones become compatible (not doing that yet in the PR).

@kvark
Copy link
Member Author

kvark commented Mar 29, 2019

@grovesNL this is ready for review. Now it does fully track bind group compatibility and prevents invalid usage of Vulkan binding rules.
I'm keeping the WIP here since I want to resolve those extra validation errors, like in #113, before doing a patch release.

@kvark kvark changed the title [0.2/WIP] First bits of draw call validation [0.2] First bits of draw call validation, better submission tracking Mar 29, 2019
@rukai
Copy link
Contributor

rukai commented Mar 30, 2019

The triangle example no longer works for me on this PR.
I get:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: IncompatibleBindGroup { index: 0 }', src/libcore/result.rs:997:5
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:39
   1: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:70
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:58
             at src/libstd/panicking.rs:200
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:215
   4: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:478
   5: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:385
   6: rust_begin_unwind
             at src/libstd/panicking.rs:312
   7: core::panicking::panic_fmt
             at src/libcore/panicking.rs:85
   8: core::result::unwrap_failed
   9: wgpu_render_pass_draw
  10: core::ops::function::impls::<impl core::ops::function::FnMut<A> for &mut F>::call_mut
  11: winit::platform::platform::x11::EventsLoop::process_event
  12: hello_triangle::main
  13: std::rt::lang_start::{{closure}}
  14: std::panicking::try::do_call
             at src/libstd/rt.rs:49
             at src/libstd/panicking.rs:297
  15: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:92
  16: std::rt::lang_start_internal
             at src/libstd/panicking.rs:276
             at src/libstd/panic.rs:388
             at src/libstd/rt.rs:48
  17: main
  18: __libc_start_main
  19: _start

@kvark
Copy link
Member Author

kvark commented Mar 30, 2019

@rukai thanks for testing! This was an issue with triangle example code, not wgpu itself. See the last commit for the fix.

@kvark
Copy link
Member Author

kvark commented Mar 30, 2019

@rukai if you want to help reviewing the change, that would be good too ;)

@rukai
Copy link
Contributor

rukai commented Mar 31, 2019

Just tried the new commit.
Compiling with debug I get this, a triangle is rendered and then resizing the window causes it to panic.

ERROR 2019-03-31T03:37:25Z: gfx_backend_vulkan: [Validation]  [ VUID-VkSwapchainCreateInfoKHR-compositeAlpha-01280 ] Object: 0x557834dd2af0 (Type = 3) | vkCreateSwapchainKHR() called with a non-supported pCreateInfo->compositeAlpha (i.e. VK_COMPOSITE_ALPHA_INHERIT_BIT_KHR).  Supported values are:
    VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR
. The Vulkan spec states: compositeAlpha must be one of the bits present in the supportedCompositeAlpha member of the VkSurfaceCapabilitiesKHR structure returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR for the surface (https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-VkSwapchainCreateInfoKHR-compositeAlpha-01280)
VUID-VkSwapchainCreateInfoKHR-compositeAlpha-01280(ERROR / SPEC): msgNum: 0 - vkCreateSwapchainKHR() called with a non-supported pCreateInfo->compositeAlpha (i.e. VK_COMPOSITE_ALPHA_INHERIT_BIT_KHR).  Supported values are:
    VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR
. The Vulkan spec states: compositeAlpha must be one of the bits present in the supportedCompositeAlpha member of the VkSurfaceCapabilitiesKHR structure returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR for the surface (https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-VkSwapchainCreateInfoKHR-compositeAlpha-01280)
    Objects: 1
        [0] 0x557834dd2af0, type: 3, name: NULL

Trying to run it again in debug I can no longer get the triangle to render, it doesnt render anything for a while then panics.

ERROR 2019-03-31T03:39:24Z: gfx_backend_vulkan: [Validation]  [ VUID-VkSwapchainCreateInfoKHR-imageExtent-01274 ] Object: 0x5576505e4af0 (Type = 3) | vkCreateSwapchainKHR() called with imageExtent = (850,1399), which is outside the bounds returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR(): currentExtent = (851,1399), minImageExtent = (851,1399), maxImageExtent = (851,1399). The Vulkan spec states: imageExtent must be between minImageExtent and maxImageExtent, inclusive, where minImageExtent and maxImageExtent are members of the VkSurfaceCapabilitiesKHR structure returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR for the surface (https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-VkSwapchainCreateInfoKHR-imageExtent-01274)
VUID-VkSwapchainCreateInfoKHR-imageExtent-01274(ERROR / SPEC): msgNum: 0 - vkCreateSwapchainKHR() called with imageExtent = (850,1399), which is outside the bounds returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR(): currentExtent = (851,1399), minImageExtent = (851,1399), maxImageExtent = (851,1399). The Vulkan spec states: imageExtent must be between minImageExtent and maxImageExtent, inclusive, where minImageExtent and maxImageExtent are members of the VkSurfaceCapabilitiesKHR structure returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR for the surface (https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-VkSwapchainCreateInfoKHR-imageExtent-01274)
    Objects: 1
        [0] 0x5576505e4af0, type: 3, name: NULL
ERROR 2019-03-31T03:39:24Z: gfx_backend_vulkan: [Validation]  [ VUID-VkSwapchainCreateInfoKHR-compositeAlpha-01280 ] Object: 0x5576505e4af0 (Type = 3) | vkCreateSwapchainKHR() called with a non-supported pCreateInfo->compositeAlpha (i.e. VK_COMPOSITE_ALPHA_INHERIT_BIT_KHR).  Supported values are:
    VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR
. The Vulkan spec states: compositeAlpha must be one of the bits present in the supportedCompositeAlpha member of the VkSurfaceCapabilitiesKHR structure returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR for the surface (https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-VkSwapchainCreateInfoKHR-compositeAlpha-01280)
VUID-VkSwapchainCreateInfoKHR-compositeAlpha-01280(ERROR / SPEC): msgNum: 0 - vkCreateSwapchainKHR() called with a non-supported pCreateInfo->compositeAlpha (i.e. VK_COMPOSITE_ALPHA_INHERIT_BIT_KHR).  Supported values are:
    VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR
. The Vulkan spec states: compositeAlpha must be one of the bits present in the supportedCompositeAlpha member of the VkSurfaceCapabilitiesKHR structure returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR for the surface (https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-VkSwapchainCreateInfoKHR-compositeAlpha-01280)
    Objects: 1
        [0] 0x5576505e4af0, type: 3, name: NULL
ERROR 2019-03-31T03:39:24Z: gfx_backend_vulkan: [Validation]  [ UNASSIGNED-CoreValidation-DrawState-SwapchainTooManyImages ] Object: 0x1925 (Type = 27) | vkAcquireNextImageKHR: Application has already acquired the maximum number of images (0x1)
UNASSIGNED-CoreValidation-DrawState-SwapchainTooManyImages(ERROR / SPEC): msgNum: 0 - vkAcquireNextImageKHR: Application has already acquired the maximum number of images (0x1)
    Objects: 1
        [0] 0x1925, type: 1000001000, name: NULL

Panic:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: OutOfHostMemory', src/libcore/result.rs:997:5

Compiling in release I get the same issue as the second time running in debug.

wgpu-native/src/command/render.rs Outdated Show resolved Hide resolved
@@ -38,21 +45,28 @@ impl BindGroupEntry {
pub fn expect_layout(
&mut self,
bind_group_layout_id: BindGroupLayoutId,
) -> Option<BindGroupId> {
) -> Expectation {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The semantics of Expectation as the return type seem a bit backwards – I'd expect to give an Expectation to expect_layout. The return value here seems more like some kind of expectation result.

}
}

pub(crate) fn provide_entry(
&mut self,
/// Attemt to set the value of the specified bind group index.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Attempt

} else {
index + 1
};
(pipeline_layout_id, TakeSome {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the iterator wrapper necessary here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish I could get that without my own iterator! Open to ideas :)
Basically, we want to go from Iterator<Option<Something>> to Iterator<Something>, stopping at the first None.

pub fn ensure_length(&mut self, length: usize) {
while self.entries.len() < length {
self.entries.push(BindGroupEntry::default());
pub(crate) fn cut_expectations(&mut self, length: usize) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it just reset_expectations?

@@ -109,8 +111,7 @@ pub extern "C" fn wgpu_compute_pass_set_pipeline(
let bing_group_guard = HUB.bind_groups.read();
Copy link
Collaborator

Choose a reason for hiding this comment

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

bind_group_guard

use std::collections::HashMap;
use std::sync::atomic::Ordering;
use std::thread;


Copy link
Collaborator

Choose a reason for hiding this comment

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

I think rustfmt will keep resetting these double lines so it's probably best not to fight it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I want to configure that!

@kvark
Copy link
Member Author

kvark commented Mar 31, 2019 via email

@kvark
Copy link
Member Author

kvark commented Mar 31, 2019

Thanks for the review! Hopefully, everything is addressed now.
bors r=grovesNL

@rukai I double checked and the hello_triangle simply doesn't handle window resizes. Might be subject to winit behavior difference on platforms. Best to check gfx-examples instead.

bors bot added a commit that referenced this pull request Mar 31, 2019
114: [0.2] First bits of draw call validation, better submission tracking r=grovesNL a=kvark

Fixes #113 

Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.com>
@bors
Copy link
Contributor

bors bot commented Mar 31, 2019

Build succeeded

@bors bors bot merged commit 06201ce into gfx-rs:v0.2 Mar 31, 2019
@kvark kvark deleted the validation branch March 31, 2019 19:45
@kvark kvark mentioned this pull request Apr 1, 2019
bors bot added a commit that referenced this pull request Apr 1, 2019
116: Validation bits from 0.2 r=kvark a=kvark

Sibling of #114 

Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.com>
mitchmindtree pushed a commit to mitchmindtree/wgpu that referenced this pull request Feb 23, 2020
114: skybox: log::debug and make rotation slower r=kvark a=m4b



Co-authored-by: m4b <m4b.github.io@gmail.com>
kvark pushed a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
114: skybox: log::debug and make rotation slower r=kvark a=m4b



Co-authored-by: m4b <m4b.github.io@gmail.com>
Patryk27 pushed a commit to Patryk27/wgpu that referenced this pull request Nov 23, 2022
* [glsl-new] handle gl_Position builtin

- Also fix width of vec4 type

* [glsl-new] add shader stage as arg to program ctor
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.

3 participants