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

[Henri Verbeet]

On Tue, 8 Oct 2019 at 12:00, Hans-Kristian Arntzen
 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().
> @@ -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?
> @@ -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)".