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

[Hans-Kristian Arntzen]

I pushed a new patchset.

On 10/22/19 3:04 PM, Henri Verbeet wrote:
> On Tue, 8 Oct 2019 at 12:00, Hans-Kristian Arntzen
> <post at arntzen-software.no> wrote:
>> @@ -4076,6 +4076,7 @@ static void d3d12_command_list_set_root_cbv(struct d3d12_command_list *list,
>>
>>       resource = vkd3d_gpu_va_allocator_dereference(&list->device->gpu_va_allocator, gpu_address);
>>       buffer_info.buffer = resource->u.vk_buffer;
>> +
>>       buffer_info.offset = gpu_address - resource->gpu_address;
>>       buffer_info.range = resource->desc.Width - buffer_info.offset;
>>       buffer_info.range = min(buffer_info.range, vk_info->device_limits.maxUniformBufferRange);
> Stray whitespace change.
>
> 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.

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.

>
>> @@ -1334,6 +1334,8 @@ static HRESULT vkd3d_init_device_caps(struct d3d12_device *device,
>>       device->feature_options.CrossAdapterRowMajorTextureSupported = FALSE;
>>       /* SPV_EXT_shader_viewport_index_layer */
>>       device->feature_options.VPAndRTArrayIndexFromAnyShaderFeedingRasterizerSupportedWithoutGSEmulation = FALSE;
>> +
>> +    /* FIXME: Does this actually work on NV which has 64k bufferImage alignment quirks with VkDeviceMemory? */
>>       device->feature_options.ResourceHeapTier = D3D12_RESOURCE_HEAP_TIER_2;
> Well, does it?
Removed the comment, but there should be some rationale why vkd3d does 
this. NV has bufferImageGranularity 64k, and if apps place buffers and 
images within that alignment, there is implicit memory aliasing. 
Although, even if TIER_1 is advertised, apps don't really seem to care 
and create heaps with all uses enabled anyways. I'll just leave it until 
I see a real app having problems with this.
>
>> @@ -546,6 +557,10 @@ static HRESULT d3d12_heap_init(struct d3d12_heap *heap,
>>       VkDeviceSize vk_memory_size;
>>       HRESULT hr;
>>       int rc;
>> +    bool buffers_allowed;
>> +    D3D12_RESOURCE_DESC resource_desc;
>> +    D3D12_RESOURCE_STATES initial_resource_state;
>> +    const struct vkd3d_vk_device_procs *vk_procs = &device->vk_procs;
> "vk_procs" is unused.
>
>> @@ -1003,13 +1078,16 @@ static void d3d12_resource_destroy(struct d3d12_resource *resource, struct d3d12
>>       if (resource->flags & VKD3D_RESOURCE_EXTERNAL)
>>           return;
>>
>> -    if (resource->gpu_address)
>> -        vkd3d_gpu_va_allocator_free(&device->gpu_va_allocator, resource->gpu_address);
>> +    if (!(resource->flags & VKD3D_RESOURCE_PLACED_BUFFER))
>> +    {
>> +        if (resource->gpu_address)
>> +            vkd3d_gpu_va_allocator_free(&device->gpu_va_allocator, resource->gpu_address);
>>
>> -    if (d3d12_resource_is_buffer(resource))
>> -        VK_CALL(vkDestroyBuffer(device->vk_device, resource->u.vk_buffer, NULL));
>> -    else
>> -        VK_CALL(vkDestroyImage(device->vk_device, resource->u.vk_image, NULL));
>> +        if (d3d12_resource_is_buffer(resource))
>> +            VK_CALL(vkDestroyBuffer(device->vk_device, resource->u.vk_buffer, NULL));
>> +        else
>> +            VK_CALL(vkDestroyImage(device->vk_device, resource->u.vk_image, NULL));
>> +    }
>>
>>       if (resource->flags & VKD3D_RESOURCE_DEDICATED_HEAP)
>>           d3d12_heap_destroy(resource->heap);
> 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.

Cheers,
Hans-Kristian