[Mesa-dev] [PATCH] egl/dri2: dri2_make_current: Release previous context's display

[Nicolas Boichat]

On Wed, Aug 10, 2016 at 9:44 AM, Michel Dänzer <michel at 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-stable at lists.freedesktop.org>
>> Reported-by: Alexandr Zelinsky <mexahotabop at w1l.ru>
>> Tested-by: Alexandr Zelinsky <mexahotabop at w1l.ru>
>> Signed-off-by: Nicolas Boichat <drinkcat at 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 at egl
> 1.4 at 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.

> 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 at egl.*" all results/master

Best,

Nicolas