swr: refactor swr_create_screen to allow for proper cleanup on error

This makes the following changes to address cleanup issues:
- Error conditions now return NULL instead of calling exit()
- swr_creen is now freed upon error, rather than leak.
- Library handle from dlopen is now closed upon swr_screen destruction

v2: Added additional context in commit msg and remove unnecessary "PUBLIC"
v3: Fix typo in commit message.

Signed-off-by: Chuck Atkins <[email protected]>
Reviewed-by: Emil Velikov <[email protected]>
Cc: Bruce Cherniak <[email protected]>
Cc: Tim Rowley <[email protected]>
cc: [email protected]

4 files changed, 79 insertions, 56 deletions

diff --git a/src/gallium/drivers/swr/swr_loader.cpp b/src/gallium/drivers/swr/swr_loader.cpp
index 7f28bdb5366..01b98046462 100644
--- a/src/gallium/drivers/swr/swr_loader.cpp
+++ b/src/gallium/drivers/swr/swr_loader.cpp
@@ -29,96 +29,98 @@
 #include <stdio.h>
 
 // Helper function to resolve the backend filename based on architecture
-inline void get_swr_arch_filename(const char arch[], char filename[])
+static bool
+swr_initialize_screen_interface(struct swr_screen *screen, const char arch[])
 {
 #ifdef HAVE_SWR_BUILTIN
-   strcpy(filename , "builtin");
+   screen->pLibrary = NULL;
+   screen->pfnSwrGetInterface = SwrGetInterface;
+   fprintf(stderr, "(using: builtin).\n");
 #else
+   char filename[256] = { 0 };
    sprintf(filename, "%sswr%s%s", UTIL_DL_PREFIX, arch, UTIL_DL_EXT);
+
+   screen->pLibrary = util_dl_open(filename);
+   if (!screen->pLibrary) {
+      fprintf(stderr, "(skipping: %s).\n", util_dl_error());
+      return false;
+   }
+
+   util_dl_proc pApiProc = util_dl_get_proc_address(screen->pLibrary,
+      "SwrGetInterface");
+   if (!pApiProc) {
+      fprintf(stderr, "(skipping: %s).\n", util_dl_error());
+      util_dl_close(screen->pLibrary);
+      screen->pLibrary = NULL;
+      return false;
+   }
+
+   screen->pfnSwrGetInterface = (PFNSwrGetInterface)pApiProc;
+   fprintf(stderr, "(using: %s).\n", filename);
 #endif
+   return true;
 }
 
