On Thu, Jun 1, 2017 at 2:15 PM, Eric Engestrom <eric.engest...@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 <nota...@gmail.com>
Not really, see https://bugs.freedesktop.org/show_bug.cgi?id=101252 > Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com> > --- > > This is unnecessary in my opinion, but doesn't hurt :) It is necessary, without it things will work most of the time but there will be rare failures. Imagine there are 2 threads that both call _eglGetNativePlatform() simultaneously: - thread 1 completes the first "if (native_platform == _EGL_INVALID_PLATFORM)" check and is preemted to do something else - thread 2 executes the whole function, does "native_platform = _EGL_NATIVE_PLATFORM" and just before returning it's preemted - thread 1 wakes up and calls _eglGetNativePlatformFromEnv() which returns _EGL_INVALID_PLATFORM because no env vars are set, updates native_platform and then gets preemted again - thread 2 wakes up and returns wrong _EGL_INVALID_PLATFORM > 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); I'd make the first "if (detected_platform == _EGL_INVALID_PLATFORM) {" block end here to avoid doing the slow cmpxchg every time, which makes it similar to how it looked before your original patch that I wanted to revert. GraÅžvydas _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev