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@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));elseVK_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