From 3b0b44f7def0acb4f7a7aef086c0bece321418a6 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Marek=20Ol=C5=A1=C3=A1k?= Date: Wed, 9 Apr 2014 01:07:52 +0200 Subject: [PATCH] winsys/radeon: fix a race condition in initialization of radeon_winsys::screen MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Create the screen in the winsys while the mutex is locked. This also results in a nice code cleanup! Reviewed-by: Michel Dänzer Reviewed-by: Christian König --- src/gallium/targets/egl-static/egl_pipe.c | 42 +++---------------- src/gallium/targets/pipe-loader/pipe_r300.c | 14 +------ src/gallium/targets/pipe-loader/pipe_r600.c | 14 +------ .../targets/pipe-loader/pipe_radeonsi.c | 14 +------ src/gallium/targets/r300/common/drm_target.c | 15 +------ src/gallium/targets/r600/common/drm_target.c | 15 +------ .../targets/radeonsi/common/drm_target.c | 15 +------ .../winsys/radeon/drm/radeon_drm_public.h | 6 ++- .../winsys/radeon/drm/radeon_drm_winsys.c | 18 +++++++- 9 files changed, 39 insertions(+), 114 deletions(-) diff --git a/src/gallium/targets/egl-static/egl_pipe.c b/src/gallium/targets/egl-static/egl_pipe.c index eb1cff9c76b..ce734fb4711 100644 --- 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 055685996e6..388b091f144 100644 --- 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 5d89aca6ec3..0c590875784 100644 --- 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 48b2b5dff3f..406ba1e1ddb 100644 --- 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 9b484469d47..dff83dacba2 100644 --- 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 ab1eec6ac9a..09250c79fb9 100644 --- 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 c695dde51ed..74980af5281 100644 --- 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 4fc62f1a400..dfcaaa4b6ef 100644 --- 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 07b3c05f2b7..0eb0c6a07b9 100644 --- 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. */ -- 2.30.2