vulkano-rs / vulkano

Safe and rich Rust wrapper around the Vulkan API
Apache License 2.0
4.45k stars 435 forks source link

Usage of PersistentDescriptorSet and how to update uniform buffer #2522

Closed zim32 closed 4 months ago

zim32 commented 4 months ago

Usage of PersistentDescriptorSet and how to update uniform buffer

If you dont understand something just leave it. If you can provide more detailed information than the template allows for, please ignore the template and present all of your findings.

Issue

This is actually not a bug but I am trying to understand how to properly use descriptor sets. From the teapot example I can see that in every frame you are allocating new uniform_buffer_subbuffer and also you are calling DescriptorSet::new.

When I compare this code with C++ examples I can see that in C++ workflow is like that: 1) Create buffers 2) Map buffers for the duration of the whole program 2) Create descriptor sets 3) Call vkUpdateDescriptorSets once 4) In render loop they call function updateUniformBuffer() which basically only memcpy into mapped buffer

So I have a question is it ok that vulkano do descriptor sets allocation per frame and create new buffer per every frame? When I try to create buffer and descriptor set outside of render loop and then inside render loop I do *uniform_buffer.write().unwrap() = uniform_data; I got error AccessConflict(DeviceRead)

marc0246 commented 4 months ago

You can take a look at the async-update example for how to do it while keeping the same descriptor sets. But no it's not a problem to allocate a new descriptor set each frame. The allocation itself takes nanoseconds because the allocator is optimized for it. The Vulkan call to update it not so much but still not too bad when the update is small. We never create a new buffer each frame.

zim32 commented 4 months ago

We never create a new buffer each frame.

Here as I understand new buffer is allocated? https://github.com/vulkano-rs/vulkano/blob/master/examples/teapot/main.rs#L343

marc0246 commented 4 months ago

That's a subbuffer, not a buffer. It also takes nanoseconds to allocate.

zim32 commented 4 months ago

Interestingly I can't understand who locks Subbuffer. Outside of render loop I create subbufer like this

let uniform_buffer = {
    let uniform_data = vs::Test {
        foo: scale_factor
    };

    let subbuffer = uniform_buffer_allocator.allocate_sized().unwrap();
    *subbuffer.write().unwrap() = uniform_data;
    subbuffer
};

Here write returns WriteGuard which is then dropped and buffer state should be reset again cpu_write_unlock. But later inside render loop I try to write to buffer again and got AccessConflict(DeviceRead) error. Can't understand who locks this buffer

marc0246 commented 4 months ago

The device is still using the buffer. This is described in detail in the async-update example.

zim32 commented 4 months ago

Ok thanks I will check. Amyway it works. Thanks for amazing library 👍

zim32 commented 4 months ago

For anybody coming to this page, this was needed to be able to remove AccessConflict(DeviceRead) error:

let command_buffer = builder.build().unwrap();
acquire_future.wait(None).unwrap(); // add this line
previous_frame_end.as_mut().unwrap().cleanup_finished(); // add this line
*uniform_buffer.write().unwrap() = uniform_data; // update uniform buffer here
zim32 commented 4 months ago

By the way if presentatiom mode is set to immediate, trying to update buffer with the code above again lead to AccessConflict(DeviceRead). Not sure how it works under the hood and is it a bug or intended behaviour

zim32 commented 4 months ago

And if I move this piece of code inside render loop:

let uniform_buffer = {
    let uniform_data = vs::Test {
        foo: scale_factor
    };

    let subbuffer = uniform_buffer_allocator.allocate_sized().unwrap();
    *subbuffer.write().unwrap() = uniform_data;
    subbuffer
};

let descriptor_set = {
    let layout = pipeline.layout().set_layouts().get(0).unwrap();

    PersistentDescriptorSet::new(
        &descriptor_set_allocator,
        layout.clone(),
        [WriteDescriptorSet::buffer(0, uniform_buffer.clone())],
        [],
    ).unwrap()
};

and set presentation mode to Immediate, it works fine several seconds but then crashes with a non-validation error occurred: a host memory allocation has failed

marc0246 commented 4 months ago

Present mode should have no bearing on synchronization. The async-update example works with any present mode (does it not?). Sounds like you have a race condition on the host.

The second issue sounds like you never call cleanup_finished.

zim32 commented 4 months ago

cleanup_finished solves the issue