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

Author: andyleiserson

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:*

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<T>(
+    texture_copy_view: &wgt::TexelCopyTextureInfo<T>,
+    aspect: hal::FormatAspects,
+    desc: &wgt::TextureDescriptor<(), Vec<wgt::TextureFormat>>,
+    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::<Vec<_>>();
 
-            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,

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,

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(

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
     ///