[Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS (v3.1)

Wu, Zhongmin zhongmin.wu at intel.com
Wed Jul 19 06:16:41 UTC 2017


Hi  Tomasz:

Thanks very much for your comments, I read it carefully, but still have some questions below. Could you please have a look. Thanks again.

-----Original Message-----
From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf Of Tomasz Figa
Sent: Wednesday, July 19, 2017 12:42 
To: Wu, Zhongmin <zhongmin.wu at intel.com>
Cc: Gao, Shuo <shuo.gao at intel.com>; Liu, Zhiquan <zhiquan.liu at intel.com>; Daniel Stone <daniels at collabora.com>; Antognolli, Rafael <rafael.antognolli at intel.com>; Emil Velikov <emil.l.velikov at gmail.com>; Chad Versace <chadversary at chromium.org>; Eric Engestrom <eric at engestrom.ch>; Marathe, Yogesh <yogesh.marathe at intel.com>; Kenneth Graunke <kenneth at whitecape.org>; Kondapally, Kalyan <kalyan.kondapally at intel.com>; Rainer Hochecker <fernetmenta at online.de>; ML mesa-dev <mesa-dev at lists.freedesktop.org>; Nicolai Hähnle <nicolai.haehnle at amd.com>; Varad Gautam <varad.gautam at collabora.com>
Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS (v3.1)

Hi Zhongmin,

Thanks for the patch. Please see my comments inline.

On Wed, Jul 19, 2017 at 12:22 PM, Zhongmin Wu <zhongmin.wu at intel.com> wrote:
> Before we queued the buffer with a invalid fence (-1), it will make 
> some benchmarks failed to test such as flatland.
>
> Now we get the out fence during the flushing buffer and then pass it 
> to SurfaceFlinger in eglSwapbuffer function.
>
> v2: a) Also implement the fence in cancelBuffer.
>     b) The last sync fence is stored in drawable object
>        rather than brw context.
>     c) format clear.
>
> v3: a) Save the last fence fd in DRI Context object.
>     b) Return the last fence if the batch buffer is empty and
>        nothing to be flushed when _intel_batchbuffer_flush_fence
>     c) Add the new interface in vbtl to set the retrieve fence
>        in the egl surface. This is just for cancelBuffer.
>     d) For queueBuffer, the fence is get with DRI fence interface.
>        For cancelBuffer, the fence is get before the context is
>        reset, and the fence is then  moved to its surface.
> v3.1 a) close fd in the new vbtl interface on none Android platform
>
> Change-Id: Ic0773c19788d612a98d1402f5b5619dab64c1bc2
> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-43936
> Reported-On: https://bugs.freedesktop.org/show_bug.cgi?id=101655
> Signed-off-by: Zhongmin Wu <zhongmin.wu at intel.com>
> Reported-by: Li, Guangli <guangli.li at intel.com>
> Tested-by: Marathe, Yogesh <yogesh.marathe at intel.com>
> ---
>  src/egl/drivers/dri2/egl_dri2.c               |   17 +++++++++++++-
>  src/egl/drivers/dri2/egl_dri2.h               |    3 +++
>  src/egl/drivers/dri2/platform_android.c       |   30 ++++++++++++++++++++++---
>  src/egl/drivers/dri2/platform_drm.c           |    1 +
>  src/egl/drivers/dri2/platform_surfaceless.c   |    1 +
>  src/egl/drivers/dri2/platform_wayland.c       |    2 ++
>  src/egl/drivers/dri2/platform_x11.c           |    2 ++
>  src/egl/drivers/dri2/platform_x11_dri3.c      |    1 +
>  src/mesa/drivers/dri/common/dri_util.c        |    3 +++
>  src/mesa/drivers/dri/common/dri_util.h        |    2 ++
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c |   25 ++++++++++++++++++---
>  11 files changed, 80 insertions(+), 7 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c 
> b/src/egl/drivers/dri2/egl_dri2.c index 020a0bc..19d346d 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1351,9 +1351,17 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *dsurf,
>     ddraw = (dsurf) ? dri2_dpy->vtbl->get_dri_drawable(dsurf) : NULL;
>     rdraw = (rsurf) ? dri2_dpy->vtbl->get_dri_drawable(rsurf) : NULL;
>     cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
> -
> +   int fence_fd = -1;
>     if (old_ctx) {
>        __DRIcontext *old_cctx = 
> dri2_egl_context(old_ctx)->dri_context;
> +      if (old_dsurf) {
> +         void * fence = dri2_dpy->fence->create_fence_fd(old_cctx, -1);
> +         if (fence) {
> +            fence_fd = dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen, fence);
> +            dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
> +         }
> +         dri2_dpy->vtbl->set_retrieve_fence(old_dsurf, fence_fd);

I think we don't need this callback. We can just set the fence in old_dsurf directly, i.e.

dri2_surf_set_release_fence(surf, fence_fd) {
   if (surf->fence_fd >= 0)
      close(surf->fence_fd);
   surf->fence_fd = fence_fd;
}

// ...

dri2_make_current()
{
// ...
      int fence_fd = -1;
      if (old_dsurf) {
         void * fence = dri2_dpy->fence->create_fence_fd(old_cctx, -1);
         if (fence) {
            fence_fd =
dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen, fence);
            dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
         }
         dri2_surf_set_release_fence(old_dsurf, fence_fd);
      }
// ...
}

