godotengine / godotengine/godot

Godot never calls vmaFlushAllocation nor vmaInvalidateAllocation

Open
#103,589 0 comments 0 reactions 0 assignees View on GitHub
bug topic:rendering
Dominant language
C++
Stars
117k
Forks
26.8k
PR merge metrics
PR metrics pending

Description

### Tested versions

- Reproducible in: 4.x.x

### System information

Godot v4.4.beta (ba21e8890) - Ubuntu 24.04.2 LTS 24.04 on X11 - X11 display driver, Multi-window, 1 monitor - Vulkan (Mobile) - integrated AMD Radeon Graphics (RADV RENOIR) - AMD Ryzen 7 5700U with Radeon Graphics (16 threads)

### Issue description

While adding UMA support to Godot I realized I wasn't calling `vmaFlushAllocation` (or its raw forms vkFlushMappedMemoryRanges/vkInvalidateMappedMemoryRanges).

These functions **must** be called when writing to /reading from `HOST_VISIBLE` memory that is not `HOST_COHERENT`.

This is not a problem on PC because none of the exposed memory types by the 3 major vendors (AMD, NVIDIA, Intel) expose memory without `HOST_COHERENT`.

However this is a problem on Android as some devices may not have `HOST_COHERENT` (or they do, but VMA didn't prefer it).

This could explain a lot of the weird bugs and crashes on Android. Failure to call flush/invalidate causes randomly to see old data (including uninitialized data) instead of the intended value. You can think of this as a rare form of race condition, although it's technically not one.

`vmaFlushAllocation` must be called after we're done writing from CPU to a GPU pointer. Right now this exclusively means Staging Buffers like the ones in `RenderingDevice::buffer_update` (and possibly buffer_clear?) + whatever is used for uploading to textures.

`vmaInvalidateAllocation` must be called before reading a pointer that was written from GPU. Right now this means functions like `RenderingDevice::buffer_get_data` & `RenderingDevice::buffer_get_data_async` + whatever is used to read from textures.

VMA shows an example on how to use them, I'm simplifying it as it applies to Godot:

```cpp
VkMemoryPropertyFlags memPropFlags;
vmaGetAllocationMemoryProperties(allocator, alloc, &memPropFlags);

memcpy(allocInfo.pMappedData, myData, myDataSize);
if(!(memPropFlags & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT)) {
vmaFlushAllocation(allocator, allocInfo., 0, VK_WHOLE_SIZE);
}
```

Though given the nature of the render graph, it'd be best if we can just flush everything before initiating all the vkCmdCopyBuffer that have the same src buffer, instead of doing one flush per vkCmdCopyBuffer.

Note that unless we're using `VK_WHOLE_SIZE`, the values must be aligned to `nonCoherentAtomSize`. VMA says they automatically take care of that.

The solution is often simple and trivial, but in my experience there's a tendency to have some details that require testing on several actual devices until you get them right.

For OgreNext [I implemented a Unit Test](https://github.com/OGRECave/ogre-next/blob/5b8aeaa1b41526c18160c5c2b4b98560bd97acda/Samples/2.0/Tests/Readback/ReadbackGameState.cpp#L219-L255) that runs indefinitely trying to identify this issue. It consists of a tight loop running as fast as possible (e.g. 30 fps, 100 fps, 1000 fps) that uploads a colour, renders it to screen and then we download the result.
We check all pixels are of the exact same colour. Failure means there's something wrong with upload or download.

If there are bugs, usually the test fails within the first 3 seconds.

**Update:** I just realized / failed to notice that Godot always requests `HOST_COHERENT` as required:

```cpp
alloc_create_info.requiredFlags = (VK_MEMORY_PROPERTY_HOST_COHERENT_BIT | VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT);
```

This is odd, because IIRC some Android GPUs do not have `HOST_COHERENT` at all. That might explain why it fails to start on some devices?

**Update 2:** Ok looking at vulkan.gpuinfo.org it appears all the relevant GPUs do have `HOST_COHERENT` available. However [they](https://vulkan.gpuinfo.org/displayreport.php?id=25935#memory) have this [oddity](https://vulkan.gpuinfo.org/displayreport.php?id=11015#memory):

- Memory that is fast for CPU -> GPU upload has `HOST_COHERENT`
- Memory that is fast for GPU -> CPU download does not have have `HOST_COHERENT`, but we force it. This means bug #101905 will manifest on those devices.
- [Memory on Adreno](https://vulkan.gpuinfo.org/displayreport.php?id=25797#memory) 6xx that is fast for CPU -> GPU upload is faster if not using `HOST_COHERENT` (at least as recommended by the driver).

**Update 3:** Related discussion in #84852 but there were a few issues I didn't notice back then:

```cpp
void *buffer_mem;
VkResult vkerr = vmaMapMemory(allocator, tmp_buffer.allocation, &buffer_mem);
ERR_FAIL_COND_V_MSG(vkerr, Vector(), "vmaMapMemory failed with error " + itos(vkerr) + ".");

#ifdef __ANDROID__
vmaInvalidateAllocation(allocator, tmp_buffer.allocation, 0, VK_WHOLE_SIZE);
#endif

Vector buffer_data;
{
buffer_data.resize(p_size);
uint8_t *w = buffer_data.ptrw();
memcpy(w, buffer_mem, p_size);
}

#ifdef __ANDROID__
vmaFlushAllocation(allocator, tmp_buffer.allocation, 0, VK_WHOLE_SIZE);
#endif

vmaUnmapMemory(allocator, tmp_buffer.allocation);

_buffer_free(&tmp_buffer);
```

That code had two issues:

1. It didn't need to call `vmaFlushAllocation` at the end.
- You need to read? -> call vmaInvalidateAllocation before
- You need to write? -> call vmaFlushAllocation afterwards
2. It didn't check if the buffer was `HOST_COHERENT`, so it was being applied to all buffers indiscriminately.

### Steps to reproduce

N / A.

It affects mostly on Android, and varies on the model and driver.

### Minimal reproduction project (MRP)

N / A

Contributor guide

Open the contributing guide

Research direction

Start with the allocation setup containing alloc_create_info.requiredFlags, then trace RenderingDevice::buffer_update, buffer_clear, buffer_get_data, buffer_get_data_async, and texture upload/read paths. Check how host-visible, non-coherent allocations are handled before GPU copies or CPU reads. Done means the affected paths correctly handle the relevant memory properties and are validated on Android devices.

Written by the indexing model from the issue text.

Assessment

Tech stack
android, cpp
Domain
computer-graphics, mobile-dev
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.