+
 struct pipe_screen *
 swr_create_screen(struct sw_winsys *winsys)
 {
-   char filename[256] = { 0 };
-   bool found = false;
-   bool is_knl = false;
-   PFNSwrGetInterface pfnSwrGetInterface = nullptr;
+   struct pipe_screen *p_screen = swr_create_screen_internal(winsys);
+   if (!p_screen) {
+      return NULL;
+   }
+
+   struct swr_screen *screen = swr_screen(p_screen);
+   screen->is_knl = false;
 
    util_cpu_detect();
 
-   if (!found && util_cpu_caps.has_avx512f && util_cpu_caps.has_avx512er) {
+   if (util_cpu_caps.has_avx512f && util_cpu_caps.has_avx512er) {
       fprintf(stderr, "SWR detected KNL instruction support ");
 #ifndef HAVE_SWR_KNL
-      fprintf(stderr, "(skipping not built).\n");
+      fprintf(stderr, "(skipping: not built).\n");
 #else
-      get_swr_arch_filename("KNL", filename);
-      found = true;
-      is_knl = true;
+      if (swr_initialize_screen_interface(screen, "KNL")) {
+         screen->is_knl = true;
+         return p_screen;
+      }
 #endif
    }
 
-   if (!found && util_cpu_caps.has_avx512f && util_cpu_caps.has_avx512bw) {
+   if (util_cpu_caps.has_avx512f && util_cpu_caps.has_avx512bw) {
       fprintf(stderr, "SWR detected SKX instruction support ");
 #ifndef HAVE_SWR_SKX
       fprintf(stderr, "(skipping not built).\n");
 #else
-      get_swr_arch_filename("SKX", filename);
-      found = true;
+      if (swr_initialize_screen_interface(screen, "SKX"))
+         return p_screen;
 #endif
    }
 
-   if (!found && util_cpu_caps.has_avx2) {
+   if (util_cpu_caps.has_avx2) {
       fprintf(stderr, "SWR detected AVX2 instruction support ");
 #ifndef HAVE_SWR_AVX2
       fprintf(stderr, "(skipping not built).\n");
 #else
-      get_swr_arch_filename("AVX2", filename);
-      found = true;
+      if (swr_initialize_screen_interface(screen, "AVX2"))
+         return p_screen;
 #endif
    }
 
-   if (!found && util_cpu_caps.has_avx) {
+   if (util_cpu_caps.has_avx) {
       fprintf(stderr, "SWR detected AVX instruction support ");
 #ifndef HAVE_SWR_AVX
       fprintf(stderr, "(skipping not built).\n");
 #else
-      get_swr_arch_filename("AVX", filename);
-      found = true;
+      if (swr_initialize_screen_interface(screen, "AVX"))
+         return p_screen;
 #endif
    }
 
-   if (!found) {
-      fprintf(stderr, "SWR could not detect a supported CPU architecture.\n");
-      exit(-1);
-   }
-
-   fprintf(stderr, "(using %s).\n", filename);
-
-#ifdef HAVE_SWR_BUILTIN
-   pfnSwrGetInterface = SwrGetInterface;
-#else
-   util_dl_library *pLibrary = util_dl_open(filename);
-   if (!pLibrary) {
-      fprintf(stderr, "SWR library load failure: %s\n", util_dl_error());
-      exit(-1);
-   }
-
-   util_dl_proc pApiProc = util_dl_get_proc_address(pLibrary, "SwrGetInterface");
-   if (!pApiProc) {
-      fprintf(stderr, "SWR library search failure: %s\n", util_dl_error());
-      exit(-1);
-   }
-
-   pfnSwrGetInterface = (PFNSwrGetInterface)pApiProc;
-#endif
-
-   struct pipe_screen *screen = swr_create_screen_internal(winsys);
-   swr_screen(screen)->is_knl = is_knl;
-   swr_screen(screen)->pfnSwrGetInterface = pfnSwrGetInterface;
+   fprintf(stderr, "SWR could not initialize a supported CPU architecture.\n");
+   swr_destroy_screen_internal(&screen);
 
-   return screen;
+   return NULL;
 }
 
 
diff --git a/src/gallium/drivers/swr/swr_public.h b/src/gallium/drivers/swr/swr_public.h
index 4b150705cd7..07ea6280cd6 100644
--- a/src/gallium/drivers/swr/swr_public.h
+++ b/src/gallium/drivers/swr/swr_public.h
@@ -25,8 +25,9 @@
 #define SWR_PUBLIC_H
 
 struct pipe_screen;
-struct sw_winsys;
 struct sw_displaytarget;
+struct sw_winsys;
+struct swr_screen;
 
 #ifdef __cplusplus
 extern "C" {
@@ -38,6 +39,9 @@ struct pipe_screen *swr_create_screen(struct sw_winsys *winsys);
 // arch-specific dll entry point
 PUBLIC struct pipe_screen *swr_create_screen_internal(struct sw_winsys *winsys);
 
+// cleanup for failed screen creation
+void swr_destroy_screen_internal(struct swr_screen **screen);
+
 #ifdef _WIN32
 void swr_gdi_swap(struct pipe_screen *screen,
                   struct pipe_resource *res,
diff --git a/src/gallium/drivers/swr/swr_screen.cpp b/src/gallium/drivers/swr/swr_screen.cpp
index b67ac25ac89..839179a3043 100644
--- a/src/gallium/drivers/swr/swr_screen.cpp
+++ b/src/gallium/drivers/swr/swr_screen.cpp
@@ -1052,6 +1052,24 @@ swr_flush_frontbuffer(struct pipe_screen *p_screen,
 }
 
 
+void
+swr_destroy_screen_internal(struct swr_screen **screen)
+{
+   struct pipe_screen *p_screen = &(*screen)->base;
+
+   swr_fence_finish(p_screen, NULL, (*screen)->flush_fence, 0);
+   swr_fence_reference(p_screen, &(*screen)->flush_fence, NULL);
+
+   JitDestroyContext((*screen)->hJitMgr);
+
+   if ((*screen)->pLibrary)
+      util_dl_close((*screen)->pLibrary);
+
+   FREE(*screen);
+   *screen = NULL;
+}
+
+
 static void
 swr_destroy_screen(struct pipe_screen *p_screen)
 {
@@ -1060,15 +1078,10 @@ swr_destroy_screen(struct pipe_screen *p_screen)
 
    fprintf(stderr, "SWR destroy screen!\n");
 
-   swr_fence_finish(p_screen, NULL, screen->flush_fence, 0);
-   swr_fence_reference(p_screen, &screen->flush_fence, NULL);
-
-   JitDestroyContext(screen->hJitMgr);
-
    if (winsys->destroy)
       winsys->destroy(winsys);
 
-   FREE(screen);
+   swr_destroy_screen_internal(&screen);
 }
 
 
@@ -1119,6 +1132,7 @@ struct pipe_screen *
 swr_create_screen_internal(struct sw_winsys *winsys)
 {
    struct swr_screen *screen = CALLOC_STRUCT(swr_screen);
+   memset(screen, 0, sizeof(struct swr_screen));
 
    if (!screen)
       return NULL;
diff --git a/src/gallium/drivers/swr/swr_screen.h b/src/gallium/drivers/swr/swr_screen.h
index 89faab182c5..b80d8c70ec9 100644
--- a/src/gallium/drivers/swr/swr_screen.h
+++ b/src/gallium/drivers/swr/swr_screen.h
@@ -28,6 +28,7 @@
 
 #include "pipe/p_screen.h"
 #include "pipe/p_defines.h"
+#include "util/u_dl.h"
 #include "util/u_format.h"
 #include "api.h"
 
@@ -50,6 +51,8 @@ struct swr_screen {
 
    HANDLE hJitMgr;
 
+   /* Dynamic backend implementations */
+   util_dl_library *pLibrary;
    PFNSwrGetInterface pfnSwrGetInterface;
 
    /* Do we run on Xeon Phi? */