wzm :================== ============================================================================================================================
I am a little confused here, you mean we can add the fd field in _EGLSurface directly ? I checked the definition of _EGLSurface, it is not like a place to add the extra member.
==================================================================================================================================================

Then we can just call dri2_surf_set_release_fence(surf, -1) in
dri2_destroy_surface() after the platform callback returns. The platform callback must set surf->fence_fd to -1 if ownership of the FD was given to the platform window system.
wzm :===============================================================================================================================================
But we can't call dri2_surf_set_release_fence(surf, -1) after platforms callback return, the surf may be freed in callback.
====================================================================================================================================================
wzm: ===============================================================================================================================================
In this patch, I just add the fd member in dri2_egl_surface for Android platform, and Only Android platform will initialize/de-initialize the fd when create and destroy the buffer.
If we move the fd member to _EGLSurface, it will be seen on all platforms, right ?  We do need a good place to initialize/de-initialize the fd. 
And none Android platform will not use such fence actually.
====================================================================================================================================================

> +      }
>        dri2_dpy->core->unbindContext(old_cctx);
>     }
>
> @@ -1403,6 +1411,13 @@ dri2_surface_get_dri_drawable(_EGLSurface *surf)
>     return dri2_surf->dri_drawable;
>  }
>
> +void
> +dri2_set_retrieve_fence(_EGLSurface *surf, int fd) {
> +       close(fd);
> +       return;
> +}
> +
>  /*
>   * Called from eglGetProcAddress() via drv->API.GetProcAddress().
>   */
> diff --git a/src/egl/drivers/dri2/egl_dri2.h 
> b/src/egl/drivers/dri2/egl_dri2.h index bbba7c0..b097345 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -150,6 +150,8 @@ struct dri2_egl_display_vtbl {
>                                   EGLuint64KHR *sbc);
>
>     __DRIdrawable *(*get_dri_drawable)(_EGLSurface *surf);
> +
> +   void (*set_retrieve_fence)(_EGLSurface *surf, int fd);
>  };
>
>  struct dri2_egl_display
> @@ -314,6 +316,7 @@ struct dri2_egl_surface
>        struct ANativeWindowBuffer *buffer;
>        int age;
>     } color_buffers[3], *back;
> +   int retrieve_fd;
>  #endif
>
>  #if defined(HAVE_SURFACELESS_PLATFORM)
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index cc2e4a6..b8d53b8 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -306,9 +306,16 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct dri2_egl_surface *dri2_sur
>      *    is responsible for closing it.
>      */
>     int fence_fd = -1;
> +   _EGLContext *ctx = _eglGetCurrentContext();
> +   struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
> +   void * fence = dri2_dpy->fence->create_fence_fd(dri2_ctx->dri_context, -1);
> +   if (fence) {
> +      fence_fd = dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen,
> +                                               fence);
> +      dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
> +   }

