[Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS (v4)
Tomasz Figa
tfiga at chromium.org
Thu Jul 20 08:04:09 UTC 2017
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 <zhongmin.wu at intel.com> wrote:
> Before we queued the buffer with a invalid fence (-1), it will
> make some benchmarks failed to test such as flatland.
>
> Now we get the out fence during the flushing buffer and then pass
> it to SurfaceFlinger in eglSwapbuffer function.
>
> v2: a) Also implement the fence in cancelBuffer.
> b) The last sync fence is stored in drawable object
> rather than brw context.
> c) format clear.
>
> v3: a) Save the last fence fd in DRI Context object.
> b) Return the last fence if the batch buffer is empty and
> nothing to be flushed when _intel_batchbuffer_flush_fence
> c) Add the new interface in vbtl to set the retrieve fence
> in the egl surface. This is just for cancelBuffer.
> d) For queueBuffer, the fence is get with DRI fence interface.
> For cancelBuffer, the fence is get before the context is
> reset, and the fence is then moved to its surface.
> v3.1 a) close fd in the new vbtl interface on none Android platform
>
> 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 <zhongmin.wu at intel.com>
> Reported-by: Li, Guangli <guangli.li at intel.com>
> Tested-by: Marathe, Yogesh <yogesh.marathe at intel.com>
> ---
> src/egl/drivers/dri2/egl_dri2.c | 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) {
> __DRIcontext *old_cctx = dri2_egl_context(old_ctx)->dri_context;
> + if (old_dsurf) {
> + dri2_surf_get_fence_fd(old_ctx, disp, old_dsurf);
> + }
nit: No need for braces for a single line if body.
> dri2_dpy->core->unbindContext(old_cctx);
> }
>
> @@ -1490,6 +1531,9 @@ static EGLBoolean
> dri2_swap_buffers(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf)
> {
> struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> + _EGLContext *ctx = _eglGetCurrentContext();
> + if (ctx && surf)
> + dri2_surf_get_fence_fd(ctx, dpy, surf);
> return dri2_dpy->vtbl->swap_buffers(drv, dpy, surf);
> }
>
> @@ -1499,6 +1543,9 @@ dri2_swap_buffers_with_damage(_EGLDriver *drv, _EGLDisplay *dpy,
> const EGLint *rects, EGLint n_rects)
> {
> struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> + _EGLContext *ctx = _eglGetCurrentContext();
> + if (ctx && surf)
> + dri2_surf_get_fence_fd(ctx, dpy, surf);
> return dri2_dpy->vtbl->swap_buffers_with_damage(drv, dpy, surf,
> rects, n_rects);
> }
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index bbba7c0..5f40f72 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -320,6 +320,7 @@ struct dri2_egl_surface
> __DRIimage *front;
> unsigned int visual;
> #endif
> + int retrieve_fd;
Please rename this to "out_fence_fd".
> };
>
> struct dri2_egl_config
> @@ -446,4 +447,8 @@ dri2_set_WL_bind_wayland_display(_EGLDriver *drv, _EGLDisplay *disp)
> void
> dri2_display_destroy(_EGLDisplay *disp);
>
> +EGLBoolean
> +dri2_surf_init(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
> + _EGLConfig *conf, const EGLint *attrib_list);
> +
> #endif /* EGL_DRI2_INCLUDED */
> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
> index cc2e4a6..84e5ae2 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -305,7 +305,8 @@ 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 fence_fd = dri2_surf->retrieve_fd;
> + dri2_surf->retrieve_fd = -1;
> dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
> fence_fd);
>
> @@ -327,8 +328,10 @@ static void
> droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf)
> {
> int ret;
> -
> - ret = dri2_surf->window->cancelBuffer(dri2_surf->window, dri2_surf->buffer, -1);
> + int fence_fd = dri2_surf->retrieve_fd;
> + dri2_surf->retrieve_fd = -1;
> + ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
> + dri2_surf->buffer, fence_fd);
> if (ret < 0) {
> _eglLog(_EGL_WARNING, "ANativeWindow::cancelBuffer failed");
> dri2_surf->base.Lost = EGL_TRUE;
> @@ -388,7 +391,7 @@ droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
> return NULL;
> }
>
> - if (!_eglInitSurface(&dri2_surf->base, disp, type, conf, attrib_list))
> + if (!dri2_surf_init(&dri2_surf->base, disp, type, conf, attrib_list))
> goto cleanup_surface;
>
> if (type == EGL_WINDOW_BIT) {
> diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
> index 8b0562c..52928a8 100644
> --- a/src/egl/drivers/dri2/platform_drm.c
> +++ b/src/egl/drivers/dri2/platform_drm.c
> @@ -111,7 +111,7 @@ dri2_drm_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
> return NULL;
> }
>
> - if (!_eglInitSurface(&dri2_surf->base, disp, type, conf, attrib_list))
> + if (!dri2_surf_init(&dri2_surf->base, disp, type, conf, attrib_list))
> goto cleanup_surf;
>
> switch (type) {
> diff --git a/src/egl/drivers/dri2/platform_surfaceless.c b/src/egl/drivers/dri2/platform_surfaceless.c
> index 0eb3fb7..be6ead0 100644
> --- a/src/egl/drivers/dri2/platform_surfaceless.c
> +++ b/src/egl/drivers/dri2/platform_surfaceless.c
> @@ -124,7 +124,7 @@ dri2_surfaceless_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
> return NULL;
> }
>
> - if (!_eglInitSurface(&dri2_surf->base, disp, type, conf, attrib_list))
> + if (!dri2_surf_init(&dri2_surf->base, disp, type, conf, attrib_list))
> goto cleanup_surface;
>
> config = dri2_get_dri_config(dri2_conf, type,
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index f746f0b..689bbd0 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -121,7 +121,7 @@ dri2_wl_create_window_surface(_EGLDriver *drv, _EGLDisplay *disp,
> return NULL;
> }
>
> - if (!_eglInitSurface(&dri2_surf->base, disp, EGL_WINDOW_BIT, conf, attrib_list))
> + if (!dri2_surf_init(&dri2_surf->base, disp, EGL_WINDOW_BIT, conf, attrib_list))
> goto cleanup_surf;
>
> if (dri2_dpy->wl_drm) {
> diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
> index 74d3a16..879c40d 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -222,7 +222,7 @@ dri2_x11_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
> return NULL;
> }
>
> - if (!_eglInitSurface(&dri2_surf->base, disp, type, conf, attrib_list))
> + if (!dri2_surf_init(&dri2_surf->base, disp, type, conf, attrib_list))
> goto cleanup_surf;
>
> dri2_surf->region = XCB_NONE;
> diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c b/src/egl/drivers/dri2/platform_x11_dri3.c
> index 3148f49..d92fb39 100644
> --- a/src/egl/drivers/dri2/platform_x11_dri3.c
> +++ b/src/egl/drivers/dri2/platform_x11_dri3.c
> @@ -183,7 +183,7 @@ dri3_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
> return NULL;
> }
>
> - if (!_eglInitSurface(&dri3_surf->base, disp, type, conf, attrib_list))
> + if (!dri2_surf_init(&dri3_surf->base, disp, type, conf, attrib_list))
> goto cleanup_surf;
>
> if (type == EGL_PBUFFER_BIT) {
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> index 5433f90..caf93c4 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
I asked you to split i965 changes into a separate patch in my previous comments.
> @@ -1086,6 +1086,8 @@ brwCreateContext(gl_api api,
> ctx->VertexProgram._MaintainTnlProgram = true;
> ctx->FragmentProgram._MaintainTexEnvProgram = true;
>
> + brw->retrieve_fd = -1;
> +
> brw_draw_init( brw );
>
> if ((flags & __DRI_CTX_FLAG_DEBUG) != 0) {
> @@ -1169,6 +1171,9 @@ intelDestroyContext(__DRIcontext * driContextPriv)
> brw->throttle_batch[1] = NULL;
> brw->throttle_batch[0] = NULL;
>
> + if (brw->retrieve_fd >= 0)
> + close(brw->retrieve_fd);
> +
> driDestroyOptionCache(&brw->optionCache);
>
> /* free the Mesa context */
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index dc4bc8f..c45bcb5 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 retrieve_fd;
Please rename.
> };
>
> /* 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..6492ae9 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -648,9 +648,19 @@ 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->retrieve_fd >= 0) {
> + close(brw->retrieve_fd);
> + brw->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);
> + brw->retrieve_fd = fd;
> + if (out_fence_fd) {
> + *out_fence_fd = (fd >=0) ? dup(fd) : -1;
> + }
No need for braces with 1-line if body.
Best regards,
Tomasz
More information about the mesa-dev
mailing list