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

Test case provokes Vulkan validation error #2959

Closed
jimblandy opened this issue Aug 12, 2022 · 3 comments · Fixed by #2961
Closed

Test case provokes Vulkan validation error #2959

jimblandy opened this issue Aug 12, 2022 · 3 comments · Fixed by #2961

Comments

@jimblandy
Copy link
Member

jimblandy commented Aug 12, 2022

I added the following test to wgpu/tests, and it provokes the complaints at bottom from the Vulkan validation layer:

//! Tests for texture copy bounds checks.

use std::num::NonZeroU32;
use crate::common::{initialize_test, TestParameters};

#[test]
fn bad_copy_origin() {
    fn try_origin(origin: wgpu::Origin3d, should_panic: bool) {
        let mut parameters = TestParameters::default();
        if should_panic {
            parameters = parameters.failure();
        }
           
        initialize_test(parameters, |ctx| {
            let texture = ctx.device.create_texture(&TEXTURE_DESCRIPTOR);
            let data = vec![255; BUFFER_SIZE as usize];
            ctx.queue.write_texture(
                wgpu::ImageCopyTexture {
                    texture: &texture,
                    mip_level: 0,
                    origin,
                    aspect: wgpu::TextureAspect::All,
                },
                &data,
                BUFFER_COPY_LAYOUT,
                TEXTURE_SIZE,
            );
        });
    }

    try_origin(wgpu::Origin3d { x: 0, y: 0, z: 0 }, false);
    try_origin(wgpu::Origin3d { x: 1, y: 0, z: 0 }, true);
    try_origin(wgpu::Origin3d { x: 0, y: 1, z: 0 }, true);
    try_origin(wgpu::Origin3d { x: 0, y: 0, z: 1 }, true);
}

const TEXTURE_SIZE: wgpu::Extent3d = wgpu::Extent3d {
    width: 64,
    height: 64,
    depth_or_array_layers: 1,
};

const TEXTURE_DESCRIPTOR: wgpu::TextureDescriptor = wgpu::TextureDescriptor {
    label: Some("CopyOrigin"),
    size: TEXTURE_SIZE,
    mip_level_count: 1,
    sample_count: 1,
    dimension: wgpu::TextureDimension::D2,
    format: wgpu::TextureFormat::Rgba8UnormSrgb,
    usage: wgpu::TextureUsages::COPY_DST.union(wgpu::TextureUsages::COPY_SRC),
};

const BYTES_PER_PIXEL: u32 = 4;

const BUFFER_SIZE: u32 = TEXTURE_SIZE.width * TEXTURE_SIZE.height * BYTES_PER_PIXEL;

const BUFFER_COPY_LAYOUT: wgpu::ImageDataLayout = wgpu::ImageDataLayout {
    offset: 0,
    bytes_per_row: NonZeroU32::new(TEXTURE_SIZE.width * BYTES_PER_PIXEL),
    rows_per_image: None,
};

Here's the validation error:

[2022-08-12T16:06:19Z ERROR wgpu_hal::vulkan::instance] VALIDATION [VUID-vkDestroyDevice-device-00378 (0x71500fba)]
    	Validation Error: [ VUID-vkDestroyDevice-device-00378 ] Object 0: handle = 0x7fa69c0d3d50, type = VK_OBJECT_TYPE_DEVICE; Object 1: handle = 0x49f000000049f, name = (wgpu internal) Staging, type = VK_OBJECT_TYPE_BUFFER; | MessageID = 0x71500fba | OBJ ERROR : For VkDevice 0x7fa69c0d3d50[], VkBuffer 0x49f000000049f[(wgpu internal) Staging] has not been destroyed. The Vulkan spec states: All child objects created on device must have been destroyed prior to destroying device (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkDestroyDevice-device-00378)
