[Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)

[Emil Velikov]

Hi Joonas,

Does getting rid of the viewport hack give you any noticeable
performance improvement ? Is there any interest in converting the
egl_dri2 backend to dri3, rather than just copying over the present bits ?

On 05/11/14 11:14, Joonas Lahtinen wrote:
> Hi,
> 
> Modified not refer to DRI3, just uses the present extension to get rid
> of the excess buffer invalidations.
> 
> Regards, Joonas
> 
> From 257e2a8c750f9dcf868cce9da8632df3cae0fcec Mon Sep 17 00:00:00 2001
> From: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Date: Wed, 5 Nov 2014 12:25:32 +0200
> Subject: [PATCH] egl: dri2: Use present extension.
> 
> Present extension is used to avoid excess buffer invalidations, because
> the XCB interface doesn't supply that information.
> 
> Signed-off-by: Daniel van der Wath <danielx.j.van.der.wath at intel.com>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
>  configure.ac                            |    5 +-
>  src/egl/drivers/dri2/egl_dri2.c         |    2 +-
>  src/egl/drivers/dri2/egl_dri2.h         |   24 ++-
>  src/egl/drivers/dri2/platform_x11.c     |  247 ++++++++++++++++++++++++++++---
>  src/mesa/drivers/dri/i965/brw_context.c |    9 +-
>  5 files changed, 262 insertions(+), 25 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index fc7d372..75d90c0 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -952,7 +952,8 @@ xyesno)
>              fi
>  
>              if test x"$enable_dri" = xyes; then
> -               dri_modules="$dri_modules xcb-dri2 >= $XCBDRI2_REQUIRED"
> +               PKG_CHECK_MODULES([PRESENTPROTO], [presentproto >= $PRESENTPROTO_REQUIRED])
> +               dri_modules="$dri_modules xcb-dri2 >= $XCBDRI2_REQUIRED xcb-present"
Afaics you are not changing anything on the dri modules (or glx/dri2) to
require the above changes. Perhaps you need to push the presentproto
check in the x11 case below ?

>              fi
>  
>              if test x"$enable_dri3" = xyes; then
> @@ -1564,7 +1565,7 @@ for plat in $egl_platforms; do
>  		;;
>  
>  	x11)
> -		PKG_CHECK_MODULES([XCB_DRI2], [x11-xcb xcb xcb-dri2 >= $XCBDRI2_REQUIRED xcb-xfixes])
> +		PKG_CHECK_MODULES([XCB_DRI2], [x11-xcb xcb xcb-dri2 >= $XCBDRI2_REQUIRED xcb-xfixes xcb-present])
>  		;;
>  
>  	drm)
[snip]
> diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
> index f8c4b70..a1445b2 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -188,6 +188,205 @@ get_xcb_screen(xcb_screen_iterator_t iter, int screen)
>      return NULL;
>  }
>  
> +/*
> + * Called by the XCB_PRESENT_COMPLETE_NOTIFY case.
> + */
> +static void
> +dri2_update_num_back(struct dri2_egl_surface *priv)
> +{
> +   priv->num_back = 1;
> +   if (priv->flipping)
> +      priv->num_back++;
> +   if (priv->base.SwapInterval == 0)
> +      priv->num_back++;
> +}
> +
This seems to be out of sync with dri3_glx. Don't you need something
similar to commit f7a355556ef5fe23056299a77414f9ad8b5e5a1d ?

[snip]
> +/**
> + *
> + * Process any present events that have been received from the X server
> + *
> + * From glx, we additionally invalidate the drawable here if there has a been a special event.
> + */
> +static void
> +dri2_flush_present_events(struct dri2_egl_display *dri2_dpy, struct dri2_egl_surface *priv)
> +{
> +   xcb_connection_t     *c = dri2_dpy->conn;
> +
> +   /* Check to see if any configuration changes have occurred
> +    * since we were last invoked
> +    */
> +   if (priv->special_event) {
> +      xcb_generic_event_t    *ev;
> +
> +      while ((ev = xcb_poll_for_special_event(c, priv->special_event)) != NULL) {
> +         xcb_present_generic_event_t *ge = (void *) ev;
> +         dri2_handle_present_event(priv, ge);
> +         _eglLog(_EGL_INFO, "DRI: Invalidating buffer 0x%x\n", priv->dri_drawable);
> +         (*dri2_dpy->flush->invalidate)(priv->dri_drawable);
Hmm why does one need to invalidate at this stage - I take that it's
related to lack of fence objects ?

[snip]
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> index e1a994a..dbadd10 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -148,6 +148,9 @@ intel_viewport(struct gl_context *ctx)
>     __DRIcontext *driContext = brw->driContext;
>  
>     if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
> +      if (unlikely(INTEL_DEBUG & DEBUG_DRI))
> +         fprintf(stderr, "invalidating drawables\n");
> +
>        dri2InvalidateDrawable(driContext->driDrawablePriv);
>        dri2InvalidateDrawable(driContext->driReadablePriv);
>     }
> @@ -252,11 +255,9 @@ brw_init_driver_functions(struct brw_context *brw,
>     _mesa_init_driver_functions(functions);
>  
>     /* GLX uses DRI2 invalidate events to handle window resizing.
> -    * Unfortunately, EGL does not - libEGL is written in XCB (not Xlib),
> -    * which doesn't provide a mechanism for snooping the event queues.
> +    * EGL uses present invalidate events to do the same.
>      *
> -    * So EGL still relies on viewport hacks to handle window resizing.
> -    * This should go away with DRI3000.
> +    * Others have to rely on viewport hacks to handle window resizing.
>      */
Bikeshed: maybe move the i965 changes in a separate patch ?


Cheers,
Emil

>     if (!brw->driContext->driScreenPriv->dri2.useInvalidate)
>        functions->Viewport = intel_viewport;
>