winsys/radeon: fix a race condition in initialization of radeon_winsys::screen

Create the screen in the winsys while the mutex is locked.
This also results in a nice code cleanup!

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>

diff --git a/src/gallium/targets/egl-static/egl_pipe.c b/src/gallium/targets/egl-static/egl_pipe.c
index eb1cff9..ce734fb 100644 (file)
--- a/src/gallium/targets/egl-static/egl_pipe.c
+++ b/src/gallium/targets/egl-static/egl_pipe.c
@@ -119,19 +119,9 @@ pipe_r300_create_screen(int fd)
 {
 #if _EGL_PIPE_R300
    struct radeon_winsys *sws;
-   struct pipe_screen *screen;
-
-   sws = radeon_drm_winsys_create(fd);
-   if (!sws)
-      return NULL;
-
-   screen = r300_screen_create(sws);
-   if (!screen)
-      return NULL;
 
-   screen = debug_screen_wrap(screen);
-
-   return screen;
+   sws = radeon_drm_winsys_create(fd, r300_screen_create);
+   return sws ? debug_screen_wrap(sws->screen) : NULL;
 #else
    return NULL;
 #endif
@@ -142,19 +132,9 @@ pipe_r600_create_screen(int fd)
 {
 #if _EGL_PIPE_R600
    struct radeon_winsys *rw;
-   struct pipe_screen *screen;
-
-   rw = radeon_drm_winsys_create(fd);
-   if (!rw)
-      return NULL;
-
-   screen = r600_screen_create(rw);
-   if (!screen)
-      return NULL;
-
-   screen = debug_screen_wrap(screen);
 
-   return screen;
+   rw = radeon_drm_winsys_create(fd, r600_screen_create);
+   return rw ? debug_screen_wrap(rw->screen) : NULL;
 #else
    return NULL;
 #endif
@@ -165,19 +145,9 @@ pipe_radeonsi_create_screen(int fd)
 {
 #if _EGL_PIPE_RADEONSI
    struct radeon_winsys *rw;
-   struct pipe_screen *screen;
-
-   rw = radeon_drm_winsys_create(fd);
-   if (!rw)
-      return NULL;
-
-   screen = radeonsi_screen_create(rw);
-   if (!screen)
-      return NULL;
 
-   screen = debug_screen_wrap(screen);
-
-   return screen;
+   rw = radeon_drm_winsys_create(fd, radeonsi_screen_create);
+   return rw ? debug_screen_wrap(rw->screen) : NULL;
 #else
    return NULL;
 #endif

diff --git a/src/gallium/targets/pipe-loader/pipe_r300.c b/src/gallium/targets/pipe-loader/pipe_r300.c
index 0556859..388b091 100644 (file)
--- a/src/gallium/targets/pipe-loader/pipe_r300.c
+++ b/src/gallium/targets/pipe-loader/pipe_r300.c
@@ -8,19 +8,9 @@ static struct pipe_screen *
 create_screen(int fd)
 {
    struct radeon_winsys *sws;
-   struct pipe_screen *screen;
 
-   sws = radeon_drm_winsys_create(fd);
-   if (!sws)
-      return NULL;
-
-   screen = r300_screen_create(sws);
-   if (!screen)
-      return NULL;
-
-   screen = debug_screen_wrap(screen);
-
-   return screen;
+   sws = radeon_drm_winsys_create(fd, r300_screen_create);
+   return sws ? debug_screen_wrap(sws->screen) : NULL;
 }
 
 PUBLIC

diff --git a/src/gallium/targets/pipe-loader/pipe_r600.c b/src/gallium/targets/pipe-loader/pipe_r600.c
index 5d89aca..0c59087 100644 (file)
--- a/src/gallium/targets/pipe-loader/pipe_r600.c
+++ b/src/gallium/targets/pipe-loader/pipe_r600.c
@@ -7,19 +7,9 @@ static struct pipe_screen *
 create_screen(int fd)
 {
    struct radeon_winsys *rw;
-   struct pipe_screen *screen;
 
-   rw = radeon_drm_winsys_create(fd);
-   if (!rw)
-      return NULL;
-
-   screen = r600_screen_create(rw);
-   if (!screen)
-      return NULL;
-
-   screen = debug_screen_wrap(screen);
-
-   return screen;
+   rw = radeon_drm_winsys_create(fd, r600_screen_create);
+   return rw ? debug_screen_wrap(rw->screen) : NULL;
 }
 
 PUBLIC

diff --git a/src/gallium/targets/pipe-loader/pipe_radeonsi.c b/src/gallium/targets/pipe-loader/pipe_radeonsi.c
index 48b2b5d..406ba1e 100644 (file)
--- a/src/gallium/targets/pipe-loader/pipe_radeonsi.c
+++ b/src/gallium/targets/pipe-loader/pipe_radeonsi.c
@@ -7,19 +7,9 @@ static struct pipe_screen *
 create_screen(int fd)
 {
    struct radeon_winsys *rw;
-   struct pipe_screen *screen;
 
-   rw = radeon_drm_winsys_create(fd);
-   if (!rw)
-      return NULL;
-
-   screen = radeonsi_screen_create(rw);
-   if (!screen)
-      return NULL;
-
-   screen = debug_screen_wrap(screen);
-
-   return screen;
+   rw = radeon_drm_winsys_create(fd, radeonsi_screen_create);
+   return rw ? debug_screen_wrap(rw->screen) : NULL;
 }
 
 PUBLIC

diff --git a/src/gallium/targets/r300/common/drm_target.c b/src/gallium/targets/r300/common/drm_target.c
index 9b48446..dff83da 100644 (file)
--- a/src/gallium/targets/r300/common/drm_target.c
+++ b/src/gallium/targets/r300/common/drm_target.c
@@ -36,19 +36,8 @@ create_screen(int fd)
 {
    struct radeon_winsys *sws;
 
-   sws = radeon_drm_winsys_create(fd);
-   if (!sws)
-      return NULL;
-
-   if (!sws->screen) {
-      sws->screen = r300_screen_create(sws);
-      if (!sws->screen)
-         return NULL;
-
-      sws->screen = debug_screen_wrap(sws->screen);
-   }
-
-   return sws->screen;
+   sws = radeon_drm_winsys_create(fd, r300_screen_create);
+   return sws ? debug_screen_wrap(sws->screen) : NULL;
 }
 
 /* Technically this is only true for kernels >= 3.12, which

diff --git a/src/gallium/targets/r600/common/drm_target.c b/src/gallium/targets/r600/common/drm_target.c
index ab1eec6..09250c7 100644 (file)
--- a/src/gallium/targets/r600/common/drm_target.c
+++ b/src/gallium/targets/r600/common/drm_target.c
@@ -35,19 +35,8 @@ static struct pipe_screen *create_screen(int fd)
 {
    struct radeon_winsys *radeon;
 
-   radeon = radeon_drm_winsys_create(fd);
-   if (!radeon)
-      return NULL;
-
-   if (!radeon->screen) {
-      radeon->screen = r600_screen_create(radeon);
-      if (!radeon->screen)
-         return NULL;
-
-      radeon->screen = debug_screen_wrap(radeon->screen);
-   }
-
-   return radeon->screen;
+   radeon = radeon_drm_winsys_create(fd, r600_screen_create);
+   return radeon ? debug_screen_wrap(radeon->screen) : NULL;
 }
 
 static const struct drm_conf_ret throttle_ret = {

diff --git a/src/gallium/targets/radeonsi/common/drm_target.c b/src/gallium/targets/radeonsi/common/drm_target.c
index c695dde..74980af 100644 (file)
--- a/src/gallium/targets/radeonsi/common/drm_target.c
+++ b/src/gallium/targets/radeonsi/common/drm_target.c
@@ -35,19 +35,8 @@ static struct pipe_screen *create_screen(int fd)
 {
    struct radeon_winsys *radeon;
 
-   radeon = radeon_drm_winsys_create(fd);
-   if (!radeon)
-      return NULL;
-
-   if (!radeon->screen) {
-      radeon->screen = radeonsi_screen_create(radeon);
-      if (!radeon->screen)
-         return NULL;
-
-      radeon->screen = debug_screen_wrap(radeon->screen);
-   }
-
-   return radeon->screen;
+   radeon = radeon_drm_winsys_create(fd, radeonsi_screen_create);
+   return radeon ? debug_screen_wrap(radeon->screen) : NULL;
 }
 
 static const struct drm_conf_ret throttle_ret = {

diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_public.h b/src/gallium/winsys/radeon/drm/radeon_drm_public.h
index 4fc62f1..dfcaaa4 100644 (file)
--- a/src/gallium/winsys/radeon/drm/radeon_drm_public.h
+++ b/src/gallium/winsys/radeon/drm/radeon_drm_public.h
@@ -4,7 +4,11 @@
 #include "pipe/p_defines.h"
 
 struct radeon_winsys;
+struct pipe_screen;
 
-struct radeon_winsys *radeon_drm_winsys_create(int fd);
+typedef struct pipe_screen *(*radeon_screen_create_t)(struct radeon_winsys *);
+
+struct radeon_winsys *
+radeon_drm_winsys_create(int fd, radeon_screen_create_t screen_create);
 
 #endif

diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
index 07b3c05..0eb0c6a 100644 (file)
--- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
+++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
@@ -586,7 +586,8 @@ static bool radeon_winsys_unref(struct radeon_winsys *ws)
     return destroy;
 }
 
-PUBLIC struct radeon_winsys *radeon_drm_winsys_create(int fd)
+PUBLIC struct radeon_winsys *
+radeon_drm_winsys_create(int fd, radeon_screen_create_t screen_create)
 {
     struct radeon_drm_winsys *ws;
 
@@ -609,7 +610,6 @@ PUBLIC struct radeon_winsys *radeon_drm_winsys_create(int fd)
     }
 
     ws->fd = fd;
-    util_hash_table_set(fd_tab, intptr_to_pointer(fd), ws);
 
     if (!do_winsys_init(ws))
         goto fail;
@@ -652,6 +652,20 @@ PUBLIC struct radeon_winsys *radeon_drm_winsys_create(int fd)
     if (ws->num_cpus > 1 && debug_get_option_thread())
         ws->thread = pipe_thread_create(radeon_drm_cs_emit_ioctl, ws);
 
+    /* Create the screen at the end. The winsys must be initialized
+     * completely.
+     *
+     * Alternatively, we could create the screen based on "ws->gen"
+     * and link all drivers into one binary blob. */
+    ws->base.screen = screen_create(&ws->base);
+    if (!ws->base.screen) {
+        radeon_winsys_destroy(&ws->base);
+        pipe_mutex_unlock(fd_tab_mutex);
+        return NULL;
+    }
+
+    util_hash_table_set(fd_tab, intptr_to_pointer(fd), ws);
+
     /* We must unlock the mutex once the winsys is fully initialized, so that
      * other threads attempting to create the winsys from the same fd will
      * get a fully initialized winsys and not just half-way initialized. */