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

[Tomasz Figa]

Hi Zhongmin,

Thanks for the new patch. Personally I think it looks much better now.
Still, there are some remaining comments.

Also, I'd advise waiting few days before posting new version, so that peo

On Thu, Jul 20, 2017 at 4:10 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
>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
>
> 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.
>
> Change-Id: Ic0773c19788d612a98d1402f5b5619dab64c1bc2
> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-43936

Please remove internal tags. They are meaningless for us.

> Reported-On: https://bugs.freedesktop.org/show_bug.cgi?id=101655
> Signed-off-by: Zhongmin Wu 
> Reported-by: Li, Guangli 
> Tested-by: Marathe, Yogesh 
> ---
>  src/egl/drivers/dri2/egl_dri2.c   |   47 
> +
>  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 +-
>  src/mesa/drivers/dri/i965/brw_context.c   |5 +++
>  src/mesa/drivers/dri/i965/brw_context.h   |1 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c |   17 +++--
>  11 files changed, 85 insertions(+), 11 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 020a0bc..94ad360 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1307,6 +1307,26 @@ 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->retrieve_fd = -1;

"retrieve_fd" is a really strange name. Please change it. I'd suggest
"out_fence_fd" as already used in i965 driver.

> +   return _eglInitSurface(surf, dpy, type, conf, attrib_list);
> +}
> +
> +void

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->retrieve_fd >=0)
> +  close(dri2_surf->retrieve_fd);
> +
> +   dri2_surf->retrieve_fd = fence_fd;
> +   return;

No need for return.

> +}
> +
>  static EGLBoolean
>  dri2_destroy_surface(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf)
>  {
> @@ -1315,9 +1335,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 +1389,12 @@ 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) {
> 

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

[Tomasz Figa]

Hi Zhongmin,

On Wed, Jul 19, 2017 at 3:16 PM, Wu, Zhongmin  wrote:
> 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-boun...@lists.freedesktop.org] On Behalf Of 
> Tomasz Figa
> Sent: Wednesday, July 19, 2017 12:42
> To: Wu, Zhongmin 
> Cc: Gao, Shuo ; Liu, Zhiquan ; 
> Daniel Stone ; Antognolli, Rafael 
> ; Emil Velikov ; Chad 
> Versace ; Eric Engestrom ; 
> Marathe, Yogesh ; Kenneth Graunke 
> ; Kondapally, Kalyan ; 
> Rainer Hochecker ; ML mesa-dev 
> ; Nicolai Hähnle ; 
> Varad Gautam 
> 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  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 
>> Reported-by: Li, Guangli 
>> Tested-by: Marathe, Yogesh 
>> ---
>>  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

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

[Wu, Zhongmin]

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-boun...@lists.freedesktop.org] On Behalf Of 
Tomasz Figa
Sent: Wednesday, July 19, 2017 12:42 
To: Wu, Zhongmin 
Cc: Gao, Shuo ; Liu, Zhiquan ; 
Daniel Stone ; Antognolli, Rafael 
; Emil Velikov ; Chad 
Versace ; Eric Engestrom ; 
Marathe, Yogesh ; Kenneth Graunke 
; Kondapally, Kalyan ; 
Rainer Hochecker ; ML mesa-dev 
; Nicolai Hähnle ; 
Varad Gautam 
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  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 
> Reported-by: Li, Guangli 
> Tested-by: Marathe, Yogesh 
> ---
>  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 f

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

[Tomasz Figa]

Hi Zhongmin,

Thanks for the patch. Please see my comments inline.

On Wed, Jul 19, 2017 at 12:22 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
>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 
> Reported-by: Li, Guangli 
> Tested-by: Marathe, Yogesh 
> ---
>  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);
  }
// ...
}

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.

> +  }
>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;

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

[Wu, Zhongmin]

sa/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;
 };
 
 /**
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;
+ }
   }
 
   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);
+ return 0;
+   }
 
if (brw->throttle_batch[0] == NULL) {
   brw->throttle_batch[0] = brw->batch.bo;
-- 
1.7.9.5

From: Tomasz Figa [tf...@chromium.org]
Sent: Friday, July 14, 2017 9:58 AM
To: Antognolli, Rafael
Cc: Chris Wilson; ML mesa-dev; Marathe, Yogesh; Wu, Zhongmin; Liu, Zhiquan; 
Kondapally, Kalyan; Chad Versace; Eric Engestrom; Emil Velikov; Rob Clark; 
Kenneth Graunke; Widawsky, Benjamin; Kristian H . Kristensen; Timothy Arceri; 
Dominik Behr
Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS (v2)

[resend from right email address, without bouncing recipients and with
all the people involved in v1... Zhongmin, please remember to add all
people participating in the discussion to CC next time.]

On Sat, Jul 15, 2017 at 1:45 AM, Rafael Antognolli
 wrote:
> On Fri, Jul 14, 2017 at 09:13:49AM +0100, Chris Wilson wrote:
>> Quoting Zhongmin Wu (2017-07-14 07:55:45)
[snip]
>> >  extern uint32_t
>> > diff --git a/src/mesa/drivers/dri/i915/intel_screen.c 
>> > b/src/mesa/drivers/dri/i915/intel_screen.c
>> > index cba5434..38d0e63 100644
>> > --- a/src/mesa/drivers/dri/i915/intel_screen.c
>> > +++ b/src/mesa/drivers/dri/i915/intel_screen.c
>> > @@ -182,6 +182,7 @@ static const struct __DRI2flushExtensionRec 
>> > intelFlushExtension = {
>> >
>> >  .flush  = intelDRI2Flush,
>> >  .invalidate = dri2InvalidateDrawable,
>> > +.get_retrieve_fd= NULL,
>> >  };
>> >
>> >  static struct intel_image_format intel_image_formats[] = {
>> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
>> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> > index 62d2fe8..9813c8c 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->driDrawablePriv &&
>> > +brw->driContext->driDrawablePriv->retrieve_fd != -1) {
>> > +close(brw->driContext->driDrawablePriv->retrieve_fd);
>> > +brw->driContext->driDrawablePriv->retrieve_fd = -1;
>> > + }
>>
>> No. Use the correct interfaces for creating a fence, stop imposing the
>> cost upon everyone else.
>
> Yeah, that's correct, it definitely shouldn't be inside intel_batchbuffer.
>
> Chris, the main problem that Zhongmin raised was that on
> droid_window_cancel_buffer the conte

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

[Kenneth Graunke]

On Friday, July 14, 2017 10:50:39 AM PDT Rafael Antognolli wrote:
> On Sat, Jul 15, 2017 at 01:58:19AM +0900, Tomasz Figa wrote:
> > > So, the right place to do so would be inside platform_android.c,
> > > right? And since I don't see any private struct that could store such 
> > > fence
> > > there, one option would be to extend the struct dri2_egl_surface for 
> > > android,
> > > adding private data there. Does that make sense?
> > 
> > I'd say these fences are not 100% Android-specific anymore. They are
> > an upstream Linux feature and can be used on other platforms as well.
> > I think wiring this properly in EGL DRI2 core would make sense,
> > especially since this is the place where the context is being
> > destroyed (platform_android doesn't handle context destruction).
> 
> OK, so I'm trying to understand what "wiring this properly in EGL DRI2 core"
> really means.
> 
> Android supports receiving a fence on its enqueue_buffer function (and some
> others), but I assume not every platform does (or wants to) do that.
> 
> So would it make sense to create a fence before calling swap_buffers,
> destroy_surface, and related functions, and storing such fence so the platform
> specific code can pass it around if needed?
> 
> And I assume that's something optional, we only do that if the DRI2 fence
> extension exists, and maybe also check for some extra flag that can be set by
> the platform?
> 
> Rafael

At some point we probably want to do explicit fencing in X with
DRI3/Present, and also Wayland.  Not sure if anything would be sharable
across the EGL android/x11/wayland platforms then.

--Ken

signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Rafael Antognolli]

On Sat, Jul 15, 2017 at 01:58:19AM +0900, Tomasz Figa wrote:
> > So, the right place to do so would be inside platform_android.c,
> > right? And since I don't see any private struct that could store such fence
> > there, one option would be to extend the struct dri2_egl_surface for 
> > android,
> > adding private data there. Does that make sense?
> 
> I'd say these fences are not 100% Android-specific anymore. They are
> an upstream Linux feature and can be used on other platforms as well.
> I think wiring this properly in EGL DRI2 core would make sense,
> especially since this is the place where the context is being
> destroyed (platform_android doesn't handle context destruction).

OK, so I'm trying to understand what "wiring this properly in EGL DRI2 core"
really means.

Android supports receiving a fence on its enqueue_buffer function (and some
others), but I assume not every platform does (or wants to) do that.

So would it make sense to create a fence before calling swap_buffers,
destroy_surface, and related functions, and storing such fence so the platform
specific code can pass it around if needed?

And I assume that's something optional, we only do that if the DRI2 fence
extension exists, and maybe also check for some extra flag that can be set by
the platform?

Rafael
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Tomasz Figa]

[resend from right email address, without bouncing recipients and with
all the people involved in v1... Zhongmin, please remember to add all
people participating in the discussion to CC next time.]

On Sat, Jul 15, 2017 at 1:45 AM, Rafael Antognolli
 wrote:
> On Fri, Jul 14, 2017 at 09:13:49AM +0100, Chris Wilson wrote:
>> Quoting Zhongmin Wu (2017-07-14 07:55:45)
[snip]
>> >  extern uint32_t
>> > diff --git a/src/mesa/drivers/dri/i915/intel_screen.c 
>> > b/src/mesa/drivers/dri/i915/intel_screen.c
>> > index cba5434..38d0e63 100644
>> > --- a/src/mesa/drivers/dri/i915/intel_screen.c
>> > +++ b/src/mesa/drivers/dri/i915/intel_screen.c
>> > @@ -182,6 +182,7 @@ static const struct __DRI2flushExtensionRec 
>> > intelFlushExtension = {
>> >
>> >  .flush  = intelDRI2Flush,
>> >  .invalidate = dri2InvalidateDrawable,
>> > +.get_retrieve_fd= NULL,
>> >  };
>> >
>> >  static struct intel_image_format intel_image_formats[] = {
>> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
>> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> > index 62d2fe8..9813c8c 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->driDrawablePriv &&
>> > +brw->driContext->driDrawablePriv->retrieve_fd != -1) {
>> > +close(brw->driContext->driDrawablePriv->retrieve_fd);
>> > +brw->driContext->driDrawablePriv->retrieve_fd = -1;
>> > + }
>>
>> No. Use the correct interfaces for creating a fence, stop imposing the
>> cost upon everyone else.
>
> Yeah, that's correct, it definitely shouldn't be inside intel_batchbuffer.
>
> Chris, the main problem that Zhongmin raised was that on
> droid_window_cancel_buffer the context has been previously destroyed already,
> so they can't create a new fence for that old context. I haven't checked this
> myself as I don't have an android build.

Yes, that's a problem. But it should be solved at the source and not
by digging around it. IMHO it's a limitation of EGL DRI2 code and
that's where it should be fixed, by using all the tools already
available (i.e. DRI2 fence extension).

>
> Still, it looks to me that they would need to store the previous fence to
> solve this.

Makes sense.

>
> So, the right place to do so would be inside platform_android.c,
> right? And since I don't see any private struct that could store such fence
> there, one option would be to extend the struct dri2_egl_surface for android,
> adding private data there. Does that make sense?

I'd say these fences are not 100% Android-specific anymore. They are
an upstream Linux feature and can be used on other platforms as well.
I think wiring this properly in EGL DRI2 core would make sense,
especially since this is the place where the context is being
destroyed (platform_android doesn't handle context destruction).

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Rafael Antognolli]

On Sat, Jul 15, 2017 at 01:52:43AM +0900, Tomasz Figa wrote:
> Hi Rafael,
> 
> On Sat, Jul 15, 2017 at 1:45 AM, Rafael Antognolli
>  wrote:
> > On Fri, Jul 14, 2017 at 09:13:49AM +0100, Chris Wilson wrote:
> >> Quoting Zhongmin Wu (2017-07-14 07:55:45)
> [snip]
> >> >  extern uint32_t
> >> > diff --git a/src/mesa/drivers/dri/i915/intel_screen.c 
> >> > b/src/mesa/drivers/dri/i915/intel_screen.c
> >> > index cba5434..38d0e63 100644
> >> > --- a/src/mesa/drivers/dri/i915/intel_screen.c
> >> > +++ b/src/mesa/drivers/dri/i915/intel_screen.c
> >> > @@ -182,6 +182,7 @@ static const struct __DRI2flushExtensionRec 
> >> > intelFlushExtension = {
> >> >
> >> >  .flush  = intelDRI2Flush,
> >> >  .invalidate = dri2InvalidateDrawable,
> >> > +.get_retrieve_fd= NULL,
> >> >  };
> >> >
> >> >  static struct intel_image_format intel_image_formats[] = {
> >> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
> >> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> > index 62d2fe8..9813c8c 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->driDrawablePriv &&
> >> > +brw->driContext->driDrawablePriv->retrieve_fd != -1) {
> >> > +close(brw->driContext->driDrawablePriv->retrieve_fd);
> >> > +brw->driContext->driDrawablePriv->retrieve_fd = -1;
> >> > + }
> >>
> >> No. Use the correct interfaces for creating a fence, stop imposing the
> >> cost upon everyone else.
> >
> > Yeah, that's correct, it definitely shouldn't be inside intel_batchbuffer.
> >
> > Chris, the main problem that Zhongmin raised was that on
> > droid_window_cancel_buffer the context has been previously destroyed 
> > already,
> > so they can't create a new fence for that old context. I haven't checked 
> > this
> > myself as I don't have an android build.
> 
> Yes, that's a problem. But it should be solved at the source and not
> by digging around it. IMHO it's a limitation of EGL DRI2 code and
> that's where it should be fixed, by using all the tools already
> available (i.e. DRI2 fence extension).

Sure, I'm not saying it shouldn't, was just trying to help.

> >
> > Still, it looks to me that they would need to store the previous fence to
> > solve this.
> 
> Makes sense.
> 
> >
> > So, the right place to do so would be inside platform_android.c,
> > right? And since I don't see any private struct that could store such fence
> > there, one option would be to extend the struct dri2_egl_surface for 
> > android,
> > adding private data there. Does that make sense?
> 
> I'd say these fences are not 100% Android-specific anymore. They are
> an upstream Linux feature and can be used on other platforms as well.
> I think wiring this properly in EGL DRI2 core would make sense,
> especially since this is the place where the context is being
> destroyed (platform_android doesn't handle context destruction).

Ah, yes, that makes sense. I'll see if I can help.

Thanks!
Rafael

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Tomasz Figa]

Hi Rafael,

On Sat, Jul 15, 2017 at 1:45 AM, Rafael Antognolli
 wrote:
> On Fri, Jul 14, 2017 at 09:13:49AM +0100, Chris Wilson wrote:
>> Quoting Zhongmin Wu (2017-07-14 07:55:45)
[snip]
>> >  extern uint32_t
>> > diff --git a/src/mesa/drivers/dri/i915/intel_screen.c 
>> > b/src/mesa/drivers/dri/i915/intel_screen.c
>> > index cba5434..38d0e63 100644
>> > --- a/src/mesa/drivers/dri/i915/intel_screen.c
>> > +++ b/src/mesa/drivers/dri/i915/intel_screen.c
>> > @@ -182,6 +182,7 @@ static const struct __DRI2flushExtensionRec 
>> > intelFlushExtension = {
>> >
>> >  .flush  = intelDRI2Flush,
>> >  .invalidate = dri2InvalidateDrawable,
>> > +.get_retrieve_fd= NULL,
>> >  };
>> >
>> >  static struct intel_image_format intel_image_formats[] = {
>> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
>> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> > index 62d2fe8..9813c8c 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->driDrawablePriv &&
>> > +brw->driContext->driDrawablePriv->retrieve_fd != -1) {
>> > +close(brw->driContext->driDrawablePriv->retrieve_fd);
>> > +brw->driContext->driDrawablePriv->retrieve_fd = -1;
>> > + }
>>
>> No. Use the correct interfaces for creating a fence, stop imposing the
>> cost upon everyone else.
>
> Yeah, that's correct, it definitely shouldn't be inside intel_batchbuffer.
>
> Chris, the main problem that Zhongmin raised was that on
> droid_window_cancel_buffer the context has been previously destroyed already,
> so they can't create a new fence for that old context. I haven't checked this
> myself as I don't have an android build.

Yes, that's a problem. But it should be solved at the source and not
by digging around it. IMHO it's a limitation of EGL DRI2 code and
that's where it should be fixed, by using all the tools already
available (i.e. DRI2 fence extension).

>
> Still, it looks to me that they would need to store the previous fence to
> solve this.

Makes sense.

>
> So, the right place to do so would be inside platform_android.c,
> right? And since I don't see any private struct that could store such fence
> there, one option would be to extend the struct dri2_egl_surface for android,
> adding private data there. Does that make sense?

I'd say these fences are not 100% Android-specific anymore. They are
an upstream Linux feature and can be used on other platforms as well.
I think wiring this properly in EGL DRI2 core would make sense,
especially since this is the place where the context is being
destroyed (platform_android doesn't handle context destruction).

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Rafael Antognolli]

On Fri, Jul 14, 2017 at 09:13:49AM +0100, Chris Wilson wrote:
> Quoting Zhongmin Wu (2017-07-14 07:55:45)
> > 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.
> > 
> > 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 
> > Reported-by: Li, Guangli 
> > Tested-by: Marathe, Yogesh 
> > ---
> >  include/GL/internal/dri_interface.h   |9 +
> >  src/egl/drivers/dri2/platform_android.c   |   20 +---
> >  src/mesa/drivers/dri/common/dri_util.c|3 +++
> >  src/mesa/drivers/dri/common/dri_util.h|2 ++
> >  src/mesa/drivers/dri/i915/intel_screen.c  |1 +
> >  src/mesa/drivers/dri/i965/intel_batchbuffer.c |   18 +-
> >  src/mesa/drivers/dri/i965/intel_screen.c  |6 ++
> >  src/mesa/drivers/dri/nouveau/nouveau_screen.c |1 +
> >  src/mesa/drivers/dri/radeon/radeon_screen.c   |1 +
> >  9 files changed, 53 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/GL/internal/dri_interface.h 
> > b/include/GL/internal/dri_interface.h
> > index fc2d4bb..7c6b08b 100644
> > --- a/include/GL/internal/dri_interface.h
> > +++ b/include/GL/internal/dri_interface.h
> > @@ -287,6 +287,15 @@ struct __DRI2flushExtensionRec {
> >  void (*flush)(__DRIdrawable *drawable);
> >  
> >  /**
> > +* This function enables retrieving fence during enqueue / cancel 
> > buffer operations
> > +*
> > +* \param drawable the drawable to invalidate
> > +*
> > +* \since 3
> > +*/
> > +int (*get_retrieve_fd)(__DRIdrawable *drawable);
> > +
> > +/**
> >   * Ask the driver to call getBuffers/getBuffersWithFormat before
> >   * it starts rendering again.
> >   *
> > diff --git a/src/egl/drivers/dri2/platform_android.c 
> > b/src/egl/drivers/dri2/platform_android.c
> > index cc2e4a6..aa99ed3 100644
> > --- a/src/egl/drivers/dri2/platform_android.c
> > +++ b/src/egl/drivers/dri2/platform_android.c
> > @@ -305,10 +305,11 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct 
> > dri2_egl_surface *dri2_sur
> >  *is passed to queueBuffer, and the ANativeWindow implementation
> >  *is responsible for closing it.
> >  */
> > -   int fence_fd = -1;
> > +   int fd = -1;
> > +   if (dri2_dpy->flush->get_retrieve_fd)
> > +  fd = dri2_dpy->flush->get_retrieve_fd(dri2_surf->dri_drawable);
> > dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
> > -  fence_fd);
> > -
> > +  fd);
> > dri2_surf->buffer->common.decRef(&dri2_surf->buffer->common);
> > dri2_surf->buffer = NULL;
> > dri2_surf->back = NULL;
> > @@ -324,11 +325,16 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct 
> > dri2_egl_surface *dri2_sur
> >  }
> >  
> >  static void
> > -droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf)
> > +droid_window_cancel_buffer(_EGLDisplay *disp,
> > +   struct dri2_egl_surface *dri2_surf)
> >  {
> > int ret;
> > -
> > -   ret = dri2_surf->window->cancelBuffer(dri2_surf->window, 
> > dri2_surf->buffer, -1);
> > +   int fd = -1;
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> > +   if (dri2_dpy->flush->get_retrieve_fd)
> > +  fd = dri2_dpy->flush->get_retrieve_fd(dri2_surf->dri_drawable);
> > +   ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
> > + dri2_surf->buffer, fd);
> > if (ret < 0) {
> >_eglLog(_EGL_WARNING, "ANativeWindow::cancelBuffer failed");
> >dri2_surf->base.Lost = EGL_TRUE;
> > @@ -469,7 +475,7 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay 
> > *disp, _EGLSurface *surf)
> >  
> > if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
> >if (dri2_surf->buffer)
> > - droid_window_cancel_buffer(dri2_surf);
> > + droid_window_cancel_buffer(disp, dri2_surf);
> >  
> >dri2_surf->window->common.decRef(&dri2_surf->window->common);
> > }
> > diff --git a/src/mesa/drivers/dri/common/dri_util.c 
> > b/src/mesa/drivers/dri/common/dri_util.c
> > index f6df488..21ee6aa 100644
> > --- a/src/mesa/drivers/dri/common/dri_util.c
> > +++ b/src/mesa/drivers/dri/common/dri_util.c
> > @@ -636,6 +636,8 @@ static void dri_put_drawable(__DRIdrawable *pdp)
> > return;
> >  
> > pdp->driScreenPriv->driver->DestroyBuffer(pdp);
> > +if (pdp->retrieve_fd != -1)
> > +   close

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