[2022-08-12T16:06:19Z ERROR wgpu_hal::vulkan::instance] 	objects: (type: DEVICE, hndl: 0x7fa69c0d3d50, name: ?), (type: BUFFER, hndl: 0x49f000000049f, name: (wgpu internal) Staging)
[2022-08-12T16:06:19Z ERROR wgpu_hal::vulkan::instance] VALIDATION [VUID-vkDestroyDevice-device-00378 (0x71500fba)]
    	Validation Error: [ VUID-vkDestroyDevice-device-00378 ] Object 0: handle = 0x7fa69c717250, type = VK_OBJECT_TYPE_DEVICE; Object 1: handle = 0x90000000009, name = (wgpu internal) Staging, type = VK_OBJECT_TYPE_BUFFER; | MessageID = 0x71500fba | OBJ ERROR : For VkDevice 0x7fa69c717250[], VkBuffer 0x90000000009[(wgpu internal) Staging] has not been destroyed. The Vulkan spec states: All child objects created on device must have been destroyed prior to destroying device (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkDestroyDevice-device-00378)
[2022-08-12T16:06:19Z ERROR wgpu_hal::vulkan::instance] 	objects: (type: DEVICE, hndl: 0x7fa69c717250, name: ?), (type: BUFFER, hndl: 0x90000000009, name: (wgpu internal) Staging)
[2022-08-12T16:06:19Z ERROR wgpu_hal::vulkan::instance] VALIDATION [VUID-vkDestroyDevice-device-00378 (0x71500fba)]
    	Validation Error: [ VUID-vkDestroyDevice-device-00378 ] Object 0: handle = 0x7fa69c5a2e30, type = VK_OBJECT_TYPE_DEVICE; Object 1: handle = 0x90000000009, name = (wgpu internal) Staging, type = VK_OBJECT_TYPE_BUFFER; | MessageID = 0x71500fba | OBJ ERROR : For VkDevice 0x7fa69c5a2e30[], VkBuffer 0x90000000009[(wgpu internal) Staging] has not been destroyed. The Vulkan spec states: All child objects created on device must have been destroyed prior to destroying device (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkDestroyDevice-device-00378)
[2022-08-12T16:06:19Z ERROR wgpu_hal::vulkan::instance] 	objects: (type: DEVICE, hndl: 0x7fa69c5a2e30, name: ?), (type: BUFFER, hndl: 0x90000000009, name: (wgpu internal) Staging)
@jimblandy
Copy link
Member Author

jimblandy commented Aug 12, 2022

Here's the language from the Vulkan spec (it's included in the validation output, but scrolled off to the side)

VUID-vkDestroyDevice-device-00378
All child objects created on device must have been destroyed prior to destroying device

@jimblandy
Copy link
Member Author

I'm going to bet this has something to do with the test being expected to panic. It is supposed to reach the handle_error_fatal call in backend::direct::Context::queue_write_texture.

@jimblandy
Copy link
Member Author

It seems like Global::queue_write_texture creates a staging buffer, but if an error occurs, it won't free it.
This upsets Vulkan validation.

Obviously, there should be some sort of Drop involved somewhere. But properly freeing the buffer entails having a &mut Device, and that's going to be "hard" to obtain in a drop implementation.

jimblandy added a commit to jimblandy/wgpu that referenced this issue Aug 13, 2022
We must ensure that the staging buffer is not leaked when an error
occurs, so allocate it as late as possible, and free it explicitly when
those fallible operations we can't move it past go awry.

Fixes gfx-rs#2959.
jimblandy added a commit to jimblandy/wgpu that referenced this issue Aug 14, 2022
We must ensure that the staging buffer is not leaked when an error
occurs, so allocate it as late as possible, and free it explicitly when
those fallible operations we can't move it past go awry.

Fixes gfx-rs#2959.
jimblandy added a commit to jimblandy/wgpu that referenced this issue Aug 14, 2022
We must ensure that the staging buffer is not leaked when an error
occurs, so allocate it as late as possible, and free it explicitly when
those fallible operations we can't move it past go awry.

Fixes gfx-rs#2959.
jimblandy added a commit that referenced this issue Aug 14, 2022
* Have `prepare_staging_buffer` take a raw hal Device.

This helps the borrow checker understand that we can borrow
`pending_writes`'s encoder and the raw Device at the same time.

* Always free staging buffers.

We must ensure that the staging buffer is not leaked when an error
occurs, so allocate it as late as possible, and free it explicitly when
those fallible operations we can't move it past go awry.

Fixes #2959.

* Some tests for texture and buffer copies.

* Add CHANGELOG.md entry.
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 a pull request may close this issue.

1 participant