Merge #1607

1607: Fix Downlevel Vertex Stage Storage Buffer Check r=kvark a=cwfitzgerald

**Connections**

Fixes a bug in #1599. Also follows up on #1583 marking all of hello-compute as a failure.

**Testing**

Tests now pass on rpi4.


Co-authored-by: Connor Fitzgerald <[email protected]>

Author: bors[bot]

wgpu-core/src/device/mod.rs

@@ -976,22 +976,33 @@ impl<A: HalApi> Device<A> {
         label: Option<&str>,
         entry_map: binding_model::BindEntryMap,
     ) -> Result<binding_model::BindGroupLayout<A>, binding_model::CreateBindGroupLayoutError> {
+        #[derive(PartialEq)]
+        enum WritableStorage {
+            Yes,
+            No,
+        }
+
         for entry in entry_map.values() {
             use wgt::BindingType as Bt;
 
             let mut required_features = wgt::Features::empty();
-            let mut required_downlevel_flags = wgt::DownlevelFlags::empty();
-            let (array_feature, is_writable_storage) = match entry.ty {
+            let (array_feature, writable_storage) = match entry.ty {
                 Bt::Buffer {
                     ty: wgt::BufferBindingType::Uniform,
                     has_dynamic_offset: false,
                     min_binding_size: _,
-                } => (Some(wgt::Features::BUFFER_BINDING_ARRAY), false),
+                } => (
+                    Some(wgt::Features::BUFFER_BINDING_ARRAY),
+                    WritableStorage::No,
+                ),
                 Bt::Buffer {
                     ty: wgt::BufferBindingType::Uniform,
                     has_dynamic_offset: true,
                     min_binding_size: _,
-                } => (Some(wgt::Features::BUFFER_BINDING_ARRAY), false),
+                } => (
+                    Some(wgt::Features::BUFFER_BINDING_ARRAY),
+                    WritableStorage::No,
+                ),
                 Bt::Buffer {
                     ty: wgt::BufferBindingType::Storage { read_only },
                     ..
@@ -1000,22 +1011,28 @@ impl<A: HalApi> Device<A> {
                         wgt::Features::BUFFER_BINDING_ARRAY
                             | wgt::Features::STORAGE_RESOURCE_BINDING_ARRAY,
                     ),
-                    !read_only,
+                    match read_only {
+                        true => WritableStorage::No,
+                        false => WritableStorage::Yes,
+                    },
+                ),
+                Bt::Sampler { .. } => (None, WritableStorage::No),
+                Bt::Texture { .. } => (
+                    Some(wgt::Features::TEXTURE_BINDING_ARRAY),
+                    WritableStorage::No,
                 ),
-                Bt::Sampler { .. } => (None, false),
-                Bt::Texture { .. } => (Some(wgt::Features::TEXTURE_BINDING_ARRAY), false),
                 Bt::StorageTexture { access, .. } => (
                     Some(
                         wgt::Features::TEXTURE_BINDING_ARRAY
                             | wgt::Features::STORAGE_RESOURCE_BINDING_ARRAY,
                     ),
                     match access {
-                        wgt::StorageTextureAccess::ReadOnly => false,
-                        wgt::StorageTextureAccess::WriteOnly => true,
+                        wgt::StorageTextureAccess::ReadOnly => WritableStorage::No,
+                        wgt::StorageTextureAccess::WriteOnly => WritableStorage::Yes,
                         wgt::StorageTextureAccess::ReadWrite => {
                             required_features |=
                                 wgt::Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES;
-                            true
+                            WritableStorage::Yes
                         }
                     },
                 ),
@@ -1030,13 +1047,14 @@ impl<A: HalApi> Device<A> {
                         error,
                     })?;
             }
