Skip to content

Conversation

@renderexpert
Copy link
Contributor

Why:

Vulkan Validation didn't work for a project for a long period. It hid problems in the Vulkan implementation on Android. This PR fixes it by adding a build option to enable Vulkan validation.

Example of output:

2024-03-17 20:23:41.871  3390-3897  OMcore                  app.organicmaps.debug                E  vulkan/vulkan_layers.cpp:162 DebugReportCallbackImpl(): Vulkan Diagnostics [ Validation ] [ COMMAND_BUFFER ] [OBJ: 12970367442591508160 LOC: 1611758864 ]: Validation Error: [ VUID-vkCmdBindDescriptorSets-pDescriptorSets-06715 ] Object 0: handle = 0xb400007815f566c0, type = VK_OBJECT_TYPE_COMMAND_BUFFER; Object 1: handle = 0x4a581c0000000195, type = VK_OBJECT_TYPE_DESCRIPTOR_SET; Object 2: handle = 0xa7baa9000000011c, type = VK_OBJECT_TYPE_BUFFER; | MessageID = 0x60117d10 | vkCmdBindDescriptorSets(): pDynamicOffsets[0] is 256, but must be zero since the buffer descriptor's range is VK_WHOLE_SIZE in descriptorSet #0 binding #0 descriptor[0]. The Vulkan spec states: For each dynamic uniform or storage buffer binding in pDescriptorSets, if the range was set with VK_WHOLE_SIZE then pDynamicOffsets which corresponds to the descriptor binding must be 0 (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkCmdBindDescriptorSets-pDescriptorSets-06715)
2024-03-17 20:23:42.769  3921-3921  DEBUG                   crash_dump64                         A        #03 pc 0000000002cce60c  /data/app/~~RWPsmHyq49EsbybiZ_Gaqg==/app.organicmaps.debug-yAxP1YLffOynlrwj4LPA6A==/lib/arm64/liborganicmaps.so (dp::vulkan::DebugReportCallbackImpl(unsigned int, VkDebugReportObjectTypeEXT, unsigned long, unsigned long, int, char const*, char const*, void*)+360) (BuildId: 2ceb1c71e8ac56a9b0c7f7648978929f5aba453e)
2024-03-17 20:23:42.769  3921-3921  DEBUG                   crash_dump64                         A        #09 pc 0000000002c5e3cc  /data/app/~~RWPsmHyq49EsbybiZ_Gaqg==/app.organicmaps.debug-yAxP1YLffOynlrwj4LPA6A==/lib/arm64/liborganicmaps.so (dp::vulkan::VulkanVertexArrayBufferImpl::RenderRange(ref_ptr<dp::GraphicsContext>, bool, dp::IndicesRange const&)+944) (BuildId: 2ceb1c71e8ac56a9b0c7f7648978929f5aba453e)

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few questions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files are downloaded by a script, right? Does it make sense to always exclude them from Release builds?

How slow is the debug build with validation enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation should not be published by performance reason, it's slow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in gradle, but it looks like this parameter can't be set for release only. I tried, but it affects on all the flavours.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it can be solved using flavors (e.g. a flavor with validation can be added and both debug and release builds can be tested). We can leave it for later.

Copy link
Contributor

@meenbeese meenbeese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit. See #7611

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it can be solved using flavors (e.g. a flavor with validation can be added and both debug and release builds can be tested). We can leave it for later.

Signed-off-by: renderexpert <expert@renderconsulting.co.uk>
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 this pull request may close these issues.

3 participants