[Mesa-dev] [PATCH mesa 2/2] egl/display: make platform detection thread-safe

[Grazvydas Ignotas]

On Thu, Jun 1, 2017 at 2:15 PM, Eric Engestrom
<eric.engestrom at imgtec.com> wrote:
> If the detections methods ever become able to return different results
> for different threads, the if chain might make threads go back and forth
> between invalid and valid platforms.
> Solve this by doing the detection in a local var and only overwriting
> the global one at the end, if no other thread has updated it since.
>
> This means the platform detected in the thread might not be the platform
> returned by the function, but this is a different issue that will need
> to be discussed when this becomes possible.
>
> Reported-by: Grazvydas Ignotas <notasas at gmail.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100877

> Signed-off-by: Eric Engestrom <eric.engestrom at imgtec.com>

It seems this has stalled. With the commit message updated (as
detailed in my previous mail, bad things can happen even with matching
thread results), and optionally putting everything in a if block to
avoid expensive cmpxchg on non-first run, this is

Reviewed-by: Grazvydas Ignotas <notasas at gmail.com>

I think you can add Emil's Acked-by too.

> ---
>
> This is unnecessary in my opinion, but doesn't hurt :)
> Putting it out there as a possible fix, but my vote is to not merge it,
> not until this per-thread-platform-detection becomes possible anyway.
> ---
>  src/egl/main/egldisplay.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/src/egl/main/egldisplay.c b/src/egl/main/egldisplay.c
> index 3f7752f53d..92586def0c 100644
> --- a/src/egl/main/egldisplay.c
> +++ b/src/egl/main/egldisplay.c
> @@ -36,6 +36,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include "c11/threads.h"
> +#include "util/u_atomic.h"
>
>  #include "eglcontext.h"
>  #include "eglcurrent.h"
> @@ -181,23 +182,27 @@ _EGLPlatformType
>  _eglGetNativePlatform(void *nativeDisplay)
>  {
>     static _EGLPlatformType native_platform = _EGL_INVALID_PLATFORM;
> +   _EGLPlatformType detected_platform = native_platform;
>     char *detection_method = NULL;
>
> -   if (native_platform == _EGL_INVALID_PLATFORM) {
> -      native_platform = _eglGetNativePlatformFromEnv();
> +   if (detected_platform == _EGL_INVALID_PLATFORM) {
> +      detected_platform = _eglGetNativePlatformFromEnv();
>        detection_method = "environment overwrite";
>     }
>
> -   if (native_platform == _EGL_INVALID_PLATFORM) {
> -      native_platform = _eglNativePlatformDetectNativeDisplay(nativeDisplay);
> +   if (detected_platform == _EGL_INVALID_PLATFORM) {
> +      detected_platform = _eglNativePlatformDetectNativeDisplay(nativeDisplay);
>        detection_method = "autodetected";
>     }
>
> -   if (native_platform == _EGL_INVALID_PLATFORM) {
> -      native_platform = _EGL_NATIVE_PLATFORM;
> +   if (detected_platform == _EGL_INVALID_PLATFORM) {
> +      detected_platform = _EGL_NATIVE_PLATFORM;
>        detection_method = "build-time configuration";
>     }
>
> +   p_atomic_cmpxchg(&native_platform, _EGL_INVALID_PLATFORM,
> +                    detected_platform);
> +
>     if (detection_method != NULL) {
>        _eglLog(_EGL_DEBUG, "Native platform type: %s (%s)",
>                egl_platforms[native_platform].name, detection_method);
> --
> Cheers,
>   Eric
>