[Mesa-dev] [PATCH] Revert "egl/display: remove unnecessary code and make it easier to read"

[Eric Engestrom]

On Thursday, 2017-06-01 02:58:17 +0300, Grazvydas Ignotas wrote:
> This reverts commit 7adb9b094894a512c019b3378eb9e3c69d830edc.
> 
> The "No functional change" claim is wrong, after that commit, the
> _eglGetNativePlatformFromEnv() function is now called every time instead
> of just once it was called before.

Oops :/
You're entirely correct, it looks like I missed the fact native_platform
is static...
_eglLog() is also called every time now, which is spammy and might be
how you noticed it?

> What's worse, when
> _eglGetNativePlatform() is called from multiple threads, the static
> native_platform variable may now alternate between _EGL_INVALID_PLATFORM
> and some other value, which causes some of the threads to see and return
> unintended result.

I think this can be solved by a simple `if (detected_platform ==
_EGL_INVALID_PLATFORM)` before the first detection method. There's no
way the current detection code can return different results in different
threads, so no back-and-forth possible, and if this ever changes to make
that possible then the old code doesn't handle that either anyway.
Shout if I'm (again) missing something :)

As an alternative to your revert, how about operating on a local var,
reading the global one once at the start and writing it once at the end,
with a compare-and-swap?
With that, the worse that would happen is two threads doing the
detection and only one of them writing the result, but (for now at
least) they would always detect the same thing so it would just be a few
CPU cycles wasted.

Sending my variant in a minute.

Cheers,
  Eric

> 
> The old code is not exactly thread safe either, but because the code
> stops changing 'native_platform' as soon as it's not
> _EGL_INVALID_PLATFORM, it should not cause problems in practice.
> 
> Cc: Eric Engestrom <eric.engestrom at imgtec.com>
> Fixes: 7adb9b0948 "egl/display: remove unnecessary code and make it easier to read"
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101252
> Signed-off-by: Grazvydas Ignotas <notasas at gmail.com>
> ---
>  src/egl/main/egldisplay.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/src/egl/main/egldisplay.c b/src/egl/main/egldisplay.c
> index 2a1f027..6e007a1 100644
> --- a/src/egl/main/egldisplay.c
> +++ b/src/egl/main/egldisplay.c
> @@ -178,28 +178,29 @@ _eglNativePlatformDetectNativeDisplay(void *nativeDisplay)
>   * Return the native platform.  It is the platform of the EGL native types.
>   */
>  _EGLPlatformType
>  _eglGetNativePlatform(void *nativeDisplay)
>  {
> -   static _EGLPlatformType native_platform;
> -   char *detection_method;
> -
> -   native_platform = _eglGetNativePlatformFromEnv();
> -   detection_method = "environment overwrite";
> -
> -   if (native_platform == _EGL_INVALID_PLATFORM) {
> -      native_platform = _eglNativePlatformDetectNativeDisplay(nativeDisplay);
> -      detection_method = "autodetected";
> -   }
> +   static _EGLPlatformType native_platform = _EGL_INVALID_PLATFORM;
> +   char *detection_method = NULL;
>  
>     if (native_platform == _EGL_INVALID_PLATFORM) {
> -      native_platform = _EGL_NATIVE_PLATFORM;
> -      detection_method = "build-time configuration";
> +      native_platform = _eglGetNativePlatformFromEnv();
> +      detection_method = "environment overwrite";
> +      if (native_platform == _EGL_INVALID_PLATFORM) {
> +         native_platform = _eglNativePlatformDetectNativeDisplay(nativeDisplay);
> +         detection_method = "autodetected";
> +         if (native_platform == _EGL_INVALID_PLATFORM) {
> +            native_platform = _EGL_NATIVE_PLATFORM;
> +            detection_method = "build-time configuration";
> +         }
> +      }
>     }
>  
> -   _eglLog(_EGL_DEBUG, "Native platform type: %s (%s)",
> -           egl_platforms[native_platform].name, detection_method);
> +   if (detection_method != NULL)
> +      _eglLog(_EGL_DEBUG, "Native platform type: %s (%s)",
> +              egl_platforms[native_platform].name, detection_method);
>  
>     return native_platform;
>  }
>  
>  
> -- 
> 2.7.4
>