[Mesa-dev] [EGL android: accquire fence implementation 2/2] i965: Queue the buffer with a sync fence for Android OS v4.1

[Tomasz Figa]

Hi Zhongmin,

Thanks for the update. Please see my comments inline.

On Fri, Jul 21, 2017 at 12:08 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
>
> v3.1 a) close fd in the new vbtl interface on none Android platform
>
> v4: a) The last fence is saved in brw context.
>     b) The retrieve fd is for all the platform but not just Android
>     c) Add a uniform dri2 interface to initialize the surface.
>
> v4.1: a) make some changes of variable name.
>       b) the patch is breaked into two patches.
>
> Change-Id: Ided54d2e193cde73a6f0feb36ac1c0056e4958f2
> Signed-off-by: Zhongmin Wu <zhongmin.wu at intel.com>
> ---
>  src/egl/drivers/dri2/egl_dri2.c             |   45 +++++++++++++++++++++++++++
>  src/egl/drivers/dri2/egl_dri2.h             |    5 +++
>  src/egl/drivers/dri2/platform_android.c     |   11 ++++---
>  src/egl/drivers/dri2/platform_drm.c         |    2 +-
>  src/egl/drivers/dri2/platform_surfaceless.c |    2 +-
>  src/egl/drivers/dri2/platform_wayland.c     |    2 +-
>  src/egl/drivers/dri2/platform_x11.c         |    2 +-
>  src/egl/drivers/dri2/platform_x11_dri3.c    |    2 +-
>  8 files changed, 62 insertions(+), 9 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 020a0bc..df4e934 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1307,6 +1307,25 @@ dri2_destroy_context(_EGLDriver *drv, _EGLDisplay *disp, _EGLContext *ctx)
>     return EGL_TRUE;
>  }
>
> +EGLBoolean
> +dri2_surf_init(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
> +        _EGLConfig *conf, const EGLint *attrib_list)
> +{
> +   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> +   dri2_surf->out_fence_fd = -1;
> +   return _eglInitSurface(surf, dpy, type, conf, attrib_list);
> +}
> +
> +static void
> +dri2_surface_set_retrieve_fence( _EGLSurface *surf, int fence_fd)

I think you forgot to rename this function too. (dri2_surface_set_out_fence).

> +{
> +   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> +   if (dri2_surf->out_fence_fd >=0)
> +      close(dri2_surf->out_fence_fd);
> +
> +   dri2_surf->out_fence_fd = fence_fd;
> +}
> +
>  static EGLBoolean
>  dri2_destroy_surface(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf)
>  {
> @@ -1315,9 +1334,26 @@ dri2_destroy_surface(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf)
>     if (!_eglPutSurface(surf))
>        return EGL_TRUE;
>
> +   dri2_surface_set_retrieve_fence(surf, -1);

Hmm, if we set it here, we would end up with the ->destroy_surface()
callback seeing -1 as the fence FD. For Android that would mean that
cancel_buffer() is called without a fence.

What I had in my mind was adding a dri2_surf_destroy() function that
would be called by platform backends before freeing the surf struct
(analogically to dri2_surf_init() after allocating the struct).

>     return dri2_dpy->vtbl->destroy_surface(drv, dpy, surf);
>  }
>

Other than the above, I think this looks reasonably.

However, depending on how costly inserting a fence is (I think it
might mean flushing a command buffer on some platforms) we might need
a mechanism for the platform backend to opt-in for fences, i.e. tell
the dri2 core code that it's interested in them, rather than
requesting them by default. I'd like to hear more opinions on this,
though.

Best regards,
Tomasz