From e2728a0056df45f87161dcb0feb2721748997732 Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Thu, 10 Feb 2022 20:28:43 +0100 Subject: Fix T95420: Cycles crash with stereo render and tiles For reasons unclear, destroying and then recreating a vertex buffer in the render OpenGL context is affecting the immediate mode vertex buffer in the draw manager OpenGL context. Instead just create a single vertex buffer and use it for the lifetime of the render OpenGL context. There's not really any need to have a separate one per tile as far as I can tell. Differential Revision: https://developer.blender.org/D14084 --- intern/cycles/blender/display_driver.cpp | 63 +++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 22 deletions(-) (limited to 'intern') diff --git a/intern/cycles/blender/display_driver.cpp b/intern/cycles/blender/display_driver.cpp index 3eab2bb8507..0ddc7def86c 100644 --- a/intern/cycles/blender/display_driver.cpp +++ b/intern/cycles/blender/display_driver.cpp @@ -480,26 +480,12 @@ class DrawTile { return false; } - if (!gl_vertex_buffer) { - glGenBuffers(1, &gl_vertex_buffer); - if (!gl_vertex_buffer) { - LOG(ERROR) << "Error allocating tile VBO."; - gl_resources_destroy(); - return false; - } - } - return true; } void gl_resources_destroy() { texture.gl_resources_destroy(); - - if (gl_vertex_buffer) { - glDeleteBuffers(1, &gl_vertex_buffer); - gl_vertex_buffer = 0; - } } inline bool ready_to_draw() const @@ -512,9 +498,6 @@ class DrawTile { /* Display parameters the texture of this tile has been updated for. */ BlenderDisplayDriver::Params params; - - /* OpenGL resources needed for drawing. */ - uint gl_vertex_buffer = 0; }; class DrawTileAndPBO { @@ -560,6 +543,30 @@ struct BlenderDisplayDriver::Tiles { tiles.clear(); } } finished_tiles; + + /* OpenGL vertex buffer needed for drawing. */ + uint gl_vertex_buffer = 0; + + bool gl_resources_ensure() + { + if (!gl_vertex_buffer) { + glGenBuffers(1, &gl_vertex_buffer); + if (!gl_vertex_buffer) { + LOG(ERROR) << "Error allocating tile VBO."; + return false; + } + } + + return true; + } + + void gl_resources_destroy() + { + if (gl_vertex_buffer) { + glDeleteBuffers(1, &gl_vertex_buffer); + gl_vertex_buffer = 0; + } + } }; BlenderDisplayDriver::BlenderDisplayDriver(BL::RenderEngine &b_engine, BL::Scene &b_scene) @@ -626,6 +633,12 @@ bool BlenderDisplayDriver::update_begin(const Params ¶ms, need_clear_ = false; } + if (!tiles_->gl_resources_ensure()) { + tiles_->gl_resources_destroy(); + gl_context_disable(); + return false; + } + if (!tiles_->current_tile.gl_resources_ensure()) { tiles_->current_tile.gl_resources_destroy(); gl_context_disable(); @@ -825,7 +838,8 @@ static void vertex_buffer_update(const DisplayDriver::Params ¶ms) static void draw_tile(const float2 &zoom, const int texcoord_attribute, const int position_attribute, - const DrawTile &draw_tile) + const DrawTile &draw_tile, + const uint gl_vertex_buffer) { if (!draw_tile.ready_to_draw()) { return; @@ -834,9 +848,9 @@ static void draw_tile(const float2 &zoom, const GLTexture &texture = draw_tile.texture; DCHECK_NE(texture.gl_id, 0); - DCHECK_NE(draw_tile.gl_vertex_buffer, 0); + DCHECK_NE(gl_vertex_buffer, 0); - glBindBuffer(GL_ARRAY_BUFFER, draw_tile.gl_vertex_buffer); + glBindBuffer(GL_ARRAY_BUFFER, gl_vertex_buffer); /* Draw at the parameters for which the texture has been updated for. This allows to always draw * texture during bordered-rendered camera view without flickering. The validness of the display @@ -956,10 +970,14 @@ void BlenderDisplayDriver::draw(const Params ¶ms) glEnableVertexAttribArray(texcoord_attribute); glEnableVertexAttribArray(position_attribute); - draw_tile(zoom_, texcoord_attribute, position_attribute, tiles_->current_tile.tile); + draw_tile(zoom_, + texcoord_attribute, + position_attribute, + tiles_->current_tile.tile, + tiles_->gl_vertex_buffer); for (const DrawTile &tile : tiles_->finished_tiles.tiles) { - draw_tile(zoom_, texcoord_attribute, position_attribute, tile); + draw_tile(zoom_, texcoord_attribute, position_attribute, tile, tiles_->gl_vertex_buffer); } display_shader_->unbind(); @@ -1062,6 +1080,7 @@ void BlenderDisplayDriver::gl_resources_destroy() tiles_->current_tile.gl_resources_destroy(); tiles_->finished_tiles.gl_resources_destroy_and_clear(); + tiles_->gl_resources_destroy(); gl_context_disable(); -- cgit v1.2.3 From 6ec83afb1db8a67d2a03931bfb7407c7e253718f Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Thu, 10 Feb 2022 17:49:56 +0100 Subject: Cycles: refactor to keep session thread alive for duration of session Instead of creating and destroying threads when starting and stopping renders, keep a single thread alive for the duration of the session. This makes it so all display driver OpenGL resource allocation and destruction can happen in the same thread. This was implemented as part of trying to solve another bug, but it did not help. Still I prefer this behavior, to eliminate potential future issues wit graphics drivers or with future Cycles display driver implementations. Differential Revision: https://developer.blender.org/D14086 --- intern/cycles/session/session.cpp | 112 ++++++++++++++++++++++++++++++-------- intern/cycles/session/session.h | 16 +++++- 2 files changed, 102 insertions(+), 26 deletions(-) (limited to 'intern') diff --git a/intern/cycles/session/session.cpp b/intern/cycles/session/session.cpp index d03063a7dda..f6e06f20aba 100644 --- a/intern/cycles/session/session.cpp +++ b/intern/cycles/session/session.cpp @@ -49,12 +49,9 @@ Session::Session(const SessionParams ¶ms_, const SceneParams &scene_params) { TaskScheduler::init(params.threads); - session_thread_ = nullptr; - delayed_reset_.do_reset = false; pause_ = false; - cancel_ = false; new_work_added_ = false; device = Device::create(params.device, stats, profiler); @@ -73,48 +70,79 @@ Session::Session(const SessionParams ¶ms_, const SceneParams &scene_params) } full_buffer_written_cb(filename); }; + + /* Create session thread. */ + session_thread_ = new thread(function_bind(&Session::thread_run, this)); } Session::~Session() { + /* Cancel any ongoing render operation. */ cancel(); - /* Make sure path tracer is destroyed before the device. This is needed because destruction might - * need to access device for device memory free. */ - /* TODO(sergey): Convert device to be unique_ptr, and rely on C++ to destruct objects in the + /* Signal session thread to end. */ + { + thread_scoped_lock session_thread_lock(session_thread_mutex_); + session_thread_state_ = SESSION_THREAD_END; + } + session_thread_cond_.notify_all(); + + /* Destroy session thread. */ + session_thread_->join(); + delete session_thread_; + + /* Destroy path tracer, before the device. This is needed because destruction might need to + * access device for device memory free. + * TODO(sergey): Convert device to be unique_ptr, and rely on C++ to destruct objects in the * pre-defined order. */ path_trace_.reset(); + /* Destroy scene and device. */ delete scene; delete device; + /* Stop task scheduler. */ TaskScheduler::exit(); } void Session::start() { - if (!session_thread_) { - session_thread_ = new thread(function_bind(&Session::run, this)); + { + /* Signal session thread to start rendering. */ + thread_scoped_lock session_thread_lock(session_thread_mutex_); + assert(session_thread_state_ == SESSION_THREAD_WAIT); + session_thread_state_ = SESSION_THREAD_RENDER; } + + session_thread_cond_.notify_all(); } void Session::cancel(bool quick) { - if (quick && path_trace_) { - path_trace_->cancel(); + /* Check if session thread is rendering. */ + bool rendering; + { + thread_scoped_lock session_thread_lock(session_thread_mutex_); + rendering = (session_thread_state_ == SESSION_THREAD_RENDER); } - if (session_thread_) { - /* wait for session thread to end */ + if (rendering) { + /* Cancel path trace operations. */ + if (quick && path_trace_) { + path_trace_->cancel(); + } + + /* Cancel other operations. */ progress.set_cancel("Exiting"); + /* Signal unpause in case the render was paused. */ { thread_scoped_lock pause_lock(pause_mutex_); pause_ = false; - cancel_ = true; } pause_cond_.notify_all(); + /* Wait for render thread to be cancelled or finished. */ wait(); } } @@ -192,11 +220,46 @@ void Session::run_main_render_loop() break; } } +} + +void Session::thread_run() +{ + while (true) { + { + thread_scoped_lock session_thread_lock(session_thread_mutex_); + + if (session_thread_state_ == SESSION_THREAD_WAIT) { + /* Continue waiting for any signal from the main thread. */ + session_thread_cond_.wait(session_thread_lock); + continue; + } + else if (session_thread_state_ == SESSION_THREAD_END) { + /* End thread immediately. */ + break; + } + } + + /* Execute a render. */ + thread_render(); + + /* Go back from rendering to waiting. */ + { + thread_scoped_lock session_thread_lock(session_thread_mutex_); + if (session_thread_state_ == SESSION_THREAD_RENDER) { + session_thread_state_ = SESSION_THREAD_WAIT; + } + } + session_thread_cond_.notify_all(); + } + /* Flush any remaining operations and destroy display driver here. This ensure + * graphics API resources are created and destroyed all in the session thread, + * which can avoid problems contexts and multiple threads. */ path_trace_->flush_display(); + path_trace_->set_display_driver(nullptr); } -void Session::run() +void Session::thread_render() { if (params.use_profiling && (params.device.type == DEVICE_CPU)) { profiler.start(); @@ -338,9 +401,9 @@ bool Session::run_wait_for_work(const RenderWork &render_work) const bool no_work = !render_work; update_status_time(pause_, no_work); - /* Only leave the loop when rendering is not paused. But even if the current render is un-paused - * but there is nothing to render keep waiting until new work is added. */ - while (!cancel_) { + /* Only leave the loop when rendering is not paused. But even if the current render is + * un-paused but there is nothing to render keep waiting until new work is added. */ + while (!progress.get_cancel()) { scoped_timer pause_timer; if (!pause_ && (render_work || new_work_added_ || delayed_reset_.do_reset)) { @@ -427,7 +490,8 @@ void Session::do_delayed_reset() tile_manager_.update(buffer_params_, scene); /* Update temp directory on reset. - * This potentially allows to finish the existing rendering with a previously configure temporary + * This potentially allows to finish the existing rendering with a previously configure + * temporary * directory in the host software and switch to a new temp directory when new render starts. */ tile_manager_.set_temp_dir(params.temp_dir); @@ -544,12 +608,14 @@ double Session::get_estimated_remaining_time() const void Session::wait() { - if (session_thread_) { - session_thread_->join(); - delete session_thread_; + /* Wait until session thread either is waiting or ending. */ + while (true) { + thread_scoped_lock session_thread_lock(session_thread_mutex_); + if (session_thread_state_ != SESSION_THREAD_RENDER) { + break; + } + session_thread_cond_.wait(session_thread_lock); } - - session_thread_ = nullptr; } bool Session::update_scene(int width, int height) diff --git a/intern/cycles/session/session.h b/intern/cycles/session/session.h index adfd1346600..4017964d4aa 100644 --- a/intern/cycles/session/session.h +++ b/intern/cycles/session/session.h @@ -172,7 +172,8 @@ class Session { BufferParams buffer_params; } delayed_reset_; - void run(); + void thread_run(); + void thread_render(); /* Update for the new iteration of the main loop in run implementation (run_cpu and run_gpu). * @@ -205,10 +206,19 @@ class Session { int2 get_effective_tile_size() const; - thread *session_thread_; + /* Session thread that performs rendering tasks decoupled from the thread + * controlling the sessions. The thread is created and destroyed along with + * the session. */ + thread *session_thread_ = nullptr; + thread_condition_variable session_thread_cond_; + thread_mutex session_thread_mutex_; + enum { + SESSION_THREAD_WAIT, + SESSION_THREAD_RENDER, + SESSION_THREAD_END, + } session_thread_state_ = SESSION_THREAD_WAIT; bool pause_ = false; - bool cancel_ = false; bool new_work_added_ = false; thread_condition_variable pause_cond_; -- cgit v1.2.3