[PATCH v2 2/2] vkd3d: Allocate one large buffer for a heap and offset into it.

[Henri Verbeet]

On Wed, 23 Oct 2019 at 14:51, Hans-Kristian Arntzen
<post at 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 at 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.