From 0cf18ae044739421824cf5fc381ce02f57f711b6 Mon Sep 17 00:00:00 2001
From: Jakob Bornecrantz <jakob@collabora.com>
Date: Sat, 10 Dec 2022 17:00:07 +0000
Subject: [PATCH] c/client: Refactor context mutex locking to helper function

---
 src/xrt/compositor/client/comp_gl_client.c    | 30 +++----
 src/xrt/compositor/client/comp_gl_client.h    | 79 +++++++++++++++----
 .../client/comp_gl_memobj_swapchain.c         |  4 +-
 .../compositor/client/comp_gl_win32_client.c  | 19 +++--
 .../compositor/client/comp_gl_xlib_client.c   | 21 +++--
 5 files changed, 101 insertions(+), 52 deletions(-)

diff --git a/src/xrt/compositor/client/comp_gl_client.c b/src/xrt/compositor/client/comp_gl_client.c
index f145cb9a8..250d7f96e 100644
--- a/src/xrt/compositor/client/comp_gl_client.c
+++ b/src/xrt/compositor/client/comp_gl_client.c
@@ -382,10 +382,10 @@ client_gl_compositor_layer_commit(struct xrt_compositor *xc, int64_t frame_id, x
 
 	sync_handle = XRT_GRAPHICS_SYNC_HANDLE_INVALID;
 
-	xrt_result_t xret = c->context_begin(xc);
+	xrt_result_t xret = client_gl_compositor_context_begin(xc);
 	if (xret == XRT_SUCCESS) {
 		sync_handle = handle_fencing_or_finish(c);
-		c->context_end(xc);
+		client_gl_compositor_context_end(xc);
 	}
 
 	COMP_TRACE_IDENT(layer_commit);
@@ -411,7 +411,7 @@ client_gl_swapchain_create(struct xrt_compositor *xc,
 	struct client_gl_compositor *c = client_gl_compositor(xc);
 	xrt_result_t xret = XRT_SUCCESS;
 
-	xret = c->context_begin(xc);
+	xret = client_gl_compositor_context_begin(xc);
 	if (xret != XRT_SUCCESS) {
 		return xret;
 	}
@@ -420,7 +420,7 @@ client_gl_swapchain_create(struct xrt_compositor *xc,
 		const char *version_str = (const char *)glGetString(GL_VERSION);
 		if (strstr(version_str, "OpenGL ES 2.") == version_str) {
 			U_LOG_E("Only one array layer is supported with OpenGL ES 2");
-			c->context_end(xc);
+			client_gl_compositor_context_end(xc);
 			return XRT_ERROR_SWAPCHAIN_FLAG_VALID_BUT_UNSUPPORTED;
 		}
 	}
@@ -428,7 +428,7 @@ client_gl_swapchain_create(struct xrt_compositor *xc,
 	int64_t vk_format = gl_format_to_vk(info->format);
 	if (vk_format == 0) {
 		U_LOG_E("Invalid format!");
-		c->context_end(xc);
+		client_gl_compositor_context_end(xc);
 		return XRT_ERROR_SWAPCHAIN_FORMAT_UNSUPPORTED;
 	}
 
@@ -439,7 +439,7 @@ client_gl_swapchain_create(struct xrt_compositor *xc,
 
 
 	if (xret != XRT_SUCCESS) {
-		c->context_end(xc);
+		client_gl_compositor_context_end(xc);
 		return xret;
 	}
 	assert(xscn != NULL);
@@ -458,13 +458,13 @@ client_gl_swapchain_create(struct xrt_compositor *xc,
 	if (NULL == c->create_swapchain(xc, info, xscn, &sc)) {
 		// Drop our reference, does NULL checking.
 		xrt_swapchain_reference(&xsc, NULL);
-		c->context_end(xc);
+		client_gl_compositor_context_end(xc);
 		return XRT_ERROR_OPENGL;
 	}
 
 	if (sc == NULL) {
 		U_LOG_E("Could not create OpenGL swapchain.");
-		c->context_end(xc);
+		client_gl_compositor_context_end(xc);
 		return XRT_ERROR_OPENGL;
 	}
 
@@ -483,7 +483,7 @@ client_gl_swapchain_create(struct xrt_compositor *xc,
 
 	glBindTexture(tex_target, prev_texture);
 
-	c->context_end(xc);
+	client_gl_compositor_context_end(xc);
 
 	*out_xsc = &sc->base.base;
 	return XRT_SUCCESS;
@@ -520,13 +520,13 @@ client_gl_compositor_close(struct client_gl_compositor *c)
 bool
 client_gl_compositor_init(struct client_gl_compositor *c,
                           struct xrt_compositor_native *xcn,
-                          client_gl_context_begin_func_t context_begin,
-                          client_gl_context_end_func_t context_end,
+                          client_gl_context_begin_locked_func_t context_begin_locked,
+                          client_gl_context_end_locked_func_t context_end_locked,
                           client_gl_swapchain_create_func_t create_swapchain,
                           client_gl_insert_fence_func_t insert_fence)
 {
-	assert(context_begin != NULL);
-	assert(context_end != NULL);
+	assert(context_begin_locked != NULL);
+	assert(context_end_locked != NULL);
 
 	c->base.base.get_swapchain_create_properties = client_gl_compositor_get_swapchain_create_properties;
 	c->base.base.create_swapchain = client_gl_swapchain_create;
@@ -546,8 +546,8 @@ client_gl_compositor_init(struct client_gl_compositor *c,
 	c->base.base.layer_commit = client_gl_compositor_layer_commit;
 	c->base.base.destroy = client_gl_compositor_destroy;
 	c->base.base.poll_events = client_gl_compositor_poll_events;
-	c->context_begin = context_begin;
-	c->context_end = context_end;
+	c->context_begin_locked = context_begin_locked;
+	c->context_end_locked = context_end_locked;
 	c->create_swapchain = create_swapchain;
 	c->insert_fence = insert_fence;
 	c->xcn = xcn;
diff --git a/src/xrt/compositor/client/comp_gl_client.h b/src/xrt/compositor/client/comp_gl_client.h
index e8a25fce8..0c61c3025 100644
--- a/src/xrt/compositor/client/comp_gl_client.h
+++ b/src/xrt/compositor/client/comp_gl_client.h
@@ -1,4 +1,4 @@
-// Copyright 2019, Collabora, Ltd.
+// Copyright 2019-2022, Collabora, Ltd.
 // SPDX-License-Identifier: BSL-1.0
 /*!
  * @file
@@ -53,20 +53,32 @@ struct client_gl_swapchain
 };
 
 /*!
- * Fetches the OpenGL context that is current on this thread and makes the OpenGL context given in the graphics binding
- * current instead. Only one thread at a time can operate on the sections between @ref client_gl_context_begin_func_t
- * and
- * @ref client_gl_context_end_func_t, therefore client_gl_context_end_func_t MUST be called to avoid blocking the next
- * thread calling @ref client_gl_context_begin_func_t.
+ * Fetches the OpenGL context that is current on this thread and makes the
+ * OpenGL context given in the graphics binding current instead. Only one thread
+ * at a time can operate on the sections between
+ * @ref client_gl_context_begin_locked_func_t and
+ * @ref client_gl_context_end_locked_func_t,
+ * therefore @ref client_gl_context_end_locked_func_t MUST be called to avoid
+ * blocking the next thread calling @ref client_gl_context_begin_locked_func_t.
  *
- * If the return value is not XRT_SUCCESS, @ref client_gl_context_end_func_t should not be called.
+ * This function must be called with the context_mutex locked held, that is
+ * handled by the helper function @ref client_gl_compositor_context_begin.
+ *
+ * If the return value is not XRT_SUCCESS,
+ * @ref client_gl_context_end_locked_func_t should not be called.
  */
-typedef xrt_result_t (*client_gl_context_begin_func_t)(struct xrt_compositor *xc);
+typedef xrt_result_t (*client_gl_context_begin_locked_func_t)(struct xrt_compositor *xc);
 
 /*!
- * Makes the OpenGL context current that was current before @ref client_gl_context_begin_func_t was called.
+ * Makes the OpenGL context current that was current before
+ * @ref client_gl_context_begin_locked_func_t was called.
+ *
+ * This function must be called with the context_mutex locked held, successful
+ * call to @ref client_gl_compositor_context_begin will ensure that. The lock is
+ * not released by this function, but @ref client_gl_compositor_context_end does
+ * release it.
  */
-typedef void (*client_gl_context_end_func_t)(struct xrt_compositor *xc);
+typedef void (*client_gl_context_end_locked_func_t)(struct xrt_compositor *xc);
 
 /*!
  * The type of a swapchain create constructor.
@@ -113,12 +125,12 @@ struct client_gl_compositor
 	/*!
 	 * Function pointer for making the OpenGL context current.
 	 */
-	client_gl_context_begin_func_t context_begin;
+	client_gl_context_begin_locked_func_t context_begin_locked;
 
 	/*!
 	 * Function pointer for restoring prior OpenGL context.
 	 */
-	client_gl_context_end_func_t context_end;
+	client_gl_context_end_locked_func_t context_end_locked;
 
 	/*!
 	 * Function pointer for creating the client swapchain.
@@ -172,8 +184,8 @@ client_gl_compositor(struct xrt_compositor *xc)
 bool
 client_gl_compositor_init(struct client_gl_compositor *c,
                           struct xrt_compositor_native *xcn,
-                          client_gl_context_begin_func_t context_begin,
-                          client_gl_context_end_func_t context_end,
+                          client_gl_context_begin_locked_func_t context_begin,
+                          client_gl_context_end_locked_func_t context_end,
                           client_gl_swapchain_create_func_t create_swapchain,
                           client_gl_insert_fence_func_t insert_fence);
 
@@ -187,6 +199,45 @@ client_gl_compositor_init(struct client_gl_compositor *c,
 void
 client_gl_compositor_close(struct client_gl_compositor *c);
 
+/*!
+ * @copydoc client_gl_context_begin_locked_func_t
+ *
+ * Helper for calling through the function pointer.
+ *
+ * @public @memberof client_gl_compositor
+ */
+static inline xrt_result_t
+client_gl_compositor_context_begin(struct xrt_compositor *xc)
+{
+	struct client_gl_compositor *cgc = client_gl_compositor(xc);
+
+	os_mutex_lock(&cgc->context_mutex);
+
+	xrt_result_t xret = cgc->context_begin_locked(xc);
+	if (xret != XRT_SUCCESS) {
+		os_mutex_unlock(&cgc->context_mutex);
+	}
+
+	return xret;
+}
+
+/*!
+ * @copydoc client_gl_context_end_locked_func_t
+ *
+ * Helper for calling through the function pointer.
+ *
+ * @public @memberof client_gl_compositor
+ */
+static inline void
+client_gl_compositor_context_end(struct xrt_compositor *xc)
+{
+	struct client_gl_compositor *cgc = client_gl_compositor(xc);
+
+	cgc->context_end_locked(xc);
+
+	os_mutex_unlock(&cgc->context_mutex);
+}
+
 
 #ifdef __cplusplus
 }
diff --git a/src/xrt/compositor/client/comp_gl_memobj_swapchain.c b/src/xrt/compositor/client/comp_gl_memobj_swapchain.c
index 00020e90e..977e9d52f 100644
--- a/src/xrt/compositor/client/comp_gl_memobj_swapchain.c
+++ b/src/xrt/compositor/client/comp_gl_memobj_swapchain.c
@@ -50,7 +50,7 @@ client_gl_memobj_swapchain_destroy(struct xrt_swapchain *xsc)
 	uint32_t image_count = sc->base.base.base.image_count;
 
 	struct client_gl_compositor *c = sc->base.gl_compositor;
-	enum xrt_result xret = c->context_begin(&c->base.base);
+	enum xrt_result xret = client_gl_compositor_context_begin(&c->base.base);
 
 	if (image_count > 0) {
 		if (xret == XRT_SUCCESS) {
@@ -64,7 +64,7 @@ client_gl_memobj_swapchain_destroy(struct xrt_swapchain *xsc)
 	}
 
 	if (xret == XRT_SUCCESS) {
-		c->context_end(&c->base.base);
+		client_gl_compositor_context_end(&c->base.base);
 	}
 
 	// Drop our reference, does NULL checking.
diff --git a/src/xrt/compositor/client/comp_gl_win32_client.c b/src/xrt/compositor/client/comp_gl_win32_client.c
index 2071e6af7..d28972ef8 100644
--- a/src/xrt/compositor/client/comp_gl_win32_client.c
+++ b/src/xrt/compositor/client/comp_gl_win32_client.c
@@ -76,14 +76,12 @@ client_gl_win32_compositor_destroy(struct xrt_compositor *xc)
 }
 
 static xrt_result_t
-client_gl_context_begin(struct xrt_compositor *xc)
+client_gl_context_begin_locked(struct xrt_compositor *xc)
 {
 	struct client_gl_win32_compositor *c = client_gl_win32_compositor(xc);
 
 	struct client_gl_context *app_ctx = &c->app_context;
 
-	os_mutex_lock(&c->base.context_mutex);
-
 	context_save_current(&c->temp_context);
 
 	bool need_make_current = !context_matches(&c->temp_context, app_ctx);
@@ -92,8 +90,6 @@ client_gl_context_begin(struct xrt_compositor *xc)
 	        (void *)c->temp_context.hGLRC, (void *)app_ctx->hGLRC);
 
 	if (need_make_current && !context_make_current(app_ctx)) {
-		os_mutex_unlock(&c->base.context_mutex);
-
 		U_LOG_E("Failed to make WGL context current");
 		// No need to restore on failure.
 		return XRT_ERROR_OPENGL;
@@ -103,7 +99,7 @@ client_gl_context_begin(struct xrt_compositor *xc)
 }
 
 static void
-client_gl_context_end(struct xrt_compositor *xc)
+client_gl_context_end_locked(struct xrt_compositor *xc)
 {
 	struct client_gl_win32_compositor *c = client_gl_win32_compositor(xc);
 
@@ -120,8 +116,6 @@ client_gl_context_end(struct xrt_compositor *xc)
 		U_LOG_E("Failed to make old WGL context current!");
 		// fall through to os_mutex_unlock even if we didn't succeed in restoring the context
 	}
-
-	os_mutex_unlock(&c->base.context_mutex);
 }
 
 static GLADapiproc
@@ -228,8 +222,13 @@ client_gl_win32_compositor_create(struct xrt_compositor_native *xcn, void *hDC,
 	// Same for the opengl library handle
 	c->opengl = opengl;
 
-	if (!client_gl_compositor_init(&c->base, xcn, client_gl_context_begin, client_gl_context_end,
-	                               client_gl_memobj_swapchain_create, NULL)) {
+	if (!client_gl_compositor_init(            //
+	        &c->base,                          //
+	        xcn,                               //
+	        client_gl_context_begin_locked,    //
+	        client_gl_context_end_locked,      //
+	        client_gl_memobj_swapchain_create, //
+	        NULL)) {                           //
 		U_LOG_E("Failed to init parent GL client compositor!");
 		FreeLibrary(opengl);
 		free(c);
diff --git a/src/xrt/compositor/client/comp_gl_xlib_client.c b/src/xrt/compositor/client/comp_gl_xlib_client.c
index 668de863f..f7b996d7c 100644
--- a/src/xrt/compositor/client/comp_gl_xlib_client.c
+++ b/src/xrt/compositor/client/comp_gl_xlib_client.c
@@ -1,4 +1,4 @@
-// Copyright 2019, Collabora, Ltd.
+// Copyright 2019-2022, Collabora, Ltd.
 // SPDX-License-Identifier: BSL-1.0
 /*!
  * @file
@@ -74,14 +74,12 @@ client_gl_xlib_compositor_destroy(struct xrt_compositor *xc)
 }
 
 static xrt_result_t
-client_gl_context_begin(struct xrt_compositor *xc)
+client_gl_context_begin_locked(struct xrt_compositor *xc)
 {
 	struct client_gl_xlib_compositor *c = client_gl_xlib_compositor(xc);
 
 	struct client_gl_context *app_ctx = &c->app_context;
 
-	os_mutex_lock(&c->base.context_mutex);
-
 	context_save_current(&c->temp_context);
 
 	bool need_make_current = !context_matches(&c->temp_context, app_ctx);
@@ -90,8 +88,6 @@ client_gl_context_begin(struct xrt_compositor *xc)
 	        (void *)c->temp_context.ctx, (void *)app_ctx->ctx);
 
 	if (need_make_current && !context_make_current(app_ctx)) {
-		os_mutex_unlock(&c->base.context_mutex);
-
 		U_LOG_E("Failed to make GLX context current");
 		// No need to restore on failure.
 		return XRT_ERROR_OPENGL;
@@ -101,7 +97,7 @@ client_gl_context_begin(struct xrt_compositor *xc)
 }
 
 static void
-client_gl_context_end(struct xrt_compositor *xc)
+client_gl_context_end_locked(struct xrt_compositor *xc)
 {
 	struct client_gl_xlib_compositor *c = client_gl_xlib_compositor(xc);
 
@@ -120,8 +116,6 @@ client_gl_context_end(struct xrt_compositor *xc)
 		        (unsigned long)current_glx_context->read, (void *)current_glx_context->ctx);
 		// fall through to os_mutex_unlock even if we didn't succeed in restoring the context
 	}
-
-	os_mutex_unlock(&c->base.context_mutex);
 }
 
 typedef void (*void_ptr_func)();
@@ -198,8 +192,13 @@ client_gl_xlib_compositor_create(struct xrt_compositor_native *xcn,
 	// Move the app context to the struct.
 	c->app_context = app_ctx;
 
-	if (!client_gl_compositor_init(&c->base, xcn, client_gl_context_begin, client_gl_context_end,
-	                               client_gl_memobj_swapchain_create, NULL)) {
+	if (!client_gl_compositor_init(            //
+	        &c->base,                          //
+	        xcn,                               //
+	        client_gl_context_begin_locked,    //
+	        client_gl_context_end_locked,      //
+	        client_gl_memobj_swapchain_create, //
+	        NULL)) {                           //
 		free(c);
 		return NULL;
 	}