Render lock: keep a patch load's flush out of an in-progress render

src/amy.c

@@ -92,70 +92,102 @@ void amy_profiles_print() {}
 #include "clipping_lookup_table.h"
 
 
-// Set up the mutex for accessing the queue during rendering (for multicore)
+// Two locks, always taken in this order when both are needed:
+//
+//   render lock  held by the render thread for a whole block (flush, render,
+//                mix), and by an ingest thread for the flush it runs before a
+//                patch load. So a load's frees and resets can't run while a
+//                render is reading oscs. Only the flush, not the load: a load
+//                takes tens of ms on an ESP32-S3, far longer than a block.
+//                Recursive for its owner: a render-thread hook or sequenced
+//                message can load a patch mid-block, and the render thread's
+//                own amy_execute_deltas() takes it inside the block's hold.
+//   queue lock   guards the delta queue itself (add_delta_to_queue, the
+//                flush), held briefly. Ordinary events take only this one, so
+//                a note-on never waits for a render.
+//
+// Each platform supplies a plain lock; the recursion is built on top, below,
+// from a per-thread depth count.
+
+#if defined(_MSC_VER)
+#define AMY_TLS __declspec(thread)
+#else
+#define AMY_TLS _Thread_local
+#endif
 
 #ifdef __EMSCRIPTEN__
 #include <emscripten/threading.h>
 #include <emscripten/wasm_worker.h>
-emscripten_lock_t amy_queue_lock = EMSCRIPTEN_LOCK_T_STATIC_INITIALIZER;
-void amy_grab_lock() {
-    emscripten_lock_busyspin_wait_acquire(&amy_queue_lock, 100);
-}
-void amy_release_lock() {
-    emscripten_lock_release(&amy_queue_lock);
-}
-void amy_init_lock() {
-}
+typedef emscripten_lock_t amy_lock_t;
+static void lock_init(amy_lock_t *l) { emscripten_lock_init(l); }
+static void lock_take(amy_lock_t *l) { emscripten_lock_busyspin_wait_acquire(l, 100); }
+static void lock_give(amy_lock_t *l) { emscripten_lock_release(l); }
+#define AMY_THREAD_LOCAL AMY_TLS
 
 #elif defined _WIN32
-CRITICAL_SECTION amy_queue_lock;
-void amy_grab_lock() {
-    EnterCriticalSection(&amy_queue_lock);
-}
-void amy_release_lock() {
-    LeaveCriticalSection(&amy_queue_lock);
-}
-void amy_init_lock() {
-    InitializeCriticalSection(&amy_queue_lock);
-}
+typedef CRITICAL_SECTION amy_lock_t;
+static void lock_init(amy_lock_t *l) { InitializeCriticalSection(l); }
+static void lock_take(amy_lock_t *l) { EnterCriticalSection(l); }
+static void lock_give(amy_lock_t *l) { LeaveCriticalSection(l); }
+#define AMY_THREAD_LOCAL AMY_TLS
+
 #elif defined _POSIX_THREADS
-pthread_mutex_t amy_queue_lock;
-void amy_grab_lock() {
-    pthread_mutex_lock(&amy_queue_lock);
-}
-void amy_release_lock() {
-    pthread_mutex_unlock(&amy_queue_lock);
-}
-void amy_init_lock() {
-    pthread_mutex_init(&amy_queue_lock, NULL);
-}
-#elif defined ESP_PLATFORM
+typedef pthread_mutex_t amy_lock_t;
+static void lock_init(amy_lock_t *l) { pthread_mutex_init(l, NULL); }
+static void lock_take(amy_lock_t *l) { pthread_mutex_lock(l); }
+static void lock_give(amy_lock_t *l) { pthread_mutex_unlock(l); }
+#define AMY_THREAD_LOCAL AMY_TLS
 
+#elif defined ESP_PLATFORM
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
 #include "freertos/semphr.h"
-SemaphoreHandle_t amy_queue_lock;
+// A FreeRTOS mutex, so a low-priority task holding it is boosted while the
+// render task waits on it.
+typedef SemaphoreHandle_t amy_lock_t;
+static void lock_init(amy_lock_t *l) { *l = xSemaphoreCreateMutex(); }
+static void lock_take(amy_lock_t *l) { xSemaphoreTake(*l, portMAX_DELAY); }
+static void lock_give(amy_lock_t *l) { xSemaphoreGive(*l); }
+#define AMY_THREAD_LOCAL AMY_TLS
+
+#else
+// Single-threaded (or not yet locked) platforms.
+typedef int amy_lock_t;
+static void lock_init(amy_lock_t *l) { (void)l; }
+static void lock_take(amy_lock_t *l) { (void)l; }
+static void lock_give(amy_lock_t *l) { (void)l; }
+// No threads to tell apart, and no promise of thread-local storage.
+#define AMY_THREAD_LOCAL
+#endif
+
+amy_lock_t amy_queue_lock;   // extern in amy.h on Windows and POSIX
+static amy_lock_t amy_render_lock;
+// How deep this thread is in the render lock. Thread-local, so each thread
+// sees only its own nesting and nothing is shared: > 0 means this thread
+// holds the lock.
+static AMY_THREAD_LOCAL int render_lock_depth = 0;
 
 void amy_grab_lock() {
-    xSemaphoreTake(amy_queue_lock, portMAX_DELAY);
+    lock_take(&amy_queue_lock);
 }
 void amy_release_lock() {
-    xSemaphoreGive( amy_queue_lock );
+    lock_give(&amy_queue_lock);
 }
