On Wed, 23 Oct 2019 at 13:47, Hans-Kristian Arntzen post@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.
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?