From 1c43ac2c42bba8528d985b9992342045a6a7180e Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Wed, 23 Jul 2025 08:39:42 -0700 Subject: [PATCH] Additional validation of buffer-texture copies (#7948) * Additional validation of buffer-texture copies Fixes #7936, but leaves a TODO for #7947 * Skip tests failing on dx12 * Update comments and change unwrap_or to expect --- cts_runner/test.lst | 22 +++++++ wgpu-core/src/command/transfer.rs | 95 ++++++++++++++++++++++++++----- wgpu-core/src/conv.rs | 3 + wgpu-core/src/device/queue.rs | 18 +++--- wgpu-types/src/lib.rs | 2 + 5 files changed, 119 insertions(+), 21 deletions(-) diff --git a/cts_runner/test.lst b/cts_runner/test.lst index 21eb6b448..3bac66e84 100644 --- a/cts_runner/test.lst +++ b/cts_runner/test.lst @@ -34,6 +34,28 @@ webgpu:api,validation,encoding,programmable,pipeline_bind_group_compat:bind_grou webgpu:api,validation,encoding,programmable,pipeline_bind_group_compat:buffer_binding,render_pipeline:* webgpu:api,validation,encoding,programmable,pipeline_bind_group_compat:sampler_binding,render_pipeline:* webgpu:api,validation,encoding,queries,general:occlusion_query,query_index:* +webgpu:api,validation,image_copy,buffer_texture_copies:depth_stencil_format,copy_usage_and_aspect:* +webgpu:api,validation,image_copy,buffer_texture_copies:depth_stencil_format,copy_buffer_size:* +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:depth_stencil_format,copy_buffer_offset:format="depth32float";aspect="depth-only";copyType="CopyT2B" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:depth_stencil_format,copy_buffer_offset:format="depth24plus-stencil8";aspect="stencil-only";copyType="CopyB2T" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:depth_stencil_format,copy_buffer_offset:format="depth24plus-stencil8";aspect="stencil-only";copyType="CopyT2B" +//mix of PASS and FAIL: other subtests of copy_buffer_offset. Related bugs: +// https://github.com/gfx-rs/wgpu/issues/7946, https://github.com/gfx-rs/wgpu/issues/7947 +webgpu:api,validation,image_copy,buffer_texture_copies:sample_count:* +webgpu:api,validation,image_copy,buffer_texture_copies:texture_buffer_usages:* +webgpu:api,validation,image_copy,buffer_texture_copies:device_mismatch:* +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="rgba8unorm";copyType="CopyB2T";dimension="2d" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="rgba8unorm";copyType="CopyT2B";dimension="2d" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="rgba8unorm";copyType="CopyB2T";dimension="3d" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="rgba8unorm";copyType="CopyT2B";dimension="3d" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="bgra8unorm";copyType="CopyB2T";dimension="2d" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="bgra8unorm";copyType="CopyT2B";dimension="2d" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="astc-4x4-unorm";copyType="CopyT2B";dimension="2d" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="astc-4x4-unorm";copyType="CopyB2T";dimension="2d" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="astc-4x4-unorm";copyType="CopyT2B";dimension="3d" +fails-if(dx12) webgpu:api,validation,image_copy,buffer_texture_copies:offset_and_bytesPerRow:format="astc-4x4-unorm";copyType="CopyB2T";dimension="3d" +//mix of PASS and FAIL: other subtests of offset_and_bytesPerRow. Related bugs: +// https://github.com/gfx-rs/wgpu/issues/7946, https://github.com/gfx-rs/wgpu/issues/7947 webgpu:api,validation,image_copy,layout_related:copy_end_overflows_u64:* webgpu:api,validation,image_copy,texture_related:format:dimension="1d";* webgpu:api,validation,queue,submit:command_buffer,device_mismatch:* diff --git a/wgpu-core/src/command/transfer.rs b/wgpu-core/src/command/transfer.rs index 4d6c8d98c..56816fa87 100644 --- a/wgpu-core/src/command/transfer.rs +++ b/wgpu-core/src/command/transfer.rs @@ -363,6 +363,70 @@ pub(crate) fn validate_linear_texture_data( Ok((bytes_in_copy, image_stride_bytes)) } +/// Validation for texture/buffer copies. +/// +/// This implements the following checks from WebGPU's [validating texture buffer copy][vtbc] +/// algorithm: +/// * The texture must not be multisampled. +/// * The copy must be from/to a single aspect of the texture. +/// * If `aligned` is true, the buffer offset must be aligned appropriately. +/// +/// The following steps in the algorithm are implemented elsewhere: +/// * Invocation of other validation algorithms. +/// * The texture usage (COPY_DST / COPY_SRC) check. +/// * The check for non-copyable depth/stencil formats. The caller must perform +/// this check using `is_valid_copy_src_format` / `is_valid_copy_dst_format` +/// before calling this function. This function will panic if +/// [`wgt::TextureFormat::block_copy_size`] returns `None` due to a +/// non-copyable format. +/// +/// [vtbc]: https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-texture-buffer-copy +pub(crate) fn validate_texture_buffer_copy( + texture_copy_view: &wgt::TexelCopyTextureInfo, + aspect: hal::FormatAspects, + desc: &wgt::TextureDescriptor<(), Vec>, + offset: BufferAddress, + aligned: bool, +) -> Result<(), TransferError> { + if desc.sample_count != 1 { + return Err(TransferError::InvalidSampleCount { + sample_count: desc.sample_count, + }); + } + + if !aspect.is_one() { + return Err(TransferError::CopyAspectNotOne); + } + + let mut offset_alignment = if desc.format.is_depth_stencil_format() { + 4 + } else { + // The case where `block_copy_size` returns `None` is currently + // unreachable both for the reason in the expect message, and also + // because the currently-defined non-copyable formats are depth/stencil + // formats so would take the `if` branch. + desc.format + .block_copy_size(Some(texture_copy_view.aspect)) + .expect("non-copyable formats should have been rejected previously") + }; + + // TODO(https://github.com/gfx-rs/wgpu/issues/7947): This does not match the spec. + // The spec imposes no alignment requirement if `!aligned`, and otherwise + // imposes only the `offset_alignment` as calculated above. wgpu currently + // can panic on alignments <4B, so we reject them here to avoid a panic. + if aligned { + offset_alignment = offset_alignment.max(4); + } else { + offset_alignment = 4; + } + + if offset % u64::from(offset_alignment) != 0 { + return Err(TransferError::UnalignedBufferOffset(offset)); + } + + Ok(()) +} + /// Validate the extent and alignment of a texture copy. /// /// Copied with minor modifications from WebGPU standard. This mostly follows @@ -884,10 +948,6 @@ impl Global { .map(|pending| pending.into_hal(dst_raw)) .collect::>(); - if !dst_base.aspect.is_one() { - return Err(TransferError::CopyAspectNotOne.into()); - } - if !conv::is_valid_copy_dst_texture_format(dst_texture.desc.format, destination.aspect) { return Err(TransferError::CopyToForbiddenTextureFormat { @@ -897,6 +957,14 @@ impl Global { .into()); } + validate_texture_buffer_copy( + destination, + dst_base.aspect, + &dst_texture.desc, + source.layout.offset, + true, // alignment required for buffer offset + )?; + let (required_buffer_bytes_in_copy, bytes_per_array_layer) = validate_linear_texture_data( &source.layout, @@ -999,12 +1067,7 @@ impl Global { src_texture .check_usage(TextureUsages::COPY_SRC) .map_err(TransferError::MissingTextureUsage)?; - if src_texture.desc.sample_count != 1 { - return Err(TransferError::InvalidSampleCount { - sample_count: src_texture.desc.sample_count, - } - .into()); - } + if source.mip_level >= src_texture.desc.mip_level_count { return Err(TransferError::InvalidMipLevel { requested: source.mip_level, @@ -1013,10 +1076,6 @@ impl Global { .into()); } - if !src_base.aspect.is_one() { - return Err(TransferError::CopyAspectNotOne.into()); - } - if !conv::is_valid_copy_src_texture_format(src_texture.desc.format, source.aspect) { return Err(TransferError::CopyFromForbiddenTextureFormat { format: src_texture.desc.format, @@ -1025,6 +1084,14 @@ impl Global { .into()); } + validate_texture_buffer_copy( + source, + src_base.aspect, + &src_texture.desc, + destination.layout.offset, + true, // alignment required for buffer offset + )?; + let (required_buffer_bytes_in_copy, bytes_per_array_layer) = validate_linear_texture_data( &destination.layout, diff --git a/wgpu-core/src/conv.rs b/wgpu-core/src/conv.rs index 5a5a27279..aa6ceb046 100644 --- a/wgpu-core/src/conv.rs +++ b/wgpu-core/src/conv.rs @@ -2,6 +2,9 @@ use wgt::TextureFormatFeatures; use crate::resource::{self, TextureDescriptor}; +// Some core-only texture format helpers. The helper methods on `TextureFormat` +// defined in wgpu-types may need to be modified along with the ones here. + pub fn is_valid_copy_src_texture_format( format: wgt::TextureFormat, aspect: wgt::TextureAspect, diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index 6b4c1478b..8a51e60a4 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -19,9 +19,9 @@ use crate::device::trace::Action; use crate::{ api_log, command::{ - extract_texture_selector, validate_linear_texture_data, validate_texture_copy_range, - ClearError, CommandAllocator, CommandBuffer, CommandEncoder, CommandEncoderError, CopySide, - TexelCopyTextureInfo, TransferError, + extract_texture_selector, validate_linear_texture_data, validate_texture_buffer_copy, + validate_texture_copy_range, ClearError, CommandAllocator, CommandBuffer, CommandEncoder, + CommandEncoderError, CopySide, TexelCopyTextureInfo, TransferError, }, conv, device::{DeviceError, WaitIdleError}, @@ -737,10 +737,6 @@ impl Queue { let (selector, dst_base) = extract_texture_selector(&destination, size, &dst)?; - if !dst_base.aspect.is_one() { - return Err(TransferError::CopyAspectNotOne.into()); - } - if !conv::is_valid_copy_dst_texture_format(dst.desc.format, destination.aspect) { return Err(TransferError::CopyToForbiddenTextureFormat { format: dst.desc.format, @@ -749,6 +745,14 @@ impl Queue { .into()); } + validate_texture_buffer_copy( + &destination, + dst_base.aspect, + &dst.desc, + data_layout.offset, + false, // alignment not required for buffer offset + )?; + // Note: `_source_bytes_per_array_layer` is ignored since we // have a staging copy, and it can have a different value. let (required_bytes_in_copy, _source_bytes_per_array_layer) = validate_linear_texture_data( diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index 256e5fa45..6b51d5046 100644 --- a/wgpu-types/src/lib.rs +++ b/wgpu-types/src/lib.rs @@ -2608,6 +2608,8 @@ impl TextureAspect { } } +// There are some additional texture format helpers in `wgpu-core/src/conv.rs`, +// that may need to be modified along with the ones here. impl TextureFormat { /// Returns the aspect-specific format of the original format ///