-void amy_init_lock() {
-    amy_queue_lock = xSemaphoreCreateMutex();
-}
-#else
 
-void amy_grab_lock() {
+void amy_grab_render_lock() {
+    if (render_lock_depth++ > 0)  return;   // already ours
+    lock_take(&amy_render_lock);
 }
-void amy_release_lock() {
+void amy_release_render_lock() {
+    if (--render_lock_depth == 0)
+        lock_give(&amy_render_lock);
 }
+
 void amy_init_lock() {
+    lock_init(&amy_queue_lock);
+    lock_init(&amy_render_lock);
 }
 
-#endif
-
 
 
 // Global state 
@@ -851,8 +883,16 @@ void amy_event_to_deltas_queue(amy_event *e, uint16_t base_osc, uint16_t oscs_pe
         if (AMY_IS_SET(e->patch_number) || AMY_IS_SET(e->num_voices) || AMY_IS_SET(e->oscs_per_voice)) {
             // Settle pending deltas without running the sequencer tick
             // service - this can execute on any sending thread (see
-            // flush_due_deltas).
+            // flush_due_deltas). A queued reset has to land before the load
+            // rebuilds the synth tables, or it wipes them afterwards.
+            // The flush runs under the render lock: it can free oscs (a
+            // released voice, a reset) that a render in progress is reading.
+            // The load itself does not: it takes tens of ms on an ESP32-S3
+            // (a 6-voice DX7 load ~50-70 ms), and a render held off that long
+            // runs the DMA ring dry. The flush is µs.
+            amy_grab_render_lock();
             flush_due_deltas();
+            amy_release_render_lock();
             patches_load_patch(e);
         }
         // Execute any other commands in this event.
@@ -2413,7 +2453,13 @@ void amy_execute_deltas() {
     sequencer_check_and_fill();
     // Make sure any CV-triggered events are added to delta queue
     update_external_cv_in();
+    // The flush can free oscs, so it runs under the render lock. Render loops
+    // already hold it across the whole block (it's recursive for them); this
+    // covers callers that execute deltas off the render thread (parse.c's
+    // sample-transfer start).
+    amy_grab_render_lock();
     flush_due_deltas();
+    amy_release_render_lock();
     AMY_PROFILE_STOP(AMY_EXECUTE_DELTAS)
 
 }

src/amy.h

@@ -1143,6 +1143,10 @@ int8_t global_init(amy_config_t c);
 void global_deinit();
 void amy_grab_lock();
 void amy_release_lock();
+// Held by the render thread across each block, and by an ingest thread across
+// a patch load; recursive for its owner. Take it before the queue lock.
+void amy_grab_render_lock();
+void amy_release_render_lock();
 void amy_deltas_reset();
 void add_delta_to_queue(struct delta *d, struct delta **queue);
 void amy_add_event_internal(amy_event *e, uint16_t base_osc);

src/api.c

@@ -250,9 +250,14 @@ void *amy_get_external_hook_context(void) {
 }
 
 output_sample_type * amy_simple_fill_buffer() {
+    // One block under the render lock: no patch load's frees or resets can
+    // run between this block's flush and the end of its mix.
+    amy_grab_render_lock();
     amy_execute_deltas();
     amy_render(0, AMY_OSCS, 0);
-    return amy_fill_buffer();
+    output_sample_type *block = amy_fill_buffer();
+    amy_release_render_lock();
+    return block;
 }
 
 

src/i2s.c

@@ -367,6 +367,11 @@ void esp_fill_audio_buffer_task() {
 #ifdef ARDUINO_SPEEDTEST
         int64_t _rl_start_t = esp_timer_get_time();
 #endif // ARDUINO_SPEEDTEST
+        // The whole block, flush to mix, under the render lock (released
+        // before the i2s write, so a patch load gets in while we wait on the
+        // DMA). esp_render_task renders its half on the other core inside
+        // this hold without taking the lock itself.
+        amy_grab_render_lock();
         // Get ready to render
         amy_execute_deltas();
 
@@ -375,6 +380,7 @@ void esp_fill_audio_buffer_task() {
 
         // Write to i2s
         output_sample_type *block = amy_fill_buffer();
+        amy_release_render_lock();
         uint32_t busy_us = (uint32_t)(amy_get_us() - t);
 	AMY_PROFILE_STOP(AMY_ESP_FILL_BUFFER)
 
@@ -503,10 +509,13 @@ int16_t *amy_render_audio() {
             xTaskNotifyGive(amy_fill_buffer_handle);  // to esp_fill_audio_buffer_task:!AMY_HAS_I2S
         }
     } else {
-        // No multithread, we have to render here.
+        // No multithread, we have to render here. (amy_update_tasks()
+        // already flushed, under the render lock.)
         int64_t t0 = amy_get_us();
+        amy_grab_render_lock();
         esp_render_on_cores();
         buf = amy_fill_buffer();
+        amy_release_render_lock();
         amy_overload_check((uint32_t)(amy_get_us() - t0));
     }
     return buf;