[Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS (v3.1)
Tomasz Figa
tfiga at chromium.org
Wed Jul 19 04:42:04 UTC 2017
Hi Zhongmin,
Thanks for the patch. Please see my comments inline.
On Wed, Jul 19, 2017 at 12:22 PM, Zhongmin Wu <zhongmin.wu at intel.com> wrote:
> Before we queued the buffer with a invalid fence (-1), it will
> make some benchmarks failed to test such as flatland.
>
> Now we get the out fence during the flushing buffer and then pass
> it to SurfaceFlinger in eglSwapbuffer function.
>
> v2: a) Also implement the fence in cancelBuffer.
> b) The last sync fence is stored in drawable object
> rather than brw context.
> c) format clear.
>
> v3: a) Save the last fence fd in DRI Context object.
> b) Return the last fence if the batch buffer is empty and
> nothing to be flushed when _intel_batchbuffer_flush_fence
> c) Add the new interface in vbtl to set the retrieve fence
> in the egl surface. This is just for cancelBuffer.
> d) For queueBuffer, the fence is get with DRI fence interface.
> For cancelBuffer, the fence is get before the context is
> reset, and the fence is then moved to its surface.
> v3.1 a) close fd in the new vbtl interface on none Android platform
>
> Change-Id: Ic0773c19788d612a98d1402f5b5619dab64c1bc2
> Tracked-On: https://jira01.devtools.intel.com/browse/OAM-43936
> Reported-On: https://bugs.freedesktop.org/show_bug.cgi?id=101655
> Signed-off-by: Zhongmin Wu <zhongmin.wu at intel.com>
> Reported-by: Li, Guangli <guangli.li at intel.com>
> Tested-by: Marathe, Yogesh <yogesh.marathe at intel.com>
> ---
> src/egl/drivers/dri2/egl_dri2.c | 17 +++++++++++++-
> src/egl/drivers/dri2/egl_dri2.h | 3 +++
> src/egl/drivers/dri2/platform_android.c | 30 ++++++++++++++++++++++---
> src/egl/drivers/dri2/platform_drm.c | 1 +
> src/egl/drivers/dri2/platform_surfaceless.c | 1 +
> src/egl/drivers/dri2/platform_wayland.c | 2 ++
> src/egl/drivers/dri2/platform_x11.c | 2 ++
> src/egl/drivers/dri2/platform_x11_dri3.c | 1 +
> src/mesa/drivers/dri/common/dri_util.c | 3 +++
> src/mesa/drivers/dri/common/dri_util.h | 2 ++
> src/mesa/drivers/dri/i965/intel_batchbuffer.c | 25 ++++++++++++++++++---
> 11 files changed, 80 insertions(+), 7 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 020a0bc..19d346d 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1351,9 +1351,17 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *dsurf,
> ddraw = (dsurf) ? dri2_dpy->vtbl->get_dri_drawable(dsurf) : NULL;
> rdraw = (rsurf) ? dri2_dpy->vtbl->get_dri_drawable(rsurf) : NULL;
> cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
> -
> + int fence_fd = -1;
> if (old_ctx) {
> __DRIcontext *old_cctx = dri2_egl_context(old_ctx)->dri_context;
> + if (old_dsurf) {
> + void * fence = dri2_dpy->fence->create_fence_fd(old_cctx, -1);
> + if (fence) {
> + fence_fd = dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen, fence);
> + dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
> + }
> + dri2_dpy->vtbl->set_retrieve_fence(old_dsurf, fence_fd);
I think we don't need this callback. We can just set the fence in
old_dsurf directly, i.e.
dri2_surf_set_release_fence(surf, fence_fd)
{
if (surf->fence_fd >= 0)
close(surf->fence_fd);
surf->fence_fd = fence_fd;
}
// ...
dri2_make_current()
{
// ...
int fence_fd = -1;
if (old_dsurf) {
void * fence = dri2_dpy->fence->create_fence_fd(old_cctx, -1);
if (fence) {
fence_fd =
dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen, fence);
dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
}
dri2_surf_set_release_fence(old_dsurf, fence_fd);
}
// ...
}
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;
> } color_buffers[3], *back;
> + int retrieve_fd;
> #endif
>
> #if defined(HAVE_SURFACELESS_PLATFORM)
> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
> index cc2e4a6..b8d53b8 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -306,9 +306,16 @@ droid_window_enqueue_buffer(_EGLDisplay *disp, struct dri2_egl_surface *dri2_sur
> * is responsible for closing it.
> */
> int fence_fd = -1;
> + _EGLContext *ctx = _eglGetCurrentContext();
> + struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
> + void * fence = dri2_dpy->fence->create_fence_fd(dri2_ctx->dri_context, -1);
> + if (fence) {
> + fence_fd = dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen,
> + fence);
> + dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
> + }
If we are already handling the make current case in the generic
egl_dri2, we can do the same with swap_buffers IMHO and have the fence
always stored in dri2_surf->fence_fd.
> dri2_surf->window->queueBuffer(dri2_surf->window, dri2_surf->buffer,
> fence_fd);
> -
No need to remove this line in this patch.
> dri2_surf->buffer->common.decRef(&dri2_surf->buffer->common);
> dri2_surf->buffer = NULL;
> dri2_surf->back = NULL;
> @@ -327,8 +334,10 @@ static void
> droid_window_cancel_buffer(struct dri2_egl_surface *dri2_surf)
> {
> int ret;
> -
> - ret = dri2_surf->window->cancelBuffer(dri2_surf->window, dri2_surf->buffer, -1);
> + int fence_fd = -1;
> + fence_fd = dup(dri2_surf->retrieve_fd);
> + ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
> + dri2_surf->buffer, fence_fd);
The surface is being destroyed here. I don't think we need to dup()
the FD. Instead we should simply set it to -1.
> if (ret < 0) {
> _eglLog(_EGL_WARNING, "ANativeWindow::cancelBuffer failed");
> dri2_surf->base.Lost = EGL_TRUE;
> @@ -434,6 +443,8 @@ droid_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
> dri2_surf->window = window;
> }
>
> + dri2_surf->retrieve_fd = -1;
> +
nit: Maybe we should think about having a dri2_init_surface() helper
that calls _eglInitSurface() and sets some dri2 specific fields for us
as well?
> return &dri2_surf->base;
>
> cleanup_surface:
> @@ -488,6 +499,9 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
>
> dri2_dpy->core->destroyDrawable(dri2_surf->dri_drawable);
>
> + if (dri2_surf->retrieve_fd != -1)
> + close(dri2_surf->retrieve_fd);
> +
I think this would make more sense in dri2_destroy_surface().
> free(dri2_surf);
>
> return EGL_TRUE;
> @@ -1073,6 +1087,15 @@ droid_create_image_khr(_EGLDriver *drv, _EGLDisplay *disp,
> }
>
> static void
> +droid_set_retrieve_fence(_EGLSurface *surf, int fd)
> +{
> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> + if (dri2_surf->retrieve_fd != -1)
> + close(dri2_surf->retrieve_fd);
> + dri2_surf->retrieve_fd = fd;
> +}
Shouldn't need this with my earlier suggestion.
> +
> +static void
> droid_flush_front_buffer(__DRIdrawable * driDrawable, void *loaderPrivate)
> {
> }
> @@ -1289,6 +1312,7 @@ static const struct dri2_egl_display_vtbl droid_display_vtbl = {
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> .get_sync_values = dri2_fallback_get_sync_values,
> .get_dri_drawable = dri2_surface_get_dri_drawable,
> + .set_retrieve_fence = droid_set_retrieve_fence,
> };
>
> static const __DRIdri2LoaderExtension droid_dri2_loader_extension = {
> diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
> index 8b0562c..97f897e 100644
> --- a/src/egl/drivers/dri2/platform_drm.c
> +++ b/src/egl/drivers/dri2/platform_drm.c
> @@ -665,6 +665,7 @@ static const struct dri2_egl_display_vtbl dri2_drm_display_vtbl = {
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> .get_sync_values = dri2_fallback_get_sync_values,
> .get_dri_drawable = dri2_surface_get_dri_drawable,
> + .set_retrieve_fence = dri2_set_retrieve_fence,
> };
>
> EGLBoolean
> diff --git a/src/egl/drivers/dri2/platform_surfaceless.c b/src/egl/drivers/dri2/platform_surfaceless.c
> index 0eb3fb7..78b2571 100644
> --- a/src/egl/drivers/dri2/platform_surfaceless.c
> +++ b/src/egl/drivers/dri2/platform_surfaceless.c
> @@ -244,6 +244,7 @@ static const struct dri2_egl_display_vtbl dri2_surfaceless_display_vtbl = {
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> .get_sync_values = dri2_fallback_get_sync_values,
> .get_dri_drawable = dri2_surface_get_dri_drawable,
> + .set_retrieve_fence = dri2_set_retrieve_fence,
> };
>
> static void
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index f746f0b..458bd6e 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -1087,6 +1087,7 @@ static const struct dri2_egl_display_vtbl dri2_wl_display_vtbl = {
> .create_wayland_buffer_from_image = dri2_wl_create_wayland_buffer_from_image,
> .get_sync_values = dri2_fallback_get_sync_values,
> .get_dri_drawable = dri2_surface_get_dri_drawable,
> + .set_retrieve_fence = dri2_set_retrieve_fence,
> };
>
> static const __DRIextension *dri2_loader_extensions[] = {
> @@ -1781,6 +1782,7 @@ static const struct dri2_egl_display_vtbl dri2_wl_swrast_display_vtbl = {
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> .get_sync_values = dri2_fallback_get_sync_values,
> .get_dri_drawable = dri2_surface_get_dri_drawable,
> + .set_retrieve_fence = dri2_set_retrieve_fence,
> };
>
> static const __DRIswrastLoaderExtension swrast_loader_extension = {
> diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
> index 74d3a16..0def3ca 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -1149,6 +1149,7 @@ static const struct dri2_egl_display_vtbl dri2_x11_swrast_display_vtbl = {
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> .get_sync_values = dri2_fallback_get_sync_values,
> .get_dri_drawable = dri2_surface_get_dri_drawable,
> + .set_retrieve_fence = dri2_set_retrieve_fence,
> };
>
> static const struct dri2_egl_display_vtbl dri2_x11_display_vtbl = {
> @@ -1170,6 +1171,7 @@ static const struct dri2_egl_display_vtbl dri2_x11_display_vtbl = {
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> .get_sync_values = dri2_x11_get_sync_values,
> .get_dri_drawable = dri2_surface_get_dri_drawable,
> + .set_retrieve_fence = dri2_set_retrieve_fence,
> };
>
> static const __DRIswrastLoaderExtension swrast_loader_extension = {
> diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c b/src/egl/drivers/dri2/platform_x11_dri3.c
> index 3148f49..cae322d 100644
> --- a/src/egl/drivers/dri2/platform_x11_dri3.c
> +++ b/src/egl/drivers/dri2/platform_x11_dri3.c
> @@ -466,6 +466,7 @@ struct dri2_egl_display_vtbl dri3_x11_display_vtbl = {
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> .get_sync_values = dri3_get_sync_values,
> .get_dri_drawable = dri3_get_dri_drawable,
> + .set_retrieve_fence = dri2_set_retrieve_fence,
> };
>
> EGLBoolean
> diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c
> index f6df488..d0465b8 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -446,6 +446,7 @@ driCreateContextAttribs(__DRIscreen *screen, int api,
> context->driScreenPriv = screen;
> context->driDrawablePriv = NULL;
> context->driReadablePriv = NULL;
> + context->retrieve_fd = -1;
>
> if (!screen->driver->CreateContext(mesa_api, modes, context,
> major_version, minor_version,
> @@ -499,6 +500,8 @@ static void
> driDestroyContext(__DRIcontext *pcp)
> {
> if (pcp) {
> + if (pcp->retrieve_fd != -1)
> + close(pcp->retrieve_fd);
> pcp->driScreenPriv->driver->DestroyContext(pcp);
> free(pcp);
> }
> diff --git a/src/mesa/drivers/dri/common/dri_util.h b/src/mesa/drivers/dri/common/dri_util.h
> index 8fcd632..f723a90 100644
> --- a/src/mesa/drivers/dri/common/dri_util.h
> +++ b/src/mesa/drivers/dri/common/dri_util.h
> @@ -218,6 +218,8 @@ struct __DRIcontextRec {
> int draw_stamp;
> int read_stamp;
> } dri2;
> +
> + int retrieve_fd;
> };
Do we need to store this in a generic DRI struct, if it's only used by
i965? IMHO it would make more sense to just store this in brw_context.
Also, let's use some more commonly used naming scheme, for example out_fence_fd.
>
> /**
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 62d2fe8..77bf8f6 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -648,9 +648,25 @@ do_flush_locked(struct brw_context *brw, int in_fence_fd, int *out_fence_fd)
> /* Add the batch itself to the end of the validation list */
> add_exec_bo(batch, batch->bo);
>
> + if (brw->driContext &&
> + brw->driContext->retrieve_fd != -1) {
> + close(brw->driContext->retrieve_fd);
> + brw->driContext->retrieve_fd = -1;
> + }
> +
> + int fd = -1;
> ret = execbuffer(dri_screen->fd, batch, hw_ctx,
> 4 * USED_BATCH(*batch),
> - in_fence_fd, out_fence_fd, flags);
> + in_fence_fd, &fd, flags);
> +
> + if (out_fence_fd != NULL) {
> + *out_fence_fd = fd;
> + if (brw->driContext)
> + brw->driContext->retrieve_fd = dup(fd);
> + } else {
> + if (brw->driContext)
> + brw->driContext->retrieve_fd = fd;
> + }
I think this code can be simplified significantly. Including my comments above:
brw->out_fence_fd = fd;
if (out_fence_fd)
*out_fence_fd = (fd >= 0) ? dup(fd) : -1;
> }
>
> throttle(brw);
> @@ -684,8 +700,11 @@ _intel_batchbuffer_flush_fence(struct brw_context *brw,
> {
> int ret;
>
> - if (USED_BATCH(brw->batch) == 0)
> - return 0;
> + if (USED_BATCH(brw->batch) == 0) {
> + if (out_fence_fd && brw->driContext->retrieve_fd != -1)
> + *out_fence_fd = dup(brw->driContext->retrieve_fd);
We need to check for >= 0 if we only allow valid FDs. Also it should
actually make sense to assign -1 if we don't have any fence.
if (out_fence_fd)
*out_fence_fd = (brw->out_fence_fd >= 0) ? dup(brw->out_fence_fd) : -1;
> + return 0;
Wrong indentation?
> + }
>
> if (brw->throttle_batch[0] == NULL) {
> brw->throttle_batch[0] = brw->batch.bo;
Generally I still believe i965 driver in its current shape inserts
commands to the buffer when brw_dri_create_fence_fd() is called, so
all the i965-specific changes should go to a separate patch, as an
additional optimization.
Best regards,
Tomasz
More information about the mesa-dev
mailing list