[Mesa-dev] [PATCH 2/2] egl/dri: Defer display destruction until the display is not in use.

[Chad Versace]

On 01/28/2015 01:45 PM, jim.bride at linux.intel.com wrote:
> From: Jim Bride <jim.bride at intel.com>
> 
> In situations where a DRI2 driver is in use and a call to eglTerminate()
> is followed by a call to eglMakeCurrent() the mandatory call to glFlush()
> inside of dri2_make_current() would cause a segmentation fault because the
> DRI2 driver was torn down and glFlush() expects certain data structures
> associated through the driver to be present.  In order to accommodate
> this the tear-down of the driver itself has been moved from dri2_terminate()
> into a new function dri2_cleanup_display().  Finally, both dri2_terminate()
> and dri2_make_current() call dri2_cleanup_display() if there are no
> resources associated with the display.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69622
> Signed-off-by: Jim Bride <jim.bride at intel.com>
> ---
>  src/egl/drivers/dri2/egl_dri2.c | 84 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 68 insertions(+), 16 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 9d5a1cf..bea5ccc 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -665,20 +665,14 @@ dri2_initialize(_EGLDriver *drv, _EGLDisplay *disp)
>  }
>  
>  /**
> - * Called via eglTerminate(), drv->API.Terminate().
> + * Cleanup display elements that are not in use.
>   */
> -static EGLBoolean
> -dri2_terminate(_EGLDriver *drv, _EGLDisplay *disp)
> +static void
> +dri2_cleanup_display(_EGLDisplay *disp)
>  {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>     unsigned i;
>  
> -   /* Release any resources that are not currently bound to
> -    * the display.  We do not want to release bound resources
> -    * because doing so could interfere with proper cleanup via
> -    * a call to eglMakeCurrent() to unbind said resources.
> -    */
> -   _eglReleaseDisplayResources(drv, disp, EGL_TRUE);
>     _eglCleanupDisplay(disp);
>  
>     if (dri2_dpy->own_dri_screen)
> @@ -728,7 +722,26 @@ dri2_terminate(_EGLDriver *drv, _EGLDisplay *disp)
>     }
>     free(dri2_dpy);
>     disp->DriverData = NULL;
> +}
> +
> +/**
> + * Called via eglTerminate(), drv->API.Terminate().
> + */
> +static EGLBoolean
> +dri2_terminate(_EGLDriver *drv, _EGLDisplay *disp)
> +{
> +   /* Release any resources that are not currently bound to
> +    * the display.  We do not want to release bound resources
> +    * because doing so could interfere with proper cleanup via
> +    * a call to eglMakeCurrent() to unbind said resources.
> +    */
> +   _eglReleaseDisplayResources(drv, disp, EGL_TRUE);
>  
> +   /* If the display has not resources (and all are tied to contexts) then
                            ^^^
                            no

> +    * go ahead and free up the display itself.
> +    */
> +   if (!disp->ResourceLists[_EGL_RESOURCE_CONTEXT])
> +      dri2_cleanup_display(disp);
>     return EGL_TRUE;
>  }
>  
> @@ -992,14 +1005,13 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *dsurf,
>     __DRIdrawable *ddraw, *rdraw;
>     __DRIcontext *cctx;
>  
> +   /* flush before context switch */
> +   dri2_drv->glFlush();
> +
>     /* make new bindings */
>     if (!_eglBindContext(ctx, dsurf, rsurf, &old_ctx, &old_dsurf, &old_rsurf))
>        return EGL_FALSE;
>  
> -   /* flush before context switch */
> -   if (old_ctx && dri2_drv->glFlush)
> -      dri2_drv->glFlush();
> -
>     ddraw = (dri2_dsurf) ? dri2_dsurf->dri_drawable : NULL;
>     rdraw = (dri2_rsurf) ? dri2_rsurf->dri_drawable : NULL;
>     cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
> @@ -1011,13 +1023,53 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *dsurf,
>  
>     if ((cctx == NULL && ddraw == NULL && rdraw == NULL) ||
>         dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) {
> -      if (old_dsurf)
> +      _EGLDisplay *old_dpy = NULL, *ref = NULL;
> +
> +      if (old_dsurf) {
> +         ref = old_dsurf->Resource.Display;
> +         old_dpy = (ref != disp) ? ref : NULL;
> +
>           drv->API.DestroySurface(drv, disp, old_dsurf);
> -      if (old_rsurf)
> +      }
> +
> +      if (old_rsurf) {
> +         if (!old_dpy) {
> +            ref = old_rsurf->Resource.Display;
> +            old_dpy = (ref != disp) ? ref : NULL;
> +         }

Nit pick. Ignore if you wish.

I think the second-level if statements are much easier to read if rewritten
with the pattern below:

if (old_rsurf) {
   old_dpy = old_rsurf->Resource.Display;
   drv->API.DestroySurface(drv, disp, old_rsurf);
}

It eliminates a nested 'if' statement, eliminates the weird `ref` variable,
and eliminates the ternary 'if' operator. And it
is functionally equivalent to the patch as-is because, if any
of the old objects exist, the old display is the same for all of them.

To make the simplification though, you would have to modify the
'if' condition farther below:
- if (old_dpy && !old_dpy->Initialized)
+ if (old_dpy && old_dpy != disp && !old_dpy->Initialized)

>           drv->API.DestroySurface(drv, disp, old_rsurf);
> -      if (old_ctx)
> +      }
> +
> +      if (old_ctx) {
> +         if (!old_dpy) {
> +            ref = old_ctx->Resource.Display;
> +            old_dpy = (ref != disp) ? ref : NULL;
> +         }
>           drv->API.DestroyContext(drv, disp, old_ctx);
> +      }
>  
> +      /* Destroy any displays that are not initialized and
> +       * have no resources linked to them.  Since all other
> +       * resources types are tied to contexts that is the
> +       * only type we need to check here.
> +       */
> +      if (old_dpy && !old_dpy->Initialized) {
> +         /* Purge any unlinked resources before checking on
> +          * display cleanup
> +          */
> +         _eglReleaseDisplayResources(drv, old_dpy, EGL_TRUE);
> +         if (old_dpy->ResourceLists[_EGL_RESOURCE_CONTEXT])

I think you mean (!old_dpy->ResourceLists[...]).

> +            dri2_cleanup_display(old_dpy);
> +      }
> +
> +      if (disp && !disp->Initialized) {
> +         /* Purge any unlinked resources before checking on
> +          * display cleanup
> +          */
> +         _eglReleaseDisplayResources(drv, disp, EGL_TRUE);
> +         if (disp->ResourceLists[_EGL_RESOURCE_CONTEXT])

Again.

> +            dri2_cleanup_display(disp);
> +      }
>        return EGL_TRUE;
>     } else {
>        /* undo the previous _eglBindContext */
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150129/ee421087/attachment-0001.sig>