If we are already handling the make current case in the generic egl_dri2, we can do the same with swap_buffers IMHO and have the fence always stored in dri2_surf->fence_fd.

>     dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
>                                    fence_fd);
> -

No need to remove this line in this patch.

>     dri2_surf->buffer->common.decRef(&dri2_surf->buffer->common);
>     dri2_surf->buffer = NULL;
>     dri2_surf->back = NULL;
> @@ -327,8 +334,10 @@ static void
>  droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf)  {
>     int ret;
> -
> -   ret = dri2_surf->window->cancelBuffer(dri2_surf->window, dri2_surf->buffer, -1);
> +   int fence_fd = -1;
> +   fence_fd = dup(dri2_surf->retrieve_fd);
> +   ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
> +                                         dri2_surf->buffer, 
> + fence_fd);

The surface is being destroyed here. I don't think we need to dup() the FD. Instead we should simply set it to -1.
wzm: =============================================================================================================================================
That is right, we can do this
And what about queuebuffer , if we apply the same style, can we set the fd to -1 in queue buffer ? In fact, I am not clear about all the possible sequences of such  functions swapbuffer,  
Flush, make current, and so on. So I used the  most safe way without prior knowledge ...
==================================================================================================================================================
>     if (ret < 0) {
>        _eglLog(_EGL_WARNING, "ANativeWindow::cancelBuffer failed");
>        dri2_surf->base.Lost = EGL_TRUE; @@ -434,6 +443,8 @@ 
> droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
>        dri2_surf->window = window;
>     }
>
> +   dri2_surf->retrieve_fd = -1;
> +

nit: Maybe we should think about having a dri2_init_surface() helper that calls _eglInitSurface() and sets some dri2 specific fields for us as well?
Wzm ===========================================================================================================================
A little confused again,  if we add the fd member in _EGLSurface as above,  we can initialize  fd to -1 in _eglInitSurface() directly.  Every surface creation on each platform will call 
_eglInitSurface(), right?  The trouble thing is where to de-initialize as we don’t have _eglDeInitSurface() in each platform's destroy path....
================================================================================================================================
>     return &dri2_surf->base;
>
>  cleanup_surface:
> @@ -488,6 +499,9 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay 
> *disp, _EGLSurface *surf)
>	
>     dri2_dpy->core->destroyDrawable(dri2_surf->dri_drawable);
>
> +   if (dri2_surf->retrieve_fd != -1)
> +      close(dri2_surf->retrieve_fd);
> +

I think this would make more sense in dri2_destroy_surface().

>     free(dri2_surf);
>
>     return EGL_TRUE;
> @@ -1073,6 +1087,15 @@ droid_create_image_khr(_EGLDriver *drv, 
> _EGLDisplay *disp,  }
>
>  static void
> +droid_set_retrieve_fence(_EGLSurface *surf, int fd) {
> +   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> +   if (dri2_surf->retrieve_fd != -1)
> +      close(dri2_surf->retrieve_fd);
> +   dri2_surf->retrieve_fd = fd;
> +}

Shouldn't need this with my earlier suggestion.