[Emil Velikov]

Hi Zhongmin Wu,

On 14 July 2017 at 07:55, 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.
>
> 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 
> Reported-by: Li, Guangli 
> Tested-by: Marathe, Yogesh 
> ---
>  include/GL/internal/dri_interface.h   |9 +
>  src/egl/drivers/dri2/platform_android.c   |   20 +---
>  src/mesa/drivers/dri/common/dri_util.c|3 +++
>  src/mesa/drivers/dri/common/dri_util.h|2 ++
>  src/mesa/drivers/dri/i915/intel_screen.c  |1 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c |   18 +-
>  src/mesa/drivers/dri/i965/intel_screen.c  |6 ++
>  src/mesa/drivers/dri/nouveau/nouveau_screen.c |1 +
>  src/mesa/drivers/dri/radeon/radeon_screen.c   |1 +
>  9 files changed, 53 insertions(+), 8 deletions(-)
>
> diff --git a/include/GL/internal/dri_interface.h 
> b/include/GL/internal/dri_interface.h
> index fc2d4bb..7c6b08b 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -287,6 +287,15 @@ struct __DRI2flushExtensionRec {
>  void (*flush)(__DRIdrawable *drawable);
>
>  /**
> +* This function enables retrieving fence during enqueue / cancel buffer 
> operations
> +*
> +* \param drawable the drawable to invalidate
> +*
> +* \since 3
> +*/
> +int (*get_retrieve_fd)(__DRIdrawable *drawable);
> +
If you get to v3 using the same approach, do not forget to incorporate
my earlier suggestions.
Reiterating from before, emphasising on the missing parts.

 - when extending the interface, _the version number must be bumped_
 - user should _check the version_ and the function pointer prior to
use, falling back to the old scheme
 - get_retrive_fd [barring the typo - retrieve], should have at least
_the fd ownership documented_

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Eric Engestrom]

On Friday, 2017-07-14 14:55:45 +0800, 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.

I don't know much about fences, sorry, shallow review.

> 
> 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 
> Reported-by: Li, Guangli 
> Tested-by: Marathe, Yogesh 
> ---
>  include/GL/internal/dri_interface.h   |9 +
>  src/egl/drivers/dri2/platform_android.c   |   20 +---
>  src/mesa/drivers/dri/common/dri_util.c|3 +++
>  src/mesa/drivers/dri/common/dri_util.h|2 ++
>  src/mesa/drivers/dri/i915/intel_screen.c  |1 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c |   18 +-
>  src/mesa/drivers/dri/i965/intel_screen.c  |6 ++
>  src/mesa/drivers/dri/nouveau/nouveau_screen.c |1 +
>  src/mesa/drivers/dri/radeon/radeon_screen.c   |1 +
>  9 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/include/GL/internal/dri_interface.h 
> b/include/GL/internal/dri_interface.h
> index fc2d4bb..7c6b08b 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -287,6 +287,15 @@ struct __DRI2flushExtensionRec {
>  void (*flush)(__DRIdrawable *drawable);
>  
>  /**
> +* This function enables retrieving fence during enqueue / cancel buffer 
> operations
> +*
> +* \param drawable the drawable to invalidate
> +*
> +* \since 3
> +*/
> +int (*get_retrieve_fd)(__DRIdrawable *drawable);
> +
> +/**
>   * Ask the driver to call getBuffers/getBuffersWithFormat before
>   * it starts rendering again.
>   *
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index cc2e4a6..aa99ed3 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -305,10 +305,11 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct 
> dri2_egl_surface *dri2_sur
>  *is passed to queueBuffer, and the ANativeWindow implementation
>  *is responsible for closing it.
>  */
> -   int fence_fd = -1;
> +   int fd = -1;
> +   if (dri2_dpy->flush->get_retrieve_fd)
> +  fd = dri2_dpy->flush->get_retrieve_fd(dri2_surf->dri_drawable);

Nit: these two lines would be the only diff in this hunk without the
rename, and `fence_fd` sounds like a better name than just `fd` :P

> dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
> -  fence_fd);
> -
> +  fd);
> dri2_surf->buffer->common.decRef(&dri2_surf->buffer->common);
> dri2_surf->buffer = NULL;
> dri2_surf->back = NULL;
> @@ -324,11 +325,16 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct 
> dri2_egl_surface *dri2_sur
>  }
>  
>  static void
> -droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf)
> +droid_window_cancel_buffer(_EGLDisplay *disp,
> +   struct dri2_egl_surface *dri2_surf)

Do we really want to mix these?
You could pass `dri2_egl_display(disp)` when calling the function.

>  {
> int ret;
> -
> -   ret = dri2_surf->window->cancelBuffer(dri2_surf->window, 
> dri2_surf->buffer, -1);
> +   int fd = -1;
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> +   if (dri2_dpy->flush->get_retrieve_fd)
> +  fd = dri2_dpy->flush->get_retrieve_fd(dri2_surf->dri_drawable);
> +   ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
> + dri2_surf->buffer, fd);
> if (ret < 0) {
>_eglLog(_EGL_WARNING, "ANativeWindow::cancelBuffer failed");
>dri2_surf->base.Lost = EGL_TRUE;
> @@ -469,7 +475,7 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, 
> _EGLSurface *surf)
>  
> if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
>if (dri2_surf->buffer)
> - droid_window_cancel_buffer(dri2_surf);
> + droid_window_cancel_buffer(disp, dri2_surf);
>  
>dri2_surf->window->common.decRef(&dri2_surf->window->common);
> }
> diff --git a/src/mesa/drivers/dri/common/dri_util.c 
> b/src/mesa/drivers/dri/common/dri_util.c
> index f6df488..21ee6aa 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -636,6 +636,8 @@ static void dri_put_drawable(__DRIdrawable *pdp)
>   return;
>  
>   pdp->driScreenPriv->driver->DestroyBuffer(pdp);
> +if (pdp->retrieve_fd != -1)
> 

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

[Wu, Zhongmin]

Add Chris Wilson


-Original Message-
From: Wu, Zhongmin 
Sent: Thursday, July 13, 2017 16:31 
To: 'Tomasz Figa' ; 'Michel Dänzer' 
Cc: Liu, Zhiquan ; Kondapally, Kalyan 
; Marathe, Yogesh ; 
'Chad Versace' ; 'Eric Engestrom' 
; 'Emil Velikov' ; 'Rob Clark' 
; 'Kenneth Graunke' ; 
Widawsky, Benjamin ; 'ML mesa-dev' 
; 'Kristian H . Kristensen' 
; 'Timothy Arceri' 
Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

By the way,  we can't use create_fence_fd in cancelBuffer even if we 
implemented the last fence feature as radeonsi driver , since the context is 
reset before cancelBuffer, and we cannot depend on the last fence in context 
object either...

So things may be tricky on Android 

We will try to modify the previous patch and save the last fence in the 
drawable object (contained in EGLSurface) rather than context object to support 
the cancelBuffer...

As for the using of last fence when the batch buffer is empty for 
create_fence_fd,  I suggest it can be another story and we will try to optimize 
it in the future...



-Original Message-
From: Wu, Zhongmin
Sent: Thursday, July 13, 2017 12:23
To: Tomasz Figa ; Michel Dänzer 
Cc: Liu, Zhiquan ; Kondapally, Kalyan 
; Marathe, Yogesh ; Chad 
Versace ; Eric Engestrom ; Emil 
Velikov ; Rob Clark ; 
Kenneth Graunke ; Widawsky, Benjamin 
; ML mesa-dev ; 
Kristian H . Kristensen ; Timothy Arceri 

Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

