video_thread_wrapper: a driver can run a call on the thread waiting for its reply

The gl driver rebuilds its HW-render framebuffers when a shader change
needs more history textures than it has. Framebuffers are not shared
between contexts, so they must be made in the core's, and under the
hardware ring that context is current on the core's thread, not the
video thread the shader command runs on. The wrapper gains what that
needs: from the video thread, while it is answering a synchronous
command, video_thread_call_on_waiter() runs a call on the thread
waiting for the reply and returns when it has run. That thread is the
core's, which is inside its wait and can service it. With no command
in flight, or off the video thread, the call runs on the caller as it
always did; threadless builds get a macro stand-in.

gl2's shader change marshals the teardown and rebuild of the HW-render
objects through it, with a flush between so the new textures are
visible to the other context before its framebuffers attach them; the
binds inside those paths are skipped under the ring, where the context
is already current where the work runs. gl3's shader change never
touched its HW target, so it needed only the bind at its end gated.

The harness models it: a driver whose set_shader asks for the call,
checked to run on the waiting thread, before the reply, once. Its
first run caught a race: the video thread can take a command and ask
before the sender has reached its wait, so waiters are counted from
the send. And a subframe read of swap_count on the main thread, the
one site of three without the wrapper guard, gets it.

gfx/drivers/gl2.c

@@ -1724,18 +1724,27 @@ static void gl2_renderchain_deinit_fbo(gl2_t *gl,
    }
 }
 
+/* Take or give back the core's context around work on its objects.
+ * Under the wrapper's hardware ring the core's context is current on
+ * the main thread and that work is marshalled there, where it already
+ * is current: nothing to bind, and binding would move it. */
+static void gl2_bind_core_context(gl2_t *gl, bool enable)
+{
+   if (     (gl->flags & GL2_FLAG_SHARED_CONTEXT_USE)
+         && !(gl->flags & GL2_FLAG_HW_RING))
+      gl->ctx_driver->bind_hw_render(gl->ctx_data, enable);
+}
+
 static void gl2_renderchain_deinit_hw_render(gl2_t *gl, gl2_renderchain_data_t *chain)
 {
-   if (gl->flags    & GL2_FLAG_SHARED_CONTEXT_USE)
-      gl->ctx_driver->bind_hw_render(gl->ctx_data, true);
+   gl2_bind_core_context(gl, true);
    if (gl->flags    & GL2_FLAG_HW_RENDER_FBO_INIT)
       gl2_delete_fb(gl->textures, gl->hw_render_fbo);
    if (chain->flags & GL2_CHAIN_FLAG_HW_RENDER_DEPTH_INIT)
       gl2_delete_rb(gl->textures, chain->hw_render_depth);
    gl->flags &= ~GL2_FLAG_HW_RENDER_FBO_INIT;
 
-   if (gl->flags    & GL2_FLAG_SHARED_CONTEXT_USE)
-      gl->ctx_driver->bind_hw_render(gl->ctx_data, false);
+   gl2_bind_core_context(gl, false);
 }
 
 static bool gl2_create_fbo_targets(gl2_t *gl, gl2_renderchain_data_t *chain)
@@ -2222,8 +2231,7 @@ static bool gl2_renderchain_init_hw_render(
 
    /* We can only share texture objects through contexts.
     * FBOs are "abstract" objects and are not shared. */
-   if (gl->flags & GL2_FLAG_SHARED_CONTEXT_USE)
-      gl->ctx_driver->bind_hw_render(gl->ctx_data, true);
+   gl2_bind_core_context(gl, true);
 
    RARCH_LOG("[GL] Initializing HW render (%ux%u).\n", width, height);
    glGetIntegerv(GL_MAX_TEXTURE_SIZE, &max_fbo_size);
@@ -2305,8 +2313,7 @@ static bool gl2_renderchain_init_hw_render(
    gl2_renderchain_bind_backbuffer();
    gl->flags |= GL2_FLAG_HW_RENDER_FBO_INIT;
 
-   if (gl->flags & GL2_FLAG_SHARED_CONTEXT_USE)
-      gl->ctx_driver->bind_hw_render(gl->ctx_data, false);
+   gl2_bind_core_context(gl, false);
    return true;
 }
 
@@ -5821,6 +5828,21 @@ static void gl2_update_tex_filter_frame(gl2_t *gl, bool video_smooth)
       gl->ctx_driver->bind_hw_render(gl->ctx_data, true);
 }
 