> +
> +static void
>  droid_flush_front_buffer(__DRIdrawable * driDrawable, void 
> *loaderPrivate)  {  } @@ -1289,6 +1312,7 @@ static const struct 
> dri2_egl_display_vtbl droid_display_vtbl = {
>     .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
>     .get_sync_values = dri2_fallback_get_sync_values,
>     .get_dri_drawable = dri2_surface_get_dri_drawable,
> +   .set_retrieve_fence = droid_set_retrieve_fence,
>  };
>
>  static const __DRIdri2LoaderExtension droid_dri2_loader_extension = { 
> diff --git a/src/egl/drivers/dri2/platform_drm.c 
> b/src/egl/drivers/dri2/platform_drm.c
> index 8b0562c..97f897e 100644
> --- a/src/egl/drivers/dri2/platform_drm.c
> +++ b/src/egl/drivers/dri2/platform_drm.c
> @@ -665,6 +665,7 @@ static const struct dri2_egl_display_vtbl dri2_drm_display_vtbl = {
>     .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
>     .get_sync_values = dri2_fallback_get_sync_values,
>     .get_dri_drawable = dri2_surface_get_dri_drawable,
> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>  };
>
>  EGLBoolean
> diff --git a/src/egl/drivers/dri2/platform_surfaceless.c 
> b/src/egl/drivers/dri2/platform_surfaceless.c
> index 0eb3fb7..78b2571 100644
> --- a/src/egl/drivers/dri2/platform_surfaceless.c
> +++ b/src/egl/drivers/dri2/platform_surfaceless.c
> @@ -244,6 +244,7 @@ static const struct dri2_egl_display_vtbl dri2_surfaceless_display_vtbl = {
>     .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
>     .get_sync_values = dri2_fallback_get_sync_values,
>     .get_dri_drawable = dri2_surface_get_dri_drawable,
> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>  };
>
>  static void
> diff --git a/src/egl/drivers/dri2/platform_wayland.c 
> b/src/egl/drivers/dri2/platform_wayland.c
> index f746f0b..458bd6e 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -1087,6 +1087,7 @@ static const struct dri2_egl_display_vtbl dri2_wl_display_vtbl = {
>     .create_wayland_buffer_from_image = dri2_wl_create_wayland_buffer_from_image,
>     .get_sync_values = dri2_fallback_get_sync_values,
>     .get_dri_drawable = dri2_surface_get_dri_drawable,
> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>  };
>
>  static const __DRIextension *dri2_loader_extensions[] = { @@ -1781,6 
> +1782,7 @@ static const struct dri2_egl_display_vtbl dri2_wl_swrast_display_vtbl = {
>     .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
>     .get_sync_values = dri2_fallback_get_sync_values,
>     .get_dri_drawable = dri2_surface_get_dri_drawable,
> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>  };
>
>  static const __DRIswrastLoaderExtension swrast_loader_extension = { 
> diff --git a/src/egl/drivers/dri2/platform_x11.c 
> b/src/egl/drivers/dri2/platform_x11.c
> index 74d3a16..0def3ca 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -1149,6 +1149,7 @@ static const struct dri2_egl_display_vtbl dri2_x11_swrast_display_vtbl = {
>     .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
>     .get_sync_values = dri2_fallback_get_sync_values,
>     .get_dri_drawable = dri2_surface_get_dri_drawable,
> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>  };
>
>  static const struct dri2_egl_display_vtbl dri2_x11_display_vtbl = { 
> @@ -1170,6 +1171,7 @@ static const struct dri2_egl_display_vtbl dri2_x11_display_vtbl = {
>     .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
>     .get_sync_values = dri2_x11_get_sync_values,
>     .get_dri_drawable = dri2_surface_get_dri_drawable,
> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>  };
>
>  static const __DRIswrastLoaderExtension swrast_loader_extension = { 
> diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c 
> b/src/egl/drivers/dri2/platform_x11_dri3.c
> index 3148f49..cae322d 100644
> --- a/src/egl/drivers/dri2/platform_x11_dri3.c
> +++ b/src/egl/drivers/dri2/platform_x11_dri3.c
> @@ -466,6 +466,7 @@ struct dri2_egl_display_vtbl dri3_x11_display_vtbl = {
>     .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
>     .get_sync_values = dri3_get_sync_values,
>     .get_dri_drawable = dri3_get_dri_drawable,
> +   .set_retrieve_fence = dri2_set_retrieve_fence,
>  };
>
>  EGLBoolean
> diff --git a/src/mesa/drivers/dri/common/dri_util.c 
> b/src/mesa/drivers/dri/common/dri_util.c
> index f6df488..d0465b8 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -446,6 +446,7 @@ driCreateContextAttribs(__DRIscreen *screen, int api,
>      context->driScreenPriv = screen;
>      context->driDrawablePriv = NULL;
>      context->driReadablePriv = NULL;
> +    context->retrieve_fd = -1;
>
>      if (!screen->driver->CreateContext(mesa_api, modes, context,
>                                         major_version, minor_version, 
> @@ -499,6 +500,8 @@ static void  driDestroyContext(__DRIcontext *pcp)  
> {
>      if (pcp) {
> +    if (pcp->retrieve_fd != -1)
> +       close(pcp->retrieve_fd);
>         pcp->driScreenPriv->driver->DestroyContext(pcp);
>         free(pcp);
>      }
> diff --git a/src/mesa/drivers/dri/common/dri_util.h 
> b/src/mesa/drivers/dri/common/dri_util.h
> index 8fcd632..f723a90 100644
> --- a/src/mesa/drivers/dri/common/dri_util.h
> +++ b/src/mesa/drivers/dri/common/dri_util.h
> @@ -218,6 +218,8 @@ struct __DRIcontextRec {
>         int draw_stamp;
>         int read_stamp;
>      } dri2;
> +
> +    int retrieve_fd;
>  };

Do we need to store this in a generic DRI struct, if it's only used by i965? IMHO it would make more sense to just store this in brw_context.

Also, let's use some more commonly used naming scheme, for example out_fence_fd.

wzm: =================================
You are right, we can just use brw_context.
=====================================

>
>  /**
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 62d2fe8..77bf8f6 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -648,9 +648,25 @@ do_flush_locked(struct brw_context *brw, int in_fence_fd, int *out_fence_fd)
>           /* Add the batch itself to the end of the validation list */
>           add_exec_bo(batch, batch->bo);
>
> +         if (brw->driContext &&
> +            brw->driContext->retrieve_fd != -1) {
> +            close(brw->driContext->retrieve_fd);
> +            brw->driContext->retrieve_fd = -1;
> +         }
> +
> +         int fd = -1;
>           ret = execbuffer(dri_screen->fd, batch, hw_ctx,
>                            4 * USED_BATCH(*batch),
> -                          in_fence_fd, out_fence_fd, flags);
> +                          in_fence_fd, &fd, flags);
> +
> +         if (out_fence_fd != NULL) {
> +            *out_fence_fd = fd;
> +            if (brw->driContext)
> +               brw->driContext->retrieve_fd = dup(fd);
> +         } else {
> +            if (brw->driContext)
> +               brw->driContext->retrieve_fd  = fd;
> +         }

I think this code can be simplified significantly. Including my comments above:

   brw->out_fence_fd = fd;
   if (out_fence_fd)
      *out_fence_fd = (fd >= 0) ? dup(fd) : -1;

>        }
>
>        throttle(brw);
> @@ -684,8 +700,11 @@ _intel_batchbuffer_flush_fence(struct brw_context 
> *brw,  {
>     int ret;
>
> -   if (USED_BATCH(brw->batch) == 0)
> -      return 0;
> +   if (USED_BATCH(brw->batch) == 0) {
> +      if (out_fence_fd && brw->driContext->retrieve_fd != -1)
> +         *out_fence_fd = dup(brw->driContext->retrieve_fd);

We need to check for >= 0 if we only allow valid FDs. Also it should actually make sense to assign -1 if we don't have any fence.

   if (out_fence_fd)
      *out_fence_fd = (brw->out_fence_fd >= 0) ? dup(brw->out_fence_fd) : -1;

> +         return 0;

Wrong indentation?

> +   }
>
>     if (brw->throttle_batch[0] == NULL) {
>        brw->throttle_batch[0] = brw->batch.bo;

Generally I still believe i965 driver in its current shape inserts commands to the buffer when brw_dri_create_fence_fd() is called, so all the i965-specific changes should go to a separate patch, as an additional optimization.
wzm:=======================================================================================================
You are right,  it is better to have a separate and entire optimization patch for  i965 fence logic first...
============================================================================================================
Best regards,
Tomasz
_______________________________________________
mesa-dev mailing list
mesa-dev at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list