On Wed, 23 Oct 2019 at 14:51, Hans-Kristian Arntzen post@arntzen-software.no wrote:
On 10/23/19 12:46 PM, Henri Verbeet wrote:
On Wed, 23 Oct 2019 at 13:47, Hans-Kristian Arntzen post@arntzen-software.no wrote:
Consistently applying heap_offset would be worse though, because it would be wrong in the case where placed buffers are not used. When looking up VA, you get the buffer for the heap.
Would it? For non-placed resources the heap offset should always be 0. Put a different way, after this patch the invariant is that the VkBuffer always starts at offset 0 of the underlying VkDeviceMemory allocation.
For placed resources before the patch, heap_offset was set to non-zero (see vkd3d_bind_heap_memory). The reason heap_offset was not needed then, was that each placed resource had it's own VK handle, and own VA allocation. Similar story now, except now only heap VkBuffer allocations can be looked up from VA allocator, but it's basically the exact same principle. GPU VA - Base VA for looked up resource gives the correct offset always.
If placed buffers have their own VA space and handle (like before this patch), you'll end up with something like:
Well yes, but isn't the entire point of this patch that they shouldn't? You'll also have issues with placed resources that alias each other.
- Heap is allocated
- Placed buffer is created at offset = 0x1000, heap_offset is also set
to 0x1000. VA = 0x10000 is allocated for the buffer.
- App tries to bind with VA = 0x10100
- Offset = VA - BaseVA = 0x100, this would be correct.
- If heap_offset is added, it's now wrong (0x10000 + 0x100 = 0x10100).
So, well, adding heap_offset in this patch will technically work (because it will always be 0), but it still feels kinda wrong to actually add it. Wouldn't just adding an assert be good enough?
I'd argue that adding the offset is the *correct* thing to do, and omitting it in cases where we know it will always be zero is an optimisation. That may still be ok though if we add a helper function with an appropriate comment to get the VkBuffer and offset from a D3D12_GPU_VIRTUAL_ADDRESS.
Yes, there is some confusion who owns the heap if DEDICATED_HEAP is used. The heap creates the resource in this case, and not the other way around. FWIW, I tried reverting back, but ended up with issues which would likely need some awkward flag to work around anyways, so PLACED_BUFFER is the best option I think.
What is the exact issue there?
if (resource->flags & VKD3D_RESOURCE_DEDICATED_HEAP) d3d12_heap_destroy(resource->heap); is called twice. The heap thinks it owns itself, and the resource thinks it owns the heap, because it's marked as owning the heap as well.
One way to solve that is to use "heap->is_private" in d3d12_resource_destroy(), instead of the VKD3D_RESOURCE_DEDICATED_HEAP flag. Another, slightly uglier, option is to do something like the following in d3d12_heap_destroy():
if ((resource = heap->buffer_resource)) { heap->buffer_resource = NULL; d3d12_resource_decref(resource); }
I think either would be cleaner than introducing an extra resource flag.