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

2017-07-20 Thread Tomasz Figa 
Hi Zhongmin,

Thanks for the update. Please see my comments inline.

On Fri, Jul 21, 2017 at 12:08 PM, Zhongmin Wu  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 
> ---
>  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-07-20 Thread Zhongmin Wu 
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 
---
 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)
+{
+   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);
return dri2_dpy->vtbl->destroy_surface(drv, dpy, surf);
 }
 
+static void
+dri2_surf_get_fence_fd(_EGLContext *ctx,
+   _EGLDisplay *dpy, _EGLSurface *surf)
+{
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
+   int fence_fd = -1;
+   __DRIcontext *dri_ctx = dri2_egl_context(ctx)->dri_context;
+   void * fence = dri2_dpy->fence->create_fence_fd(dri_ctx, -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_surface_set_retrieve_fence(surf, fence_fd);
+}
+
 /**
  * Called via eglMakeCurrent(), drv->API.MakeCurrent().
  */
@@ -1352,8 +1388,11 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
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)
+ dri2_surf_get_fence_fd(old_ctx, disp, old_dsurf);
   dri2_dpy->core->unbindContext(old_cctx);
}
 
@@ -1490,6 +1529,9 @@ static EGLBoolean
 dri2_swap_buffers(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf)
 {
struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
+   _EGLContext *ctx = _eglGetCurrentContext();
+   if (ctx && surf)
+  dri2_surf_get_fence_fd(ctx, dpy, surf);
return dri2_dpy->vtbl->swap_buffers(drv, dpy, surf);
 }
 
@@ -1499,6 +1541,9 @@ dri2_swap_buffers_with_damage(_EGLDriver *drv, 
_EGLDisplay *dpy,
   const EGLint *rects, EGLint n_rects)
 {
struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
+   _EGLContext *ctx = _eglGetCurrentContext();
+   if (ctx && surf)
+  dri2_surf_get_fence_fd(ctx, dpy, surf);
return dri2_dpy->vtbl->swap_buffers_with_damage(drv, dpy, surf,
rects, n_rects);
 }
diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
index bbba7c0..ca36dd9 100644
--- a/src/egl/drivers/dri2/egl_dri2.h