test_resolve_non_issued_query_data has a WaW hazard between CopyResource and ResolveQueryData

vkd3dclosed

HansKristian-Work/vkd3d-proton#963 · opened 2022-01-11 by werman · updated 2022-02-28 · 4 comments · github

Wwerman 2022-01-11 github

    ID3D12GraphicsCommandList_EndQuery(command_list, query_heap, D3D12_QUERY_TYPE_TIMESTAMP, 0);
    ID3D12GraphicsCommandList_CopyResource(command_list, readback_buffer, upload_buffer);
    ID3D12GraphicsCommandList_EndQuery(command_list, query_heap, D3D12_QUERY_TYPE_TIMESTAMP, 3);
    ID3D12GraphicsCommandList_ResolveQueryData(command_list, query_heap,
            D3D12_QUERY_TYPE_TIMESTAMP, 0, 4, readback_buffer, 0);

I'm not sure about D3D12 but VK to avoid WaW hazard would expect a memory barrier between vkCmdCopyBuffer and vkCmdCopyQueryPoolResults. There is a test in Crucible that is based on test_resolve_non_issued_query_data but has a barrier.

At the moment test_resolve_non_issued_query_data fails on Turnip.

Ddoitsujin maintainer 2022-01-12 github

This is not an issue with the test but rather our implementation. D3D12 does not support anything like TRANSFER->TRANSFER barriers so that needs to be taken care of by the driver.

What's missing exactly on the Vulkan side? I'm guessing the CopyResource stuff doesn't finish before ResolveQueryData starts?

HHansKristian-Work maintainer 2022-01-12 github

Ideally, we should probably do basic hazard tracking for overlapping TRANSFERs to buffers. Especially with queries I would expect apps to do lots of back to back resolves on the same target buffers. Could probably keep it as simple as possible, detecting the obvious patterns, and be conservative otherwise.

Wwerman 2022-01-12 github

What's missing exactly on the Vulkan side? I'm guessing the CopyResource stuff doesn't finish before ResolveQueryData starts?

In Turnip vkCmdCopyQueryPoolResults happens through the different means than other transfer operations, so the result of vkCmdCopyBuffer could stuck in cache and therefor happen after vkCmdCopyQueryPoolResults. Thus we need a barrier to flush the cache.

So yes, we need TRANSFER->TRANSFER barrier.

HHansKristian-Work maintainer 2022-02-28 github

This is fixed.

Nothing extracted yet.