On Thu, Aug 11, 2016 at 12:10 AM, Nicolas Boichat <drink...@chromium.org> wrote: > On Wed, Aug 10, 2016 at 9:44 AM, Michel Dänzer <mic...@daenzer.net> wrote: >> On 10/08/16 03:00 PM, Nicolas Boichat wrote: >>> eglMakeCurrent can also be used to change the active display. In that >>> case, we need to decrement ref_count of the previous display (possibly >>> destroying it), and increment it on the next display. >>> >>> Also, old_dsurf/old_rsurf cannot be non-NULL if old_ctx is NULL, so >>> we only need to test if old_ctx is non-NULL. >>> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97214 >>> Fixes: 9ee683f877 (egl/dri2: Add reference count for dri2_egl_display) >>> Cc: "12.0" <mesa-sta...@lists.freedesktop.org> >>> Reported-by: Alexandr Zelinsky <mexahota...@w1l.ru> >>> Tested-by: Alexandr Zelinsky <mexahota...@w1l.ru> >>> Signed-off-by: Nicolas Boichat <drink...@chromium.org> >>> --- >>> src/egl/drivers/dri2/egl_dri2.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/egl/drivers/dri2/egl_dri2.c >>> b/src/egl/drivers/dri2/egl_dri2.c >>> index 3205a36..701e42a 100644 >>> --- a/src/egl/drivers/dri2/egl_dri2.c >>> +++ b/src/egl/drivers/dri2/egl_dri2.c >>> @@ -1285,8 +1285,10 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay >>> *disp, _EGLSurface *dsurf, >>> >>> if (!unbind) >>> dri2_dpy->ref_count++; >>> - if (old_dsurf || old_rsurf || old_ctx) >>> - dri2_display_release(disp); >>> + if (old_ctx) { >>> + EGLDisplay old_disp = >>> _eglGetDisplayHandle(old_ctx->Resource.Display); >>> + dri2_display_release(old_disp); >>> + } >> >> Unfortunately, this change breaks the piglit test "spec@egl >> 1.4@eglterminate then unbind context", because old_ctx != NULL but >> old_ctx->Resource.Display == NULL. Modifying the test to >> >> if (old_ctx && old_ctx->Resource.Display) { >> >> fixes the regression and doesn't seem to cause any other problems. > > This is probably wrong as the display will leak (it definitely should > be freed after calling eglTerminate + eglMakeCurrent(NULL)). > > I think I know where the problem is (the call to > _eglReleaseDisplayResources happens too early), I'm not sure what's > the best solution. I'll have a look.
Turns out we destroy old_ctx just above, and then use it again here, fix is simple (swap a few lines). Patch v2 to follow. Thanks! >> Alexandr, does the patch still fix your problem with that modification? >> >> >> Nicolas, this regression is also reproducible with >> LIBGL_ALWAYS_SOFTWARE=1 . Please get used to testing your changes like >> that and only send out changes for review which don't cause any regressions. > > Managed to build piglit, and run it using a locally built mesa. Are > the commands below what you would use? > > export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$MESA_DIR/lib > export LIBGL_DRIVERS_PATH=$MESA_DIR/lib/gallium > export EGL_DRIVERS_PATH=$MESA_DIR/lib > export EGL_LOG_LEVEL=debug > export LIBGL_ALWAYS_SOFTWARE=1 > ./piglit run -p x11_egl -t "spec@egl.*" all results/master I realized that running with --valgrind is quite useful (it's slow, but since there are only 20+ tests, it's fine). _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
