FD3D12ContextCommon - Potentially Incorrect Closure

Development Rendering
, ,
1

Hello,

Between 5.5 and 5.6, FD3D12ContextCommon::CloseCommandList() changed the order of internal events causing FlushResourceBarriers() to be called prior to a check statement which ensured the commandlist was open.

Internally, FlushResourceBarriers() appears to open the commandlist if it’s currently closed, this breaks the order of events that would occur during opening/closure of an a derived commandlist. FD3D12CommandContext inherits from FD3D12ContextCommon and will perform the following operations if CloseCommandList() is called on an unopened list, with pending barriers:

1. FD3D12CommandContext::CloseCommandList()

a. FD3D12DescriptorCache::CloseCommandList() is called

b. FD3D12ContextCommon::CloseCommandList() is called

2. FD3D12ContextCommon::CloseCommandList() calls FlushResourceBarriers() which will then try to open the commandlist

3. FD3D12CommandContext::OpenCommandList()

a. FD3D12ContextCommon::OpenCommandList() is called

b. FD3D12DescriptorCache::OpenCommandList() is called

4. FD3D12ContextCommon::CloseCommandList() completes

The result is that the descriptor cache and any subsequent dependencies are left in an ‘open’ state while the underlying commandlist is closed.

We’ve spotted this in our version of the engine as we have dependent code that is reliant on Open/Close occuring in the correct order. We’ve also applied a fix locally, though are open to better options:

void FD3D12ContextCommon::CloseCommandList()
{
	checkf(IsPendingCommands(), TEXT("The command list is empty."));

	// <#CCP_MOD>
	// CommandList has pending resource barriers still
	// Open/Close here so that inherited closures occur in order.
	if (IsPendingCommands() && !IsOpen())
	{
		OpenCommandList();
		CloseCommandList();
 
		return;
	}
	// </#CCP_MOD>
	
	// Do this before we insert the final timestamp to ensure we're timing all the work on the command list.
	// If the command list only has barrier work to do, this will open the command list for the first time
	FlushResourceBarriers();
	...

Thanks,

Jared

2

Hi Jared,

thanks for reporting this, the low level barriers change for DX12 from 5.5 to 5.6 was significant, the code improved even more in 5.7 with enhanced barriers support.

We’ll check if your fix still apply and if we still need that and what is the best option for 5.6.

Thanks,

Daniele

3

Hey Jared,

the problem is still in 5.8, to fix it we are thinking of switching in FD3D12CommandContext::CloseCommandList the order of the StateCache::CloseCommandlist and FD3D12ContextCommon::CloseCommandList, something like this:

...
void FD3D12CommandContext::CloseCommandList()
{
	// FD3D12ContextCommon::CloseCommandList call FlushResourceBarriers that might actually Open the command list
	FD3D12ContextCommon::CloseCommandList();
 
	StateCache.GetDescriptorCache()->CloseCommandList();
...

would this address the order in your case too?

4

Thanks for verifying it Jared, from the code analysis and the test we made we think it is safe to invert the order, the change is submitted in Main in CL 48687062.

5

Hi Daniele,

Your suggested change also works. I wasn’t sure whether it was ideal just because there might be things in the DescriptorCache closure that are dependent on the commandlist. However, if you’re happy that the change of order isn’t a problem, then it works for us as well.

Thanks,

Jared