diff --git a/CHANGELOG.md b/CHANGELOG.md index 25771ea3c9..69a13ec534 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,8 +28,9 @@ Top level categories: Bottom level categories: -- naga - General +- naga +- Validation - DX12 - Vulkan - Metal @@ -50,14 +51,20 @@ Bottom level categories: #### General - BREAKING: Migrated from the `maxInterStageShaderComponents` limit to `maxInterStageShaderVariables`, which changes validation in a way that should not affect most programs. This follows the latest changes of the WebGPU spec. By @ErichDonGubler in [#8652](https://github.com/gfx-rs/wgpu/pull/8652), [#8792](https://github.com/gfx-rs/wgpu/pull/8792). -- Fixed validation of the texture format in GPUDepthStencilState when neither depth nor stencil is actually enabled. By @andyleiserson in [#8766](https://github.com/gfx-rs/wgpu/pull/8766). - Tracing support has been restored. By @andyleiserson in [#8429](https://github.com/gfx-rs/wgpu/pull/8429). +- Allow using a shader that defines I/O for dual-source blending in a pipeline that does not make use of it. By @andyleiserson in [#8856](https://github.com/gfx-rs/wgpu/pull/8856). #### naga - The validator checks that override-sized arrays have a positive size, if overrides have been resolved. By @andyleiserson in [#8822](https://github.com/gfx-rs/wgpu/pull/8822). - Fix some cases where f16 constants were not working. By @andyleiserson in [#8816](https://github.com/gfx-rs/wgpu/pull/8816). +#### Validation + +- Fixed validation of the texture format in GPUDepthStencilState when neither depth nor stencil is actually enabled. By @andyleiserson in [#8766](https://github.com/gfx-rs/wgpu/pull/8766). +- Check that depth bias is not used with non-triangle topologies. By @andyleiserson in [#8856](https://github.com/gfx-rs/wgpu/pull/8856). +- Check that if the shader outputs `frag_depth`, then the pipeline must have a depth attachment. By @andyleiserson in [#8856](https://github.com/gfx-rs/wgpu/pull/8856). + #### GLES - `DisplayHandle` should now be passed to `InstanceDescriptor` for correct EGL initialization on Wayland. By @MarijnS95 in [#8012](https://github.com/gfx-rs/wgpu/pull/8012) diff --git a/cts_runner/test.lst b/cts_runner/test.lst index 5f751b9b4b..4a2d22abd2 100644 --- a/cts_runner/test.lst +++ b/cts_runner/test.lst @@ -163,8 +163,11 @@ webgpu:api,validation,render_pass,render_pass_descriptor:attachments,* webgpu:api,validation,render_pass,render_pass_descriptor:color_attachments,* webgpu:api,validation,render_pass,render_pass_descriptor:resolveTarget,* webgpu:api,validation,render_pass,resolve:resolve_attachment:* +webgpu:api,validation,render_pipeline,depth_stencil_state:depth_bias:* +webgpu:api,validation,render_pipeline,depth_stencil_state:depth_write,frag_depth:* webgpu:api,validation,render_pipeline,depth_stencil_state:format:* webgpu:api,validation,render_pipeline,depth_stencil_state:stencil_write:* +webgpu:api,validation,render_pipeline,fragment_state:dual_source_blending,use_blend_src:* webgpu:api,validation,render_pipeline,inter_stage:max_shader_variable_location:isAsync=false;* //FAIL: webgpu:api,validation,render_pipeline,inter_stage:max_shader_variable_location:isAsync=true;* // https://github.com/gfx-rs/wgpu/pull/8712 diff --git a/tests/tests/wgpu-gpu/dual_source_blending.rs b/tests/tests/wgpu-gpu/dual_source_blending.rs index bbb6c1ff3d..cfa5897772 100644 --- a/tests/tests/wgpu-gpu/dual_source_blending.rs +++ b/tests/tests/wgpu-gpu/dual_source_blending.rs @@ -182,30 +182,26 @@ async fn dual_source_blending_enabled(ctx: TestingContext) { ..render_pipeline_descriptor_template.clone() }); - // Failure mode: + // Happy path: // blend operator dual source: no // shader handling dual source: yes - fail( - &ctx.device, - || { - let _ = ctx - .device - .create_render_pipeline(&wgpu::RenderPipelineDescriptor { - fragment: Some(wgpu::FragmentState { - module: &fragment_shader_with_dual_source_blending, - entry_point: None, - targets: &[Some(wgpu::ColorTargetState { - format: wgpu::TextureFormat::Rgba8Unorm, - blend: None, - write_mask: wgpu::ColorWrites::all(), - })], - compilation_options: Default::default(), - }), - ..render_pipeline_descriptor_template.clone() - }); - }, - Some("Shader entry point expects the pipeline to make use of dual-source blending."), - ); + // (It is okay for the shader to define dual-source I/O that the pipeline + // does not use.) + let _ = ctx + .device + .create_render_pipeline(&wgpu::RenderPipelineDescriptor { + fragment: Some(wgpu::FragmentState { + module: &fragment_shader_with_dual_source_blending, + entry_point: None, + targets: &[Some(wgpu::ColorTargetState { + format: wgpu::TextureFormat::Rgba8Unorm, + blend: None, + write_mask: wgpu::ColorWrites::all(), + })], + compilation_options: Default::default(), + }), + ..render_pipeline_descriptor_template.clone() + }); // Failure mode: // blend operator dual source: yes @@ -229,6 +225,6 @@ async fn dual_source_blending_enabled(ctx: TestingContext) { ..render_pipeline_descriptor_template.clone() }); }, - Some("Pipeline expects the shader entry point to make use of dual-source blending."), + Some("Pipeline uses dual-source blending, but the shader does not support it"), ); } diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index 9103a326c3..3adc55e526 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -3896,14 +3896,12 @@ impl Device { let mut vertex_steps; let mut vertex_buffers; let mut total_attributes; - let mut shader_expects_dual_source_blending = false; - let mut pipeline_expects_dual_source_blending = false; + let mut dual_source_blending = false; + let mut has_depth_attachment = false; if let pipeline::RenderPipelineVertexProcessor::Vertex(ref vertex) = desc.vertex { vertex_steps = Vec::with_capacity(vertex.buffers.len()); vertex_buffers = Vec::with_capacity(vertex.buffers.len()); total_attributes = 0; - shader_expects_dual_source_blending = false; - pipeline_expects_dual_source_blending = false; for (i, vb_state) in vertex.buffers.iter().enumerate() { // https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-gpuvertexbufferlayout @@ -4123,7 +4121,7 @@ impl Device { if factor.ref_second_blend_source() { self.require_features(wgt::Features::DUAL_SOURCE_BLENDING)?; if i == 0 { - pipeline_expects_dual_source_blending = true; + dual_source_blending = true; break; } else { return Err(pipeline::CreateRenderPipelineError @@ -4169,7 +4167,9 @@ impl Device { } let aspect = hal::FormatAspects::from(ds.format); - if ds.is_depth_enabled() && !aspect.contains(hal::FormatAspects::DEPTH) { + if aspect.contains(hal::FormatAspects::DEPTH) { + has_depth_attachment = true; + } else if ds.is_depth_enabled() { break 'error Some(pipeline::DepthStencilStateError::FormatNotDepth(ds.format)); } if ds.stencil.is_enabled() && !aspect.contains(hal::FormatAspects::STENCIL) { @@ -4205,6 +4205,16 @@ impl Device { if ds.bias.clamp != 0.0 { self.require_downlevel_flags(wgt::DownlevelFlags::DEPTH_BIAS_CLAMP)?; } + + if (ds.bias.is_enabled() || ds.bias.clamp != 0.0) + && !desc.primitive.topology.is_triangles() + { + return Err(pipeline::CreateRenderPipelineError::DepthStencilState( + pipeline::DepthStencilStateError::DepthBiasWithIncompatibleTopology( + desc.primitive.topology, + ), + )); + } } if !target_specified { @@ -4378,7 +4388,10 @@ impl Device { let fragment_entry_point_name; let fragment_stage = match desc.fragment { Some(ref fragment_state) => { - let stage = validation::ShaderStageForValidation::Fragment; + let stage = validation::ShaderStageForValidation::Fragment { + dual_source_blending, + has_depth_attachment, + }; let stage_bit = stage.to_wgt_bit(); let shader_module = &fragment_state.stage.module; @@ -4413,15 +4426,6 @@ impl Device { validated_stages |= stage_bit; } - if let Some(ref interface) = shader_module.interface { - shader_expects_dual_source_blending = interface - .fragment_uses_dual_source_blending(&fragment_entry_point_name) - .map_err(|error| pipeline::CreateRenderPipelineError::Stage { - stage: stage_bit, - error, - })?; - } - Some(hal::ProgrammableStage { module: shader_module.raw(), entry_point: &fragment_entry_point_name, @@ -4434,17 +4438,6 @@ impl Device { None => None, }; - if !pipeline_expects_dual_source_blending && shader_expects_dual_source_blending { - return Err( - pipeline::CreateRenderPipelineError::ShaderExpectsPipelineToUseDualSourceBlending, - ); - } - if pipeline_expects_dual_source_blending && !shader_expects_dual_source_blending { - return Err( - pipeline::CreateRenderPipelineError::PipelineExpectsShaderToUseDualSourceBlending, - ); - } - if validated_stages.contains(wgt::ShaderStages::FRAGMENT) { for (i, output) in io.varyings.iter() { match color_targets.get(*i as usize) { diff --git a/wgpu-core/src/pipeline.rs b/wgpu-core/src/pipeline.rs index 9395d59335..aa4ab255a8 100644 --- a/wgpu-core/src/pipeline.rs +++ b/wgpu-core/src/pipeline.rs @@ -625,6 +625,8 @@ pub enum DepthStencilStateError { FormatNotStencil(wgt::TextureFormat), #[error("Sample count {0} is not supported by format {1:?} on this device. The WebGPU spec guarantees {2:?} samples are supported by this format. With the TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES feature your device supports {3:?}.")] InvalidSampleCount(u32, wgt::TextureFormat, Vec, Vec), + #[error("Depth bias is not compatible with non-triangle topology {0:?}")] + DepthBiasWithIncompatibleTopology(wgt::PrimitiveTopology), } #[derive(Clone, Debug, Error)] @@ -702,10 +704,6 @@ pub enum CreateRenderPipelineError { factor: wgt::BlendFactor, target: u32, }, - #[error("Pipeline expects the shader entry point to make use of dual-source blending.")] - PipelineExpectsShaderToUseDualSourceBlending, - #[error("Shader entry point expects the pipeline to make use of dual-source blending.")] - ShaderExpectsPipelineToUseDualSourceBlending, #[error("{}", concat!( "At least one color attachment or depth-stencil attachment was expected, ", "but no render target for the pipeline was specified." @@ -742,8 +740,6 @@ impl WebGpuError for CreateRenderPipelineError { | Self::Stage { .. } | Self::UnalignedShader { .. } | Self::BlendFactorOnUnsupportedTarget { .. } - | Self::PipelineExpectsShaderToUseDualSourceBlending - | Self::ShaderExpectsPipelineToUseDualSourceBlending | Self::NoTargetSpecified | Self::PipelineConstants { .. } | Self::VertexAttributeStrideTooLarge { .. } => return ErrorType::Validation, diff --git a/wgpu-core/src/validation.rs b/wgpu-core/src/validation.rs index 0e0c4578a8..74482a00ea 100644 --- a/wgpu-core/src/validation.rs +++ b/wgpu-core/src/validation.rs @@ -396,6 +396,10 @@ pub enum StageError { MissingPrimitiveIndex, #[error("DrawId cannot be used in the same pipeline as a task shader")] DrawIdError, + #[error("Pipeline uses dual-source blending, but the shader does not support it")] + InvalidDualSourceBlending, + #[error("Fragment shader writes depth, but pipeline does not have a depth attachment")] + MissingFragDepthAttachment, } impl WebGpuError for StageError { @@ -428,7 +432,9 @@ impl WebGpuError for StageError { | Self::TaskPayloadMustMatch { .. } | Self::InvalidPrimitiveIndex | Self::MissingPrimitiveIndex - | Self::DrawIdError => return ErrorType::Validation, + | Self::DrawIdError + | Self::InvalidDualSourceBlending + | Self::MissingFragDepthAttachment => return ErrorType::Validation, }; e.webgpu_error_type() } @@ -1563,7 +1569,10 @@ impl Interface { }); } } - ShaderStageForValidation::Fragment => { + ShaderStageForValidation::Fragment { + dual_source_blending, + has_depth_attachment, + } => { let mut max_fragment_shader_input_variables = self.limits.max_inter_stage_shader_variables; @@ -1631,6 +1640,23 @@ impl Interface { }); } } + + // If the pipeline uses dual-source blending, then the shader + // must configure appropriate I/O, but it is not an error to + // use a shader that defines the I/O in a pipeline that only + // uses one blend source. + if dual_source_blending && !entry_point.dual_source_blending { + return Err(StageError::InvalidDualSourceBlending); + } + + if entry_point + .outputs + .iter() + .any(|varying| matches!(varying, Varying::BuiltIn(naga::BuiltIn::FragDepth))) + && !has_depth_attachment + { + return Err(StageError::MissingFragDepthAttachment); + } } _ => (), } @@ -1753,7 +1779,10 @@ pub enum ShaderStageForValidation { compare_function: Option, }, Mesh, - Fragment, + Fragment { + dual_source_blending: bool, + has_depth_attachment: bool, + }, Compute, Task, } @@ -1763,7 +1792,7 @@ impl ShaderStageForValidation { match self { Self::Vertex { .. } => naga::ShaderStage::Vertex, Self::Mesh => naga::ShaderStage::Mesh, - Self::Fragment => naga::ShaderStage::Fragment, + Self::Fragment { .. } => naga::ShaderStage::Fragment, Self::Compute => naga::ShaderStage::Compute, Self::Task => naga::ShaderStage::Task, } @@ -1772,7 +1801,7 @@ impl ShaderStageForValidation { pub fn to_wgt_bit(&self) -> wgt::ShaderStages { match self { Self::Vertex { .. } => wgt::ShaderStages::VERTEX, - Self::Mesh { .. } => wgt::ShaderStages::MESH, + Self::Mesh => wgt::ShaderStages::MESH, Self::Fragment { .. } => wgt::ShaderStages::FRAGMENT, Self::Compute => wgt::ShaderStages::COMPUTE, Self::Task => wgt::ShaderStages::TASK, diff --git a/wgpu-types/src/render.rs b/wgpu-types/src/render.rs index 35c2c3f5b0..9a3246bf72 100644 --- a/wgpu-types/src/render.rs +++ b/wgpu-types/src/render.rs @@ -298,6 +298,15 @@ impl PrimitiveTopology { Self::LineStrip | Self::TriangleStrip => true, } } + + /// Returns true for triangle topologies. + #[must_use] + pub fn is_triangles(&self) -> bool { + match *self { + Self::TriangleList | Self::TriangleStrip => true, + Self::PointList | Self::LineList | Self::LineStrip => false, + } + } } /// Vertex winding order which classifies the "front" face of a triangle.