Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option for using custom heaps and support D3D_FEATURE_LEVEL_1_0_CORE #469

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jeffbloo
Copy link
Contributor

No description provided.

@jeffbloo jeffbloo requested a review from jstoecker June 23, 2023 02:02
Comment on lines +47 to +54
Microsoft::WRL::ComPtr<ID3D12Resource> CreateBuffer(
uint64_t sizeInBytes,
D3D12_CPU_PAGE_PROPERTY cpuPageProperty = D3D12_CPU_PAGE_PROPERTY_WRITE_COMBINE,
D3D12_MEMORY_POOL memoryPoolPreference = D3D12_MEMORY_POOL_L0,
D3D12_RESOURCE_FLAGS resourceFlags = D3D12_RESOURCE_FLAG_ALLOW_UNORDERED_ACCESS,
uint64_t alignment = 0,
D3D12_HEAP_FLAGS heapFlags = D3D12_HEAP_FLAG_NONE);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Can you also delete the TODO on line 39? :)

}

if (data.empty())
{
// No need to create an upload resource if the source data is empty.
return defaultBuffer;
return buffer;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the heap is L0 and CPU-writable, there's no need to create the extra upload heap.

Suggested change
if (cpuPageProperty == D3D12_CPU_PAGE_PROPERTY_WRITE_COMBINE && memoryPoolPreference == D3D12_MEMORY_POOL_L0)
{
// Map buffer directly, if possible.
void* bufferData = nullptr;
THROW_IF_FAILED(buffer->Map(0, nullptr, &bufferData));
memcpy(bufferData, data.data(), data.size());
buffer->Unmap(0, nullptr);
return buffer;
}

Comment on lines +103 to +104
D3D12_CPU_PAGE_PROPERTY cpuPageProperty = D3D12_CPU_PAGE_PROPERTY_WRITE_COMBINE,
D3D12_MEMORY_POOL memoryPoolPreference = D3D12_MEMORY_POOL_L0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would default these to D3D12_MEMORY_POOL_UNKNOWN and D3D12_MEMORY_POOL_UNKNOWN, respectively, since custom heaps are opt-in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - thanks. This was an artifact of a previous approach.

}

std::vector<std::byte> Device::Download(Microsoft::WRL::ComPtr<ID3D12Resource> defaultBuffer)
std::vector<std::byte> Device::Download(Microsoft::WRL::ComPtr<ID3D12Resource> buffer)
Copy link
Contributor

@jstoecker jstoecker Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like upload, you can avoid the readback heap and just copy straight out of the buffer here if it's L0 and CPU-visible (call GetHeapProperties).

@@ -119,6 +119,11 @@ CommandLineArgs::CommandLineArgs(int argc, char** argv)
"Binds input and output resources are allocated from custom heaps. Write-combined caching and system memory (L0) are used when this is specified.",
cxxopts::value<bool>()
)
(
"g, set_stable_power_state",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "g" for set_stable_power_state? I'd prefer to reserve letters for common command line options where the letter is similar to the full option name.

@@ -233,6 +233,11 @@ void Executor::operator()(const Model::DispatchCommand& command)
{
Timer loopTimer, iterationTimer, bindTimer, dispatchTimer;

if (m_commandLineArgs.GetSetStablePowerState())
{
THROW_IF_FAILED(m_device->D3D()->SetStablePowerState(TRUE));
Copy link
Contributor

@jstoecker jstoecker Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of throwing, can you log a warning or error that dev mode needs to be enabled?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants