From 88f812bf9ad538d0c96149b751c3db75c29ae4b5 Mon Sep 17 00:00:00 2001 From: Miguel Pozo Date: Wed, 9 Jul 2025 15:11:29 +0200 Subject: [PATCH] Fix #141253: Bring back the global Draw lock Alternative solution to #141392 / #141564. As a recap, the DST global lock (which prevented running drawing code from multiple threads concurrently) was removed for 4.5 (#134690). One unforeseen issue is that Images (and their GPUTextures) are shared across dependency graphs (and therefore multiple threads), meaning we are running into data race issues with them. @fclem did #141392 and I continued it #141564. However, this is only a partial solution, parts of the GPUTexture API and the whole BKE_image API are still unsafe. Trying to solve all the possible underlying issues seems unrealistic for 4.5 given the time frame and that the extension of the code affected by this issue is quite large. So this PR just brings the 4.4 locking behavior instead, which, while risky on its own, seems much safer to me than the alternative. This effectively undoes the improvements from #134690 by disabling concurrent rendering, but instead of reverting all the code, it just ensures we hold the lock in the same places we did in 4.4. This means there's some redundant code that is not technically needed anymore, like the `submission_mutex`, but it's probably best to make as few modifications as possible, given how close we are to release and that this is only intended as a temporary measure. Pull Request: https://projects.blender.org/blender/blender/pulls/141618 --- source/blender/draw/DRW_engine.hh | 13 +++++++-- .../draw/engines/eevee/eevee_material.cc | 6 +++- .../blender/draw/intern/draw_gpu_context.cc | 29 ++++++++++++++++--- source/blender/draw/tests/draw_testing.cc | 12 ++++---- 4 files changed, 46 insertions(+), 14 deletions(-) diff --git a/source/blender/draw/DRW_engine.hh b/source/blender/draw/DRW_engine.hh index 6c84017af5d..9b6802a6e07 100644 --- a/source/blender/draw/DRW_engine.hh +++ b/source/blender/draw/DRW_engine.hh @@ -128,13 +128,20 @@ void DRW_render_gpencil(RenderEngine *engine, Depsgraph *depsgraph); void DRW_render_context_enable(Render *render); void DRW_render_context_disable(Render *render); +void DRW_mutexes_init(); +void DRW_mutexes_exit(); + +/* Mutex to lock the drw manager and avoid concurrent context usage. + * Equivalent to the old DST lock. + * Brought back to 4.5 due to unforeseen issues causing data races and race conditions with Images + * and GPUTextures. (See #141253) */ +void DRW_lock_start(); +void DRW_lock_end(); + /* Critical section for GPUShader usage. Can be removed when we have threadsafe GPUShader class. */ void DRW_submission_start(); void DRW_submission_end(); -void DRW_submission_mutex_init(); -void DRW_submission_mutex_exit(); - void DRW_gpu_context_create(); void DRW_gpu_context_destroy(); /** diff --git a/source/blender/draw/engines/eevee/eevee_material.cc b/source/blender/draw/engines/eevee/eevee_material.cc index f3695243596..a9fe0d71330 100644 --- a/source/blender/draw/engines/eevee/eevee_material.cc +++ b/source/blender/draw/engines/eevee/eevee_material.cc @@ -207,13 +207,17 @@ void MaterialModule::end_sync() for (auto i : range) { GPUMaterialTexture *tex = texture_loading_queue_[i]; ImageUser *iuser = tex->iuser_available ? &tex->iuser : nullptr; - BKE_image_tag_time(tex->ima); BKE_image_get_tile(tex->ima, 0); ImBuf *imbuf = BKE_image_acquire_ibuf(tex->ima, iuser, nullptr); BKE_image_release_ibuf(tex->ima, imbuf, nullptr); } }); + /* Tag time is not thread-safe. */ + for (GPUMaterialTexture *tex : texture_loading_queue_) { + BKE_image_tag_time(tex->ima); + } + /* Upload to the GPU (create GPUTexture). This part still requires a valid GPU context and * is not easily parallelized. */ for (GPUMaterialTexture *tex : texture_loading_queue_) { diff --git a/source/blender/draw/intern/draw_gpu_context.cc b/source/blender/draw/intern/draw_gpu_context.cc index 6b55bfa644a..70b579b6c03 100644 --- a/source/blender/draw/intern/draw_gpu_context.cc +++ b/source/blender/draw/intern/draw_gpu_context.cc @@ -28,18 +28,33 @@ * between render engine instances, we cannot allow pass submissions in a concurrent manner. * \{ */ +static TicketMutex *draw_mutex = nullptr; static TicketMutex *submission_mutex = nullptr; -void DRW_submission_mutex_init() +void DRW_mutexes_init() { + draw_mutex = BLI_ticket_mutex_alloc(); submission_mutex = BLI_ticket_mutex_alloc(); } -void DRW_submission_mutex_exit() +void DRW_mutexes_exit() { + BLI_ticket_mutex_free(draw_mutex); BLI_ticket_mutex_free(submission_mutex); } +void DRW_lock_start() +{ + bool locked = BLI_ticket_mutex_lock_check_recursive(draw_mutex); + BLI_assert(locked); + UNUSED_VARS_NDEBUG(locked); +} + +void DRW_lock_end() +{ + BLI_ticket_mutex_unlock(draw_mutex); +} + void DRW_submission_start() { bool locked = BLI_ticket_mutex_lock_check_recursive(submission_mutex); @@ -95,6 +110,7 @@ class ContextShared { void enable() { + DRW_lock_start(); /* IMPORTANT: We don't support immediate mode in render mode! * This shall remain in effect until immediate mode supports * multiple threads. */ @@ -124,6 +140,7 @@ class ContextShared { GPU_render_end(); BLI_ticket_mutex_unlock(mutex_); + DRW_lock_end(); } }; @@ -145,7 +162,7 @@ void DRW_gpu_context_create() { BLI_assert(viewport_context == nullptr); /* Ensure it's called once */ - DRW_submission_mutex_init(); + DRW_mutexes_init(); viewport_context = MEM_new(__func__); preview_context = MEM_new(__func__); @@ -164,7 +181,7 @@ void DRW_gpu_context_destroy() if (viewport_context == nullptr) { return; } - DRW_submission_mutex_exit(); + DRW_mutexes_exit(); MEM_SAFE_DELETE(viewport_context); MEM_SAFE_DELETE(preview_context); @@ -237,12 +254,14 @@ void DRW_system_gpu_render_context_enable(void *re_system_gpu_context) /* If thread is main you should use DRW_gpu_context_enable(). */ BLI_assert(!BLI_thread_is_main()); + DRW_lock_start(); WM_system_gpu_context_activate(re_system_gpu_context); } void DRW_system_gpu_render_context_disable(void *re_system_gpu_context) { WM_system_gpu_context_release(re_system_gpu_context); + DRW_lock_end(); } void DRW_blender_gpu_render_context_enable(void *re_gpu_context) @@ -346,6 +365,7 @@ void DRW_xr_drawing_begin() { /* XXX: See comment on #DRW_system_gpu_context_get(). */ + DRW_lock_start(); BLI_ticket_mutex_lock(viewport_context->mutex_); } @@ -354,6 +374,7 @@ void DRW_xr_drawing_end() /* XXX: See comment on #DRW_system_gpu_context_get(). */ BLI_ticket_mutex_unlock(viewport_context->mutex_); + DRW_lock_end(); } #endif diff --git a/source/blender/draw/tests/draw_testing.cc b/source/blender/draw/tests/draw_testing.cc index 86f541f4191..a21fd5c5958 100644 --- a/source/blender/draw/tests/draw_testing.cc +++ b/source/blender/draw/tests/draw_testing.cc @@ -14,12 +14,12 @@ namespace blender::draw { void DrawOpenGLTest::SetUp() { GPUOpenGLTest::SetUp(); - DRW_submission_mutex_init(); + DRW_mutexes_init(); } void DrawOpenGLTest::TearDown() { - DRW_submission_mutex_exit(); + DRW_mutexes_exit(); GPUOpenGLTest::TearDown(); } #endif @@ -28,12 +28,12 @@ void DrawOpenGLTest::TearDown() void DrawMetalTest::SetUp() { GPUMetalTest::SetUp(); - DRW_submission_mutex_init(); + DRW_mutexes_init(); } void DrawMetalTest::TearDown() { - DRW_submission_mutex_exit(); + DRW_mutexes_exit(); GPUMetalTest::TearDown(); } #endif @@ -42,12 +42,12 @@ void DrawMetalTest::TearDown() void DrawVulkanTest::SetUp() { GPUVulkanTest::SetUp(); - DRW_submission_mutex_init(); + DRW_mutexes_init(); } void DrawVulkanTest::TearDown() { - DRW_submission_mutex_exit(); + DRW_mutexes_exit(); GPUVulkanTest::TearDown(); } #endif