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

[Hans-Kristian Arntzen]

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:
>> On 10/22/19 3:04 PM, Henri Verbeet wrote:
>>> What I'm guessing happened here is that you changed the code to
>>> account for resource->heap_offset, but then realised it's always 0 for
>>> anything that has its own VA. That's convenient here, but also a
>>> little annoying to have to reason about; I'd probably prefer to
>>> consistently apply the heap offset anywhere we use a buffer offset.
>>> Worse, I think there are a few places that do need the heap offset
>>> applied, but don't in this patch. E.g.,
>>> d3d12_command_list_CopyTextureRegion(),
>>> d3d12_command_list_ResourceBarrier(),
>>> d3d12_command_list_ClearUnorderedAccessViewUint().
>> 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:

- 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 fixed the copy/clear commands, but I'm a bit concerned the test suite
>> did not catch this. I changed the UAV clear test to use placed resources
>> at least in a follow-up patch.
>>
> Support for placed resources is a relatively recent addition to vkd3d,
> so most of the tests currently use committed resources. It would be
> good to increase the coverage, but that's something to be aware of,
> yes.
>
>>> Do we really need VKD3D_RESOURCE_PLACED_BUFFER? It seems largely
>>> equivalent to "!(resource->flags & VKD3D_RESOURCE_DEDICATED_HEAP) &&
>>> d3d12_resource_is_buffer(resource)".
>> 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.

Cheers,
Hans-Kristian