Hi Tomasz:

That is right,  what we needed is just the "last fence", and then where to 
record it and how to get it.  

We will check the solutions on radeonsi by Michel, if create_fence_fd can also 
get the last fence, of course we can use such DRI2 fence extension API in 
queuebuffer and canclebuffer.

-Original Message-
From: Tomasz Figa [mailto:tf...@chromium.org]
Sent: Thursday, July 13, 2017 12:08
To: Michel Dänzer ; Wu, Zhongmin 
Cc: Liu, Zhiquan ; Kondapally, Kalyan 
; Marathe, Yogesh ; Chad 
Versace ; Eric Engestrom ; Emil 
Velikov ; Rob Clark ; 
Kenneth Graunke ; Widawsky, Benjamin 
; ML mesa-dev ; 
Kristian H . Kristensen ; Timothy Arceri 

Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

Hi Zhongmin, Michel,

On Thu, Jul 13, 2017 at 12:21 PM, Michel Dänzer  wrote:
> On 13/07/17 09:28 AM, Wu, Zhongmin wrote:
>> Hi Tomasz
>>
>> Thanks,  but I am afraid we have to use the last fence from the last buffer 
>> flushing , According to my understanding:
>>
>> If the glFlush was called before swapbuffer ,there would be no new fence 
>> after that since the batch buffer may be flushed out, no batch flushing, no 
>> fence generated.
>>
>> 
>> You can check the function "_intel_batchbuffer_flush_fence",  at the very 
>> beginning, it will check the content of the buffer, if it is empty, it will 
>> return and do nothing.
>> =
>>
>> So, as the same reason,  it may not work if we use 
>> dri2_dpy->fence->create_fence_fd,   the process is below.
>>
>> Swapbuffer (==> flush batch buffer) ==>  create_fence_fd(===> flush 
>> batch buffer and get fd)
>>
>> You can see, the second buffer flushing may get nothing (not to 
>> mention  the first buffer flushing may get nothing either)

First of all, the sequence of calls is like this:

- brw_dri_create_fence_fd()
  - brw_fence_insert_locked()
- brw_emit_mi_flush() <--- this emits commands to the command buffer...
- intel_batchbuffer_flush_fence() <--- this should flush the buffer and get 
a fence

but...

>
> Apologies for jumping into this discussion, and possibly missing some 
> context.
>
> FWIW, it should be possible to re-use the previous fence when there is 
> no new work to flush. See e.g. commits
>
> f1be3d8cdde1 ("radeonsi: don't flush an empty IB if the only thing we
>need is a fence")
> 800efb0690e9 ("radeonsi: Flush when we're asked to return a fence but
>don't have one yet")
>
> for how the radeonsi driver handles this.

I agree with Michel that it would make more sense if Intel driver kept the last 
fence and returned it using when DRI2 fence extension is called.

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Wu, Zhongmin]

Hi Chris Wilson, 

Thanks for your advice, but we have some discussions on another thread,  and 
now we do meet some tricky things about how to implement such fence needed by 
Android, and why current API in codes cannot meet the requirement.

I will add you in that loop and hope U can help us to find ways to handle it. 
Thanks again !



-Original Message-
From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of 
Chris Wilson
Sent: Friday, July 14, 2017 16:14 
To: Wu, Zhongmin ; mesa-dev@lists.freedesktop.org
Cc: b...@freedesktop.org; Marathe, Yogesh 
Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS (v2)

Quoting Zhongmin Wu (2017-07-14 07:55:45)
> 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.
> 
> 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 
> Reported-by: Li, Guangli 
> Tested-by: Marathe, Yogesh 
> ---
>  include/GL/internal/dri_interface.h   |9 +
>  src/egl/drivers/dri2/platform_android.c   |   20 +---
>  src/mesa/drivers/dri/common/dri_util.c|3 +++
>  src/mesa/drivers/dri/common/dri_util.h|2 ++
>  src/mesa/drivers/dri/i915/intel_screen.c  |1 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c |   18 +-
>  src/mesa/drivers/dri/i965/intel_screen.c  |6 ++
>  src/mesa/drivers/dri/nouveau/nouveau_screen.c |1 +
>  src/mesa/drivers/dri/radeon/radeon_screen.c   |1 +
>  9 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/include/GL/internal/dri_interface.h 
> b/include/GL/internal/dri_interface.h
> index fc2d4bb..7c6b08b 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -287,6 +287,15 @@ struct __DRI2flushExtensionRec {
>  void (*flush)(__DRIdrawable *drawable);
>  
>  /**
> +* This function enables retrieving fence during enqueue / cancel buffer 
> operations
> +*
> +* \param drawable the drawable to invalidate
> +*
> +* \since 3
> +*/
> +int (*get_retrieve_fd)(__DRIdrawable *drawable);
> +
> +/**
>   * Ask the driver to call getBuffers/getBuffersWithFormat before
>   * it starts rendering again.
>   *
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index cc2e4a6..aa99ed3 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -305,10 +305,11 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct 
> dri2_egl_surface *dri2_sur
>  *is passed to queueBuffer, and the ANativeWindow implementation
>  *is responsible for closing it.
>  */
> -   int fence_fd = -1;
> +   int fd = -1;
> +   if (dri2_dpy->flush->get_retrieve_fd)
> +  fd = dri2_dpy->flush->get_retrieve_fd(dri2_surf->dri_drawable);
> dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
> -  fence_fd);
> -
> +  fd);
> dri2_surf->buffer->common.decRef(&dri2_surf->buffer->common);
> dri2_surf->buffer = NULL;
> dri2_surf->back = NULL;
> @@ -324,11 +325,16 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, 
> struct dri2_egl_surface *dri2_sur  }
>  
>  static void
> -droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf)
> +droid_window_cancel_buffer(_EGLDisplay *disp,
> +   struct dri2_egl_surface *dri2_surf)
>  {
> int ret;
> -
> -   ret = dri2_surf->window->cancelBuffer(dri2_surf->window, 
> dri2_surf->buffer, -1);
> +   int fd = -1;
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> +   if (dri2_dpy->flush->get_retrieve_fd)
> +  fd = dri2_dpy->flush->get_retrieve_fd(dri2_surf->dri_drawable);
> +   ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
> + dri2_surf->buffer, fd);
> if (ret < 0) {
>_eglLog(_EGL_WARNING, "ANativeWindow::cancelBuffer failed");
>dri2_surf->base.Lost 

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

[Chris Wilson]

Quoting Zhongmin Wu (2017-07-14 07:55:45)
> 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.
> 
> 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 
> Reported-by: Li, Guangli 
> Tested-by: Marathe, Yogesh 
> ---
>  include/GL/internal/dri_interface.h   |9 +
>  src/egl/drivers/dri2/platform_android.c   |   20 +---
>  src/mesa/drivers/dri/common/dri_util.c|3 +++
>  src/mesa/drivers/dri/common/dri_util.h|2 ++
>  src/mesa/drivers/dri/i915/intel_screen.c  |1 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c |   18 +-
>  src/mesa/drivers/dri/i965/intel_screen.c  |6 ++
>  src/mesa/drivers/dri/nouveau/nouveau_screen.c |1 +
>  src/mesa/drivers/dri/radeon/radeon_screen.c   |1 +
>  9 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/include/GL/internal/dri_interface.h 
> b/include/GL/internal/dri_interface.h
> index fc2d4bb..7c6b08b 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -287,6 +287,15 @@ struct __DRI2flushExtensionRec {
>  void (*flush)(__DRIdrawable *drawable);
>  
>  /**
> +* This function enables retrieving fence during enqueue / cancel buffer 
> operations
> +*
> +* \param drawable the drawable to invalidate
> +*
> +* \since 3
> +*/
> +int (*get_retrieve_fd)(__DRIdrawable *drawable);
> +
> +/**
>   * Ask the driver to call getBuffers/getBuffersWithFormat before
>   * it starts rendering again.
>   *
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index cc2e4a6..aa99ed3 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -305,10 +305,11 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct 
> dri2_egl_surface *dri2_sur
>  *is passed to queueBuffer, and the ANativeWindow implementation
>  *is responsible for closing it.
>  */
> -   int fence_fd = -1;
> +   int fd = -1;
> +   if (dri2_dpy->flush->get_retrieve_fd)
> +  fd = dri2_dpy->flush->get_retrieve_fd(dri2_surf->dri_drawable);
> dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
> -  fence_fd);
> -
> +  fd);
> dri2_surf->buffer->common.decRef(&dri2_surf->buffer->common);
> dri2_surf->buffer = NULL;
> dri2_surf->back = NULL;
> @@ -324,11 +325,16 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct 
> dri2_egl_surface *dri2_sur
>  }
>  
>  static void
> -droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf)
> +droid_window_cancel_buffer(_EGLDisplay *disp,
> +   struct dri2_egl_surface *dri2_surf)
>  {
> int ret;
> -
> -   ret = dri2_surf->window->cancelBuffer(dri2_surf->window, 
> dri2_surf->buffer, -1);
> +   int fd = -1;
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> +   if (dri2_dpy->flush->get_retrieve_fd)
> +  fd = dri2_dpy->flush->get_retrieve_fd(dri2_surf->dri_drawable);
> +   ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
> + dri2_surf->buffer, fd);
> if (ret < 0) {
>_eglLog(_EGL_WARNING, "ANativeWindow::cancelBuffer failed");
>dri2_surf->base.Lost = EGL_TRUE;
> @@ -469,7 +475,7 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, 
> _EGLSurface *surf)
>  
> if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
>if (dri2_surf->buffer)
> - droid_window_cancel_buffer(dri2_surf);
> + droid_window_cancel_buffer(disp, dri2_surf);
>  
>dri2_surf->window->common.decRef(&dri2_surf->window->common);
> }
> diff --git a/src/mesa/drivers/dri/common/dri_util.c 
> b/src/mesa/drivers/dri/common/dri_util.c
> index f6df488..21ee6aa 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -636,6 +636,8 @@ static void dri_put_drawable(__DRIdrawable *pdp)
> return;
>  
> pdp->driScreenPriv->driver->DestroyBuffer(pdp);
> +if (pdp->retrieve_fd != -1)
> +   close(pdp->retrieve_fd);
> free(pdp);
>  }
>  }
> @@ -661,6 +663,7 @@ driCreateNewDrawable(__DRIscreen *screen,
>  pdraw->lastStamp = 0;
>  pdraw->w = 0;
>  pdraw->h = 0;
> +pdraw->retrieve_fd = -1;
>  
>  dri_get_drawable(pdraw);
>  
> diff --git a/src/mesa/drivers/

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

[Chris Wilson]

Quoting Wu, Zhongmin (2017-07-13 09:31:15)
> As for the using of last fence when the batch buffer is empty for 
> create_fence_fd,  I suggest it can be another story and we will try to 
> optimize it in the future...

Note that is a backend problem. If you call a driver interface to create
a fence and it fails to insert a fence into the context timeline, that
is a bug in the driver. As you are were talking about the i965 behaviour
of skipping empty batches, you will note that brw_fence_insert_locked()
ensures that the batch is not empty (and that the drawable is resolved).
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Wu, Zhongmin]

By the way,  we can't use create_fence_fd in cancelBuffer even if we 
implemented the last fence feature as radeonsi driver , since the context is 
reset before cancelBuffer, and we cannot depend on the last fence in context 
object either...

So things may be tricky on Android 

We will try to modify the previous patch and save the last fence in the 
drawable object (contained in EGLSurface) rather than context object to support 
the cancelBuffer...

As for the using of last fence when the batch buffer is empty for 
create_fence_fd,  I suggest it can be another story and we will try to optimize 
it in the future...



-Original Message-
From: Wu, Zhongmin 
Sent: Thursday, July 13, 2017 12:23 
To: Tomasz Figa ; Michel Dänzer 
Cc: Liu, Zhiquan ; Kondapally, Kalyan 
; Marathe, Yogesh ; Chad 
Versace ; Eric Engestrom ; Emil 
Velikov ; Rob Clark ; 
Kenneth Graunke ; Widawsky, Benjamin 
; ML mesa-dev ; 
Kristian H . Kristensen ; Timothy Arceri 

Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

Hi Tomasz:

That is right,  what we needed is just the "last fence", and then where to 
record it and how to get it.  

We will check the solutions on radeonsi by Michel, if create_fence_fd can also 
get the last fence, of course we can use such DRI2 fence extension API in 
queuebuffer and canclebuffer.

-Original Message-
From: Tomasz Figa [mailto:tf...@chromium.org]
Sent: Thursday, July 13, 2017 12:08
To: Michel Dänzer ; Wu, Zhongmin 
Cc: Liu, Zhiquan ; Kondapally, Kalyan 
; Marathe, Yogesh ; Chad 
Versace ; Eric Engestrom ; Emil 
Velikov ; Rob Clark ; 
Kenneth Graunke ; Widawsky, Benjamin 
; ML mesa-dev ; 
Kristian H . Kristensen ; Timothy Arceri 

Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

Hi Zhongmin, Michel,

On Thu, Jul 13, 2017 at 12:21 PM, Michel Dänzer  wrote:
> On 13/07/17 09:28 AM, Wu, Zhongmin wrote:
>> Hi Tomasz
>>
>> Thanks,  but I am afraid we have to use the last fence from the last buffer 
>> flushing , According to my understanding:
>>
>> If the glFlush was called before swapbuffer ,there would be no new fence 
>> after that since the batch buffer may be flushed out, no batch flushing, no 
>> fence generated.
>>
>> 
>> You can check the function "_intel_batchbuffer_flush_fence",  at the very 
>> beginning, it will check the content of the buffer, if it is empty, it will 
>> return and do nothing.
>> =
>>
>> So, as the same reason,  it may not work if we use 
>> dri2_dpy->fence->create_fence_fd,   the process is below.
>>
>> Swapbuffer (==> flush batch buffer) ==>  create_fence_fd(===> flush 
>> batch buffer and get fd)
>>
>> You can see, the second buffer flushing may get nothing (not to 
>> mention  the first buffer flushing may get nothing either)

First of all, the sequence of calls is like this:

- brw_dri_create_fence_fd()
  - brw_fence_insert_locked()
- brw_emit_mi_flush() <--- this emits commands to the command buffer...
- intel_batchbuffer_flush_fence() <--- this should flush the buffer and get 
a fence

but...

