From afad3550607ad90bc2634d4144098c904f89c0c2 Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Thu, 8 May 2025 22:38:55 +0200 Subject: [PATCH] Fix: Properly free Vulkan interop handle for Cycles Unlike OpenGL and Metal, this handle is not shared, but rather Cycles has to take ownership of it. This required a fair amount of refactoring to ensure the handle is closed, ownership is properly transferred, and the handle is recreated once when the pixel buffer is modified. --- intern/cycles/app/opengl/display_driver.cpp | 35 ++++---- intern/cycles/app/opengl/display_driver.h | 4 +- intern/cycles/blender/display_driver.cpp | 76 ++++++++-------- intern/cycles/blender/display_driver.h | 6 +- .../cycles/device/cuda/graphics_interop.cpp | 81 +++++++++++------ intern/cycles/device/cuda/graphics_interop.h | 18 ++-- intern/cycles/device/graphics_interop.h | 2 +- intern/cycles/device/hip/graphics_interop.cpp | 48 +++++------ intern/cycles/device/hip/graphics_interop.h | 13 +-- intern/cycles/device/metal/graphics_interop.h | 4 +- .../cycles/device/metal/graphics_interop.mm | 24 ++++-- intern/cycles/hydra/display_driver.cpp | 31 ++++--- intern/cycles/hydra/display_driver.h | 7 +- intern/cycles/integrator/path_trace.cpp | 4 +- intern/cycles/integrator/path_trace.h | 2 +- .../cycles/integrator/path_trace_display.cpp | 17 ++-- intern/cycles/integrator/path_trace_display.h | 4 +- .../cycles/integrator/path_trace_work_gpu.cpp | 2 +- intern/cycles/session/CMakeLists.txt | 1 + intern/cycles/session/display_driver.cpp | 86 +++++++++++++++++++ intern/cycles/session/display_driver.h | 70 ++++++++++----- intern/cycles/session/session.cpp | 2 +- source/blender/gpu/GPU_texture.hh | 7 ++ source/blender/gpu/vulkan/vk_pixel_buffer.cc | 26 +++--- source/blender/gpu/vulkan/vk_pixel_buffer.hh | 1 - 25 files changed, 363 insertions(+), 208 deletions(-) create mode 100644 intern/cycles/session/display_driver.cpp diff --git a/intern/cycles/app/opengl/display_driver.cpp b/intern/cycles/app/opengl/display_driver.cpp index 923d3ba56a8..6027575c134 100644 --- a/intern/cycles/app/opengl/display_driver.cpp +++ b/intern/cycles/app/opengl/display_driver.cpp @@ -77,7 +77,8 @@ bool OpenGLDisplayDriver::update_begin(const Params ¶ms, /* Texture did change, and no pixel storage was provided. Tag for an explicit zeroing out to * avoid undefined content. */ - texture_.need_clear = true; + texture_.need_zero = true; + graphics_interop_buffer_.clear(); } /* Update PBO dimensions if needed. @@ -128,13 +129,13 @@ half4 *OpenGLDisplayDriver::map_texture_buffer() LOG(ERROR) << "Error mapping OpenGLDisplayDriver pixel buffer object."; } - if (texture_.need_clear) { + if (texture_.need_zero) { const int64_t texture_width = texture_.width; const int64_t texture_height = texture_.height; memset(reinterpret_cast(mapped_rgba_pixels), 0, texture_width * texture_height * sizeof(half4)); - texture_.need_clear = false; + texture_.need_zero = false; } return mapped_rgba_pixels; @@ -158,20 +159,19 @@ GraphicsInteropDevice OpenGLDisplayDriver::graphics_interop_get_device() return interop_device; } -GraphicsInteropBuffer OpenGLDisplayDriver::graphics_interop_get_buffer() +void OpenGLDisplayDriver::graphics_interop_update_buffer() { - GraphicsInteropBuffer interop_buffer; + if (graphics_interop_buffer_.is_empty()) { + graphics_interop_buffer_.assign(GraphicsInteropDevice::OPENGL, + texture_.gl_pbo_id, + texture_.buffer_width * texture_.buffer_height * + sizeof(half4)); + } - interop_buffer.width = texture_.buffer_width; - interop_buffer.height = texture_.buffer_height; - interop_buffer.type = GraphicsInteropDevice::OPENGL; - interop_buffer.handle = texture_.gl_pbo_id; - interop_buffer.size = texture_.buffer_width * texture_.buffer_height * sizeof(half4); - - interop_buffer.need_clear = texture_.need_clear; - texture_.need_clear = false; - - return interop_buffer; + if (texture_.need_zero) { + graphics_interop_buffer_.zero(); + texture_.need_zero = false; + } } void OpenGLDisplayDriver::graphics_interop_activate() @@ -190,13 +190,13 @@ void OpenGLDisplayDriver::graphics_interop_deactivate() void OpenGLDisplayDriver::clear() { - texture_.need_clear = true; + texture_.need_zero = true; } void OpenGLDisplayDriver::draw(const Params ¶ms) { /* See do_update_begin() for why no locking is required here. */ - if (texture_.need_clear) { + if (texture_.need_zero) { /* Texture is requested to be cleared and was not yet cleared. * Do early return which should be equivalent of drawing all-zero texture. */ return; @@ -346,6 +346,7 @@ bool OpenGLDisplayDriver::gl_texture_resources_ensure() /* Creation finished with a success. */ texture_.is_created = true; + graphics_interop_buffer_.clear(); return true; } diff --git a/intern/cycles/app/opengl/display_driver.h b/intern/cycles/app/opengl/display_driver.h index af305b74ade..c8afb12cac2 100644 --- a/intern/cycles/app/opengl/display_driver.h +++ b/intern/cycles/app/opengl/display_driver.h @@ -40,7 +40,7 @@ class OpenGLDisplayDriver : public DisplayDriver { void unmap_texture_buffer() override; GraphicsInteropDevice graphics_interop_get_device() override; - GraphicsInteropBuffer graphics_interop_get_buffer() override; + void graphics_interop_update_buffer() override; void draw(const Params ¶ms) override; @@ -85,7 +85,7 @@ class OpenGLDisplayDriver : public DisplayDriver { bool need_update = false; /* Content of the texture is to be filled with zeroes. */ - std::atomic need_clear = true; + std::atomic need_zero = true; /* Dimensions of the texture in pixels. */ int width = 0; diff --git a/intern/cycles/blender/display_driver.cpp b/intern/cycles/blender/display_driver.cpp index 2d111e1f6a1..906bff955a6 100644 --- a/intern/cycles/blender/display_driver.cpp +++ b/intern/cycles/blender/display_driver.cpp @@ -313,8 +313,10 @@ class DisplayGPUPixelBuffer { return *this; } - bool gpu_resources_ensure(const uint new_width, const uint new_height) + bool gpu_resources_ensure(const uint new_width, const uint new_height, bool &buffer_recreated) { + buffer_recreated = false; + const size_t required_size = sizeof(half4) * new_width * new_height; /* Try to re-use the existing PBO if it has usable size. */ @@ -322,6 +324,7 @@ class DisplayGPUPixelBuffer { if (new_width != width || new_height != height || GPU_pixel_buffer_size(gpu_pixel_buffer) < required_size) { + buffer_recreated = true; gpu_resources_destroy(); } } @@ -333,6 +336,7 @@ class DisplayGPUPixelBuffer { /* Create pixel buffer if not already created. */ if (!gpu_pixel_buffer) { gpu_pixel_buffer = GPU_pixel_buffer_create(required_size); + buffer_recreated = true; } if (gpu_pixel_buffer == nullptr) { @@ -473,7 +477,7 @@ void BlenderDisplayDriver::next_tile_begin() /* Moving to the next tile without giving render data for the current tile is not an expected * situation. */ - DCHECK(!need_clear_); + DCHECK(!need_zero_); /* Texture should have been updated from the PBO at this point. */ DCHECK(!tiles_->current_tile.need_update_texture_pixels); @@ -504,9 +508,9 @@ bool BlenderDisplayDriver::update_begin(const Params ¶ms, /* Clear storage of all finished tiles when display clear is requested. * Do it when new tile data is provided to handle the display clear flag in a single place. * It also makes the logic reliable from the whether drawing did happen or not point of view. */ - if (need_clear_) { + if (need_zero_) { tiles_->finished_tiles.gl_resources_destroy_and_clear(); - need_clear_ = false; + need_zero_ = false; } /* Update PBO dimensions if needed. @@ -520,15 +524,22 @@ bool BlenderDisplayDriver::update_begin(const Params ¶ms, * mode faster. */ const int buffer_width = params.size.x; const int buffer_height = params.size.y; + bool interop_recreated = false; - if (!current_tile_buffer_object.gpu_resources_ensure(buffer_width, buffer_height) || + if (!current_tile_buffer_object.gpu_resources_ensure( + buffer_width, buffer_height, interop_recreated) || !current_tile.texture.gpu_resources_ensure(texture_width, texture_height)) { + graphics_interop_buffer_.clear(); tiles_->current_tile.gpu_resources_destroy(); gpu_context_disable(); return false; } + if (interop_recreated) { + graphics_interop_buffer_.clear(); + } + /* Store an updated parameters of the current tile. * In theory it is only needed once per update of the tile, but doing it on every update is * the easiest and is not expensive. */ @@ -648,36 +659,29 @@ GraphicsInteropDevice BlenderDisplayDriver::graphics_interop_get_device() return interop_device; } -GraphicsInteropBuffer BlenderDisplayDriver::graphics_interop_get_buffer() +void BlenderDisplayDriver::graphics_interop_update_buffer() { - GraphicsInteropBuffer interop_buffer; + if (graphics_interop_buffer_.is_empty()) { + GraphicsInteropDevice::Type type = GraphicsInteropDevice::NONE; + switch (GPU_backend_get_type()) { + case GPU_BACKEND_OPENGL: + type = GraphicsInteropDevice::OPENGL; + break; + case GPU_BACKEND_VULKAN: + type = GraphicsInteropDevice::VULKAN; + break; + case GPU_BACKEND_METAL: + type = GraphicsInteropDevice::METAL; + break; + case GPU_BACKEND_NONE: + case GPU_BACKEND_ANY: + break; + } - interop_buffer.width = tiles_->current_tile.buffer_object.width; - interop_buffer.height = tiles_->current_tile.buffer_object.height; - - GPUPixelBufferNativeHandle handle = GPU_pixel_buffer_get_native_handle( - tiles_->current_tile.buffer_object.gpu_pixel_buffer); - - switch (GPU_backend_get_type()) { - case GPU_BACKEND_OPENGL: - interop_buffer.type = GraphicsInteropDevice::OPENGL; - break; - case GPU_BACKEND_VULKAN: - interop_buffer.type = GraphicsInteropDevice::VULKAN; - break; - case GPU_BACKEND_METAL: - interop_buffer.type = GraphicsInteropDevice::METAL; - break; - case GPU_BACKEND_NONE: - case GPU_BACKEND_ANY: - interop_buffer.type = GraphicsInteropDevice::NONE; - break; + GPUPixelBufferNativeHandle handle = GPU_pixel_buffer_get_native_handle( + tiles_->current_tile.buffer_object.gpu_pixel_buffer); + graphics_interop_buffer_.assign(type, handle.handle, handle.size); } - - interop_buffer.handle = handle.handle; - interop_buffer.size = handle.size; - - return interop_buffer; } void BlenderDisplayDriver::graphics_interop_activate() @@ -694,9 +698,9 @@ void BlenderDisplayDriver::graphics_interop_deactivate() * Drawing. */ -void BlenderDisplayDriver::clear() +void BlenderDisplayDriver::zero() { - need_clear_ = true; + need_zero_ = true; } void BlenderDisplayDriver::set_zoom(const float zoom_x, const float zoom_y) @@ -808,7 +812,7 @@ void BlenderDisplayDriver::draw(const Params ¶ms) { gpu_context_lock(); - if (need_clear_) { + if (need_zero_) { /* Texture is requested to be cleared and was not yet cleared. * * Do early return which should be equivalent of drawing all-zero texture. @@ -936,6 +940,8 @@ void BlenderDisplayDriver::gpu_resources_destroy() display_shader_.reset(); + graphics_interop_buffer_.clear(); + tiles_->current_tile.gpu_resources_destroy(); tiles_->finished_tiles.gl_resources_destroy_and_clear(); diff --git a/intern/cycles/blender/display_driver.h b/intern/cycles/blender/display_driver.h index 1e73570740f..1263f7bb5d4 100644 --- a/intern/cycles/blender/display_driver.h +++ b/intern/cycles/blender/display_driver.h @@ -98,7 +98,7 @@ class BlenderDisplayDriver : public DisplayDriver { void graphics_interop_activate() override; void graphics_interop_deactivate() override; - void clear() override; + void zero() override; void set_zoom(const float zoom_x, const float zoom_y); @@ -114,7 +114,7 @@ class BlenderDisplayDriver : public DisplayDriver { void unmap_texture_buffer() override; GraphicsInteropDevice graphics_interop_get_device() override; - GraphicsInteropBuffer graphics_interop_get_buffer() override; + void graphics_interop_update_buffer() override; void draw(const Params ¶ms) override; @@ -138,7 +138,7 @@ class BlenderDisplayDriver : public DisplayDriver { bool background_; /* Content of the display is to be filled with zeroes. */ - std::atomic need_clear_ = true; + std::atomic need_zero_ = true; unique_ptr display_shader_; diff --git a/intern/cycles/device/cuda/graphics_interop.cpp b/intern/cycles/device/cuda/graphics_interop.cpp index 60ea4951f88..b515884fec5 100644 --- a/intern/cycles/device/cuda/graphics_interop.cpp +++ b/intern/cycles/device/cuda/graphics_interop.cpp @@ -9,6 +9,14 @@ # include "device/cuda/device_impl.h" # include "device/cuda/util.h" +# include "session/display_driver.h" + +# ifdef _WIN32 +# include "util/windows.h" +# else +# include +# endif + CCL_NAMESPACE_BEGIN CUDADeviceGraphicsInterop::CUDADeviceGraphicsInterop(CUDADeviceQueue *queue) @@ -22,58 +30,65 @@ CUDADeviceGraphicsInterop::~CUDADeviceGraphicsInterop() free(); } -void CUDADeviceGraphicsInterop::set_buffer(const GraphicsInteropBuffer &interop_buffer) +void CUDADeviceGraphicsInterop::set_buffer(GraphicsInteropBuffer &interop_buffer) { - const int64_t new_buffer_area = int64_t(interop_buffer.width) * interop_buffer.height; + CUDAContextScope scope(device_); - assert(interop_buffer.size >= interop_buffer.width * interop_buffer.height * sizeof(half4)); - - need_clear_ = interop_buffer.need_clear; - - if (!interop_buffer.need_recreate) { - if (native_type_ == interop_buffer.type && native_handle_ == interop_buffer.handle && - native_size_ == interop_buffer.size && buffer_area_ == new_buffer_area) - { - return; - } + if (interop_buffer.is_empty()) { + free(); + return; } - CUDAContextScope scope(device_); + need_zero_ |= interop_buffer.take_zero(); + + if (!interop_buffer.has_new_handle()) { + return; + } free(); - native_type_ = interop_buffer.type; - native_handle_ = interop_buffer.handle; - native_size_ = interop_buffer.size; - buffer_area_ = new_buffer_area; - - switch (interop_buffer.type) { + switch (interop_buffer.get_type()) { case GraphicsInteropDevice::OPENGL: { - const CUresult result = cuGraphicsGLRegisterBuffer( - &cu_graphics_resource_, interop_buffer.handle, CU_GRAPHICS_MAP_RESOURCE_FLAGS_NONE); + const CUresult result = cuGraphicsGLRegisterBuffer(&cu_graphics_resource_, + interop_buffer.take_handle(), + CU_GRAPHICS_MAP_RESOURCE_FLAGS_NONE); if (result != CUDA_SUCCESS) { LOG(ERROR) << "Error registering OpenGL buffer: " << cuewErrorString(result); + break; } + + buffer_size_ = interop_buffer.get_size(); break; } case GraphicsInteropDevice::VULKAN: { CUDA_EXTERNAL_MEMORY_HANDLE_DESC external_memory_handle_desc = {}; # ifdef _WIN32 + /* cuImportExternalMemory will not take ownership of the handle. */ + vulkan_windows_handle_ = interop_buffer.take_handle(); external_memory_handle_desc.type = CU_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32; external_memory_handle_desc.handle.win32.handle = reinterpret_cast( - interop_buffer.handle); + vulkan_windows_handle_); # else + /* cuImportExternalMemory will take ownership of the handle. */ external_memory_handle_desc.type = CU_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD; - external_memory_handle_desc.handle.fd = interop_buffer.handle; + external_memory_handle_desc.handle.fd = interop_buffer.take_handle(); # endif - external_memory_handle_desc.size = interop_buffer.size; + external_memory_handle_desc.size = interop_buffer.get_size(); const CUresult result = cuImportExternalMemory(&cu_external_memory_, &external_memory_handle_desc); if (result != CUDA_SUCCESS) { +# ifdef _WIN32 + CloseHandle(HANDLE(vulkan_windows_handle_)); + vulkan_windows_handle_ = 0; +# else + close(external_memory_handle_desc.handle.fd); +# endif LOG(ERROR) << "Error importing Vulkan memory: " << cuewErrorString(result); break; } + buffer_size_ = interop_buffer.get_size(); + CUDA_EXTERNAL_MEMORY_BUFFER_DESC external_memory_buffer_desc = {}; external_memory_buffer_desc.size = external_memory_handle_desc.size; external_memory_buffer_desc.offset = 0; @@ -111,11 +126,10 @@ device_ptr CUDADeviceGraphicsInterop::map() cu_buffer = cu_external_memory_ptr_; } - if (cu_buffer && need_clear_) { - cuda_device_assert( - device_, cuMemsetD8Async(cu_buffer, 0, buffer_area_ * sizeof(half4), queue_->stream())); + if (cu_buffer && need_zero_) { + cuda_device_assert(device_, cuMemsetD8Async(cu_buffer, 0, buffer_size_, queue_->stream())); - need_clear_ = false; + need_zero_ = false; } return static_cast(cu_buffer); @@ -144,6 +158,17 @@ void CUDADeviceGraphicsInterop::free() } cu_external_memory_ptr_ = 0; + +# ifdef _WIN32 + if (vulkan_windows_handle_) { + CloseHandle(HANDLE(vulkan_windows_handle_)); + vulkan_windows_handle_ = 0; + } +# endif + + buffer_size_ = 0; + + need_zero_ = false; } CCL_NAMESPACE_END diff --git a/intern/cycles/device/cuda/graphics_interop.h b/intern/cycles/device/cuda/graphics_interop.h index d663e1472e0..db60f614e2e 100644 --- a/intern/cycles/device/cuda/graphics_interop.h +++ b/intern/cycles/device/cuda/graphics_interop.h @@ -30,7 +30,7 @@ class CUDADeviceGraphicsInterop : public DeviceGraphicsInterop { CUDADeviceGraphicsInterop &operator=(const CUDADeviceGraphicsInterop &other) = delete; CUDADeviceGraphicsInterop &operator=(CUDADeviceGraphicsInterop &&other) = delete; - void set_buffer(const GraphicsInteropBuffer &interop_buffer) override; + void set_buffer(GraphicsInteropBuffer &interop_buffer) override; device_ptr map() override; void unmap() override; @@ -39,22 +39,22 @@ class CUDADeviceGraphicsInterop : public DeviceGraphicsInterop { CUDADeviceQueue *queue_ = nullptr; CUDADevice *device_ = nullptr; - /* Native handle. */ - GraphicsInteropDevice::Type native_type_ = GraphicsInteropDevice::NONE; - int64_t native_handle_ = 0; - size_t native_size_ = 0; - - /* Buffer area in pixels of the corresponding PBO. */ - int64_t buffer_area_ = 0; + /* Size of the buffer in bytes. */ + size_t buffer_size_ = 0; /* The destination was requested to be cleared. */ - bool need_clear_ = false; + bool need_zero_ = false; /* CUDA resources. */ CUgraphicsResource cu_graphics_resource_ = nullptr; CUexternalMemory cu_external_memory_ = nullptr; CUdeviceptr cu_external_memory_ptr_ = 0; + /* Vulkan handle to free. */ +# ifdef _WIN32 + int64_t vulkan_windows_handle_ = 0; +# endif + void free(); }; diff --git a/intern/cycles/device/graphics_interop.h b/intern/cycles/device/graphics_interop.h index 1573ee9478f..da04d561893 100644 --- a/intern/cycles/device/graphics_interop.h +++ b/intern/cycles/device/graphics_interop.h @@ -21,7 +21,7 @@ class DeviceGraphicsInterop { /* Update this device-side graphics interoperability buffer with the given destination * resource information. */ - virtual void set_buffer(const GraphicsInteropBuffer &interop_buffer) = 0; + virtual void set_buffer(GraphicsInteropBuffer &interop_buffer) = 0; virtual device_ptr map() = 0; virtual void unmap() = 0; diff --git a/intern/cycles/device/hip/graphics_interop.cpp b/intern/cycles/device/hip/graphics_interop.cpp index 6aae4caa6f2..6fb198fb8ad 100644 --- a/intern/cycles/device/hip/graphics_interop.cpp +++ b/intern/cycles/device/hip/graphics_interop.cpp @@ -22,38 +22,35 @@ HIPDeviceGraphicsInterop::~HIPDeviceGraphicsInterop() free(); } -void HIPDeviceGraphicsInterop::set_buffer(const GraphicsInteropBuffer &interop_buffer) +void HIPDeviceGraphicsInterop::set_buffer(GraphicsInteropBuffer &interop_buffer) { - const int64_t new_buffer_area = int64_t(interop_buffer.width) * interop_buffer.height; + HIPContextScope scope(device_); - assert(interop_buffer.size >= interop_buffer.width * interop_buffer.height * sizeof(half4)); - - need_clear_ = interop_buffer.need_clear; - - if (!interop_buffer.need_recreate) { - if (native_type_ == interop_buffer.type && native_handle_ == interop_buffer.handle && - native_size_ == interop_buffer.size && buffer_area_ == new_buffer_area) - { - return; - } + if (interop_buffer.is_empty()) { + free(); + return; + } + + need_zero_ |= interop_buffer.take_zero(); + + if (!interop_buffer.has_new_handle()) { + return; } - HIPContextScope scope(device_); free(); - native_type_ = interop_buffer.type; - native_handle_ = interop_buffer.handle; - native_size_ = interop_buffer.size; - buffer_area_ = new_buffer_area; - - switch (interop_buffer.type) { + switch (interop_buffer.get_type()) { case GraphicsInteropDevice::OPENGL: { const hipError_t result = hipGraphicsGLRegisterBuffer( - &hip_graphics_resource_, interop_buffer.handle, hipGraphicsRegisterFlagsNone); + &hip_graphics_resource_, interop_buffer.take_handle(), hipGraphicsRegisterFlagsNone); if (result != hipSuccess) { LOG(ERROR) << "Error registering OpenGL buffer: " << hipewErrorString(result); + break; } + + buffer_size_ = interop_buffer.get_size(); + break; } case GraphicsInteropDevice::VULKAN: @@ -82,11 +79,10 @@ device_ptr HIPDeviceGraphicsInterop::map() hip_buffer = hip_external_memory_ptr_; } - if (hip_buffer && need_clear_) { - hip_device_assert( - device_, hipMemsetD8Async(hip_buffer, 0, buffer_area_ * sizeof(half4), queue_->stream())); + if (hip_buffer && need_zero_) { + hip_device_assert(device_, hipMemsetD8Async(hip_buffer, 0, buffer_size_, queue_->stream())); - need_clear_ = false; + need_zero_ = false; } return static_cast(hip_buffer); @@ -110,6 +106,10 @@ void HIPDeviceGraphicsInterop::free() } hip_external_memory_ptr_ = 0; + + buffer_size_ = 0; + + need_zero_ = false; } CCL_NAMESPACE_END diff --git a/intern/cycles/device/hip/graphics_interop.h b/intern/cycles/device/hip/graphics_interop.h index 12e8175959e..a6217937e66 100644 --- a/intern/cycles/device/hip/graphics_interop.h +++ b/intern/cycles/device/hip/graphics_interop.h @@ -28,7 +28,7 @@ class HIPDeviceGraphicsInterop : public DeviceGraphicsInterop { HIPDeviceGraphicsInterop &operator=(const HIPDeviceGraphicsInterop &other) = delete; HIPDeviceGraphicsInterop &operator=(HIPDeviceGraphicsInterop &&other) = delete; - void set_buffer(const GraphicsInteropBuffer &interop_buffer) override; + void set_buffer(GraphicsInteropBuffer &interop_buffer) override; device_ptr map() override; void unmap() override; @@ -37,16 +37,11 @@ class HIPDeviceGraphicsInterop : public DeviceGraphicsInterop { HIPDeviceQueue *queue_ = nullptr; HIPDevice *device_ = nullptr; - /* Native handle. */ - GraphicsInteropDevice::Type native_type_ = GraphicsInteropDevice::NONE; - int64_t native_handle_ = 0; - size_t native_size_ = 0; - - /* Buffer area in pixels of the corresponding PBO. */ - int64_t buffer_area_ = 0; + /* Size of the buffer in bytes. */ + size_t buffer_size_ = 0; /* The destination was requested to be cleared. */ - bool need_clear_ = false; + bool need_zero_ = false; hipGraphicsResource hip_graphics_resource_ = nullptr; hipDeviceptr_t hip_external_memory_ptr_ = 0; diff --git a/intern/cycles/device/metal/graphics_interop.h b/intern/cycles/device/metal/graphics_interop.h index 899c2cbde5c..dbe6cc7f5e3 100644 --- a/intern/cycles/device/metal/graphics_interop.h +++ b/intern/cycles/device/metal/graphics_interop.h @@ -26,7 +26,7 @@ class MetalDeviceGraphicsInterop : public DeviceGraphicsInterop { MetalDeviceGraphicsInterop &operator=(const MetalDeviceGraphicsInterop &other) = delete; MetalDeviceGraphicsInterop &operator=(MetalDeviceGraphicsInterop &&other) = delete; - void set_buffer(const GraphicsInteropBuffer &interop_buffer) override; + void set_buffer(GraphicsInteropBuffer &interop_buffer) override; device_ptr map() override; void unmap() override; @@ -40,7 +40,7 @@ class MetalDeviceGraphicsInterop : public DeviceGraphicsInterop { size_t size_ = 0; /* The destination was requested to be cleared. */ - bool need_clear_ = false; + bool need_zero_ = false; }; CCL_NAMESPACE_END diff --git a/intern/cycles/device/metal/graphics_interop.mm b/intern/cycles/device/metal/graphics_interop.mm index 2a1d5f6269d..c033068f08f 100644 --- a/intern/cycles/device/metal/graphics_interop.mm +++ b/intern/cycles/device/metal/graphics_interop.mm @@ -17,21 +17,29 @@ MetalDeviceGraphicsInterop::MetalDeviceGraphicsInterop(MetalDeviceQueue *queue) MetalDeviceGraphicsInterop::~MetalDeviceGraphicsInterop() = default; -void MetalDeviceGraphicsInterop::set_buffer(const GraphicsInteropBuffer &interop_buffer) +void MetalDeviceGraphicsInterop::set_buffer(GraphicsInteropBuffer &interop_buffer) { - /* Trivial implementation due to unified memory. */ - if (interop_buffer.type == GraphicsInteropDevice::METAL) { - need_clear_ |= interop_buffer.need_clear; - mem_.mtlBuffer = reinterpret_cast>(interop_buffer.handle); - size_ = interop_buffer.width * interop_buffer.height * sizeof(half4); + if (interop_buffer.is_empty()) { + mem_.mtlBuffer = nullptr; + size_ = 0; + return; } + + need_zero_ |= interop_buffer.take_zero(); + + if (!interop_buffer.has_new_handle()) { + return; + } + + mem_.mtlBuffer = reinterpret_cast>(interop_buffer.take_handle()); + size_ = interop_buffer.get_size(); } device_ptr MetalDeviceGraphicsInterop::map() { - if (mem_.mtlBuffer && need_clear_) { + if (mem_.mtlBuffer && need_zero_) { memset([mem_.mtlBuffer contents], 0, size_); - need_clear_ = false; + need_zero_ = false; } return device_ptr(&mem_); diff --git a/intern/cycles/hydra/display_driver.cpp b/intern/cycles/hydra/display_driver.cpp index a0af7836be3..f42bf7add9c 100644 --- a/intern/cycles/hydra/display_driver.cpp +++ b/intern/cycles/hydra/display_driver.cpp @@ -72,6 +72,7 @@ void HdCyclesDisplayDriver::gl_context_create() if (!gl_pbo_id_) { glGenBuffers(1, &gl_pbo_id_); + graphics_interop_buffer_.clear(); } } @@ -142,6 +143,7 @@ bool HdCyclesDisplayDriver::update_begin(const Params ¶ms, glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); pbo_size_ = params.full_size; + graphics_interop_buffer_.clear(); } need_update_ = true; @@ -179,9 +181,9 @@ half4 *HdCyclesDisplayDriver::map_texture_buffer() auto *const mapped_rgba_pixels = static_cast( glMapBuffer(GL_PIXEL_UNPACK_BUFFER, GL_WRITE_ONLY)); - if (need_clear_ && mapped_rgba_pixels) { + if (need_zero_ && mapped_rgba_pixels) { memset(mapped_rgba_pixels, 0, sizeof(half4) * pbo_size_.x * pbo_size_.y); - need_clear_ = false; + need_zero_ = false; } return mapped_rgba_pixels; @@ -201,20 +203,17 @@ GraphicsInteropDevice HdCyclesDisplayDriver::graphics_interop_get_device() return interop_device; } -GraphicsInteropBuffer HdCyclesDisplayDriver::graphics_interop_get_buffer() +void HdCyclesDisplayDriver::graphics_interop_update_buffer() { - GraphicsInteropBuffer interop_buffer; + if (graphics_interop_buffer_.is_empty()) { + graphics_interop_buffer_.assign( + GraphicsInteropDevice::OPENGL, gl_pbo_id_, pbo_size_.x * pbo_size_.y * sizeof(half4)); + } - interop_buffer.width = pbo_size_.x; - interop_buffer.height = pbo_size_.y; - interop_buffer.type = GraphicsInteropDevice::OPENGL; - interop_buffer.handle = gl_pbo_id_; - interop_buffer.size = pbo_size_.x * pbo_size_.y * sizeof(half4); - - interop_buffer.need_clear = need_clear_; - need_clear_ = false; - - return interop_buffer; + if (need_zero_) { + graphics_interop_buffer_.zero(); + need_zero_ = false; + } } void HdCyclesDisplayDriver::graphics_interop_activate() @@ -227,9 +226,9 @@ void HdCyclesDisplayDriver::graphics_interop_deactivate() gl_context_disable(); } -void HdCyclesDisplayDriver::clear() +void HdCyclesDisplayDriver::zero() { - need_clear_ = true; + need_zero_ = true; } void HdCyclesDisplayDriver::draw(const Params ¶ms) diff --git a/intern/cycles/hydra/display_driver.h b/intern/cycles/hydra/display_driver.h index 8166c2bdbe2..01a27cb5699 100644 --- a/intern/cycles/hydra/display_driver.h +++ b/intern/cycles/hydra/display_driver.h @@ -33,12 +33,12 @@ class HdCyclesDisplayDriver final : public CCL_NS::DisplayDriver { void unmap_texture_buffer() override; GraphicsInteropDevice graphics_interop_get_device() override; - GraphicsInteropBuffer graphics_interop_get_buffer() override; + void graphics_interop_update_buffer() override; void graphics_interop_activate() override; void graphics_interop_deactivate() override; - void clear() override; + void zero() override; void draw(const Params ¶ms) override; @@ -60,7 +60,8 @@ class HdCyclesDisplayDriver final : public CCL_NS::DisplayDriver { unsigned int gl_pbo_id_ = 0; CCL_NS::int2 pbo_size_ = CCL_NS::make_int2(0, 0); bool need_update_ = false; - std::atomic_bool need_clear_ = false; + std::atomic_bool need_zero_ = false; + std::atomic_bool need_recreate_interop_ = false; void *gl_render_sync_ = nullptr; void *gl_upload_sync_ = nullptr; diff --git a/intern/cycles/integrator/path_trace.cpp b/intern/cycles/integrator/path_trace.cpp index f011476b604..4084c627631 100644 --- a/intern/cycles/integrator/path_trace.cpp +++ b/intern/cycles/integrator/path_trace.cpp @@ -654,10 +654,10 @@ void PathTrace::set_display_driver(unique_ptr driver) } } -void PathTrace::clear_display() +void PathTrace::zero_display() { if (display_) { - display_->clear(); + display_->zero(); } } diff --git a/intern/cycles/integrator/path_trace.h b/intern/cycles/integrator/path_trace.h index 4f6ee62e9d5..a27f5fffb9c 100644 --- a/intern/cycles/integrator/path_trace.h +++ b/intern/cycles/integrator/path_trace.h @@ -107,7 +107,7 @@ class PathTrace { void set_display_driver(unique_ptr driver); /* Clear the display buffer by filling it in with all zeroes. */ - void clear_display(); + void zero_display(); /* Perform drawing of the current state of the DisplayDriver. */ void draw(); diff --git a/intern/cycles/integrator/path_trace_display.cpp b/intern/cycles/integrator/path_trace_display.cpp index 626964003f3..b26f2449b46 100644 --- a/intern/cycles/integrator/path_trace_display.cpp +++ b/intern/cycles/integrator/path_trace_display.cpp @@ -5,6 +5,7 @@ #include "integrator/path_trace_display.h" #include "session/buffers.h" +#include "session/display_driver.h" #include "util/log.h" @@ -191,26 +192,30 @@ GraphicsInteropDevice PathTraceDisplay::graphics_interop_get_device() return driver_->graphics_interop_get_device(); } -GraphicsInteropBuffer PathTraceDisplay::graphics_interop_get_buffer() +GraphicsInteropBuffer &PathTraceDisplay::graphics_interop_get_buffer() { + GraphicsInteropBuffer &interop_buffer = driver_->graphics_interop_get_buffer(); DCHECK(!texture_buffer_state_.is_mapped); DCHECK(update_state_.is_active); if (texture_buffer_state_.is_mapped) { LOG(ERROR) << "Attempt to use graphics interoperability mode while the texture buffer is mapped."; - return GraphicsInteropBuffer(); + interop_buffer.clear(); + return interop_buffer; } if (!update_state_.is_active) { LOG(ERROR) << "Attempt to use graphics interoperability outside of PathTraceDisplay update."; - return GraphicsInteropBuffer(); + interop_buffer.clear(); + return interop_buffer; } /* Assume that interop will write new values to the texture. */ mark_texture_updated(); - return driver_->graphics_interop_get_buffer(); + driver_->graphics_interop_update_buffer(); + return interop_buffer; } void PathTraceDisplay::graphics_interop_activate() @@ -227,9 +232,9 @@ void PathTraceDisplay::graphics_interop_deactivate() * Drawing. */ -void PathTraceDisplay::clear() +void PathTraceDisplay::zero() { - driver_->clear(); + driver_->zero(); } bool PathTraceDisplay::draw() diff --git a/intern/cycles/integrator/path_trace_display.h b/intern/cycles/integrator/path_trace_display.h index 35b28d52b79..704775fcc5d 100644 --- a/intern/cycles/integrator/path_trace_display.h +++ b/intern/cycles/integrator/path_trace_display.h @@ -116,7 +116,7 @@ class PathTraceDisplay { /* Get PathTraceDisplay graphics interoperability information which acts as a destination for the * device API. */ GraphicsInteropDevice graphics_interop_get_device(); - GraphicsInteropBuffer graphics_interop_get_buffer(); + GraphicsInteropBuffer &graphics_interop_get_buffer(); /* (De)activate GPU display for graphics interoperability outside of regular display update * routines. */ @@ -138,7 +138,7 @@ class PathTraceDisplay { * * If the GPU display supports graphics interoperability then the zeroing the display is to be * delegated to the device via the `GraphicsInterop`. */ - void clear(); + void zero(); /* Draw the current state of the texture. * diff --git a/intern/cycles/integrator/path_trace_work_gpu.cpp b/intern/cycles/integrator/path_trace_work_gpu.cpp index 5cff6a7e15b..f03e73b03e6 100644 --- a/intern/cycles/integrator/path_trace_work_gpu.cpp +++ b/intern/cycles/integrator/path_trace_work_gpu.cpp @@ -1041,7 +1041,7 @@ bool PathTraceWorkGPU::copy_to_display_interop(PathTraceDisplay *display, device_graphics_interop_ = queue_->graphics_interop_create(); } - const GraphicsInteropBuffer interop_buffer = display->graphics_interop_get_buffer(); + GraphicsInteropBuffer &interop_buffer = display->graphics_interop_get_buffer(); device_graphics_interop_->set_buffer(interop_buffer); const device_ptr d_rgba_half = device_graphics_interop_->map(); diff --git a/intern/cycles/session/CMakeLists.txt b/intern/cycles/session/CMakeLists.txt index c144ab310de..81f0461d35a 100644 --- a/intern/cycles/session/CMakeLists.txt +++ b/intern/cycles/session/CMakeLists.txt @@ -11,6 +11,7 @@ set(INC_SYS set(SRC buffers.cpp + display_driver.cpp denoising.cpp merge.cpp session.cpp diff --git a/intern/cycles/session/display_driver.cpp b/intern/cycles/session/display_driver.cpp new file mode 100644 index 00000000000..8340fe725a7 --- /dev/null +++ b/intern/cycles/session/display_driver.cpp @@ -0,0 +1,86 @@ +/* SPDX-FileCopyrightText: 2021-2025 Blender Foundation + * + * SPDX-License-Identifier: Apache-2.0 */ + +#include "session/display_driver.h" + +#ifdef _WIN32 +# include "util/windows.h" +#else +# include +#endif + +CCL_NAMESPACE_BEGIN + +GraphicsInteropBuffer::~GraphicsInteropBuffer() +{ + clear(); +} + +void GraphicsInteropBuffer::assign(GraphicsInteropDevice::Type type, int64_t handle, size_t size) +{ + clear(); + + type_ = type; + handle_ = handle; + own_handle_ = true; + size_ = size; +} + +bool GraphicsInteropBuffer::is_empty() const +{ + return handle_ == 0; +} + +void GraphicsInteropBuffer::zero() +{ + need_zero_ = true; +} + +void GraphicsInteropBuffer::clear() +{ + if (type_ == GraphicsInteropDevice::VULKAN && handle_ && own_handle_) { +#ifdef _WIN32 + CloseHandle(HANDLE(handle_)); +#else + close(handle_); +#endif + } + + type_ = GraphicsInteropDevice::NONE; + handle_ = 0; + size_ = 0; + need_zero_ = false; + own_handle_ = false; +} + +GraphicsInteropDevice::Type GraphicsInteropBuffer::get_type() const +{ + return type_; +} + +size_t GraphicsInteropBuffer::get_size() const +{ + return size_; +} + +bool GraphicsInteropBuffer::has_new_handle() const +{ + return own_handle_; +} + +bool GraphicsInteropBuffer::take_zero() +{ + bool need_zero = need_zero_; + need_zero_ = false; + return need_zero; +} + +int64_t GraphicsInteropBuffer::take_handle() +{ + assert(own_handle_); + own_handle_ = false; + return handle_; +} + +CCL_NAMESPACE_END diff --git a/intern/cycles/session/display_driver.h b/intern/cycles/session/display_driver.h index 3cf146b97ed..b2f5e2c9121 100644 --- a/intern/cycles/session/display_driver.h +++ b/intern/cycles/session/display_driver.h @@ -33,34 +33,55 @@ class GraphicsInteropDevice { * with RGBA channels. */ class GraphicsInteropBuffer { public: - /* Dimensions of the buffer, in pixels. */ - int width = 0; - int height = 0; + GraphicsInteropBuffer() = default; + ~GraphicsInteropBuffer(); + GraphicsInteropBuffer(const GraphicsInteropBuffer &other) = delete; + GraphicsInteropBuffer &operator=(const GraphicsInteropBuffer &other) = delete; + GraphicsInteropBuffer(GraphicsInteropBuffer &&other) = delete; + GraphicsInteropBuffer &operator=(GraphicsInteropBuffer &&other) = delete; + + /* Display Driver API. */ + + /* Assign handle. For Vulkan, this transfers ownership of the handle. */ + void assign(GraphicsInteropDevice::Type type, int64_t handle, size_t size); + /* Is a handle assigned? */ + bool is_empty() const; + /* Zero memory. */ + void zero(); + /* Clear handle. */ + void clear(); + + /* Device graphics interop API. */ + + /* Get type of handle. */ + GraphicsInteropDevice::Type get_type() const; + /* Get size of buffer. */ + size_t get_size() const; + + /* Is there a new handle to take ownership of? */ + bool has_new_handle() const; + /* Take ownership of the handle. */ + int64_t take_handle(); + + /* Take ownership of zeroing the buffer. */ + bool take_zero(); + + protected: /* The handle is expected to be: * - OpenGL: pixel buffer object ID. * - Vulkan on Windows: opaque handle for VkBuffer. * - Vulkan on Unix: opaque file descriptor for VkBuffer. * - Metal: MTLBuffer with unified memory. */ - GraphicsInteropDevice::Type type = GraphicsInteropDevice::NONE; - int64_t handle = 0; + GraphicsInteropDevice::Type type_ = GraphicsInteropDevice::NONE; + int64_t handle_ = 0; + bool own_handle_ = false; - /* Actual size of the memory, which must be `>= width * height sizeof(half4)`. */ - size_t size = 0; + /* Actual size of the memory, which must be `>= width * height * sizeof(half4)`. */ + size_t size_ = 0; /* Clear the entire buffer before doing partial write to it. */ - bool need_clear = false; - - /* Enforce re-creation of the graphics interop object. - * - * When this field is true then the graphics interop will be re-created no matter what the - * rest of the configuration is. - * When this field is false the graphics interop will be re-created if the PBO or buffer size - * did change. - * - * This allows to ensure graphics interop is re-created when there is a possibility that an - * underlying PBO was re-allocated but did not change its ID. */ - bool need_recreate = false; + bool need_zero_ = false; }; /* Display driver for efficient interactive display of renders. @@ -130,14 +151,19 @@ class DisplayDriver { virtual half4 *map_texture_buffer() = 0; virtual void unmap_texture_buffer() = 0; + GraphicsInteropBuffer graphics_interop_buffer_; + + /* Graphics interop to avoid CPU - GPU transfer. See GraphicsInteropBuffer for details. */ virtual GraphicsInteropDevice graphics_interop_get_device() { return GraphicsInteropDevice(); } - virtual GraphicsInteropBuffer graphics_interop_get_buffer() + virtual void graphics_interop_update_buffer() {} + + GraphicsInteropBuffer &graphics_interop_get_buffer() { - return GraphicsInteropBuffer(); + return graphics_interop_buffer_; } /* (De)activate graphics context required for editing or deleting the graphics interop @@ -149,7 +175,7 @@ class DisplayDriver { virtual void graphics_interop_deactivate(){}; /* Clear the display buffer by filling it with zeros. */ - virtual void clear() = 0; + virtual void zero() = 0; /* Draw the render using the native graphics API. * diff --git a/intern/cycles/session/session.cpp b/intern/cycles/session/session.cpp index c4e1f4953af..90914e99041 100644 --- a/intern/cycles/session/session.cpp +++ b/intern/cycles/session/session.cpp @@ -159,7 +159,7 @@ bool Session::ready_to_reset() void Session::run_main_render_loop() { - path_trace_->clear_display(); + path_trace_->zero_display(); while (true) { RenderWork render_work = run_update_for_next_iteration(); diff --git a/source/blender/gpu/GPU_texture.hh b/source/blender/gpu/GPU_texture.hh index 12b923a1439..1483193d6b6 100644 --- a/source/blender/gpu/GPU_texture.hh +++ b/source/blender/gpu/GPU_texture.hh @@ -1357,6 +1357,13 @@ size_t GPU_pixel_buffer_size(GPUPixelBuffer *pixel_buf); /** * Return the native handle of the \a pix_buf to use for graphic interoperability registration. + * + * - OpenGL: pixel buffer object ID. + * - Vulkan on Windows: opaque handle for VkBuffer. + * - Vulkan on Unix: opaque file descriptor for VkBuffer. + * - Metal: MTLBuffer with unified memory. + * + * For Vulkan, the caller is responsible for closing the handle. */ struct GPUPixelBufferNativeHandle { int64_t handle = 0; diff --git a/source/blender/gpu/vulkan/vk_pixel_buffer.cc b/source/blender/gpu/vulkan/vk_pixel_buffer.cc index 61cd020994f..b5b60802ae5 100644 --- a/source/blender/gpu/vulkan/vk_pixel_buffer.cc +++ b/source/blender/gpu/vulkan/vk_pixel_buffer.cc @@ -40,7 +40,6 @@ void VKPixelBuffer::create(bool memory_export) buffer_initialized_ = true; buffer_memory_export_ = memory_export; - native_handle_ = GPUPixelBufferNativeHandle{}; } void *VKPixelBuffer::map() @@ -57,16 +56,13 @@ void VKPixelBuffer::unmap() GPUPixelBufferNativeHandle VKPixelBuffer::get_native_handle() { - /* Initialize once. */ - if (buffer_initialized_) { - return native_handle_; - } - VKDevice &device = VKBackend::get().device; + GPUPixelBufferNativeHandle native_handle; + /* Functionality supported? */ if (!device.extensions_get().external_memory) { - return native_handle_; + return native_handle; } /* Create buffer. */ @@ -77,7 +73,7 @@ GPUPixelBufferNativeHandle VKPixelBuffer::get_native_handle() VkDeviceMemory memory = buffer_.export_memory_get(memory_size); if (memory == nullptr) { CLOG_ERROR(&LOG, "Failed to get device memory for Vulkan pixel buffer"); - return native_handle_; + return native_handle; } #ifdef _WIN32 @@ -91,11 +87,11 @@ GPUPixelBufferNativeHandle VKPixelBuffer::get_native_handle() HANDLE handle = 0; if (device.functions.vkGetMemoryWin32Handle(device.vk_handle(), &info, &handle) != VK_SUCCESS) { CLOG_ERROR(&LOG, "Failed to get Windows handle for Vulkan pixel buffer"); - return native_handle_; + return native_handle; } - native_handle_.handle = int64_t(handle); - native_handle_.size = memory_size; + native_handle.handle = int64_t(handle); + native_handle.size = memory_size; #else /* Opaque file descriptor. */ VkMemoryGetFdInfoKHR info = {}; @@ -107,14 +103,14 @@ GPUPixelBufferNativeHandle VKPixelBuffer::get_native_handle() int fd = -1; if (device.functions.vkGetMemoryFd(device.vk_handle(), &info, &fd) != VK_SUCCESS) { CLOG_ERROR(&LOG, "Failed to get file descriptor for Vulkan pixel buffer"); - return native_handle_; + return native_handle; } - native_handle_.handle = int64_t(fd); - native_handle_.size = memory_size; + native_handle.handle = int64_t(fd); + native_handle.size = memory_size; #endif - return native_handle_; + return native_handle; } size_t VKPixelBuffer::get_size() diff --git a/source/blender/gpu/vulkan/vk_pixel_buffer.hh b/source/blender/gpu/vulkan/vk_pixel_buffer.hh index c27d41b6955..d4d63e89127 100644 --- a/source/blender/gpu/vulkan/vk_pixel_buffer.hh +++ b/source/blender/gpu/vulkan/vk_pixel_buffer.hh @@ -18,7 +18,6 @@ class VKPixelBuffer : public PixelBuffer { VKBuffer buffer_; bool buffer_initialized_ = false; bool buffer_memory_export_ = false; - GPUPixelBufferNativeHandle native_handle_; public: VKPixelBuffer(size_t size);