+/* The HW-render rebuild on a shader change, as a call the wrapper can
+ * run on the core's thread; see gl2_set_shader. */
+static void gl2_deinit_hw_render_cb(void *data)
+{
+   gl2_t *gl = (gl2_t*)data;
+   gl2_renderchain_deinit_hw_render(gl, (gl2_renderchain_data_t*)gl->renderchain_data);
+}
+
+static void gl2_init_hw_render_cb(void *data)
+{
+   gl2_t *gl = (gl2_t*)data;
+   gl2_renderchain_init_hw_render(gl,
+         (gl2_renderchain_data_t*)gl->renderchain_data, gl->tex_w, gl->tex_h);
+}
+
 static bool gl2_set_shader(void *data,
       enum rarch_shader_type type, const char *path)
 {
@@ -5902,9 +5924,13 @@ static bool gl2_set_shader(void *data,
 
    if (textures > gl->textures) /* Have to reinit a bit. */
    {
+      /* The HW-render framebuffers live in the core's context. Under
+       * the wrapper's ring that context is current on the core's
+       * thread, which is waiting for this command: the teardown and
+       * rebuild go there. Textures and renderbuffers are shared and
+       * can be made here; framebuffers are not. */
       if ((gl->flags & GL2_FLAG_HW_RENDER_USE) && (gl->flags & GL2_FLAG_FBO_INITED))
-         gl2_renderchain_deinit_hw_render(gl, (gl2_renderchain_data_t*)
-               gl->renderchain_data);
+         video_thread_call_on_waiter(gl2_deinit_hw_render_cb, gl);
 
       glDeleteTextures(gl->textures, gl->texture);
 #if defined(HAVE_PSGL)
@@ -5916,11 +5942,12 @@ static bool gl2_set_shader(void *data,
       RARCH_LOG("[GL] Using %u textures.\n", gl->textures);
       gl2_init_textures(gl);
       gl2_init_textures_data(gl);
+      /* The new textures must be visible to the other context before
+       * its framebuffers attach them. */
+      glFlush();
 
       if (gl->flags & GL2_FLAG_HW_RENDER_USE)
-         gl2_renderchain_init_hw_render(gl,
-               (gl2_renderchain_data_t*)gl->renderchain_data,
-               gl->tex_w, gl->tex_h);
+         video_thread_call_on_waiter(gl2_init_hw_render_cb, gl);
    }
 
    gl2_renderchain_init(gl,
@@ -5929,8 +5956,7 @@ static bool gl2_set_shader(void *data,
 
    /* Apparently need to set viewport for passes when we aren't using FBOs. */
    gl2_set_shader_viewports(gl, video_scale_integer);
-   if (gl->flags & GL2_FLAG_SHARED_CONTEXT_USE)
-      gl->ctx_driver->bind_hw_render(gl->ctx_data, true);
+   gl2_bind_core_context(gl, true);
 
    return true;
 

gfx/drivers/gl3.c

@@ -3894,7 +3894,8 @@ static bool gl3_set_shader(void *data,
    if (!gl3_init_filter_chain_with_path(gl, path))
       return false;
 
-   if (gl->flags & GL3_FLAG_USE_SHARED_CONTEXT)
+   if (     (gl->flags & GL3_FLAG_USE_SHARED_CONTEXT)
+         && !(gl->flags & GL3_FLAG_HW_RING))
       gl->ctx_driver->bind_hw_render(gl->ctx_data, true);
 
    return true;

gfx/video_driver.c

@@ -6028,7 +6028,11 @@ void video_driver_frame(const void *data, unsigned width,
    if (render_frame && vid && vid->frame)
    {
       video_info.current_subframe = 0;
-      video_info.swap_count       = video_st->swap_count;
+#ifdef HAVE_THREADS
+      /* The video thread owns and stamps this under the wrapper. */
+      if (!video_st->thread_wrapper_active)
+#endif
+         video_info.swap_count    = video_st->swap_count;
       if (vid->frame(
                video_st->data, data, width, height,
                video_st->frame_count, (unsigned)pitch,

gfx/video_driver.h

@@ -1140,6 +1140,7 @@ bool video_thread_hw_allowed(void);
 /* No wrapper, no video thread: the count is only ever read and written
  * from this thread. */
 #define video_thread_swap_count() (video_state_get_ptr()->swap_count)
+#define video_thread_call_on_waiter(fn, data) ((fn)(data))
 #endif
 
 bool video_driver_has_focus(void);

gfx/video_thread_wrapper.c

@@ -108,6 +108,10 @@ static void video_thread_send_packet(thread_video_t *thr,
 
    thr->send_cmd  = pkt->type;
    thr->reply_cmd = CMD_VIDEO_NONE;
+   /* Counted from the send, not from the wait that follows it: the
+    * video thread may take the command and ask for a call on this
+    * thread before this thread has reached its wait. */
+   thr->waiter_call.waiters++;
 
    scond_signal(thr->cond_thread);
    slock_unlock(thr->lock);
@@ -178,8 +182,25 @@ static void video_thread_wait_reply(thread_video_t *thr, thread_packet_t *pkt)
 
    VIDEO_THREAD_CMD_WAIT_ENTER(thr);
    while (pkt->type != thr->reply_cmd)
+   {
+      if (thr->waiter_call.pending)
+      {
+         /* The video thread needs this thread: run its call here, with
+          * the lock dropped, and tell it. */
+         void (*fn)(void*) = thr->waiter_call.fn;
+         void *data        = thr->waiter_call.data;
+         thr->waiter_call.pending = false;
+         slock_unlock(thr->lock);
+         fn(data);
+         slock_lock(thr->lock);
+         thr->waiter_call.done = true;
+         scond_signal(thr->waiter_call.cond);
+         continue;
+      }
       if (!video_thread_pump_wait(thr->cond_reply, thr->lock))
          scond_wait(thr->cond_reply, thr->lock);
+   }
+   thr->waiter_call.waiters--;
    VIDEO_THREAD_CMD_WAIT_LEAVE(thr);
 
    *pkt               = thr->cmd_data;
@@ -255,6 +276,34 @@ static void video_thread_send_and_wait_user_to_thread(thread_video_t *thr, threa
    video_thread_user_release(thr);
 }
 
+void video_thread_call_on_waiter(void (*fn)(void *data), void *data)
+{
+   thread_video_t *thr = (thread_video_t*)video_state_get_ptr()->data;
+   if (!thr || !fn)
+      return;
+   if (!video_driver_thread_wrapper_active() || !video_thread_is_self(thr))
+   {
+      fn(data);
+      return;
+   }
+   slock_lock(thr->lock);
+   if (!thr->waiter_call.waiters)
+   {
+      /* No one to hand it to: the call runs here, as it always did. */
+      slock_unlock(thr->lock);
+      fn(data);
+      return;
+   }
+   thr->waiter_call.fn      = fn;
+   thr->waiter_call.data    = data;
+   thr->waiter_call.done    = false;
+   thr->waiter_call.pending = true;
+   scond_signal(thr->cond_reply);
+   while (!thr->waiter_call.done)
+      scond_wait(thr->waiter_call.cond, thr->lock);
+   slock_unlock(thr->lock);
+}
+
 static void thread_update_driver_state(thread_video_t *thr)
 {
 #ifdef HAVE_MENU
@@ -1268,6 +1317,8 @@ static bool video_thread_init(thread_video_t *thr,
       return false;
    if (!(thr->frame.lock  = slock_new()))
       return false;
+   if (!(thr->waiter_call.cond = scond_new()))
+      return false;
    if (!(thr->cond_reply  = scond_new()))
       return false;
    if (!(thr->cond_ring   = scond_new()))
@@ -1461,6 +1512,7 @@ static void video_thread_free(void *data)
       slock_free(thr->alpha_lock);
       slock_free(thr->lock);
       scond_free(thr->cond_reply);
+      scond_free(thr->waiter_call.cond);
       scond_free(thr->cond_ring);
       scond_free(thr->cond_thread);
       scond_free(thr->cond_user);

gfx/video_thread_wrapper.h

@@ -383,6 +383,23 @@ typedef struct thread_video
     * type; only asserted on in debug builds. */
    uintptr_t cond_reply_waiter;
    unsigned cond_reply_waiters;
+   /* A call the video thread needs run on the thread that is waiting
+    * for its reply - the core's thread, which holds the core's GL
+    * context. Posted from the video thread while a synchronous command
+    * is in flight, run by the waiter inside its wait loop, and the
+    * video thread waits for done. See video_thread_call_on_waiter(). */
+   struct
+   {
+      void (*fn)(void *data);
+      void *data;
+      scond_t *cond;
+      /* The number of commands sent and not yet answered, each with a
+       * thread that will wait for the reply and can service a call
+       * there; without one, the caller runs it itself. */
+      unsigned waiters;
+      bool pending;
+      bool done;
+   } waiter_call;
 
    bool alive;
    bool focus;
@@ -454,6 +471,13 @@ bool video_thread_pacing_stats(bool *display_pacing,
  * wrapper (or from the video thread) it is the plain value. */
 uint64_t video_thread_swap_count(void);
 
+/* From the video thread, while it is answering a synchronous command:
+ * runs fn on the thread waiting for the reply and returns when it has
+ * run. That thread is the core's, which is where a driver must go to
+ * touch the core's GL context under the hardware ring. From any other
+ * thread, or with no waiter, fn simply runs on the caller. */
+void video_thread_call_on_waiter(void (*fn)(void *data), void *data);
+
 void video_thread_wait_idle(void);
 
 RETRO_END_DECLS

samples/gfx/threaded_video/threaded_video_test.c

@@ -941,6 +941,72 @@ static void lane_display_pacing(void)
 /*   frames from the lent slot. With it off, no ask is granted.         */
 /* ------------------------------------------------------------------ */
 
+/* A driver answering a synchronous command on the video thread can ask
+ * for a call on the thread that is waiting for the reply - the core's
+ * thread, which holds the core's GL context under the hardware ring.
+ * The gl driver uses it to rebuild the core's framebuffers on a shader
+ * change. Modelled here: the driver's set_shader asks for one and the
+ * lane checks the call ran on the main thread, before the reply, and
+ * exactly once. */
+static video_driver_t        wclane_driver;
+static const video_driver_t *wclane_inner;
+static uintptr_t             wclane_call_thread;
+static unsigned              wclane_calls;
+static bool                  wclane_before_reply;
+
+static void wclane_cb(void *data)
+{
+   (void)data;
+   wclane_call_thread = sthread_get_current_thread_id();
+   wclane_calls++;
+}
+
+static bool wclane_set_shader(void *data, enum rarch_shader_type type, const char *path)
+{
+   bool ret;
+   video_thread_call_on_waiter(wclane_cb, NULL);
+   wclane_before_reply = (wclane_calls == 1);
+   ret = wclane_inner->set_shader ? wclane_inner->set_shader(data, type, path) : true;
+   return ret;
+}
+
+static void lane_waiter_call(void)
+{
+   thread_video_t *thr;
+   uintptr_t self;
+   video_driver_state_t *video_st = video_state_get_ptr();
+
+   set_threaded_via_setting(true);
+   run_frames(3);
+   expect_wrapper(true, "waiter-call lane");
+   thr           = (thread_video_t*)video_st->data;
+   video_thread_wait_idle();
+   wclane_inner  = thr->driver;
+   wclane_driver = *thr->driver;
+   wclane_driver.set_shader = wclane_set_shader;
+   thr->driver   = &wclane_driver;
+   wclane_calls  = 0;
+   wclane_call_thread = 0;
+   self          = sthread_get_current_thread_id();
+
+   /* Through the wrapper: the main thread sends set_shader and waits;
+    * the video thread's driver asks for the call. */
+   video_st->current_video->set_shader(video_st->data, RARCH_SHADER_NONE, NULL);
+
+   CHECK(wclane_calls == 1, "waiter call ran %u times, expected 1", wclane_calls);
+   CHECK(wclane_call_thread == self,
+         "waiter call ran on the wrong thread (not the one waiting for the reply)");
+   CHECK(wclane_before_reply, "waiter call had not run when the command continued");
+
+   /* With no waiter - called outside a command, from the video thread's
+    * own frame - it must run on the caller rather than hang: the driver
+    * asks from inside frame() here. */
+   video_thread_wait_idle();
+   thr->driver   = wclane_inner;
+   run_frames(2);
+   printf("   waiter-call lane: call ran on the waiting thread, before the reply\n");
+}
+
 static void lane_zero_copy(void)
 {
    unsigned had = failures;
@@ -1079,6 +1145,7 @@ int main(int argc, char *argv[])
    lane_font_marshal();
    lane_display_pacing();
    lane_zero_copy();
+   lane_waiter_call();
 
    /* Orderly shutdown: the teardown barriers are part of what is
     * under test. */