Refactor descriptor heaps in D3D12 driver.#113244
Refactor descriptor heaps in D3D12 driver.#113244akien-mga merged 1 commit intogodotengine:masterfrom
Conversation
blueskythlikesclouds
force-pushed
the
d3d12-descriptor-refactor
branch
from
c29b98a to
07e96f9
Compare
blueskythlikesclouds
force-pushed
the
d3d12-descriptor-refactor
branch
4 times, most recently
from
8055a8d to
1164064
Compare
clayjohn
left a comment
clayjohn
left a comment
There was a problem hiding this comment.
I'm not competent enough with D3D12 to do a qualified review. But I have done a cursory red flag review and everything looks okay. I am just a bit hesitant about deleting the old project settings.
| custom_prop_info["rendering/rendering_device/d3d12/max_sampler_descriptors_per_frame"] = PropertyInfo(Variant::INT, "rendering/rendering_device/d3d12/max_sampler_descriptors_per_frame", PROPERTY_HINT_RANGE, "256,2048"); | ||
| GLOBAL_DEF_RST("rendering/rendering_device/d3d12/max_misc_descriptors_per_frame", 512); | ||
| custom_prop_info["rendering/rendering_device/d3d12/max_misc_descriptors_per_frame"] = PropertyInfo(Variant::INT, "rendering/rendering_device/d3d12/max_misc_descriptors_per_frame", PROPERTY_HINT_RANGE, "32,4096"); | ||
| GLOBAL_DEF_RST("rendering/rendering_device/d3d12/max_resource_descriptors", 65536); |
There was a problem hiding this comment.
These changes will break compatibility unfortunately. Probably not in a way that matters, but I am not 100% sure this is safe to do.
@akien-mga Do you have a better sense of whether deleting project settings like this will break projects? Alternatively, do we have a way of deprecating project settings gracefully?
There was a problem hiding this comment.
In general, removing project setting should not break projects on its own.
But if this could be set to some value (e.g., higher limit) that would not work with new default, old property still can be read by compatibility code:
#ifndef DISABLE_DEPRECATED
if (ProjectSettings::get_singleton()->has_setting("rendering/rendering_device/d3d12/max_resource_descriptors_per_frame")) {
int64_t limit = GLOBAL_GET("rendering/rendering_device/d3d12/max_resource_descriptors_per_frame");
// adjust curent limit and pring warning
}
#endifThere was a problem hiding this comment.
The meaning of both values are very incompatible since one was designed around current frame usage and the new one covers global application allocation, so I think the right move here is indeed to remove it like Skyth did. This is one of those values you'd only increase if your project ran into issues (and it's a reasonable limit by default to not overallocate excessively) and it should be very apparent for the user if it's necessary.
There was a problem hiding this comment.
Thank you both for the clarification!
I second what Dario says. This should be treated as a removal + addition of new settings, not a replacement of an old setting with a new one where the values should be carried over.
DarioSamo
left a comment
DarioSamo
left a comment
There was a problem hiding this comment.
Nothing jumped out to me as obviously wrong during the review, but figuring out any deeper issues in this will require actually running into the scenarios. I find the new design to be much clearer however, so we should go ahead with this.
| custom_prop_info["rendering/rendering_device/d3d12/max_sampler_descriptors_per_frame"] = PropertyInfo(Variant::INT, "rendering/rendering_device/d3d12/max_sampler_descriptors_per_frame", PROPERTY_HINT_RANGE, "256,2048"); | ||
| GLOBAL_DEF_RST("rendering/rendering_device/d3d12/max_misc_descriptors_per_frame", 512); | ||
| custom_prop_info["rendering/rendering_device/d3d12/max_misc_descriptors_per_frame"] = PropertyInfo(Variant::INT, "rendering/rendering_device/d3d12/max_misc_descriptors_per_frame", PROPERTY_HINT_RANGE, "32,4096"); | ||
| GLOBAL_DEF_RST("rendering/rendering_device/d3d12/max_resource_descriptors", 65536); |
There was a problem hiding this comment.
The meaning of both values are very incompatible since one was designed around current frame usage and the new one covers global application allocation, so I think the right move here is indeed to remove it like Skyth did. This is one of those values you'd only increase if your project ran into issues (and it's a reasonable limit by default to not overallocate excessively) and it should be very apparent for the user if it's necessary.
|
Needs rebase. |
clayjohn
left a comment
clayjohn
left a comment
There was a problem hiding this comment.
My only concern was whether removing a project setting would break compatibility, since it doesn't (for the reasons explained by Dario) we should go ahead with this and remove the old project settings without trying to convert the values since the meaning of the new settings is different
blueskythlikesclouds
force-pushed
the
d3d12-descriptor-refactor
branch
from
1164064 to
a8d3ece
Compare
|
Thanks! |
6 participants
This PR refactors descriptor heaps in the D3D12 driver to simplify the code and make it closer to how Vulkan works.
Previously, uniform sets were first written into CPU visible descriptor heaps and then copied into GPU visible heaps at command recording time. This caused unnecessary CPU overhead, especially when using pipelines with incompatible root signatures.
The new approach mimicks Vulkan's behavior. Each shader now produces a complete root signature, regardless of which resources the DXIL actually uses. This guarantees descriptor ranges to be compatible across shaders, similar to how Vulkan pipeline layout compatibility works. Uniform sets allocate space directly from the GPU visible heap at creation time, and descriptors are written to it directly.
At binding time, all that needs to be done is just bind the GPU descriptor handles, since all the descriptor ranges are already compatible. This saves a lot of CPU time.
Dynamic uniform and storage buffers are implemented using root descriptors so their GPU virtual addresses can be bound directly at command recording time to avoid copying descriptors.
RTV/DSV heap pooling has been replaced with the same D3D12MA virtual block allocator used for the GPU visible descriptor heaps. The pooling has to stay due to NVIDIA being unable to allocate more than 4096 descriptor heaps.
Read-only storage textures are no longer converted to SRVs. There doesn't seem to be a real performance benefit on desktop GPUs, and it complicates descriptor heap management. Enhanced barriers were also actually incorrectly transitioning to UAV layout when the shader was expecting SRV, which is no longer the case with this PR.