Confusion between alignment and stride

[apayen]

Hello!

First of all, thank you very much for undertaking this project. C++ kinda pisses me off these day, so I just decided to give rust a chance for my vulkan engine :p

Now, on to the matter: Running both samples gave me a black screen, so I did a renderdoc capture. I found out that all 3 vertices for the triangle had the same position/color.

The culprit is here:

        let mut vert_align = Align::new(vert_ptr,
                                        vertex_input_buffer_memory_req.alignment,
                                        vertex_input_buffer_memory_req.size);

    pub unsafe fn new(ptr: *mut (), alignment: vk::DeviceSize, size: vk::DeviceSize) -> Self {
        let padding = calc_padding(size_of::<T>() as vk::DeviceSize, alignment);
        let elem_size = size_of::<T>() as vk::DeviceSize + padding;

On my system, vertex_input_buffer_memory_req.alignment == 256 as well as vertex_input_buffer_memory_req.size. Hence, it would copy only 1 element.

Alignment is supposed to be for the buffer offset (in case the buffer is not bound to device memory with an offset of 0. With my driver, buffer has to be aligned on 256 bytes boundary). Alignment of individual elements inside the buffer is the stride (which is, in this case, specified by the vertex binding description as mem::size_of::<Vertex>() == 32).

I fixed it on my side by removing + padding, and voila! Triangle (and texture) appeared.

I'm not making a pull request for this as I'm not sure if the issue is the implementation of Align itself, or using Align to copy the vertices; that depends on what else you use Align for, and the intended behavior/design.

On a side note, this is just my personal taste here, but I don't like much the vk::Format::R8g8b8a8Unorm for formats (as the first component is UP, but others are low). I prefer the way they are defined in vulkan.hpp (vk::Format::eR8G8B8A8Unorm). I think it would make more sense to keep the same notation here (including the e) for copy pasting.

Cheers!

[MaikKlein]

Yes, thanks for opening the issue. I got a bit confused while writing Align. As you have said vertex_input_buffer_memory_req.alignment is only for the offset inside the memory allocation. What I actually wanted to do is to write multiple values inside a single buffer.

I just haven't updated the examples yet and I should probably rename Alignto Stride. (I am not sure yet)

So this means if you create a uniform buffer with more than one value, you have to make sure that you know the correct alignment for each element and this value should be put inside Align::new. In the examples I incorrectly put in the alignment of the allocation but I should have used https://doc.rust-lang.org/std/mem/fn.align_of.html. I also implemented that if the alignment is the same as in Rust, it will just just do a copy_from_slice under the hood. But the user has to check manually for the alignment see minUniformBufferOffsetAlignment for example.

I updated the examples. Maybe I should rename Align::new to Stride::with_alignment? I think that might be a better name.

[MaikKlein]

To be clear most of the time the stride is the same as in rust, and you can do

        let mut slice = Align::new(
            vert_ptr,
            align_of::<Vertex>() as u64,
            vertex_input_buffer_memory_req.size,
        );

This basically means that you can also use https://doc.rust-lang.org/std/slice/fn.from_raw_parts_mut.html for all buffers in the examples. But sometimes it might differ.

[MaikKlein]

I currently use this function in my engine to get the correct alignment

pub fn align_buffer<T>(usage: vk::BufferUsageFlags, props: &vk::PhysicalDeviceProperties) -> u64 {
    use std::mem::align_of;
    use std::cmp::max;
    let mut align = align_of::<T>() as u64;
    if usage.intersects(vk::BUFFER_USAGE_UNIFORM_BUFFER_BIT) {
        align = max(align, props.limits.min_uniform_buffer_offset_alignment);
    }
    if usage.intersects(vk::BUFFER_USAGE_STORAGE_BUFFER_BIT) {
        align = max(align, props.limits.min_storage_buffer_offset_alignment);
    }
    if usage.intersects(vk::BUFFER_USAGE_STORAGE_TEXEL_BUFFER_BIT) {
        align = max(align, props.limits.min_texel_buffer_offset_alignment);
    }
    align
}

[MaikKlein]

I just fixed found a small bug, when the size is bigger than the size slice of the slice. Could you try to run the newest master and tell me if that fixes your issue?

[apayen]

I think we should be clear on terminology:

• Alignment: address/offset must be a multiple of the alignment
• Stride: size of 1 element in an array (including padding)
• Size: total size of an array
• For a single element, size == stride

align_of::<Vertex>() will return 4 (alignment of a float). So with that we're actually copying more vertices. size_of::<Vertex>() returns 32 (8 floats).

What I understand is that you want to specify the stride, and allow copying a structure with a different stride (like having seperate position and color arrays that you copy in the same buffer).

In this case, I suggest that we write it this way:

vert_ptr.copy_from_slice(&vertices, stride)
vert_ptr.offset(offset).align(alignment).copy_from_slice(&vertices, stride)

• offset and align would return an iterator/slice of the device memory. we can align offset using new_offset = (old_offset + alignment - 1) & !(alignment - 1) (assuming alignment is power of 2).
• then, copy_from_slice would do the copy, and if stride == size_of::<T>() use the fast path
• copy_from_slice would return self to allow chaining copy_from_slice(..).copy_from_slice(..).offset(..).copy_from_slice(..)

so with seperate position and color arrays, we would have:

let stride = size_of::<Position>() + size_of::<Color>();
vert_ptr.copy_from_slice(&positions, stride)
        .offset(size_of::<Position>()).align(align_of::<Color>())
        .copy_from_slice(&colors, stride);

[apayen]

hmm, rather than

let stride = size_of::<Position>() + size_of::<Color>();

should do

let stride = compute_stride!(Position, Color)

to account for padding between members

[MaikKlein]

What I understand is that you want to specify the stride, and allow copying a structure with a different stride (like having seperate position and color arrays that you copy in the same buffer).

I think you misunderstand. Let us say you want to create a uniform buffer filled with some Vector3<f32>. These are 3 floats => 12 bytes.

Then a Vec in memory would be contiguous. [Vector3, Vector3, Vector3...] but in Vulkan this is not true. For example on my GPU I have to align each element to 16 bytes, otherwise I can not use the offset. What this means is that I need to insert a 4 byte padding here.

[Vector3, 4 bytes, Vector3, 4 bytes, Vector3...] Otherwise the offset in bytes of each element is not aligned to 16 bytes.

Also this means that the buffer, if the buffer should to contain 3 Vector3, has to be of size 3 16 and not 3 * 12.

See https://github.com/SaschaWillems/Vulkan/tree/master/dynamicuniformbuffer for a specific example.

[MaikKlein]

minUniformBufferOffsetAlignment is the minimum required alignment, in bytes, for the offset member of the VkDescriptorBufferInfo structure for uniform buffers. When a descriptor of type VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER or VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC is updated, the offset must be an integer multiple of this limit. Similarly, dynamic offsets for uniform buffers must be multiples of this limit.

I hope this clarifies what I am trying to do, I could still be wrong here.

[apayen]

Ah yes, if you have multiple uniform buffers in the same vk::Buffer, each uniform buffer indeed needs to be aligned (because on vulkan's side, this is not an array of struct, as every descriptor is individual - so is the memory pointed to).

With my suggestion above, you could write:

vert_ptr.copy_from_slice(&positions, aligned_size<T>(16))

That way, it's pretty explicit that the stride is the size of T aligned to 16 bytes

For uniform buffers, I use a single huge buffer that is cleared at the beginning of the frame. In pseudo code, every time I need to create a descriptor (of any size), I use

// beginning of the frame recording
bigbuffer.ptr = device.map(bigbuffer.buffer, 0, bigbuffer.size);
bigbuffer.offset = 0;

// every time I need to write a uniform buffer
bigbuffer.offset = align(offset, alignment);
bindDescriptorSet(..., bigbuffer.offset);
memcpy(bigbuffer.ptr + bigbuffer.offset, constant_buffer.ptr, constant_buffer.size);
bigbuffer.offset += constant_buffer.size

// end of frame, before queue
device.unmap(bigbuffer.buffer)

That way, I allocate only one buffer per backbuffer in my swap chain during init, and map/unmap only once per frame.

That would translate in rust to (with my previous suggestion)

bigbuffer_ptr = bigbuffer_ptr.align(alignment);
// bind descriptor set here //
bigbuffer_ptr.copy(&element)
bigbuffer_ptr = bigbuffer_ptr.offset(sizeof::<T>())

which I think is pretty explicit too

[MaikKlein]

Yeah that works, I am doing it a bit different. I still create one buffer per render pass, which gets a an allocation from a linear allocator. Then I just upload everything and draw. I was also thinking if I maybe should switch from big allocations to just big buffers, but I am still allocating big and create many buffers.

API wise I actually think it is currently okayish. It is also only in the util module and it is completely optional. I wasn't even sure if I should add it to this library. And AlignByteslice can just be removed.

I guess I could implement a trait for the void ptr.