Vulkan: Render graph core #120427
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#120427
Loading…
Reference in New Issue
No description provided.
Delete Branch "Jeroen-Bakker/blender:vulkan/render-graph-core"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Design Task: blender/blender#118330
This PR adds the core of the render graph. The render graph isn't used.
Current implementation of the Vulkan Backend is slow by design. We
focused on stability, before performance. With the new introduced render
graph the focus will shift to performance and keep the stability at where
it is.
Some highlights:
VKRenderGraph
).VKResourceStateTracker
).All nodes in the graph is executed in the order they were added. (
VKScheduler
).or writes to (output links)
track of which nodes needs which stamp of a resource.
and image layout (for image resources) are stored. This allows the render graph
to find out how a resource was used in the past and will be used in the future.
That is important to construct pipeline barriers that don't stall the whole GPU.
Defined nodes
This implementation has nodes for:
Each node has a node info, create info and data struct. The create info
contains all data to construct the node, including the links of the graph.
The data struct only contains the data stored inside the node. The node info
contains the node specific implementation.
Resources
Before a render graph can be used, the resources should be registered
to
VKResourceStateTracker
. In the final implementation this will be owned bythe
VKDevice
. Registration of resources can be done by callingVKResources.add_buffer
orVKResources.add_image
.Render graph
Nodes can be added to the render graph. When adding a node its read/
write dependencies are extracted and converted into links (
VKNodeInfo. build_links
).When the caller wants to have a resource up to date the functions
VKRenderGraph.submit_for_read
orVKRenderGraph.submit_for_present
can be called.
These functions will select and order the nodes that are needed
and convert them to
vkCmd*
commands. These commands include pipelinebarrier and image layout transitions.
The
vkCmd
are recorded into a command buffer which is sent to thedevice queue.
Walking the graph
Walking the render graph isn't implemented yet. The idea is to have a
Map<ResourceWithStamp, Vector<NodeHandle>> consumers
andMap<ResourceWithStamp, NodeHandle> producers
. These attributes canbe stored in the render graph and created when building the links, or
can be created inside the VKScheduler as a variable. The exact detail
which one would be better is unclear as there aren't any users yet. At
the moment the scheduler would need them we need to figure out the best
way to store and retrieve the consumers/producers.
Unit tests
The render graph can be tested by enabling
WITH_GTEST
and usevk_render_graph
as a filter.WIP: Vulkan: Render graph coreto Vulkan: Render graph coreI still believe all tests should be in their own test folder in the
vulkan/render_graph/test
. This matches the vast majority of the other modules inside blender source folder.Also add a diagram to explain how it is the node resources that are containing links of the graph and not the node themselves.
@ -0,0 +17,4 @@
namespace blender::gpu::render_graph {
/**
* Base class for node class.
I think
Class
is redundant in the name. We should rename it toVKNodeInfo
.Then move the
VKNodeType
inside this class. So it would becomeVKNodeInfo::Type
. However, it might be tricky with the templates.::Type
requires a concrete type to access, or put concrete part in a subclass. I keptVKNodeType
to not confuse other areas in the code@ -0,0 +39,4 @@
VKBoundPipelines active_pipelines;
VkPipelineStageFlags src_stage_mask_ = VK_PIPELINE_STAGE_NONE;
VkPipelineStageFlags dst_stage_mask_ = VK_PIPELINE_STAGE_NONE;
active_pipelines
andsrc/dst_stage_mask_
are state tracking members. Put them in a sstate
structure for clarity.@ -0,0 +50,4 @@
* Needs to be called before each build_image/buffer. It ensures that swapchain images are reset
* to the correct layout and that the pipelines are reset.
*/
void reset(VKRenderGraph &render_graph);
Move it to private and call it inside
build_image/buffer
.@ -0,0 +53,4 @@
void reset(VKRenderGraph &render_graph);
/**
* Build the commands to update the given vk_image to the last version
last version
is ambiguous. Use whatever naming you will choose in place ofResourceVersion
.Also dont forget
.
. ;)@ -0,0 +55,4 @@
/**
* Build the commands to update the given vk_image to the last version
*/
void build_image(VKRenderGraph &render_graph, VkImage vk_image);
Put the command buffer as additionnal "out" parameter. So it is clearer were the commands are built.
@ -0,0 +64,4 @@
/**
* After the commands have been submitted the nodes that have been send to the GPU can be
* removed.
Make it clear when this is called and what purpose it fills.
@ -0,0 +29,4 @@
* prefetched and removing a level of indirection. A consequence is that we cannot use class based
* nodes.
*/
struct VKNodeData {
Rename to
VKNode
. This is the actual data that is put inside the graph.@ -0,0 +31,4 @@
break;
}
memset(&node_data, 0, sizeof(VKNodeData));
Is that needed? Feel like paranoid clear. If you initialize the node correctly (clear on allocation) this shouldn't be needed.
VKNode instances are kept around and can be reused. This is clarified in the documentation. In order to make it clear to developer during debugging the state is reset in this case.
@ -0,0 +19,4 @@
class VKCommandBuilder;
class VKNodes {
This class is never used anywhere else than inside the render graph and is very limited in complexity. The real only function that it contains is
add_node
. I would suggest to merge it inside the rendergraph.@ -0,0 +5,4 @@
/** \file
* \ingroup gpu
*
* Render graph is a render solution that is able to track resource usages in a single submission
I think this description is too shallow. It only refers to one aspect of the graph.
The render graph primarily is a a graph of GPU commands that are then serialized into command buffers. The submission order can be altered and barriers are added for resource sync.
@ -0,0 +37,4 @@
class VKScheduler;
class VKRenderGraph : public NonCopyable {
VKResourceDependencies resource_dependencies_;
Small description of members.
@ -0,0 +46,4 @@
/**
* Not owning pointer to device resources.
*
* Is marked optional as device could
Unfinished comment. Also out of date?
@ -0,0 +56,4 @@
/**
* Free all resources held by the render graph.
*/
void deinit();
Since the graph is now context based, this function shouldn't be necessary? Otherwise it should become the destructor.
Incorrect. Has to do what Blender/window manager does when exiting. I documented and renamed the method to free_data.
@ -0,0 +63,4 @@
* Add a node to the render graph.
*/
template<typename NodeClass, typename NodeCreateInfo>
void add_node(const NodeCreateInfo &create_info)
Maybe it is worth trying a solution to avoid the boiler plate code below. But not really important.
Boiler plating is still needed, but have been minimized by extracting the CreateInfo from the NodeInfo.
@ -0,0 +68,4 @@
std::scoped_lock lock(resources_.mutex_get());
NodeHandle handle = nodes_.add_node<NodeClass, NodeCreateInfo>(create_info);
NodeClass node_class;
node_class.build_resource_dependencies(
Maybe explain here why you need to use a instance of a class just to call a function that could be static.
@ -0,0 +107,4 @@
}
/**
* Submit the commands to readback the given vk_buffer to the command queue.
Add precision about what this function will do in the future.
Submit partial graph to be able to read the expected result of the rendering commands affecting the given vk_buffer
.@ -0,0 +122,4 @@
void submit_for_present(VkImage vk_swapchain_image);
private:
friend class VKCommandBuilder;
Put on the top
@ -0,0 +3,4 @@
* SPDX-License-Identifier: GPL-2.0-or-later */
/** \file
* \ingroup gpu
Add comment
@ -0,0 +15,4 @@
#include "vk_resources.hh"
#include "vk_types.hh"
namespace blender::gpu::render_graph {
The name
VKResourceDependencies
could be change to a container nameVKResourceLinkPool
or something alike.@ -0,0 +16,4 @@
#include "vk_types.hh"
namespace blender::gpu::render_graph {
class VKResourceDependencies : NonCopyable, NonMovable {
Missing a description of what this class is. Either on top of the file or on the class.
@ -0,0 +18,4 @@
namespace blender::gpu::render_graph {
class VKResourceDependencies : NonCopyable, NonMovable {
public:
struct ResourceUsage {
Make it clear that this is the link inside the graph. Consider renaming it to
ResourceLink
or something.@ -0,0 +19,4 @@
* List for working with handles and items.
* Reference to the first empty slot is stored internally to stop iterating over all the elements.
*/
template<typename Handle, typename Item> class VKResourceHandles {
So it is used by Nodes and Resources. This needs to be clear this is a general purpose class. Find a better name.
@ -0,0 +23,4 @@
private:
const int64_t grow_size_ = 64;
Handle first_empty_slot_ = 0;
Vector<std::optional<Item>> items_;
For speed, prefer using a
BitVector
to store the free items. Also check with geometry node team if a similar structure exists (and potentially implement it in BLI).@ -0,0 +3,4 @@
* SPDX-License-Identifier: GPL-2.0-or-later */
/** \file
* \ingroup gpu
Description!!!
@ -0,0 +36,4 @@
* The resource can be destroyed by the application
* when it isn't used anymore.
*/
APPLICATION,
Mention that in practice this is every resources but swapchain images.
@ -0,0 +56,4 @@
* images. Each resource can have multiple versions; every time a resource is changed (written to)
* a new version is tracked.
*/
class VKResources {
This is a container for resource handles. It should be better named.
@ -0,0 +65,4 @@
* pixel layout on the device.
*/
struct Resource {
VkBuffer vk_buffer = VK_NULL_HANDLE;
Prefer a union + type enum.
@ -0,0 +76,4 @@
/**
* Current version of the resource in the graph.
*/
ResourceVersion version = 0;
The concept of a version is a bit abstract. In fact this is just a modification counter/timestamp. So maybe find a better name.
@ -0,0 +138,4 @@
VersionedResource get_buffer_and_increase_version(VkBuffer vk_buffer);
/**
* Return the current version of the resource.
The documentation of these functions should mention their usage rather than repeating w
hat they communicate with their name.
@ -0,0 +178,4 @@
*/
std::mutex &mutex_get()
{
return mutex_;
Make it public.
@ -0,0 +193,4 @@
/* When a command buffer is reset the resources are re-synced. During the syncing the command
* builder attributes are resized to reduce reallocations. */
friend class VKCommandBuilder;
Move to top
@ -0,0 +26,4 @@
* This scheduler selects all nodes in the order they were added to the render graph.
*
* This is an initial implementation and should be enhanced for:
* - Moving data transfer and compute before drawing, when they are scheduled between drawing nodes
Add that (in the future) it should prune branches of the graph that are not linked to anything.
@ -0,0 +3,4 @@
* SPDX-License-Identifier: GPL-2.0-or-later */
/** \file
* \ingroup gpu
Maybe try to disolve this file into the other headers.
@ -0,0 +33,4 @@
VKBoundPipeline graphics;
};
// TODO: This is a copy constructor in disguise.
What is this TODO?
@blender-bot build
How are you going to walk over the graph from node to resource to node?
Now,
Link
doesn't have any reference of the writer (or reader) node and you don't have links perResourceWithStamp
.@ -0,0 +1,214 @@
/* SPDX-FileCopyrightText: 2024 Blender Authors
For consistency, the file should be called
vk_render_graph_node.hh
@ -0,0 +32,4 @@
/**
* Class containing all links inside the render graph.
*/
class VKRenderGraphLinks : NonCopyable, NonMovable {
I still think we should get rid of this class and move everything that is inside to the
rendergraph
namespace. This class only exists for use with thebuild_links
which effectively should get the whole graph anyway or just return a vector of links.I will see what can be done and where the cyclic include will go to. The point of splitting up has a technical reason when using header only implementations.
You're proposing to use:
RenderGraph -> Node -> NodeInfo -> RenderGraph which introduces a cyclic include. The current implementation fixes that by adding the links to a separate header file. A fix would be to create compile units for all Nodes and remove all the template stuff. Might be fine, but is more work then and will introduce more files...
Another consequence is that the RenderGraph API will contain functions about building links, which might trigger developers that they are responsible for adding them as well.
For walking the nodes, this can be solved inside the render graph or scheduler
Map<ResourceWithStamp, NodeHandle> resource_producers_;
andMap<ResourceWithStamp, Vector<NodeHandle>> resource_consumers_;
. At this moment it is to soon to select the best location and is not needed until we implement more advanced scheduler mechanism. I will add a note and point out when we want to address this.@blender-bot build
@blender-bot build
Issue with tests are related to how hexidecimal numbers are formatted.
@blender-bot build
@blender-bot build
@blender-bot build
@blender-bot build
@ -0,0 +76,4 @@
VKRenderGraphLink link = {};
link.resource = resource_handle;
link.vk_access_flags = vk_access_flags;
link.vk_image_layout = vk_image_layout;
Prefer
node_links.inputs.append({resource_handle, vk_access_flags, vk_image_layout})
as I think this can more easily be built in place instead of a copy. The members are strongly typed so I think this is fine.I think having the
add_input/output
functions are quite redundant, but if you want to keep them for documentation/clarity then it's fine.