-            if entry.visibility.contains(wgt::ShaderStages::VERTEX) {
-                required_downlevel_flags |= wgt::DownlevelFlags::VERTEX_ACCESSABLE_STORAGE_BUFFERS;
-            }
-            if is_writable_storage && entry.visibility.contains(wgt::ShaderStages::VERTEX) {
+            if writable_storage == WritableStorage::Yes
+                && entry.visibility.contains(wgt::ShaderStages::VERTEX)
+            {
                 required_features |= wgt::Features::VERTEX_WRITABLE_STORAGE;
             }
-            if is_writable_storage && entry.visibility.contains(wgt::ShaderStages::FRAGMENT) {
+            if writable_storage == WritableStorage::Yes
+                && entry.visibility.contains(wgt::ShaderStages::FRAGMENT)
+            {
                 self.require_downlevel_flags(wgt::DownlevelFlags::FRAGMENT_WRITABLE_STORAGE)
                     .map_err(binding_model::BindGroupLayoutEntryError::MissingDownlevelFlags)
                     .map_err(|error| binding_model::CreateBindGroupLayoutError::Entry {
@@ -1051,13 +1069,6 @@ impl<A: HalApi> Device<A> {
                     binding: entry.binding,
                     error,
                 })?;
-
-            self.require_downlevel_flags(required_downlevel_flags)
-                .map_err(binding_model::BindGroupLayoutEntryError::MissingDownlevelFlags)
-                .map_err(|error| binding_model::CreateBindGroupLayoutError::Entry {
-                    binding: entry.binding,
-                    error,
-                })?;
         }
 
         let mut hal_bindings = entry_map.values().cloned().collect::<Vec<_>>();

wgpu-hal/src/gles/adapter.rs

@@ -180,17 +180,39 @@ impl super::Adapter {
             naga::back::glsl::Version::Embedded(value)
         };
 
+        let vertex_shader_storage_blocks =
+            gl.get_parameter_i32(glow::MAX_VERTEX_SHADER_STORAGE_BLOCKS) as u32;
+        let fragment_shader_storage_blocks =
+            gl.get_parameter_i32(glow::MAX_FRAGMENT_SHADER_STORAGE_BLOCKS) as u32;
+
+        let vertex_shader_storage_textures =
+            gl.get_parameter_i32(glow::MAX_VERTEX_IMAGE_UNIFORMS) as u32;
+        let fragment_shader_storage_textures =
+            gl.get_parameter_i32(glow::MAX_FRAGMENT_IMAGE_UNIFORMS) as u32;
+
+        // WORKAROUND:
+        // In order to work around an issue with GL on RPI4 and similar, we ignore a zero vertex ssbo count if there are vertex sstos. (more info: https://github.com/gfx-rs/wgpu/pull/1607#issuecomment-874938961)
+        // The hardware does not want us to write to these SSBOs, but GLES cannot express that. We detect this case and disable writing to SSBOs.
+        let vertex_ssbo_false_zero =
+            vertex_shader_storage_blocks == 0 && vertex_shader_storage_textures != 0;
+
+        let max_storage_buffers_per_shader_stage = if vertex_ssbo_false_zero {
+            // We only care about fragment here as the 0 is a lie.
+            log::warn!("Max vertex shader SSBO == 0 and SSTO != 0. Interpreting as false zero.");
+            fragment_shader_storage_blocks
+        } else {
+            vertex_shader_storage_blocks.min(fragment_shader_storage_blocks)
+        };
+
         let mut features = wgt::Features::empty() | wgt::Features::TEXTURE_COMPRESSION_ETC2;
         features.set(
             wgt::Features::DEPTH_CLAMPING,
             extensions.contains("GL_EXT_depth_clamp"),
         );
-        features.set(wgt::Features::VERTEX_WRITABLE_STORAGE, ver >= (3, 1));
-
-        let vertex_shader_storage_blocks =
-            gl.get_parameter_i32(glow::MAX_VERTEX_SHADER_STORAGE_BLOCKS);
-        let fragment_shader_storage_blocks =
-            gl.get_parameter_i32(glow::MAX_FRAGMENT_SHADER_STORAGE_BLOCKS);
+        features.set(
+            wgt::Features::VERTEX_WRITABLE_STORAGE,
+            ver >= (3, 1) && !vertex_ssbo_false_zero,
+        );
 
         let mut downlevel_flags = wgt::DownlevelFlags::empty()
             | wgt::DownlevelFlags::DEVICE_LOCAL_IMAGE_COPIES
@@ -210,10 +232,6 @@ impl super::Adapter {
             wgt::DownlevelFlags::INDEPENDENT_BLENDING,
             ver >= (3, 2) || extensions.contains("GL_EXT_draw_buffers_indexed"),
         );
-        downlevel_flags.set(
-            wgt::DownlevelFlags::VERTEX_ACCESSABLE_STORAGE_BUFFERS,
-            vertex_shader_storage_blocks > 0,
-        );
 
         let max_texture_size = gl.get_parameter_i32(glow::MAX_TEXTURE_SIZE) as u32;
         let max_texture_3d_size = gl.get_parameter_i32(glow::MAX_3D_TEXTURE_SIZE) as u32;
@@ -228,14 +246,6 @@ impl super::Adapter {
         let max_uniform_buffers_per_shader_stage =
             gl.get_parameter_i32(glow::MAX_VERTEX_UNIFORM_BLOCKS)
                 .min(gl.get_parameter_i32(glow::MAX_FRAGMENT_UNIFORM_BLOCKS)) as u32;
-        let max_storage_buffers_per_shader_stage = if vertex_shader_storage_blocks > 0 {
-            vertex_shader_storage_blocks.min(fragment_shader_storage_blocks)
-        } else {
-            fragment_shader_storage_blocks
-        } as u32;
-
-        let max_storage_textures_per_shader_stage =
-            gl.get_parameter_i32(glow::MAX_FRAGMENT_IMAGE_UNIFORMS) as u32;
 
         let limits = wgt::Limits {
             max_texture_dimension_1d: max_texture_size,
@@ -248,7 +258,8 @@ impl super::Adapter {
             max_sampled_textures_per_shader_stage: super::MAX_TEXTURE_SLOTS as u32,
             max_samplers_per_shader_stage: super::MAX_SAMPLERS as u32,
             max_storage_buffers_per_shader_stage,
-            max_storage_textures_per_shader_stage,
+            max_storage_textures_per_shader_stage: vertex_shader_storage_textures
+                .min(fragment_shader_storage_textures),
             max_uniform_buffers_per_shader_stage,
             max_uniform_buffer_binding_size: gl.get_parameter_i32(glow::MAX_UNIFORM_BLOCK_SIZE)
                 as u32,

wgpu-types/src/lib.rs

@@ -702,8 +702,6 @@ bitflags::bitflags! {
         const COMPARISON_SAMPLERS = 0x0000_0100;
         /// Supports different blending modes per color target.
         const INDEPENDENT_BLENDING = 0x0000_0200;
-        /// Supports attaching storage buffers to vertex shaders.
-        const VERTEX_ACCESSABLE_STORAGE_BUFFERS = 0x0000_0400;
 
 
         /// Supports samplers with anisotropic filtering. Note this isn't actually required by WebGPU,

wgpu/examples/hello-compute/tests.rs

@@ -9,7 +9,7 @@ use common::{initialize_test, TestParameters};
 #[test]
 fn test_compute_1() {
     initialize_test(
-        TestParameters::default().specific_failure(None, None, Some("V3D"), false),
+        TestParameters::default().specific_failure(None, None, Some("V3D"), true),
         |ctx| {
             let input = &[1, 2, 3, 4];
 
@@ -26,7 +26,7 @@ fn test_compute_1() {
 #[test]
 fn test_compute_2() {
     initialize_test(
-        TestParameters::default().specific_failure(None, None, Some("V3D"), false),
+        TestParameters::default().specific_failure(None, None, Some("V3D"), true),
         |ctx| {
             let input = &[5, 23, 10, 9];
 
@@ -43,7 +43,7 @@ fn test_compute_2() {
 #[test]
 fn test_compute_overflow() {
     initialize_test(
-        TestParameters::default().specific_failure(None, None, Some("V3D"), false),
+        TestParameters::default().specific_failure(None, None, Some("V3D"), true),
         |ctx| {
             let input = &[77031, 837799, 8400511, 63728127];
             pollster::block_on(assert_execute_gpu(
@@ -61,7 +61,7 @@ fn test_multithreaded_compute() {
     initialize_test(
         TestParameters::default()
             .backend_failure(wgpu::Backends::GL)
-            .specific_failure(Some(wgpu::Backends::VULKAN), None, Some("V3D"), false),
+            .specific_failure(None, None, Some("V3D"), true),
         |ctx| {
             use std::{sync::mpsc, thread, time::Duration};
 

wgpu/examples/shadow/main.rs

@@ -542,7 +542,7 @@ impl framework::Example for Example {
                         },
                         wgpu::BindGroupLayoutEntry {
                             binding: 1, // lights
-                            visibility: wgpu::ShaderStages::VERTEX | wgpu::ShaderStages::FRAGMENT,
+                            visibility: wgpu::ShaderStages::FRAGMENT,
                             ty: wgpu::BindingType::Buffer {
                                 ty: wgpu::BufferBindingType::Storage { read_only: true },
                                 has_dynamic_offset: false,

wgpu/tests/common/mod.rs

@@ -75,7 +75,7 @@ pub struct FailureCase {
     backends: Option<wgpu::Backends>,
     vendor: Option<usize>,
     adapter: Option<String>,
-    segfault: bool,
+    skip: bool,
 }
 
 // This information determines if a test should run.
@@ -137,7 +137,7 @@ impl TestParameters {
             backends: None,
             vendor: None,
             adapter: None,
-            segfault: false,
+            skip: false,
         });
         self
     }
@@ -148,7 +148,7 @@ impl TestParameters {
             backends: Some(backends),
             vendor: None,
             adapter: None,
-            segfault: false,
+            skip: false,
         });
         self
     }
@@ -165,13 +165,13 @@ impl TestParameters {
         backends: Option<Backends>,
         vendor: Option<usize>,
         device: Option<&'static str>,
-        segfault: bool,
+        skip: bool,
     ) -> Self {
         self.failures.push(FailureCase {
             backends,
             vendor,
             adapter: device.as_ref().map(AsRef::as_ref).map(str::to_lowercase),
-            segfault,
+            skip,
         });
         self
     }
@@ -257,7 +257,7 @@ pub fn initialize_test(parameters: TestParameters, test_function: impl FnOnce(Te
             && expect_failure_adapter.unwrap_or(true)
         {
             if always {
-                Some((FailureReasons::ALWAYS, failure.segfault))
+                Some((FailureReasons::ALWAYS, failure.skip))
             } else {
                 let mut reason = FailureReasons::empty();
                 reason.set(
@@ -272,18 +272,15 @@ pub fn initialize_test(parameters: TestParameters, test_function: impl FnOnce(Te
                     FailureReasons::ADAPTER,
                     expect_failure_adapter.unwrap_or(false),
                 );
-                Some((reason, failure.segfault))
+                Some((reason, failure.skip))
             }
         } else {
             None
         }
     });
 
     if let Some((reason, true)) = failure_reason {
-        println!(
-            "EXPECTED TEST FAILURE SKIPPED DUE TO SEGFAULT: {:?}",
-            reason
-        );
+        println!("EXPECTED TEST FAILURE SKIPPED: {:?}", reason);
         return;
     }
 
@@ -297,7 +294,7 @@ pub fn initialize_test(parameters: TestParameters, test_function: impl FnOnce(Te
             // Print out reason for the failure
             println!("GOT EXPECTED TEST FAILURE: {:?}", reason);
         }
-    } else if let Some(reason) = failure_reason {
+    } else if let Some((reason, _)) = failure_reason {
         // We expected to fail, but things passed
         panic!("UNEXPECTED TEST PASS: {:?}", reason);
     } else {