>
> Apologies for jumping into this discussion, and possibly missing some 
> context.
>
> FWIW, it should be possible to re-use the previous fence when there is 
> no new work to flush. See e.g. commits
>
> f1be3d8cdde1 ("radeonsi: don't flush an empty IB if the only thing we
>need is a fence")
> 800efb0690e9 ("radeonsi: Flush when we're asked to return a fence but
>don't have one yet")
>
> for how the radeonsi driver handles this.

I agree with Michel that it would make more sense if Intel driver kept the last 
fence and returned it using when DRI2 fence extension is called.

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Wu, Zhongmin]

Hi Tomasz:

That is right,  what we needed is just the "last fence", and then where to 
record it and how to get it.  

We will check the solutions on radeonsi by Michel, if create_fence_fd can also 
get the last fence, of course we can use such DRI2 fence extension API in 
queuebuffer and canclebuffer.

-Original Message-
From: Tomasz Figa [mailto:tf...@chromium.org] 
Sent: Thursday, July 13, 2017 12:08 
To: Michel Dänzer ; Wu, Zhongmin 
Cc: Liu, Zhiquan ; Kondapally, Kalyan 
; Marathe, Yogesh ; Chad 
Versace ; Eric Engestrom ; Emil 
Velikov ; Rob Clark ; 
Kenneth Graunke ; Widawsky, Benjamin 
; ML mesa-dev ; 
Kristian H . Kristensen ; Timothy Arceri 

Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

Hi Zhongmin, Michel,

On Thu, Jul 13, 2017 at 12:21 PM, Michel Dänzer  wrote:
> On 13/07/17 09:28 AM, Wu, Zhongmin wrote:
>> Hi Tomasz
>>
>> Thanks,  but I am afraid we have to use the last fence from the last buffer 
>> flushing , According to my understanding:
>>
>> If the glFlush was called before swapbuffer ,there would be no new fence 
>> after that since the batch buffer may be flushed out, no batch flushing, no 
>> fence generated.
>>
>> 
>> You can check the function "_intel_batchbuffer_flush_fence",  at the very 
>> beginning, it will check the content of the buffer, if it is empty, it will 
>> return and do nothing.
>> =
>>
>> So, as the same reason,  it may not work if we use 
>> dri2_dpy->fence->create_fence_fd,   the process is below.
>>
>> Swapbuffer (==> flush batch buffer) ==>  create_fence_fd(===> flush 
>> batch buffer and get fd)
>>
>> You can see, the second buffer flushing may get nothing (not to 
>> mention  the first buffer flushing may get nothing either)

First of all, the sequence of calls is like this:

- brw_dri_create_fence_fd()
  - brw_fence_insert_locked()
- brw_emit_mi_flush() <--- this emits commands to the command buffer...
- intel_batchbuffer_flush_fence() <--- this should flush the buffer and get 
a fence

but...

>
> Apologies for jumping into this discussion, and possibly missing some 
> context.
>
> FWIW, it should be possible to re-use the previous fence when there is 
> no new work to flush. See e.g. commits
>
> f1be3d8cdde1 ("radeonsi: don't flush an empty IB if the only thing we
>need is a fence")
> 800efb0690e9 ("radeonsi: Flush when we're asked to return a fence but
>don't have one yet")
>
> for how the radeonsi driver handles this.

I agree with Michel that it would make more sense if Intel driver kept the last 
fence and returned it using when DRI2 fence extension is called.

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Tomasz Figa]

Hi Zhongmin, Michel,

On Thu, Jul 13, 2017 at 12:21 PM, Michel Dänzer  wrote:
> On 13/07/17 09:28 AM, Wu, Zhongmin wrote:
>> Hi Tomasz
>>
>> Thanks,  but I am afraid we have to use the last fence from the last buffer 
>> flushing , According to my understanding:
>>
>> If the glFlush was called before swapbuffer ,there would be no new fence 
>> after that since the batch buffer may be flushed out, no batch flushing, no 
>> fence generated.
>>
>> 
>> You can check the function "_intel_batchbuffer_flush_fence",  at the very 
>> beginning, it will check the content of the buffer, if it is empty, it will 
>> return and do nothing.
>> =
>>
>> So, as the same reason,  it may not work if we use 
>> dri2_dpy->fence->create_fence_fd,   the process is below.
>>
>> Swapbuffer (==> flush batch buffer) ==>  create_fence_fd(===> flush batch 
>> buffer and get fd)
>>
>> You can see, the second buffer flushing may get nothing (not to mention  the 
>> first buffer flushing may get nothing either)

First of all, the sequence of calls is like this:

- brw_dri_create_fence_fd()
  - brw_fence_insert_locked()
- brw_emit_mi_flush() <--- this emits commands to the command buffer...
- intel_batchbuffer_flush_fence() <--- this should flush the
buffer and get a fence

but...

>
> Apologies for jumping into this discussion, and possibly missing some
> context.
>
> FWIW, it should be possible to re-use the previous fence when there is
> no new work to flush. See e.g. commits
>
> f1be3d8cdde1 ("radeonsi: don't flush an empty IB if the only thing we
>need is a fence")
> 800efb0690e9 ("radeonsi: Flush when we're asked to return a fence but
>don't have one yet")
>
> for how the radeonsi driver handles this.

I agree with Michel that it would make more sense if Intel driver kept
the last fence and returned it using when DRI2 fence extension is
called.

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Michel Dänzer]

On 13/07/17 09:28 AM, Wu, Zhongmin wrote:
> Hi Tomasz
> 
> Thanks,  but I am afraid we have to use the last fence from the last buffer 
> flushing , According to my understanding:
> 
> If the glFlush was called before swapbuffer ,there would be no new fence 
> after that since the batch buffer may be flushed out, no batch flushing, no 
> fence generated.
> 
> 
> You can check the function "_intel_batchbuffer_flush_fence",  at the very 
> beginning, it will check the content of the buffer, if it is empty, it will 
> return and do nothing.
> =
> 
> So, as the same reason,  it may not work if we use 
> dri2_dpy->fence->create_fence_fd,   the process is below.
> 
> Swapbuffer (==> flush batch buffer) ==>  create_fence_fd(===> flush batch 
> buffer and get fd)
> 
> You can see, the second buffer flushing may get nothing (not to mention  the 
> first buffer flushing may get nothing either)

Apologies for jumping into this discussion, and possibly missing some
context.

FWIW, it should be possible to re-use the previous fence when there is
no new work to flush. See e.g. commits

f1be3d8cdde1 ("radeonsi: don't flush an empty IB if the only thing we
   need is a fence")
800efb0690e9 ("radeonsi: Flush when we're asked to return a fence but
   don't have one yet")

for how the radeonsi driver handles this.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Wu, Zhongmin]

Hi Tomasz

Thanks,  but I am afraid we have to use the last fence from the last buffer 
flushing , According to my understanding:

If the glFlush was called before swapbuffer ,there would be no new fence 
after that since the batch buffer may be flushed out, no batch flushing, no 
fence generated.


You can check the function "_intel_batchbuffer_flush_fence",  at the very 
beginning, it will check the content of the buffer, if it is empty, it will 
return and do nothing.
=

So, as the same reason,  it may not work if we use 
dri2_dpy->fence->create_fence_fd,   the process is below.

Swapbuffer (==> flush batch buffer) ==>  create_fence_fd(===> flush batch 
buffer and get fd)

You can see, the second buffer flushing may get nothing (not to mention  the 
first buffer flushing may get nothing either)

-Original Message-
From: Tomasz Figa [mailto:tf...@chromium.org] 
Sent: Wednesday, July 12, 2017 19:24 
To: Wu, Zhongmin 
Cc: Emil Velikov ; Liu, Zhiquan 
; ML mesa-dev ; Chad 
Versace ; Eric Engestrom ; 
Marathe, Yogesh ; Rob Clark 
; Kenneth Graunke ; Widawsky, 
Benjamin ; Kondapally, Kalyan 
; Kristian H . Kristensen 
; Timothy Arceri 
Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

Hi Zhongmin,

On Wed, Jul 12, 2017 at 9:55 AM, Wu, Zhongmin  wrote:
> Thanks very much for Emil and Tomasz's suggestions.
>
> We can see that the out fence needed in queue buffer is get from batch buffer 
> flushing.
>
> Now we  may flush bath buffer at below places:
>
> 1.   Create sync ===> flush buffer and get out fence from driver
> 2.Do glFLush()   >flush buffer  ( not ask for out fence from 
> driver)
> 3.swapbuffer >  flush buffer ( not ask for  out fence from 
> drvier,   queue buffer with fd -1 )
> ...
>
>
> So,   in my opinion,  we don't need to get the out fence with help of other 
> paths in swapbuffer. we just need to save the last out fence in the latest 
> batch buffer flushing, and give it to Android with queue buffer in each 
> swapbuffer.
>
> ( for example,   if glflush is just  called before swapbuffer,   the 
> following buffer flushing will returned directly as the batch buffer is empty 
> now, we can use the out fence got in glflush, and using create sync API may 
> still not get the fence. )

Do we really need to use the same fence? If glFlush() was called just before 
eglSwapBuffers(), there were no commands inserted in between and the new fence 
would just be inserted directly after the one used for glFlush(). I don't think 
it's an issue. (And the client is actually doing something strange, because 
there is no need to call
glFlush() directly before eglSwapBuffers().)

>
> But you are right, we need to change the method in the patch, the patch is 
> ill-considered.  As Emil said,  the canclebuffer is missed.
> We can't save the latest fence fd in the context object, as the context is 
> reset yet before calling canclebuffer.  Then it is not good to add the call 
> back in __DRI2flushExtensionRec,   We do need a better design for this.

How about my suggestion to use the DRI2 flush extension 
(dri2_dpy->fence->create_fence_fd)? I think the following in droid_swap_buffers 
could work:

   if (dri2_dpy->fence && dri2_dpy->fence->create_fence_fd)
  fence_fd = dri2_dpy->fence->create_fence_fd(ctx, -1);

I think you might also need to bypass flush drawable, as AFAICT it would defeat 
the purpose of out-fence by waiting synchronously for the GPU to finish.

Best regards,
Tomasz

>
> -Original Message-
> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> Sent: Wednesday, July 12, 2017 1:41
> To: Tomasz Figa 
> Cc: Wu, Zhongmin ; Marathe, Yogesh 
> ; Widawsky, Benjamin 
> ; Liu, Zhiquan ; 
> Eric Engestrom ; Rob Clark 
> ; Kenneth Graunke ; 
> Kondapally, Kalyan ; ML mesa-dev 
> ; Timothy Arceri 
> ; Kristian H . Kristensen 
> ; Chad Versace ; 
> db...@chromium.org
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] 
> i965: Queue the buffer with a sync fence for Android OS
>
> On 11 July 2017 at 16:23, Tomasz Figa  wrote:
>> Hi Zhongmin,
>>
>> On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin  wrote:
>>> By the way,
>>>
>>> For cancelBuffer, sorry I forget such function, thanks for notice. It 
>>> should also pass the same fence fd as the queuebuffer.
>>>
>>> And Yogesh, you mentioned the gallium,   is it another platform supported 
>>> by mesa ?  I am sorry I have no idea about this,  could you please help to 
>>> check this ?
>>>
>>> I think we can co-work with mesa team to work out an acceptable fix which 
>>&

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

[Tomasz Figa]

Hi Zhongmin,

On Wed, Jul 12, 2017 at 9:55 AM, Wu, Zhongmin  wrote:
> Thanks very much for Emil and Tomasz's suggestions.
>
> We can see that the out fence needed in queue buffer is get from batch buffer 
> flushing.
>
> Now we  may flush bath buffer at below places:
>
> 1.   Create sync ===> flush buffer and get out fence from driver
> 2.Do glFLush()   >flush buffer  ( not ask for out fence from 
> driver)
> 3.swapbuffer >  flush buffer ( not ask for  out fence from 
> drvier,   queue buffer with fd -1 )
> ...
>
>
> So,   in my opinion,  we don't need to get the out fence with help of other 
> paths in swapbuffer. we just need to save the last out fence in the latest 
> batch buffer flushing, and give it to Android with queue buffer in each 
> swapbuffer.
>
> ( for example,   if glflush is just  called before swapbuffer,   the 
> following buffer flushing will returned directly as the batch buffer is empty 
> now, we can use the out fence got in glflush, and using create sync API may 
> still not get the fence. )

Do we really need to use the same fence? If glFlush() was called just
before eglSwapBuffers(), there were no commands inserted in between
and the new fence would just be inserted directly after the one used
for glFlush(). I don't think it's an issue. (And the client is
actually doing something strange, because there is no need to call
glFlush() directly before eglSwapBuffers().)

>
> But you are right, we need to change the method in the patch, the patch is 
> ill-considered.  As Emil said,  the canclebuffer is missed.
> We can't save the latest fence fd in the context object, as the context is 
> reset yet before calling canclebuffer.  Then it is not good to add the call 
> back in __DRI2flushExtensionRec,   We do need a better design for this.

How about my suggestion to use the DRI2 flush extension
(dri2_dpy->fence->create_fence_fd)? I think the following in
droid_swap_buffers could work:

   if (dri2_dpy->fence && dri2_dpy->fence->create_fence_fd)
  fence_fd = dri2_dpy->fence->create_fence_fd(ctx, -1);

I think you might also need to bypass flush drawable, as AFAICT it
would defeat the purpose of out-fence by waiting synchronously for the
GPU to finish.

Best regards,
Tomasz

>
> -Original Message-
> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> Sent: Wednesday, July 12, 2017 1:41
> To: Tomasz Figa 
> Cc: Wu, Zhongmin ; Marathe, Yogesh 
> ; Widawsky, Benjamin ; 
> Liu, Zhiquan ; Eric Engestrom ; Rob 
> Clark ; Kenneth Graunke ; 
> Kondapally, Kalyan ; ML mesa-dev 
> ; Timothy Arceri ; 
> Kristian H . Kristensen ; Chad Versace 
> ; db...@chromium.org
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
> Queue the buffer with a sync fence for Android OS
>
> On 11 July 2017 at 16:23, Tomasz Figa  wrote:
>> Hi Zhongmin,
>>
>> On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin  wrote:
>>> By the way,
>>>
>>> For cancelBuffer, sorry I forget such function, thanks for notice. It 
>>> should also pass the same fence fd as the queuebuffer.
>>>
>>> And Yogesh, you mentioned the gallium,   is it another platform supported 
>>> by mesa ?  I am sorry I have no idea about this,  could you please help to 
>>> check this ?
>>>
>>> I think we can co-work with mesa team to work out an acceptable fix which 
>>> can meet the requirement of Android without any break on other platforms.
>>
>> One thing needs clarifying here. Release fences from EGL are _not_ a
>> requirement. It is an optional feature. Android compliance suites pass
>> fully without Android sync fence support in Mesa at all.
>>
>> Other than that, it's been taking long enough and I agree that we
>> should finally wire both acquire and release fence support in EGL and
>> related drivers. Otherwise we can forget about getting good user
>> experience on Android.
>>
> Right, I'm not trying to say otherwise.
>
> The strange part, IMHO, is that now flatland has a hard requirement on both 
> fences, where the [developer-side of the] documentation does not say anything 
> about this.
> This sounds a bit backwards. I believe documentation update is in order?
>
> FWIW I was under the impression that EGL_ANDROID_native_fence_sync can be 
> used in flatland. Although as Rob mentioned... not sure if the extension is 
> available since the EGL meta seems to block/strip it out.
>
>
>> On a technical side, the EGL change needs to take into account that
>> not all drivers support fences and so it needs to have a fallback to
>> old behavior for those wh

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

[Tomasz Figa]

Hi Yogesh,

On Wed, Jul 12, 2017 at 2:18 AM, Marathe, Yogesh
 wrote:
>> -Original Message-
>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
>> Of Tomasz Figa
>> Sent: Tuesday, July 11, 2017 9:59 PM
>> To: Marathe, Yogesh 
>> Cc: Gao, Shuo ; Liu, Zhiquan ;
>> Kondapally, Kalyan ; Chad Versace
>> ; Eric Engestrom ; Emil
>> Velikov ; Wu, Zhongmin
>> ; Kenneth Graunke ; Rob
>> Clark ; Widawsky, Benjamin
>> ; ML mesa-dev > d...@lists.freedesktop.org>; Kristian H . Kristensen
>> ; Timothy Arceri 
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
>> Queue the buffer with a sync fence for Android OS
>>
>> Hi Yogesh,
>>
>> On Wed, Jul 12, 2017 at 1:09 AM, Marathe, Yogesh
>>  wrote:
>> > Hello Figa, Few caveats on that approach
>>
>> (I'm Tomasz, by the way)
>
> My bad Tomasz.

No problem. :)

>
>> >> Queue the buffer with a sync fence for Android OS
>> >>
>> >> Now for real, sorry guys... (Seriously gmail why you do this to me.)
>> >>
>> >> -chuanbo.w...@intel.com (bounces)
>> >> +"Gao, Shuo"  (forgot to add originally, sorry)
>> >>
>> >> On Wed, Jul 12, 2017 at 12:23 AM, Tomasz Figa 
>> wrote:
>> >> > Hi Zhongmin,
>> >> >
>> >> > On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin
>> >> > 
>> >> wrote:
>> >> >> By the way,
>> >> >>
>> >> >> For cancelBuffer, sorry I forget such function, thanks for notice.
>> >> >> It should
>> >> also pass the same fence fd as the queuebuffer.
>> >> >>
>> >> >> And Yogesh, you mentioned the gallium,   is it another platform 
>> >> >> supported
>> by
>> >> mesa ?  I am sorry I have no idea about this,  could you please help
>> >> to check this ?
>> >> >>
>> >> >> I think we can co-work with mesa team to work out an acceptable
>> >> >> fix which
>> >> can meet the requirement of Android without any break on other platforms.
>> >> >
>> >> > One thing needs clarifying here. Release fences from EGL are _not_
>> >> > a requirement. It is an optional feature. Android compliance suites
>> >> > pass fully without Android sync fence support in Mesa at all.
>> >> >
>> >> > Other than that, it's been taking long enough and I agree that we
>> >> > should finally wire both acquire and release fence support in EGL
>> >> > and related drivers. Otherwise we can forget about getting good
>> >> > user experience on Android.
>> >> >
>> >> > On a technical side, the EGL change needs to take into account that
>> >> > not all drivers support fences and so it needs to have a fallback
>> >> > to old behavior for those which don't.
>> >> >
>> >> > Other than that, correct me if I'm wrong, but could we just use the
>> >> > DRI2 fence extension instead of adding some custom callbacks? I can
>> >> > see that a normal client request to create a sync fence would end
>> >> > up calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
>> >> > Could we do the same?
>> >
>> > May be here we need to look at complete sequence eglCreateSyncKHR ->
>> > _eglCreateSync -> dri2_create_sync, as eglCreateSyncKHR  seems entry
>> > point and its doing attrib/type checks before reaching
>> > dri2_create_sync(). Also, dri2_create_sync is static, can't be called
>> > directly, there needs to be an entry point / interface.
>> >
>>
>> I think you misunderstood my suggestion. I didn't mean dri2_create_sync(), 
>> but
>> rather using the DRI2 fence extension directly, just as dri2_create_sync() 
>> does.
>> You can access dri2_egl_display from Android EGL code and in fact it already
>> uses other extensions like this.
>
> Sorry, I'm still searching around. To try this out, can you please specify, 
> which
> functions did you mean by DRI2 fence extension? An example within EGL code
> would help. Please note we need to call these from platform_android.c finally.

I meant using dri2_dpy->fence->create_fence_fd() directly, if the
available (i.e. dri2_dpy->fence and dri2_dpy->fence->create_fence_fd
are non-NULL) or falling back to -1 if not

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

[Wu, Zhongmin]

Thanks very much for Emil and Tomasz's suggestions.

We can see that the out fence needed in queue buffer is get from batch buffer 
flushing.

Now we  may flush bath buffer at below places:

1.   Create sync ===> flush buffer and get out fence from driver
2.Do glFLush()   >flush buffer  ( not ask for out fence from driver)
3.swapbuffer >  flush buffer ( not ask for  out fence from drvier,  
 queue buffer with fd -1 )
...


So,   in my opinion,  we don't need to get the out fence with help of other 
paths in swapbuffer. we just need to save the last out fence in the latest 
batch buffer flushing, and give it to Android with queue buffer in each 
swapbuffer.

( for example,   if glflush is just  called before swapbuffer,   the following 
buffer flushing will returned directly as the batch buffer is empty now, we can 
use the out fence got in glflush, and using create sync API may still not get 
the fence. ) 

But you are right, we need to change the method in the patch, the patch is 
ill-considered.  As Emil said,  the canclebuffer is missed. 
We can't save the latest fence fd in the context object, as the context is 
reset yet before calling canclebuffer.  Then it is not good to add the call 
back in __DRI2flushExtensionRec,   We do need a better design for this.

-Original Message-
From: Emil Velikov [mailto:emil.l.veli...@gmail.com] 
Sent: Wednesday, July 12, 2017 1:41 
To: Tomasz Figa 
Cc: Wu, Zhongmin ; Marathe, Yogesh 
; Widawsky, Benjamin ; 
Liu, Zhiquan ; Eric Engestrom ; Rob 
Clark ; Kenneth Graunke ; 
Kondapally, Kalyan ; ML mesa-dev 
; Timothy Arceri ; 
Kristian H . Kristensen ; Chad Versace 
; db...@chromium.org
Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

On 11 July 2017 at 16:23, Tomasz Figa  wrote:
> Hi Zhongmin,
>
> On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin  wrote:
>> By the way,
>>
>> For cancelBuffer, sorry I forget such function, thanks for notice. It should 
>> also pass the same fence fd as the queuebuffer.
>>
>> And Yogesh, you mentioned the gallium,   is it another platform supported by 
>> mesa ?  I am sorry I have no idea about this,  could you please help to 
>> check this ?
>>
>> I think we can co-work with mesa team to work out an acceptable fix which 
>> can meet the requirement of Android without any break on other platforms.
>
> One thing needs clarifying here. Release fences from EGL are _not_ a 
> requirement. It is an optional feature. Android compliance suites pass 
> fully without Android sync fence support in Mesa at all.
>
> Other than that, it's been taking long enough and I agree that we 
> should finally wire both acquire and release fence support in EGL and 
> related drivers. Otherwise we can forget about getting good user 
> experience on Android.
>
Right, I'm not trying to say otherwise.

The strange part, IMHO, is that now flatland has a hard requirement on both 
fences, where the [developer-side of the] documentation does not say anything 
about this.
This sounds a bit backwards. I believe documentation update is in order?

FWIW I was under the impression that EGL_ANDROID_native_fence_sync can be used 
in flatland. Although as Rob mentioned... not sure if the extension is 
available since the EGL meta seems to block/strip it out.


> On a technical side, the EGL change needs to take into account that 
> not all drivers support fences and so it needs to have a fallback to 
> old behavior for those which don't.
>

> Other than that, correct me if I'm wrong, but could we just use the
> DRI2 fence extension instead of adding some custom callbacks? I can 
> see that a normal client request to create a sync fence would end up 
> calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
> Could we do the same?
>
Reusing existing API would be ideal.

If not, Zhongmin/Yogesh please note:
 - when extending the interface, the version number must be bumped
 - user should check the version and the function pointer prior to use, falling 
back to the old scheme
 - get_retrive_fd [barring the typo - retrieve], should have at least the fd 
ownership documented

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 11 July 2017 at 16:23, Tomasz Figa  wrote:
> Hi Zhongmin,
>
> On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin  wrote:
>> By the way,
>>
>> For cancelBuffer, sorry I forget such function, thanks for notice. It should 
>> also pass the same fence fd as the queuebuffer.
>>
>> And Yogesh, you mentioned the gallium,   is it another platform supported by 
>> mesa ?  I am sorry I have no idea about this,  could you please help to 
>> check this ?
>>
>> I think we can co-work with mesa team to work out an acceptable fix which 
>> can meet the requirement of Android without any break on other platforms.
>
> One thing needs clarifying here. Release fences from EGL are _not_ a
> requirement. It is an optional feature. Android compliance suites pass
> fully without Android sync fence support in Mesa at all.
>
> Other than that, it's been taking long enough and I agree that we
> should finally wire both acquire and release fence support in EGL and
> related drivers. Otherwise we can forget about getting good user
> experience on Android.
>
Right, I'm not trying to say otherwise.

The strange part, IMHO, is that now flatland has a hard requirement on
both fences, where the [developer-side of the] documentation does not
say anything about this.
This sounds a bit backwards. I believe documentation update is in order?

FWIW I was under the impression that EGL_ANDROID_native_fence_sync can
be used in flatland. Although as Rob mentioned... not sure if the
extension is available since the EGL meta seems to block/strip it out.


> On a technical side, the EGL change needs to take into account that
> not all drivers support fences and so it needs to have a fallback to
> old behavior for those which don't.
>

> Other than that, correct me if I'm wrong, but could we just use the
> DRI2 fence extension instead of adding some custom callbacks? I can
> see that a normal client request to create a sync fence would end up
> calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
> Could we do the same?
>
Reusing existing API would be ideal.

If not, Zhongmin/Yogesh please note:
 - when extending the interface, the version number must be bumped
 - user should check the version and the function pointer prior to
use, falling back to the old scheme
 - get_retrive_fd [barring the typo - retrieve], should have at least
the fd ownership documented

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marathe, Yogesh]

> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Tomasz Figa
> Sent: Tuesday, July 11, 2017 9:59 PM
> To: Marathe, Yogesh 
> Cc: Gao, Shuo ; Liu, Zhiquan ;
> Kondapally, Kalyan ; Chad Versace
> ; Eric Engestrom ; Emil
> Velikov ; Wu, Zhongmin
> ; Kenneth Graunke ; Rob
> Clark ; Widawsky, Benjamin
> ; ML mesa-dev  d...@lists.freedesktop.org>; Kristian H . Kristensen
> ; Timothy Arceri 
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
> Queue the buffer with a sync fence for Android OS
> 
> Hi Yogesh,
> 
> On Wed, Jul 12, 2017 at 1:09 AM, Marathe, Yogesh
>  wrote:
> > Hello Figa, Few caveats on that approach
> 
> (I'm Tomasz, by the way)

My bad Tomasz.

> >> Queue the buffer with a sync fence for Android OS
> >>
> >> Now for real, sorry guys... (Seriously gmail why you do this to me.)
> >>
> >> -chuanbo.w...@intel.com (bounces)
> >> +"Gao, Shuo"  (forgot to add originally, sorry)
> >>
> >> On Wed, Jul 12, 2017 at 12:23 AM, Tomasz Figa 
> wrote:
> >> > Hi Zhongmin,
> >> >
> >> > On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin
> >> > 
> >> wrote:
> >> >> By the way,
> >> >>
> >> >> For cancelBuffer, sorry I forget such function, thanks for notice.
> >> >> It should
> >> also pass the same fence fd as the queuebuffer.
> >> >>
> >> >> And Yogesh, you mentioned the gallium,   is it another platform 
> >> >> supported
> by
> >> mesa ?  I am sorry I have no idea about this,  could you please help
> >> to check this ?
> >> >>
> >> >> I think we can co-work with mesa team to work out an acceptable
> >> >> fix which
> >> can meet the requirement of Android without any break on other platforms.
> >> >
> >> > One thing needs clarifying here. Release fences from EGL are _not_
> >> > a requirement. It is an optional feature. Android compliance suites
> >> > pass fully without Android sync fence support in Mesa at all.
> >> >
> >> > Other than that, it's been taking long enough and I agree that we
> >> > should finally wire both acquire and release fence support in EGL
> >> > and related drivers. Otherwise we can forget about getting good
> >> > user experience on Android.
> >> >
> >> > On a technical side, the EGL change needs to take into account that
> >> > not all drivers support fences and so it needs to have a fallback
> >> > to old behavior for those which don't.
> >> >
> >> > Other than that, correct me if I'm wrong, but could we just use the
> >> > DRI2 fence extension instead of adding some custom callbacks? I can
> >> > see that a normal client request to create a sync fence would end
> >> > up calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
> >> > Could we do the same?
> >
> > May be here we need to look at complete sequence eglCreateSyncKHR ->
> > _eglCreateSync -> dri2_create_sync, as eglCreateSyncKHR  seems entry
> > point and its doing attrib/type checks before reaching
> > dri2_create_sync(). Also, dri2_create_sync is static, can't be called
> > directly, there needs to be an entry point / interface.
> >
> 
> I think you misunderstood my suggestion. I didn't mean dri2_create_sync(), but
> rather using the DRI2 fence extension directly, just as dri2_create_sync() 
> does.
> You can access dri2_egl_display from Android EGL code and in fact it already
> uses other extensions like this.

Sorry, I'm still searching around. To try this out, can you please specify, 
which
functions did you mean by DRI2 fence extension? An example within EGL code 
would help. Please note we need to call these from platform_android.c finally.

> 
> Best regards,
> Tomasz
> 
> >> >
> >> > [1]
> >> > https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/eg
> >> > l_d
> >> > ri2.c#n2772
> >> >
> >> > + Kristian, Chad and Dominik who have been looking into sync fence
> >> > integration with our EGL drivers.
> >> >
> >> > Best regards,
> >> > Tomasz
> >> >
> >> >>
> >> >> -Original Message-
> >> >> From: Wu, Zhongmin
> >> >> Sent: Tuesday, July 11, 2017 8:40
> >

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

[Tomasz Figa]

Hi Yogesh,

On Wed, Jul 12, 2017 at 1:09 AM, Marathe, Yogesh
 wrote:
> Hello Figa, Few caveats on that approach

(I'm Tomasz, by the way)

>
>> -Original Message-
>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
>> Of Tomasz Figa
>> Sent: Tuesday, July 11, 2017 8:58 PM
>> To: Wu, Zhongmin 
>> Cc: Gao, Shuo ; Liu, Zhiquan ;
>> ML mesa-dev ; Emil Velikov
>> ; Chad Versace ; Eric
>> Engestrom ; Marathe, Yogesh
>> ; Rob Clark ; Kenneth
>> Graunke ; Widawsky, Benjamin
>> ; Kondapally, Kalyan
>> ; Kristian H . Kristensen
>> ; Timothy Arceri 
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
>> Queue the buffer with a sync fence for Android OS
>>
>> Now for real, sorry guys... (Seriously gmail why you do this to me.)
>>
>> -chuanbo.w...@intel.com (bounces)
>> +"Gao, Shuo"  (forgot to add originally, sorry)
>>
>> On Wed, Jul 12, 2017 at 12:23 AM, Tomasz Figa  wrote:
>> > Hi Zhongmin,
>> >
>> > On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin 
>> wrote:
>> >> By the way,
>> >>
>> >> For cancelBuffer, sorry I forget such function, thanks for notice. It 
>> >> should
>> also pass the same fence fd as the queuebuffer.
>> >>
>> >> And Yogesh, you mentioned the gallium,   is it another platform supported 
>> >> by
>> mesa ?  I am sorry I have no idea about this,  could you please help to 
>> check this
>> ?
>> >>
>> >> I think we can co-work with mesa team to work out an acceptable fix which
>> can meet the requirement of Android without any break on other platforms.
>> >
>> > One thing needs clarifying here. Release fences from EGL are _not_ a
>> > requirement. It is an optional feature. Android compliance suites pass
>> > fully without Android sync fence support in Mesa at all.
>> >
>> > Other than that, it's been taking long enough and I agree that we
>> > should finally wire both acquire and release fence support in EGL and
>> > related drivers. Otherwise we can forget about getting good user
>> > experience on Android.
>> >
>> > On a technical side, the EGL change needs to take into account that
>> > not all drivers support fences and so it needs to have a fallback to
>> > old behavior for those which don't.
>> >
>> > Other than that, correct me if I'm wrong, but could we just use the
>> > DRI2 fence extension instead of adding some custom callbacks? I can
>> > see that a normal client request to create a sync fence would end up
>> > calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
>> > Could we do the same?
>
> May be here we need to look at complete sequence
> eglCreateSyncKHR ->  _eglCreateSync -> dri2_create_sync,
> as eglCreateSyncKHR  seems entry point and its doing attrib/type checks
> before reaching  dri2_create_sync(). Also, dri2_create_sync is static,
> can't be called directly, there needs to be an entry point / interface.
>

I think you misunderstood my suggestion. I didn't mean
dri2_create_sync(), but rather using the DRI2 fence extension
directly, just as dri2_create_sync() does. You can access
dri2_egl_display from Android EGL code and in fact it already uses
other extensions like this.

Best regards,
Tomasz

>> >
>> > [1]
>> > https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/egl_d
>> > ri2.c#n2772
>> >
>> > + Kristian, Chad and Dominik who have been looking into sync fence
>> > integration with our EGL drivers.
>> >
>> > Best regards,
>> > Tomasz
>> >
>> >>
>> >> -Original Message-
>> >> From: Wu, Zhongmin
>> >> Sent: Tuesday, July 11, 2017 8:40
>> >> To: 'Emil Velikov' ; Marathe, Yogesh
>> >> 
>> >> Cc: Widawsky, Benjamin ; Liu, Zhiquan
>> >> ; Eric Engestrom ; Rob
>> >> Clark ; Tomasz Figa ;
>> >> Kenneth Graunke ; Kondapally, Kalyan
>> >> ; ML mesa-dev
>> >> ; Timothy Arceri
>> >> ; Chuanbo Weng 
>> >> Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation]
>> >> i965: Queue the buffer with a sync fence for Android OS
>> >>
>> >> Hi Emil and Yogesh
>> >> Thank you for your comments,  and thanks Yogesh for giving the
>> >> detailed explanations
>> >>
>> >>
>> >> 

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

[Marathe, Yogesh]

Hello Figa, Few caveats on that approach

> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Tomasz Figa
> Sent: Tuesday, July 11, 2017 8:58 PM
> To: Wu, Zhongmin 
> Cc: Gao, Shuo ; Liu, Zhiquan ;
> ML mesa-dev ; Emil Velikov
> ; Chad Versace ; Eric
> Engestrom ; Marathe, Yogesh
> ; Rob Clark ; Kenneth
> Graunke ; Widawsky, Benjamin
> ; Kondapally, Kalyan
> ; Kristian H . Kristensen
> ; Timothy Arceri 
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
> Queue the buffer with a sync fence for Android OS
> 
> Now for real, sorry guys... (Seriously gmail why you do this to me.)
> 
> -chuanbo.w...@intel.com (bounces)
> +"Gao, Shuo"  (forgot to add originally, sorry)
> 
> On Wed, Jul 12, 2017 at 12:23 AM, Tomasz Figa  wrote:
> > Hi Zhongmin,
> >
> > On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin 
> wrote:
> >> By the way,
> >>
> >> For cancelBuffer, sorry I forget such function, thanks for notice. It 
> >> should
> also pass the same fence fd as the queuebuffer.
> >>
> >> And Yogesh, you mentioned the gallium,   is it another platform supported 
> >> by
> mesa ?  I am sorry I have no idea about this,  could you please help to check 
> this
> ?
> >>
> >> I think we can co-work with mesa team to work out an acceptable fix which
> can meet the requirement of Android without any break on other platforms.
> >
> > One thing needs clarifying here. Release fences from EGL are _not_ a
> > requirement. It is an optional feature. Android compliance suites pass
> > fully without Android sync fence support in Mesa at all.
> >
> > Other than that, it's been taking long enough and I agree that we
> > should finally wire both acquire and release fence support in EGL and
> > related drivers. Otherwise we can forget about getting good user
> > experience on Android.
> >
> > On a technical side, the EGL change needs to take into account that
> > not all drivers support fences and so it needs to have a fallback to
> > old behavior for those which don't.
> >
> > Other than that, correct me if I'm wrong, but could we just use the
> > DRI2 fence extension instead of adding some custom callbacks? I can
> > see that a normal client request to create a sync fence would end up
> > calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
> > Could we do the same?

May be here we need to look at complete sequence
eglCreateSyncKHR ->  _eglCreateSync -> dri2_create_sync, 
as eglCreateSyncKHR  seems entry point and its doing attrib/type checks
before reaching  dri2_create_sync(). Also, dri2_create_sync is static, 
can't be called directly, there needs to be an entry point / interface.

> >
> > [1]
> > https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/egl_d
> > ri2.c#n2772
> >
> > + Kristian, Chad and Dominik who have been looking into sync fence
> > integration with our EGL drivers.
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> -----Original Message-----
> >> From: Wu, Zhongmin
> >> Sent: Tuesday, July 11, 2017 8:40
> >> To: 'Emil Velikov' ; Marathe, Yogesh
> >> 
> >> Cc: Widawsky, Benjamin ; Liu, Zhiquan
> >> ; Eric Engestrom ; Rob
> >> Clark ; Tomasz Figa ;
> >> Kenneth Graunke ; Kondapally, Kalyan
> >> ; ML mesa-dev
> >> ; Timothy Arceri
> >> ; Chuanbo Weng 
> >> Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation]
> >> i965: Queue the buffer with a sync fence for Android OS
> >>
> >> Hi Emil and Yogesh
> >> Thank you for your comments,  and thanks Yogesh for giving the
> >> detailed explanations
> >>
> >>
> >> And according to the document of Android below
> (https://source.android.com/devices/graphics/arch-bq-gralloc):
> >>
> >> Recent Android devices support the sync framework, which enables the
> system to do nifty things when combined with hardware components that can
> manipulate graphics data asynchronously. For example, a producer can submit a
> series of OpenGL ES drawing commands and then enqueue the output buffer
> before rendering completes. The buffer is accompanied by a fence that signals
> when the contents are ready.
> >>
> >>
> >> I think the things is very clear, that is if the rendering is completed 
> >> already
> when we call queueBuffer() in mesa ?   If not, we should queue the buffer 
> with a
> fence wh

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

[Tomasz Figa]

-chuanbo.w...@intel.com (bounces)
+"Gao, Shuo"  (forgot to add originally, sorry)

On Wed, Jul 12, 2017 at 12:23 AM, Tomasz Figa  wrote:
> Hi Zhongmin,
>
> On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin  wrote:
>> By the way,
>>
>> For cancelBuffer, sorry I forget such function, thanks for notice. It should 
>> also pass the same fence fd as the queuebuffer.
>>
>> And Yogesh, you mentioned the gallium,   is it another platform supported by 
>> mesa ?  I am sorry I have no idea about this,  could you please help to 
>> check this ?
>>
>> I think we can co-work with mesa team to work out an acceptable fix which 
>> can meet the requirement of Android without any break on other platforms.
>
> One thing needs clarifying here. Release fences from EGL are _not_ a
> requirement. It is an optional feature. Android compliance suites pass
> fully without Android sync fence support in Mesa at all.
>
> Other than that, it's been taking long enough and I agree that we
> should finally wire both acquire and release fence support in EGL and
> related drivers. Otherwise we can forget about getting good user
> experience on Android.
>
> On a technical side, the EGL change needs to take into account that
> not all drivers support fences and so it needs to have a fallback to
> old behavior for those which don't.
>
> Other than that, correct me if I'm wrong, but could we just use the
> DRI2 fence extension instead of adding some custom callbacks? I can
> see that a normal client request to create a sync fence would end up
> calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
> Could we do the same?
>
> [1] 
> https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/egl_dri2.c#n2772
>
> + Kristian, Chad and Dominik who have been looking into sync fence
> integration with our EGL drivers.
>
> Best regards,
> Tomasz
>
>>
>> -Original Message-
>> From: Wu, Zhongmin
>> Sent: Tuesday, July 11, 2017 8:40
>> To: 'Emil Velikov' ; Marathe, Yogesh 
>> 
>> Cc: Widawsky, Benjamin ; Liu, Zhiquan 
>> ; Eric Engestrom ; Rob Clark 
>> ; Tomasz Figa ; Kenneth 
>> Graunke ; Kondapally, Kalyan 
>> ; ML mesa-dev ; 
>> Timothy Arceri ; Chuanbo Weng 
>> Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
>> Queue the buffer with a sync fence for Android OS
>>
>> Hi Emil and Yogesh
>> Thank you for your comments,  and thanks Yogesh for giving the detailed 
>> explanations
>>
>>
>> And according to the document of Android below 
>> (https://source.android.com/devices/graphics/arch-bq-gralloc):
>>
>> Recent Android devices support the sync framework, which enables the system 
>> to do nifty things when combined with hardware components that can 
>> manipulate graphics data asynchronously. For example, a producer can submit 
>> a series of OpenGL ES drawing commands and then enqueue the output buffer 
>> before rendering completes. The buffer is accompanied by a fence that 
>> signals when the contents are ready.
>>
>>
>> I think the things is very clear, that is if the rendering is completed 
>> already when we call queueBuffer() in mesa ?   If not, we should queue the 
>> buffer with a fence which will be signaled when the buffer is ready.
>>
>>
>>
>> -Original Message-----
>> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
>> Sent: Tuesday, July 11, 2017 1:18
>> To: Marathe, Yogesh 
>> Cc: Wu, Zhongmin ; Widawsky, Benjamin 
>> ; Liu, Zhiquan ; Eric 
>> Engestrom ; Rob Clark ; Tomasz 
>> Figa ; Kenneth Graunke ; 
>> Kondapally, Kalyan ; ML mesa-dev 
>> ; Timothy Arceri ; 
>> Chuanbo Weng 
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
>> Queue the buffer with a sync fence for Android OS
>>
>> On 10 July 2017 at 15:26, Marathe, Yogesh  wrote:
>>> Hello Emil, My two cents since I too spent some time on this.
>>>
>>>> -----Original Message-
>>>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
>>>> Behalf Of Emil Velikov
>>>> Sent: Monday, July 10, 2017 4:41 PM
>>>> To: Wu, Zhongmin 
>>>> Cc: Widawsky, Benjamin ; Liu, Zhiquan
>>>> ; Eric Engestrom ; Rob
>>>> Clark ; Tomasz Figa ;
>>>> Kenneth Graunke ; Kondapally, Kalyan
>>>> ; ML mesa-dev >>> d...@lists.freedesktop.org>; Timothy Arceri ;
>>>> Chuanbo Weng 
>>>> Subject: Re: [Mesa-dev] [EGL android: ac

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

[Tomasz Figa]

Now for real, sorry guys... (Seriously gmail why you do this to me.)

-chuanbo.w...@intel.com (bounces)
+"Gao, Shuo"  (forgot to add originally, sorry)

On Wed, Jul 12, 2017 at 12:23 AM, Tomasz Figa  wrote:
> Hi Zhongmin,
>
> On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin  wrote:
>> By the way,
>>
>> For cancelBuffer, sorry I forget such function, thanks for notice. It should 
>> also pass the same fence fd as the queuebuffer.
>>
>> And Yogesh, you mentioned the gallium,   is it another platform supported by 
>> mesa ?  I am sorry I have no idea about this,  could you please help to 
>> check this ?
>>
>> I think we can co-work with mesa team to work out an acceptable fix which 
>> can meet the requirement of Android without any break on other platforms.
>
> One thing needs clarifying here. Release fences from EGL are _not_ a
> requirement. It is an optional feature. Android compliance suites pass
> fully without Android sync fence support in Mesa at all.
>
> Other than that, it's been taking long enough and I agree that we
> should finally wire both acquire and release fence support in EGL and
> related drivers. Otherwise we can forget about getting good user
> experience on Android.
>
> On a technical side, the EGL change needs to take into account that
> not all drivers support fences and so it needs to have a fallback to
> old behavior for those which don't.
>
> Other than that, correct me if I'm wrong, but could we just use the
> DRI2 fence extension instead of adding some custom callbacks? I can
> see that a normal client request to create a sync fence would end up
> calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
> Could we do the same?
>
> [1] 
> https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/egl_dri2.c#n2772
>
> + Kristian, Chad and Dominik who have been looking into sync fence
> integration with our EGL drivers.
>
> Best regards,
> Tomasz
>
>>
>> -Original Message-
>> From: Wu, Zhongmin
>> Sent: Tuesday, July 11, 2017 8:40
>> To: 'Emil Velikov' ; Marathe, Yogesh 
>> 
>> Cc: Widawsky, Benjamin ; Liu, Zhiquan 
>> ; Eric Engestrom ; Rob Clark 
>> ; Tomasz Figa ; Kenneth 
>> Graunke ; Kondapally, Kalyan 
>> ; ML mesa-dev ; 
>> Timothy Arceri ; Chuanbo Weng 
>> Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
>> Queue the buffer with a sync fence for Android OS
>>
>> Hi Emil and Yogesh
>> Thank you for your comments,  and thanks Yogesh for giving the detailed 
>> explanations
>>
>>
>> And according to the document of Android below 
>> (https://source.android.com/devices/graphics/arch-bq-gralloc):
>>
>> Recent Android devices support the sync framework, which enables the system 
>> to do nifty things when combined with hardware components that can 
>> manipulate graphics data asynchronously. For example, a producer can submit 
>> a series of OpenGL ES drawing commands and then enqueue the output buffer 
>> before rendering completes. The buffer is accompanied by a fence that 
>> signals when the contents are ready.
>>
>>
>> I think the things is very clear, that is if the rendering is completed 
>> already when we call queueBuffer() in mesa ?   If not, we should queue the 
>> buffer with a fence which will be signaled when the buffer is ready.
>>
>>
>>
>> -Original Message-----
>> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
>> Sent: Tuesday, July 11, 2017 1:18
>> To: Marathe, Yogesh 
>> Cc: Wu, Zhongmin ; Widawsky, Benjamin 
>> ; Liu, Zhiquan ; Eric 
>> Engestrom ; Rob Clark ; Tomasz 
>> Figa ; Kenneth Graunke ; 
>> Kondapally, Kalyan ; ML mesa-dev 
>> ; Timothy Arceri ; 
>> Chuanbo Weng 
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
>> Queue the buffer with a sync fence for Android OS
>>
>> On 10 July 2017 at 15:26, Marathe, Yogesh  wrote:
>>> Hello Emil, My two cents since I too spent some time on this.
>>>
>>>> -----Original Message-
>>>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
>>>> Behalf Of Emil Velikov
>>>> Sent: Monday, July 10, 2017 4:41 PM
>>>> To: Wu, Zhongmin 
>>>> Cc: Widawsky, Benjamin ; Liu, Zhiquan
>>>> ; Eric Engestrom ; Rob
>>>> Clark ; Tomasz Figa ;
>>>> Kenneth Graunke ; Kondapally, Kalyan
>>>> ; ML mesa-dev >>> d...@lists.freedesktop.org>; Timothy Arceri ;
>>>> Chuanbo

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

[Tomasz Figa]

Hi Zhongmin,

On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin  wrote:
> By the way,
>
> For cancelBuffer, sorry I forget such function, thanks for notice. It should 
> also pass the same fence fd as the queuebuffer.
>
> And Yogesh, you mentioned the gallium,   is it another platform supported by 
> mesa ?  I am sorry I have no idea about this,  could you please help to check 
> this ?
>
> I think we can co-work with mesa team to work out an acceptable fix which can 
> meet the requirement of Android without any break on other platforms.

One thing needs clarifying here. Release fences from EGL are _not_ a
requirement. It is an optional feature. Android compliance suites pass
fully without Android sync fence support in Mesa at all.

Other than that, it's been taking long enough and I agree that we
should finally wire both acquire and release fence support in EGL and
related drivers. Otherwise we can forget about getting good user
experience on Android.

On a technical side, the EGL change needs to take into account that
not all drivers support fences and so it needs to have a fallback to
old behavior for those which don't.

Other than that, correct me if I'm wrong, but could we just use the
DRI2 fence extension instead of adding some custom callbacks? I can
see that a normal client request to create a sync fence would end up
calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
Could we do the same?

[1] 
https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/egl_dri2.c#n2772

+ Kristian, Chad and Dominik who have been looking into sync fence
integration with our EGL drivers.

Best regards,
Tomasz

>
> -Original Message-
> From: Wu, Zhongmin
> Sent: Tuesday, July 11, 2017 8:40
> To: 'Emil Velikov' ; Marathe, Yogesh 
> 
> Cc: Widawsky, Benjamin ; Liu, Zhiquan 
> ; Eric Engestrom ; Rob Clark 
> ; Tomasz Figa ; Kenneth Graunke 
> ; Kondapally, Kalyan ; ML 
> mesa-dev ; Timothy Arceri 
> ; Chuanbo Weng 
> Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
> Queue the buffer with a sync fence for Android OS
>
> Hi Emil and Yogesh
> Thank you for your comments,  and thanks Yogesh for giving the detailed 
> explanations
>
>
> And according to the document of Android below 
> (https://source.android.com/devices/graphics/arch-bq-gralloc):
>
> Recent Android devices support the sync framework, which enables the system 
> to do nifty things when combined with hardware components that can manipulate 
> graphics data asynchronously. For example, a producer can submit a series of 
> OpenGL ES drawing commands and then enqueue the output buffer before 
> rendering completes. The buffer is accompanied by a fence that signals when 
> the contents are ready.
>
>
> I think the things is very clear, that is if the rendering is completed 
> already when we call queueBuffer() in mesa ?   If not, we should queue the 
> buffer with a fence which will be signaled when the buffer is ready.
>
>
>
> -Original Message-
> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> Sent: Tuesday, July 11, 2017 1:18
> To: Marathe, Yogesh 
> Cc: Wu, Zhongmin ; Widawsky, Benjamin 
> ; Liu, Zhiquan ; Eric 
> Engestrom ; Rob Clark ; Tomasz 
> Figa ; Kenneth Graunke ; 
> Kondapally, Kalyan ; ML mesa-dev 
> ; Timothy Arceri ; 
> Chuanbo Weng 
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
> Queue the buffer with a sync fence for Android OS
>
> On 10 July 2017 at 15:26, Marathe, Yogesh  wrote:
>> Hello Emil, My two cents since I too spent some time on this.
>>
>>> -Original Message-
>>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
>>> Behalf Of Emil Velikov
>>> Sent: Monday, July 10, 2017 4:41 PM
>>> To: Wu, Zhongmin 
>>> Cc: Widawsky, Benjamin ; Liu, Zhiquan
>>> ; Eric Engestrom ; Rob
>>> Clark ; Tomasz Figa ;
>>> Kenneth Graunke ; Kondapally, Kalyan
>>> ; ML mesa-dev >> d...@lists.freedesktop.org>; Timothy Arceri ;
>>> Chuanbo Weng 
>>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
>>> Queue the buffer with a sync fence for Android OS
>>>
>>> Hi Zhongmin Wu,
>>>
>>> Above all, a bit of a disclaimer: I'm by no means an expert on the
>>> topic so take the following with a pinch of salt.
>>>
>>> On 10 July 2017 at 03:11, 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 buffe

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

[Wu, Zhongmin]

Add Gao, Shuo
-Original Message-
From: Wu, Zhongmin 
Sent: Tuesday, July 11, 2017 10:03 
To: 'Emil Velikov' ; Marathe, Yogesh 

Cc: Widawsky, Benjamin ; Liu, Zhiquan 
; 'Eric Engestrom' ; 'Rob Clark' 
; 'Tomasz Figa' ; 'Kenneth 
Graunke' ; Kondapally, Kalyan 
; 'ML mesa-dev' ; 
'Timothy Arceri' ; 'Chuanbo Weng' 

Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

By the way, 

For cancelBuffer, sorry I forget such function, thanks for notice. It should 
also pass the same fence fd as the queuebuffer.

And Yogesh, you mentioned the gallium,   is it another platform supported by 
mesa ?  I am sorry I have no idea about this,  could you please help to check 
this ?

I think we can co-work with mesa team to work out an acceptable fix which can 
meet the requirement of Android without any break on other platforms.

-Original Message-
From: Wu, Zhongmin
Sent: Tuesday, July 11, 2017 8:40
To: 'Emil Velikov' ; Marathe, Yogesh 

Cc: Widawsky, Benjamin ; Liu, Zhiquan 
; Eric Engestrom ; Rob Clark 
; Tomasz Figa ; Kenneth Graunke 
; Kondapally, Kalyan ; ML 
mesa-dev ; Timothy Arceri 
; Chuanbo Weng 
Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

Hi Emil and Yogesh
Thank you for your comments,  and thanks Yogesh for giving the detailed 
explanations 


And according to the document of Android below 
(https://source.android.com/devices/graphics/arch-bq-gralloc):

Recent Android devices support the sync framework, which enables the system to 
do nifty things when combined with hardware components that can manipulate 
graphics data asynchronously. For example, a producer can submit a series of 
OpenGL ES drawing commands and then enqueue the output buffer before rendering 
completes. The buffer is accompanied by a fence that signals when the contents 
are ready.


I think the things is very clear, that is if the rendering is completed already 
when we call queueBuffer() in mesa ?   If not, we should queue the buffer with 
a fence which will be signaled when the buffer is ready.



-Original Message-
From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
Sent: Tuesday, July 11, 2017 1:18
To: Marathe, Yogesh 
Cc: Wu, Zhongmin ; Widawsky, Benjamin 
; Liu, Zhiquan ; Eric 
Engestrom ; Rob Clark ; Tomasz 
Figa ; Kenneth Graunke ; Kondapally, 
Kalyan ; ML mesa-dev 
; Timothy Arceri ; 
Chuanbo Weng 
Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

On 10 July 2017 at 15:26, Marathe, Yogesh  wrote:
> Hello Emil, My two cents since I too spent some time on this.
>
>> -Original Message-
>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On 
>> Behalf Of Emil Velikov
>> Sent: Monday, July 10, 2017 4:41 PM
>> To: Wu, Zhongmin 
>> Cc: Widawsky, Benjamin ; Liu, Zhiquan 
>> ; Eric Engestrom ; Rob 
>> Clark ; Tomasz Figa ; 
>> Kenneth Graunke ; Kondapally, Kalyan 
>> ; ML mesa-dev > d...@lists.freedesktop.org>; Timothy Arceri ; 
>> Chuanbo Weng 
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
>> Queue the buffer with a sync fence for Android OS
>>
>> Hi Zhongmin Wu,
>>
>> Above all, a bit of a disclaimer: I'm by no means an expert on the 
>> topic so take the following with a pinch of salt.
>>
>> On 10 July 2017 at 03:11, 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.
>> >
>> Having a closer look it seems that the issue can be summarised as follows:
>>  - flatland intercepts/interacts ANativeWindow::{de,}queueBuffer (how 
>> about
>> ANativeWindow::cancelBuffer?)
>>  - the program expects that a sync fd is available for both dequeue 
>> and queue
>>
>> At the same time:
>>  - the ANativeWindow documentation does _not_ state such requirement
>>  - even if it did, that will be somewhat wrong, since 
>> ANativeWindow::queueBuffer is called by eglSwapBuffers() Where the 
>> latter documentation clearly states - "... performs an implicit flush ... 
>> glFlush ...
>> vgFlush"
>>
>> My take is that if flatland/Android framework does want an explicit 
>> sync point it should insert one with the EGL API.
>> There could be alternative solutions, but the proposed patch seems 
>> wrong IMHO.
>
> In fact, I could work

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

[Wu, Zhongmin]

By the way, 

For cancelBuffer, sorry I forget such function, thanks for notice. It should 
also pass the same fence fd as the queuebuffer.

And Yogesh, you mentioned the gallium,   is it another platform supported by 
mesa ?  I am sorry I have no idea about this,  could you please help to check 
this ?

I think we can co-work with mesa team to work out an acceptable fix which can 
meet the requirement of Android without any break on other platforms.

-Original Message-
From: Wu, Zhongmin 
Sent: Tuesday, July 11, 2017 8:40 
To: 'Emil Velikov' ; Marathe, Yogesh 

Cc: Widawsky, Benjamin ; Liu, Zhiquan 
; Eric Engestrom ; Rob Clark 
; Tomasz Figa ; Kenneth Graunke 
; Kondapally, Kalyan ; ML 
mesa-dev ; Timothy Arceri 
; Chuanbo Weng 
Subject: RE: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

Hi Emil and Yogesh
Thank you for your comments,  and thanks Yogesh for giving the detailed 
explanations 


And according to the document of Android below 
(https://source.android.com/devices/graphics/arch-bq-gralloc):

Recent Android devices support the sync framework, which enables the system to 
do nifty things when combined with hardware components that can manipulate 
graphics data asynchronously. For example, a producer can submit a series of 
OpenGL ES drawing commands and then enqueue the output buffer before rendering 
completes. The buffer is accompanied by a fence that signals when the contents 
are ready.


I think the things is very clear, that is if the rendering is completed already 
when we call queueBuffer() in mesa ?   If not, we should queue the buffer with 
a fence which will be signaled when the buffer is ready.



-Original Message-
From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
Sent: Tuesday, July 11, 2017 1:18
To: Marathe, Yogesh 
Cc: Wu, Zhongmin ; Widawsky, Benjamin 
; Liu, Zhiquan ; Eric 
Engestrom ; Rob Clark ; Tomasz 
Figa ; Kenneth Graunke ; Kondapally, 
Kalyan ; ML mesa-dev 
; Timothy Arceri ; 
Chuanbo Weng 
Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

On 10 July 2017 at 15:26, Marathe, Yogesh  wrote:
> Hello Emil, My two cents since I too spent some time on this.
>
>> -Original Message-
>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On 
>> Behalf Of Emil Velikov
>> Sent: Monday, July 10, 2017 4:41 PM
>> To: Wu, Zhongmin 
>> Cc: Widawsky, Benjamin ; Liu, Zhiquan 
>> ; Eric Engestrom ; Rob 
>> Clark ; Tomasz Figa ; 
>> Kenneth Graunke ; Kondapally, Kalyan 
>> ; ML mesa-dev > d...@lists.freedesktop.org>; Timothy Arceri ; 
>> Chuanbo Weng 
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
>> Queue the buffer with a sync fence for Android OS
>>
>> Hi Zhongmin Wu,
>>
>> Above all, a bit of a disclaimer: I'm by no means an expert on the 
>> topic so take the following with a pinch of salt.
>>
>> On 10 July 2017 at 03:11, 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.
>> >
>> Having a closer look it seems that the issue can be summarised as follows:
>>  - flatland intercepts/interacts ANativeWindow::{de,}queueBuffer (how 
>> about
>> ANativeWindow::cancelBuffer?)
>>  - the program expects that a sync fd is available for both dequeue 
>> and queue
>>
>> At the same time:
>>  - the ANativeWindow documentation does _not_ state such requirement
>>  - even if it did, that will be somewhat wrong, since 
>> ANativeWindow::queueBuffer is called by eglSwapBuffers() Where the 
>> latter documentation clearly states - "... performs an implicit flush ... 
>> glFlush ...
>> vgFlush"
>>
>> My take is that if flatland/Android framework does want an explicit 
>> sync point it should insert one with the EGL API.
>> There could be alternative solutions, but the proposed patch seems 
>> wrong IMHO.
>
> In fact, I could work this around in producer  (Surface::queueBuffer) 
> by ignoring the (-1) passed and by creating a sync using egl APIs. I see two 
> problems with that.
>
> - Before getting a fd using eglDupNativeFenceFDANDROID(), you need a 
> glFlush(),
>this costs additional cycles for each queueBuffer transaction on each 
> BufferItem and
>I believe fd is also signaled due to this. (so I don’t know what we'll get 
> by waiting on
>that fd on consumer side).
> - AFAIK, the whole idea o

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

[Wu, Zhongmin]

Hi Emil and Yogesh
Thank you for your comments,  and thanks Yogesh for giving the detailed 
explanations 


And according to the document of Android below 
(https://source.android.com/devices/graphics/arch-bq-gralloc):

Recent Android devices support the sync framework, which enables the system to 
do nifty things when combined with hardware components that can manipulate 
graphics data asynchronously. For example, a producer can submit a series of 
OpenGL ES drawing commands and then enqueue the output buffer before rendering 
completes. The buffer is accompanied by a fence that signals when the contents 
are ready.


I think the things is very clear, that is if the rendering is completed already 
when we call queueBuffer() in mesa ?   If not, we should queue the buffer with 
a fence which will be signaled when the buffer is ready.



-Original Message-
From: Emil Velikov [mailto:emil.l.veli...@gmail.com] 
Sent: Tuesday, July 11, 2017 1:18 
To: Marathe, Yogesh 
Cc: Wu, Zhongmin ; Widawsky, Benjamin 
; Liu, Zhiquan ; Eric 
Engestrom ; Rob Clark ; Tomasz 
Figa ; Kenneth Graunke ; Kondapally, 
Kalyan ; ML mesa-dev 
; Timothy Arceri ; 
Chuanbo Weng 
Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965: 
Queue the buffer with a sync fence for Android OS

On 10 July 2017 at 15:26, Marathe, Yogesh  wrote:
> Hello Emil, My two cents since I too spent some time on this.
>
>> -Original Message-
>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On 
>> Behalf Of Emil Velikov
>> Sent: Monday, July 10, 2017 4:41 PM
>> To: Wu, Zhongmin 
>> Cc: Widawsky, Benjamin ; Liu, Zhiquan 
>> ; Eric Engestrom ; Rob 
>> Clark ; Tomasz Figa ; 
>> Kenneth Graunke ; Kondapally, Kalyan 
>> ; ML mesa-dev > d...@lists.freedesktop.org>; Timothy Arceri ; 
>> Chuanbo Weng 
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
>> Queue the buffer with a sync fence for Android OS
>>
>> Hi Zhongmin Wu,
>>
>> Above all, a bit of a disclaimer: I'm by no means an expert on the 
>> topic so take the following with a pinch of salt.
>>
>> On 10 July 2017 at 03:11, 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.
>> >
>> Having a closer look it seems that the issue can be summarised as follows:
>>  - flatland intercepts/interacts ANativeWindow::{de,}queueBuffer (how 
>> about
>> ANativeWindow::cancelBuffer?)
>>  - the program expects that a sync fd is available for both dequeue 
>> and queue
>>
>> At the same time:
>>  - the ANativeWindow documentation does _not_ state such requirement
>>  - even if it did, that will be somewhat wrong, since 
>> ANativeWindow::queueBuffer is called by eglSwapBuffers() Where the 
>> latter documentation clearly states - "... performs an implicit flush ... 
>> glFlush ...
>> vgFlush"
>>
>> My take is that if flatland/Android framework does want an explicit 
>> sync point it should insert one with the EGL API.
>> There could be alternative solutions, but the proposed patch seems 
>> wrong IMHO.
>
> In fact, I could work this around in producer  (Surface::queueBuffer) 
> by ignoring the (-1) passed and by creating a sync using egl APIs. I see two 
> problems with that.
>
> - Before getting a fd using eglDupNativeFenceFDANDROID(), you need a 
> glFlush(),
>this costs additional cycles for each queueBuffer transaction on each 
> BufferItem and
>I believe fd is also signaled due to this. (so I don’t know what we'll get 
> by waiting on
>that fd on consumer side).
> - AFAIK, the whole idea of explicit sync revolves around being able to pass 
> fds created
>   by driver between processes and this one breaks that chain. If we work this 
> around in
>   upper layers, explicit sync feature will have to be fixed for every other 
> lib that may use
>   lib mesa underneath.
>
> For these reasons, I still believe we should fix it here. Of course, 
> you and Rob have very valid points on cancelBuffer and about not 
> breaking gallium respectively, those need to be taken care of.
>
What I'm saying is - seems like the app/framework does something silly or at 
least undocumented.
Fixing things in Mesa may be the right thing to do, but without more 
information, its everyone's guess who's got it wrong.

As Rob asked earlier - can we get an a simple test case or some pseudo code 
illustrating the whole thing?

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 10 July 2017 at 15:26, Marathe, Yogesh  wrote:
> Hello Emil, My two cents since I too spent some time on this.
>
>> -Original Message-
>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
>> Of Emil Velikov
>> Sent: Monday, July 10, 2017 4:41 PM
>> To: Wu, Zhongmin 
>> Cc: Widawsky, Benjamin ; Liu, Zhiquan
>> ; Eric Engestrom ; Rob Clark
>> ; Tomasz Figa ; Kenneth
>> Graunke ; Kondapally, Kalyan
>> ; ML mesa-dev > d...@lists.freedesktop.org>; Timothy Arceri ; Chuanbo
>> Weng 
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
>> Queue the buffer with a sync fence for Android OS
>>
>> Hi Zhongmin Wu,
>>
>> Above all, a bit of a disclaimer: I'm by no means an expert on the topic so 
>> take
>> the following with a pinch of salt.
>>
>> On 10 July 2017 at 03:11, 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.
>> >
>> Having a closer look it seems that the issue can be summarised as follows:
>>  - flatland intercepts/interacts ANativeWindow::{de,}queueBuffer (how about
>> ANativeWindow::cancelBuffer?)
>>  - the program expects that a sync fd is available for both dequeue and queue
>>
>> At the same time:
>>  - the ANativeWindow documentation does _not_ state such requirement
>>  - even if it did, that will be somewhat wrong, since
>> ANativeWindow::queueBuffer is called by eglSwapBuffers() Where the latter
>> documentation clearly states - "... performs an implicit flush ... glFlush 
>> ...
>> vgFlush"
>>
>> My take is that if flatland/Android framework does want an explicit sync 
>> point it
>> should insert one with the EGL API.
>> There could be alternative solutions, but the proposed patch seems wrong
>> IMHO.
>
> In fact, I could work this around in producer  (Surface::queueBuffer) by 
> ignoring the (-1)
> passed and by creating a sync using egl APIs. I see two problems with that.
>
> - Before getting a fd using eglDupNativeFenceFDANDROID(), you need a 
> glFlush(),
>this costs additional cycles for each queueBuffer transaction on each 
> BufferItem and
>I believe fd is also signaled due to this. (so I don’t know what we'll get 
> by waiting on
>that fd on consumer side).
> - AFAIK, the whole idea of explicit sync revolves around being able to pass 
> fds created
>   by driver between processes and this one breaks that chain. If we work this 
> around in
>   upper layers, explicit sync feature will have to be fixed for every other 
> lib that may use
>   lib mesa underneath.
>
> For these reasons, I still believe we should fix it here. Of course, you and 
> Rob have very
> valid points on cancelBuffer and about not breaking gallium respectively, 
> those need to
> be taken care of.
>
What I'm saying is - seems like the app/framework does something silly
or at least undocumented.
Fixing things in Mesa may be the right thing to do, but without more
information, its everyone's guess who's got it wrong.

As Rob asked earlier - can we get an a simple test case or some pseudo
code illustrating the whole thing?

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marathe, Yogesh]

Hello Emil, My two cents since I too spent some time on this.

> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Emil Velikov
> Sent: Monday, July 10, 2017 4:41 PM
> To: Wu, Zhongmin 
> Cc: Widawsky, Benjamin ; Liu, Zhiquan
> ; Eric Engestrom ; Rob Clark
> ; Tomasz Figa ; Kenneth
> Graunke ; Kondapally, Kalyan
> ; ML mesa-dev  d...@lists.freedesktop.org>; Timothy Arceri ; Chuanbo
> Weng 
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
> Queue the buffer with a sync fence for Android OS
> 
> Hi Zhongmin Wu,
> 
> Above all, a bit of a disclaimer: I'm by no means an expert on the topic so 
> take
> the following with a pinch of salt.
> 
> On 10 July 2017 at 03:11, 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.
> >
> Having a closer look it seems that the issue can be summarised as follows:
>  - flatland intercepts/interacts ANativeWindow::{de,}queueBuffer (how about
> ANativeWindow::cancelBuffer?)
>  - the program expects that a sync fd is available for both dequeue and queue
> 
> At the same time:
>  - the ANativeWindow documentation does _not_ state such requirement
>  - even if it did, that will be somewhat wrong, since
> ANativeWindow::queueBuffer is called by eglSwapBuffers() Where the latter
> documentation clearly states - "... performs an implicit flush ... glFlush ...
> vgFlush"
> 
> My take is that if flatland/Android framework does want an explicit sync 
> point it
> should insert one with the EGL API.
> There could be alternative solutions, but the proposed patch seems wrong
> IMHO.

In fact, I could work this around in producer  (Surface::queueBuffer) by 
ignoring the (-1)
passed and by creating a sync using egl APIs. I see two problems with that.

- Before getting a fd using eglDupNativeFenceFDANDROID(), you need a glFlush(),
   this costs additional cycles for each queueBuffer transaction on each 
BufferItem and 
   I believe fd is also signaled due to this. (so I don’t know what we'll get 
by waiting on 
   that fd on consumer side).
- AFAIK, the whole idea of explicit sync revolves around being able to pass fds 
created 
  by driver between processes and this one breaks that chain. If we work this 
around in 
  upper layers, explicit sync feature will have to be fixed for every other lib 
that may use
  lib mesa underneath.

For these reasons, I still believe we should fix it here. Of course, you and 
Rob have very
valid points on cancelBuffer and about not breaking gallium respectively, those 
need to
be taken care of.

> 
> Regards,
> Emil
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Rob Clark]

On Sun, Jul 9, 2017 at 10:11 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.
>
> 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 
> Reported-by: Li, Guangli 
> Tested-by: Marathe, Yogesh 
> ---
>  include/GL/internal/dri_interface.h   |2 ++
>  src/egl/drivers/dri2/platform_android.c   |   10 +++---
>  src/mesa/drivers/dri/i965/brw_context.c   |7 ++-
>  src/mesa/drivers/dri/i965/brw_context.h   |1 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c |   15 ++-
>  src/mesa/drivers/dri/i965/intel_screen.c  |7 +++
>  6 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/include/GL/internal/dri_interface.h 
> b/include/GL/internal/dri_interface.h
> index fc2d4bb..8760aec 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -316,6 +316,8 @@ struct __DRI2flushExtensionRec {
>   __DRIdrawable *drawable,
>   unsigned flags,
>   enum __DRI2throttleReason throttle_reason);
> +
> +int (*get_retrive_fd)(__DRIcontext *ctx);
>  };
>
>
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index bfa20f8..844bb8d 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -289,10 +289,14 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct 
> dri2_egl_surface *dri2_sur
>  *is passed to queueBuffer, and the ANativeWindow implementation
>  *is responsible for closing it.
>  */
> -   int fence_fd = -1;
> -   dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
> -  fence_fd);
>
> +   _EGLContext *ctx = _eglGetCurrentContext();
> +   struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
> +
> +   int fd = -1;
> +   fd = dri2_dpy->flush->get_retrive_fd(dri2_ctx->dri_context);

so from a quick look at this patch, I suspect this will cause gallium
drivers to start crashing without implementing this new function in
mesa/st.  (Or is someone already working on that?)

Possibly an if(dri2_dpy->flush->get_retrive_fd) is sufficient

BR,
-R


> +   dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
> +  fd);
> dri2_surf->buffer->common.decRef(&dri2_surf->buffer->common);
> dri2_surf->buffer = NULL;
> dri2_surf->back = NULL;
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
> b/src/mesa/drivers/dri/i965/brw_context.c
> index 5433f90..f74ae91 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -940,6 +940,7 @@ brwCreateContext(gl_api api,
> brw->screen = screen;
> brw->bufmgr = screen->bufmgr;
>
> +   brw->retrive_fd = -1;
> brw->gen = devinfo->gen;
> brw->gt = devinfo->gt;
> brw->is_g4x = devinfo->is_g4x;
> @@ -1176,8 +1177,12 @@ intelDestroyContext(__DRIcontext * driContextPriv)
>
> ralloc_free(brw);
> driContextPriv->driverPrivate = NULL;
> -}
>
> +   if(brw->retrive_fd != -1) {
> +   close(brw->retrive_fd);
> +   brw->retrive_fd = -1;
> +   }
> +}
>  GLboolean
>  intelUnbindContext(__DRIcontext * driContextPriv)
>  {
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
> b/src/mesa/drivers/dri/i965/brw_context.h
> index dc4bc8f..8f277c3 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1217,6 +1217,7 @@ struct brw_context
>
> __DRIcontext *driContext;
> struct intel_screen *screen;
> +   int retrive_fd;
>  };
>
>  /* brw_clear.c */
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 62d2fe8..31515b2 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -648,9 +648,22 @@ 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->retrive_fd != -1) {
> + close(brw->retrive_fd);
> + brw->retrive_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;
> + 

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

[Emil Velikov]

Hi Zhongmin Wu,

Above all, a bit of a disclaimer: I'm by no means an expert on the
topic so take the following with a pinch of salt.

On 10 July 2017 at 03:11, 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.
>
Having a closer look it seems that the issue can be summarised as follows:
 - flatland intercepts/interacts ANativeWindow::{de,}queueBuffer (how
about ANativeWindow::cancelBuffer?)
 - the program expects that a sync fd is available for both dequeue and queue

At the same time:
 - the ANativeWindow documentation does _not_ state such requirement
 - even if it did, that will be somewhat wrong, since
ANativeWindow::queueBuffer is called by eglSwapBuffers()
Where the latter documentation clearly states - "... performs an
implicit flush ... glFlush ... vgFlush"

My take is that if flatland/Android framework does want an explicit
sync point it should insert one with the EGL API.
There could be alternative solutions, but the proposed patch seems wrong IMHO.

Regards,
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Chris Wilson]

Quoting Zhongmin Wu (2017-07-07 09:07:06)
> Before we queued the buffer with a invalid fence (-1), it will
> make some benchmarks failed to test such as flatland.

Create a fence, pass fence-fd to android? Instead of forcing a lot of
busy work and using up another precious resource for everyone else.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev