From a96aabc6f65f463ebd74f7b437d58385625973fa Mon Sep 17 00:00:00 2001 From: Jason Fielder Date: Tue, 31 Oct 2023 16:45:12 +0100 Subject: [PATCH 1/3] Metal: Resolve premature memory release After previous changes to allow command buffers to not require execution and completion in submission order, guarantees for releasing freed buffers back to the memory pool within the frame life time had changed. This could mean a released buffer could be returned to the memory pool prematurely, if a subsequent command buffer completes before a previously submitted one, flagging a resource as no longer in use by the GPU, while it still may be in use by the orignal command buffer. This PR defers final reference count release for buffers being actively used until the following call to GPU_render_step, to ensure that buffers freed will be available for the lifetime of the frame, covering all command submissions, rather than just within the lifetime of the command buffer submission within which a buffer was freed. Authored by Apple: Michael Parkin-White Pull Request: https://projects.blender.org/blender/blender/pulls/114329 --- source/blender/gpu/metal/mtl_backend.mm | 1 - source/blender/gpu/metal/mtl_memory.hh | 3 +++ source/blender/gpu/metal/mtl_memory.mm | 9 +++++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/source/blender/gpu/metal/mtl_backend.mm b/source/blender/gpu/metal/mtl_backend.mm index 28f3466c477..d39ab394769 100644 --- a/source/blender/gpu/metal/mtl_backend.mm +++ b/source/blender/gpu/metal/mtl_backend.mm @@ -156,7 +156,6 @@ void MTLBackend::render_step() MTLContext::get_global_memory_manager()->get_current_safe_list(); if (cmd_free_buffer_list->should_flush()) { MTLContext::get_global_memory_manager()->begin_new_safe_list(); - cmd_free_buffer_list->decrement_reference(); } } diff --git a/source/blender/gpu/metal/mtl_memory.hh b/source/blender/gpu/metal/mtl_memory.hh index 5fab340c6cc..719c13ed327 100644 --- a/source/blender/gpu/metal/mtl_memory.hh +++ b/source/blender/gpu/metal/mtl_memory.hh @@ -418,6 +418,9 @@ class MTLBufferPool { std::atomic current_free_list_; std::atomic allocations_in_pool_; + /* Previous list, to be released after one full frame. */ + MTLSafeFreeList *prev_free_buffer_list_ = nullptr; + public: void init(id device); ~MTLBufferPool(); diff --git a/source/blender/gpu/metal/mtl_memory.mm b/source/blender/gpu/metal/mtl_memory.mm index ff972c25807..b2ddefe13fa 100644 --- a/source/blender/gpu/metal/mtl_memory.mm +++ b/source/blender/gpu/metal/mtl_memory.mm @@ -425,8 +425,17 @@ MTLSafeFreeList *MTLBufferPool::get_current_safe_list() void MTLBufferPool::begin_new_safe_list() { safelist_lock_.lock(); + MTLSafeFreeList *previous_list = prev_free_buffer_list_; + MTLSafeFreeList *active_list = get_current_safe_list(); current_free_list_ = new MTLSafeFreeList(); + prev_free_buffer_list_ = active_list; safelist_lock_.unlock(); + + /* Release final reference for previous list. + * Note: Outside of lock as this function itself locks. */ + if (previous_list) { + previous_list->decrement_reference(); + } } void MTLBufferPool::ensure_buffer_pool(MTLResourceOptions options) From 2893dc8ab7bbf9b2079c153edd307176b3e4210f Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Tue, 31 Oct 2023 17:16:48 +0100 Subject: [PATCH 2/3] Fix: Broken node tool group data-block references Currently the node tool node group and data-blocks referenced by it may not be part of the active dependency graph. This means we cannot retrieve their evaluated geometry when executing the operator. Since operators almost always use the evaluated geometry of other objects, and since geometry nodes is mostly set up to deal with evaluated data-blocks currently, this must be fixed. Instead, set up a temporary dependency graph and add the selected objects and the data-blocks used by the node group. That graph is evaluated to give simple access to evaluated data-blocks. Unfortunately this will cause more work than necessary in a few ways: 1. Selected objects are reevaluated an extra time before execution. 2. All data-blocks referenced by the group are completely evaluated again. 3. The node group itself is reevaluated, which recreates the function graph. These may or may not become bottlenecks in the future, but it's best to keep it simple late in the release process. And between a completely broken feature and a potentially slow feature, the choice is clear! Pull Request: https://projects.blender.org/blender/blender/pulls/114293 --- .../editors/geometry/node_group_operator.cc | 55 ++++++++-- source/blender/modifiers/intern/MOD_nodes.cc | 96 +---------------- .../nodes/NOD_geometry_nodes_execute.hh | 5 + .../nodes/intern/geometry_nodes_execute.cc | 100 ++++++++++++++++++ 4 files changed, 150 insertions(+), 106 deletions(-) diff --git a/source/blender/editors/geometry/node_group_operator.cc b/source/blender/editors/geometry/node_group_operator.cc index 3006377d9ce..75312c8ccdb 100644 --- a/source/blender/editors/geometry/node_group_operator.cc +++ b/source/blender/editors/geometry/node_group_operator.cc @@ -39,6 +39,7 @@ #include "DNA_scene_types.h" #include "DEG_depsgraph.hh" +#include "DEG_depsgraph_build.hh" #include "DEG_depsgraph_query.hh" #include "RNA_access.hh" @@ -167,6 +168,7 @@ static void store_result_geometry(Main &bmain, Object &object, bke::GeometrySet geometry) { + geometry.ensure_owns_direct_data(); switch (object.type) { case OB_CURVES: { Curves &curves = *static_cast(object.data); @@ -225,10 +227,34 @@ static void store_result_geometry(Main &bmain, } } +/** + * Create a dependency graph referencing all data-blocks used by the tree, and all selected + * objects. Adding the selected objects is necessary because they are currently compared by pointer + * to other evaluated objects inside of geometry nodes. + */ +static Depsgraph *build_depsgraph_from_indirect_ids(Main &bmain, + Scene &scene, + ViewLayer &view_layer, + const bNodeTree &node_tree_orig, + const Span objects) +{ + Set ids_for_relations; + bool needs_own_transform_relation = false; + nodes::find_node_tree_dependencies(node_tree_orig, ids_for_relations, needs_own_transform_relation); + + Vector ids; + ids.append(&node_tree_orig.id); + ids.extend(objects.cast()); + ids.insert(ids.size(), ids_for_relations.begin(), ids_for_relations.end()); + + Depsgraph *depsgraph = DEG_graph_new(&bmain, &scene, &view_layer, DAG_EVAL_VIEWPORT); + DEG_graph_build_from_ids(depsgraph, const_cast(ids.data()), ids.size()); + return depsgraph; +} + static int run_node_group_exec(bContext *C, wmOperator *op) { Main *bmain = CTX_data_main(C); - Depsgraph *depsgraph = CTX_data_ensure_evaluated_depsgraph(C); Scene *scene = CTX_data_scene(C); ViewLayer *view_layer = CTX_data_view_layer(C); Object *active_object = CTX_data_active_object(C); @@ -240,11 +266,24 @@ static int run_node_group_exec(bContext *C, wmOperator *op) } const eObjectMode mode = eObjectMode(active_object->mode); - const bNodeTree *node_tree = get_node_group(*C, *op->ptr, op->reports); - if (!node_tree) { + const bNodeTree *node_tree_orig = get_node_group(*C, *op->ptr, op->reports); + if (!node_tree_orig) { return OPERATOR_CANCELLED; } + uint objects_len = 0; + Object **objects = BKE_view_layer_array_from_objects_in_mode_unique_data( + scene, view_layer, CTX_wm_view3d(C), &objects_len, mode); + BLI_SCOPED_DEFER([&]() { MEM_SAFE_FREE(objects); }); + + Depsgraph *depsgraph = build_depsgraph_from_indirect_ids( + *bmain, *scene, *view_layer, *node_tree_orig, {objects, objects_len}); + DEG_evaluate_on_refresh(depsgraph); + BLI_SCOPED_DEFER([&]() { DEG_graph_free(depsgraph); }); + + const bNodeTree *node_tree = reinterpret_cast( + DEG_get_evaluated_id(depsgraph, const_cast(&node_tree_orig->id))); + const nodes::GeometryNodesLazyFunctionGraphInfo *lf_graph_info = nodes::ensure_geometry_nodes_lazy_function_graph(*node_tree); if (lf_graph_info == nullptr) { @@ -257,10 +296,6 @@ static int run_node_group_exec(bContext *C, wmOperator *op) return OPERATOR_CANCELLED; } - uint objects_len = 0; - Object **objects = BKE_view_layer_array_from_objects_in_mode_unique_data( - scene, view_layer, CTX_wm_view3d(C), &objects_len, mode); - OperatorComputeContext compute_context(op->type->idname); for (Object *object : Span(objects, objects_len)) { @@ -269,8 +304,8 @@ static int run_node_group_exec(bContext *C, wmOperator *op) } nodes::GeoNodesOperatorData operator_eval_data{}; operator_eval_data.depsgraph = depsgraph; - operator_eval_data.self_object = object; - operator_eval_data.scene = scene; + operator_eval_data.self_object = DEG_get_evaluated_object(depsgraph, object); + operator_eval_data.scene = DEG_get_evaluated_scene(depsgraph); bke::GeometrySet geometry_orig = get_original_geometry_eval_copy(*object); @@ -290,8 +325,6 @@ static int run_node_group_exec(bContext *C, wmOperator *op) WM_event_add_notifier(C, NC_GEOM | ND_DATA, object->data); } - MEM_SAFE_FREE(objects); - return OPERATOR_FINISHED; } diff --git a/source/blender/modifiers/intern/MOD_nodes.cc b/source/blender/modifiers/intern/MOD_nodes.cc index 28403304dc0..1a1263b096f 100644 --- a/source/blender/modifiers/intern/MOD_nodes.cc +++ b/source/blender/modifiers/intern/MOD_nodes.cc @@ -112,98 +112,6 @@ static void init_data(ModifierData *md) nmd->runtime->cache = std::make_shared(); } -static void add_used_ids_from_sockets(const ListBase &sockets, Set &ids) -{ - LISTBASE_FOREACH (const bNodeSocket *, socket, &sockets) { - switch (socket->type) { - case SOCK_OBJECT: { - if (Object *object = ((bNodeSocketValueObject *)socket->default_value)->value) { - ids.add(&object->id); - } - break; - } - case SOCK_COLLECTION: { - if (Collection *collection = ((bNodeSocketValueCollection *)socket->default_value)->value) - { - ids.add(&collection->id); - } - break; - } - case SOCK_MATERIAL: { - if (Material *material = ((bNodeSocketValueMaterial *)socket->default_value)->value) { - ids.add(&material->id); - } - break; - } - case SOCK_TEXTURE: { - if (Tex *texture = ((bNodeSocketValueTexture *)socket->default_value)->value) { - ids.add(&texture->id); - } - break; - } - case SOCK_IMAGE: { - if (Image *image = ((bNodeSocketValueImage *)socket->default_value)->value) { - ids.add(&image->id); - } - break; - } - } - } -} - -/** - * \note We can only check properties here that cause the dependency graph to update relations when - * they are changed, otherwise there may be a missing relation after editing. So this could check - * more properties like whether the node is muted, but we would have to accept the cost of updating - * relations when those properties are changed. - */ -static bool node_needs_own_transform_relation(const bNode &node) -{ - if (node.type == GEO_NODE_COLLECTION_INFO) { - const NodeGeometryCollectionInfo &storage = *static_cast( - node.storage); - return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE; - } - - if (node.type == GEO_NODE_OBJECT_INFO) { - const NodeGeometryObjectInfo &storage = *static_cast( - node.storage); - return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE; - } - - if (node.type == GEO_NODE_SELF_OBJECT) { - return true; - } - if (node.type == GEO_NODE_DEFORM_CURVES_ON_SURFACE) { - return true; - } - - return false; -} - -static void process_nodes_for_depsgraph(const bNodeTree &tree, - Set &ids, - bool &r_needs_own_transform_relation, - Set &checked_groups) -{ - if (!checked_groups.add(&tree)) { - return; - } - - tree.ensure_topology_cache(); - for (const bNode *node : tree.all_nodes()) { - add_used_ids_from_sockets(node->inputs, ids); - add_used_ids_from_sockets(node->outputs, ids); - r_needs_own_transform_relation |= node_needs_own_transform_relation(*node); - } - - for (const bNode *node : tree.group_nodes()) { - if (const bNodeTree *sub_tree = reinterpret_cast(node->id)) { - process_nodes_for_depsgraph(*sub_tree, ids, r_needs_own_transform_relation, checked_groups); - } - } -} - static void find_used_ids_from_settings(const NodesModifierSettings &settings, Set &ids) { IDP_foreach_property( @@ -259,9 +167,7 @@ static void update_depsgraph(ModifierData *md, const ModifierUpdateDepsgraphCont bool needs_own_transform_relation = false; Set used_ids; find_used_ids_from_settings(nmd->settings, used_ids); - Set checked_groups; - process_nodes_for_depsgraph( - *nmd->node_group, used_ids, needs_own_transform_relation, checked_groups); + nodes::find_node_tree_dependencies(*nmd->node_group, used_ids, needs_own_transform_relation); if (ctx->object->type == OB_CURVES) { Curves *curves_id = static_cast(ctx->object->data); diff --git a/source/blender/nodes/NOD_geometry_nodes_execute.hh b/source/blender/nodes/NOD_geometry_nodes_execute.hh index 2e269409201..54952203324 100644 --- a/source/blender/nodes/NOD_geometry_nodes_execute.hh +++ b/source/blender/nodes/NOD_geometry_nodes_execute.hh @@ -7,6 +7,7 @@ #include "BLI_compute_context.hh" #include "BLI_function_ref.hh" #include "BLI_multi_value_map.hh" +#include "BLI_set.hh" #include "BKE_idprop.hh" #include "BKE_node.h" @@ -29,6 +30,10 @@ class GeoModifierLog; namespace blender::nodes { +void find_node_tree_dependencies(const bNodeTree &tree, + Set &r_ids, + bool &r_needs_own_transform_relation); + StringRef input_use_attribute_suffix(); StringRef input_attribute_name_suffix(); diff --git a/source/blender/nodes/intern/geometry_nodes_execute.cc b/source/blender/nodes/intern/geometry_nodes_execute.cc index 4dc5b862a95..1fbd35f13a9 100644 --- a/source/blender/nodes/intern/geometry_nodes_execute.cc +++ b/source/blender/nodes/intern/geometry_nodes_execute.cc @@ -30,6 +30,106 @@ namespace geo_log = blender::nodes::geo_eval_log; namespace blender::nodes { +static void add_used_ids_from_sockets(const ListBase &sockets, Set &ids) +{ + LISTBASE_FOREACH (const bNodeSocket *, socket, &sockets) { + switch (socket->type) { + case SOCK_OBJECT: { + if (Object *object = ((bNodeSocketValueObject *)socket->default_value)->value) { + ids.add(reinterpret_cast(object)); + } + break; + } + case SOCK_COLLECTION: { + if (Collection *collection = ((bNodeSocketValueCollection *)socket->default_value)->value) + { + ids.add(reinterpret_cast(collection)); + } + break; + } + case SOCK_MATERIAL: { + if (Material *material = ((bNodeSocketValueMaterial *)socket->default_value)->value) { + ids.add(reinterpret_cast(material)); + } + break; + } + case SOCK_TEXTURE: { + if (Tex *texture = ((bNodeSocketValueTexture *)socket->default_value)->value) { + ids.add(reinterpret_cast(texture)); + } + break; + } + case SOCK_IMAGE: { + if (Image *image = ((bNodeSocketValueImage *)socket->default_value)->value) { + ids.add(reinterpret_cast(image)); + } + break; + } + } + } +} + +/** + * \note We can only check properties here that cause the dependency graph to update relations when + * they are changed, otherwise there may be a missing relation after editing. So this could check + * more properties like whether the node is muted, but we would have to accept the cost of updating + * relations when those properties are changed. + */ +static bool node_needs_own_transform_relation(const bNode &node) +{ + if (node.type == GEO_NODE_COLLECTION_INFO) { + const NodeGeometryCollectionInfo &storage = *static_cast( + node.storage); + return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE; + } + + if (node.type == GEO_NODE_OBJECT_INFO) { + const NodeGeometryObjectInfo &storage = *static_cast( + node.storage); + return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE; + } + + if (node.type == GEO_NODE_SELF_OBJECT) { + return true; + } + if (node.type == GEO_NODE_DEFORM_CURVES_ON_SURFACE) { + return true; + } + + return false; +} + +static void process_nodes_for_depsgraph(const bNodeTree &tree, + Set &ids, + bool &r_needs_own_transform_relation, + Set &checked_groups) +{ + if (!checked_groups.add(&tree)) { + return; + } + + tree.ensure_topology_cache(); + for (const bNode *node : tree.all_nodes()) { + add_used_ids_from_sockets(node->inputs, ids); + add_used_ids_from_sockets(node->outputs, ids); + r_needs_own_transform_relation |= node_needs_own_transform_relation(*node); + } + + for (const bNode *node : tree.group_nodes()) { + if (const bNodeTree *sub_tree = reinterpret_cast(node->id)) { + process_nodes_for_depsgraph(*sub_tree, ids, r_needs_own_transform_relation, checked_groups); + } + } +} + +void find_node_tree_dependencies(const bNodeTree &tree, + Set &r_ids, + bool &r_needs_own_transform_relation) +{ + Set checked_groups; + process_nodes_for_depsgraph(tree, r_ids, r_needs_own_transform_relation, checked_groups); +} + StringRef input_use_attribute_suffix() { return "_use_attribute"; From 4b891b4afee909aeec175a1805db3e8374787081 Mon Sep 17 00:00:00 2001 From: Sergey Sharybin Date: Tue, 31 Oct 2023 17:29:03 +0100 Subject: [PATCH 3/3] CMake: Give kernel compilation targets access to console Done by passing USES_TERMINAL to the add_custom_command(). This allows to see sub-command messages early on, before they are finished executing. This should help buildbots to "see" that the kernels are still being compiled and not kill the build because it did not output anything in a long time. Pull Request: https://projects.blender.org/blender/blender/pulls/114327 --- intern/cycles/kernel/CMakeLists.txt | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/intern/cycles/kernel/CMakeLists.txt b/intern/cycles/kernel/CMakeLists.txt index 604ccb81359..a0111fb13e1 100644 --- a/intern/cycles/kernel/CMakeLists.txt +++ b/intern/cycles/kernel/CMakeLists.txt @@ -504,12 +504,14 @@ if(WITH_CYCLES_CUDA_BINARIES) add_custom_command( OUTPUT ${cuda_file} COMMAND ${CCACHE_PROGRAM} ${cuda_nvcc_executable} ${_cuda_nvcc_args} - DEPENDS ${kernel_sources}) + DEPENDS ${kernel_sources} + USES_TERMINAL) else() add_custom_command( OUTPUT ${cuda_file} COMMAND ${cuda_nvcc_executable} ${_cuda_nvcc_args} - DEPENDS ${kernel_sources}) + DEPENDS ${kernel_sources} + USES_TERMINAL) endif() unset(_cuda_nvcc_args) @@ -646,7 +648,8 @@ if(WITH_CYCLES_HIP_BINARIES AND WITH_CYCLES_DEVICE_HIP) add_custom_command( OUTPUT ${hip_file} COMMAND ${hip_command} ${hip_flags} - DEPENDS ${kernel_sources}) + DEPENDS ${kernel_sources} + USES_TERMINAL) delayed_install("${CMAKE_CURRENT_BINARY_DIR}" "${hip_file}" ${CYCLES_INSTALL_PATH}/lib) list(APPEND hip_fatbins ${hip_file}) endmacro() @@ -714,7 +717,8 @@ if(WITH_CYCLES_DEVICE_HIPRT AND WITH_CYCLES_HIP_BINARIES) add_custom_command( OUTPUT ${bitcode_file} COMMAND ${hiprt_compile_command} ${hiprt_compile_flags} - DEPENDS ${kernel_sources}) + DEPENDS ${kernel_sources} + USES_TERMINAL) if(WIN32) set(hiprt_link_command ${CMAKE_COMMAND}) set(hiprt_link_flags -E env "HIP_PATH=${HIP_ROOT_DIR}" @@ -734,7 +738,8 @@ if(WITH_CYCLES_DEVICE_HIPRT AND WITH_CYCLES_HIP_BINARIES) add_custom_command( OUTPUT ${hiprt_file} COMMAND ${hiprt_link_command} ${hiprt_link_flags} - DEPENDS ${bitcode_file}) + DEPENDS ${bitcode_file} + USES_TERMINAL) delayed_install("${CMAKE_CURRENT_BINARY_DIR}" "${hiprt_file}" ${CYCLES_INSTALL_PATH}/lib) add_custom_target(cycles_kernel_hiprt ALL DEPENDS ${hiprt_file}) cycles_set_solution_folder(cycles_kernel_hiprt) @@ -785,7 +790,8 @@ if(WITH_CYCLES_DEVICE_OPTIX AND WITH_CYCLES_CUDA_BINARIES) ${cuda_flags} ${input} WORKING_DIRECTORY - "${CMAKE_CURRENT_SOURCE_DIR}") + "${CMAKE_CURRENT_SOURCE_DIR}" + USES_TERMINAL) list(APPEND optix_ptx ${output}) @@ -1044,7 +1050,8 @@ if(WITH_CYCLES_DEVICE_ONEAPI) "$<$:${sycl_compiler_flags_Debug}>" "$<$:${sycl_compiler_flags_Release}>" COMMAND_EXPAND_LISTS - DEPENDS ${cycles_oneapi_kernel_sources}) + DEPENDS ${cycles_oneapi_kernel_sources} + USES_TERMINAL) else() if(NOT IGC_INSTALL_DIR) get_filename_component(IGC_INSTALL_DIR "${sycl_compiler_root}/../lib/igc" ABSOLUTE) @@ -1069,7 +1076,8 @@ if(WITH_CYCLES_DEVICE_ONEAPI) "$<$:${sycl_compiler_flags_Debug_str}>" "$<$:${sycl_compiler_flags_Release_str}>" COMMAND_EXPAND_LISTS - DEPENDS ${cycles_oneapi_kernel_sources}) + DEPENDS ${cycles_oneapi_kernel_sources} + USES_TERMINAL) endif() if(NOT WITH_BLENDER)