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

[Nicolas Boichat]

On Thu, Aug 11, 2016 at 12:10 AM, Nicolas Boichat <drinkcat at chromium.org> wrote:
> 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.

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 at 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).