On Mon, Jul 18, 2016 at 4:37 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > On 18 July 2016 at 04:19, Nicolas Boichat <drink...@chromium.org> wrote: >> On Fri, Jul 15, 2016 at 9:03 PM, Emil Velikov <emil.l.veli...@gmail.com> >> wrote: >>> On 15 July 2016 at 09:28, Nicolas Boichat <drink...@chromium.org> wrote: >>>> android.opengl.cts.WrapperTest#testGetIntegerv1 CTS test calls >>>> eglTerminate, followed by eglReleaseThread. In that case, the >>>> display will not be initialized when eglReleaseThread calls >>>> MakeCurrent with NULL parameters, to unbind the context, which >>>> causes a a segfault in drv->API.MakeCurrent (dri2_make_current), >>>> either in glFlush or in a latter call. >>>> >>>> A similar case is observed in this bug: >>>> https://bugs.freedesktop.org/show_bug.cgi?id=69622, where the test >>>> call eglTerminate, then eglMakeCurrent(dpy, NULL, NULL, NULL). >>>> >>>> This patch: >>>> 1. Validates that the display is initialized, if ctx/dsurf/rsurf >>>> are not all NULL. >>>> 2. Does not call glFlush/unBindContext is there is no display. >>>> 3. However, it still goes through the normal path as >>>> drv->API.DestroyContext decrements the reference count on the >>>> context, and frees the structure. >>>> >>>> Cc: "11.2 12.0" <mesa-sta...@lists.freedesktop.org> >>>> Signed-off-by: Nicolas Boichat <drink...@chromium.org> >>>> --- >>>> >>>> I did not actually test the case reported on the bug, only the CTS >>>> test, would be nice if someone can try it out (Rhys? Chad?). >>>> >>>> Applies after "egl/dri2: dri2_make_current: Set EGL error if bindContext >>>> fails", but wanted to keep the 2 patches separate as they are different >>>> problems and discussions. >>>> >>>> src/egl/drivers/dri2/egl_dri2.c | 13 ++++++++++--- >>>> 1 file changed, 10 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/src/egl/drivers/dri2/egl_dri2.c >>>> b/src/egl/drivers/dri2/egl_dri2.c >>>> index 2e97d85..3269e52 100644 >>>> --- a/src/egl/drivers/dri2/egl_dri2.c >>>> +++ b/src/egl/drivers/dri2/egl_dri2.c >>>> @@ -1166,7 +1166,8 @@ dri2_destroy_context(_EGLDriver *drv, _EGLDisplay >>>> *disp, _EGLContext *ctx) >>>> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); >>>> >>>> if (_eglPutContext(ctx)) { >>>> - dri2_dpy->core->destroyContext(dri2_ctx->dri_context); >>>> + if (dri2_dpy) >>> Not sure if this is the right place/we need this. >>> >>> When calling eglDestroyContext (after eglTerminate or not) this cannot >>> happen since the _EGL_CHECK_CONTEXT macro will dive into the _eglCheck >>> functions and will bail out since the since the dpy is no initialized. >>> >>> The only case we can reach this is from dri2_make_current. See below for >>> more. >> >> Yes, that's correct. >> >>>> @@ -1189,6 +1190,10 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay >>>> *disp, _EGLSurface *dsurf, >>>> __DRIdrawable *ddraw, *rdraw; >>>> __DRIcontext *cctx; >>>> >>>> + /* Check that the display is initialized */ >>>> + if ((ctx != NULL || dsurf != NULL || rsurf != NULL) && !dri2_dpy) >>>> + return _eglError(EGL_NOT_INITIALIZED, "eglMakeCurrent error"); >>>> + >>> Was doing to say "the first few lines in eglMakeCurrent) should >>> already handle these" only to realise that dri2_dpy doesn't wrap >>> around/subclass the respective _EGLfoo type (_EGLDisplay), like the >>> dri2_egl_{surface,context...} :-\ >>> >>> At the same time, dri2_make_current cannot (should not really) do >>> anything in the !dri2_dpy case, since we cannot: >>> - call unbind the old ctx bind the new one, or even restore/rebind >>> the original ctx >>> - free any of the ctx/surf resources. The latter should already be >>> gone, as upon eglTerminate walk over the ctx/surf lists and tear them >>> all down (see _eglReleaseDisplayResources). >>> >>> Thus something like the following should be fine imho. >>> >>> if (!dri2_dpy) >>> /* copy/reword some my earlier comment in here */ >>> return 0; >> >> Yes, that was my first approach, but we'd still leak the current dri2_ctx. >> >> If I trace the reference counting right: >> 1. eglCreateContext => API.CreateContext => >> - _eglInitResource => RefCount = 1 >> - _eglLinkContext => RefCount = 2 >> 2. eglMakeCurrent => API.MakeCurrent => _eglBindContext => >> eglGetContext => RefCount = 3 >> 3. eglTerminate => API.Terminate => _eglReleaseDisplayResources => >> - _eglUnlinkContext => _eglUnlinkResource => Refcount = 2 >> - API.DestroyContext => RefCount = 1 (so context structure is not freed) >> 4. eglReleaseThread (or eglMakeCurrent(dpy, NULL, NULL, NULL)) => >> API.MakeCurrent(drv, disp, NULL, NULL, NULL) => API.DestroyContext => >> Refcount = 0 and structure is freed (with my current patch). >> >> Well, the structure is freed, but driDestroyContext is never called >> (so pcp->driScreenPriv->driver->DestroyContext is not called), which, >> looks a bit concerning... >> > This is exactly what made me question the proposed approach. > >> So one way around this is to get eglTerminate to destroy the current> >> context(s). But then the spec says >> (https://www.khronos.org/registry/egl/sdk/docs/man/html/eglTerminate.xhtml): >> If contexts or surfaces associated with display is current to any >> thread, they are not released until they are no longer current as a >> result of eglMakeCurrent. >> > The way I see/read it: > - 'leaking' ctx/surf resources if the user has not called > [eglMakeCurrent(... !NULL, !NULL) + eglTerminate +] eglReleaseThread > or eglMakeCurrent(...NULL,NULL) [+ eglTerminate] is expected. > - we need to defer dpy vtbls (and active dri dpy connection?) in > eglReleaseThread or ctx/surf will leak > > IMHO the better approach is to defer the dpy teardown (in the case of > missing eglMakeCurrent(...NULL,NULL)) to eglReleaseThread. Since that > one is more evasive (but shouldn't be too crazy) we could get this > patch, and revert to 'if (!dri2_dpy) return;' once everything else is > handled. > > Not sure when we'll get time for that one, so we want an inline > XXX/TODO or two that roughly explains the idea.
Turns out this patch is quite leaky in this CTS test: android.opengl.cts.WrapperTest#testThreadCleanup, which basically runs eglInitialize, eglMakeCurrent, eglTerminate, eglReleaseThread in separate thread, 1000 times in a row. (before, it would crash, with this patch, it leaks a lot of memory) I managed to add some reference counting to dri2_dpy instead, I'll upload a patch soon. > Related: from a very brief look we seems to be leaking the > _EGLsync/_EGLimage resources. > > Regards, > Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
