[Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS (v3.1)
Wu, Zhongmin
zhongmin.wu at intel.com
Wed Jul 19 06:16:41 UTC 2017
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-bounces at lists.freedesktop.org] On Behalf Of Tomasz Figa
Sent: Wednesday, July 19, 2017 12:42
To: Wu, Zhongmin <zhongmin.wu at intel.com>
Cc: Gao, Shuo <shuo.gao at intel.com>; Liu, Zhiquan <zhiquan.liu at intel.com>; Daniel Stone <daniels at collabora.com>; Antognolli, Rafael <rafael.antognolli at intel.com>; Emil Velikov <emil.l.velikov at gmail.com>; Chad Versace <chadversary at chromium.org>; Eric Engestrom <eric at engestrom.ch>; Marathe, Yogesh <yogesh.marathe at intel.com>; Kenneth Graunke <kenneth at whitecape.org>; Kondapally, Kalyan <kalyan.kondapally at intel.com>; Rainer Hochecker <fernetmenta at online.de>; ML mesa-dev <mesa-dev at lists.freedesktop.org>; Nicolai Hähnle <nicolai.haehnle at amd.com>; Varad Gautam <varad.gautam at collabora.com>
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 <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);
}
// ...
}
wzm :================== ============================================================================================================================
I am a little confused here, you mean we can add the fd field in _EGLSurface directly ? I checked the definition of _EGLSurface, it is not like a place to add the extra member.
==================================================================================================================================================
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.
wzm :===============================================================================================================================================
But we can't call dri2_surf_set_release_fence(surf, -1) after platforms callback return, the surf may be freed in callback.
====================================================================================================================================================
wzm: ===============================================================================================================================================
In this patch, I just add the fd member in dri2_egl_surface for Android platform, and Only Android platform will initialize/de-initialize the fd when create and destroy the buffer.
If we move the fd member to _EGLSurface, it will be seen on all platforms, right ? We do need a good place to initialize/de-initialize the fd.
And none Android platform will not use such fence actually.
====================================================================================================================================================
> + }
> 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.
wzm: =============================================================================================================================================
That is right, we can do this
And what about queuebuffer , if we apply the same style, can we set the fd to -1 in queue buffer ? In fact, I am not clear about all the possible sequences of such functions swapbuffer,
Flush, make current, and so on. So I used the most safe way without prior knowledge ...
==================================================================================================================================================
> 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?
Wzm ===========================================================================================================================
A little confused again, if we add the fd member in _EGLSurface as above, we can initialize fd to -1 in _eglInitSurface() directly. Every surface creation on each platform will call
_eglInitSurface(), right? The trouble thing is where to de-initialize as we don’t have _eglDeInitSurface() in each platform's destroy path....
================================================================================================================================
> 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.
wzm: =================================
You are right, we can just use brw_context.
=====================================
>
> /**
> 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.
wzm:=======================================================================================================
You are right, it is better to have a separate and entire optimization patch for i965 fence logic first...
============================================================================================================
Best regards,
Tomasz
_______________________________________________
mesa-dev mailing list
mesa-dev at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list