google / amber

Amber is a multi-API shader test framework
Apache License 2.0
193 stars 65 forks source link

Add support to define descriptor buffer offset and range #947

Closed ilkkasaa closed 3 years ago

ilkkasaa commented 3 years ago

Descriptor buffers can now be bound with byte offset and range via DESCRIPTOR_OFFSET and DESCRIPTOR_RANGE parameters.

Descriptor array elements can now point to same buffer.

Fixes problem with dynamic uniform/storage buffers: when a dynamic offset is > 0, VK_WHOLE_SIZE must not be used as buffer range.

Fixes #945

ilkkasaa commented 3 years ago

Yeah, that's a good point! So, now it doesn't loop over maps anymore. The transfer images/buffers are still stored in maps for easier inserts and amber_buffer->transfer_buffer lookups, but when looping, the amber_buffers vector is used to get the keys or an ordered vector of amber_buffer->transfer_buffer pairs.

ilkkasaa commented 3 years ago

While doing this PR, I was thinking, that now we can share a buffer between descriptors within a descriptor array, but the same doesn't apply to separate bindings or descriptor sets. e.g. following Amber code is valid, but it will actually create two buffers with same contents:

BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 0 
BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 1

The real problem is with buffers that the shaders can update, like SSBOs, e.g. this doesn't even work:

BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 0
BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 1

I think that Amber needs some completely different way of handling descriptor buffers to resolve this problem, but that's probably out of this PR's scope.

paulthomson commented 3 years ago
[arm64-v8a] Compile++      : amber <= buffer_descriptor.cc
../src/vulkan/buffer_descriptor.cc:158:19: error: 'auto' not allowed in lambda parameter
        [&](const auto& buffer) { return buffer.first == amber_buffer; });
                  ^~~~
In file included from ../src/vulkan/buffer_descriptor.cc:15:
In file included from ../src/vulkan/buffer_descriptor.h:19:
In file included from /tmpfs/src/android-ndk-r21/sources/cxx-stl/llvm-libc++/include/unordered_map:409:
In file included from /tmpfs/src/android-ndk-r21/sources/cxx-stl/llvm-libc++/include/__hash_table:17:
/tmpfs/src/android-ndk-r21/sources/cxx-stl/llvm-libc++/include/algorithm:933:13: error: no matching function for call to object of type '(lambda at ../src/vulkan/buffer_descriptor.cc:158:9)'
        if (__pred(*__first))
            ^~~~~~
../src/vulkan/buffer_descriptor.cc:156:30: note: in instantiation of function template specialization 'std::__ndk1::find_if<std::__ndk1::__wrap_iter<std::__ndk1::pair<amber::Buffer *, amber::vulkan::Resource *> *>, (lambda at ../src/vulkan/buffer_descriptor.cc:158:9)>' requested here
    const auto& image = std::find_if(
                             ^
../src/vulkan/buffer_descriptor.cc:158:9: note: candidate function not viable: no known conversion from 'std::__ndk1::pair<amber::Buffer *, amber::vulkan::Resource *>' to 'int &' for 1st argument
        [&](const auto& buffer) { return buffer.first == amber_buffer; });
        ^
2 errors generated.
make: *** [/tmpfs/src/android-ndk-r21/build/core/build-binary.mk:478: /tmpfs/src/github/amber/build/app/local/arm64-v8a/objs/amber/src/vulkan/buffer_descriptor.o] Error 1
ilkkasaa commented 3 years ago

Thanks, I'll fix that.

paulthomson commented 3 years ago

While doing this PR, I was thinking, that now we can share a buffer between descriptors within a descriptor array, but the same doesn't apply to separate bindings or descriptor sets. e.g. following Amber code is valid, but it will actually create two buffers with same contents:

BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 0 
BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 1

The real problem is with buffers that the shaders can update, like SSBOs, e.g. this doesn't even work:

BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 0
BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 1

I think that Amber needs some completely different way of handling descriptor buffers to resolve this problem, but that's probably out of this PR's scope.

Oh really!? That is a shame. I think we should fix this, if you can think of a way. But, as you say, not necessarily in this PR.

ilkkasaa commented 3 years ago

While doing this PR, I was thinking, that now we can share a buffer between descriptors within a descriptor array, but the same doesn't apply to separate bindings or descriptor sets. e.g. following Amber code is valid, but it will actually create two buffers with same contents:

BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 0 
BIND BUFFER buf0 AS uniform DESCRIPTOR_SET 0 BINDING 1

The real problem is with buffers that the shaders can update, like SSBOs, e.g. this doesn't even work:

BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 0
BIND BUFFER buf0 AS storage DESCRIPTOR_SET 0 BINDING 1

I think that Amber needs some completely different way of handling descriptor buffers to resolve this problem, but that's probably out of this PR's scope.

Oh really!? That is a shame. I think we should fix this, if you can think of a way. But, as you say, not necessarily in this PR.

Yeah, I have some ideas. I can look into that after this PR.