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 <zhongmin...@intel.com> Cc: Gao, Shuo <shuo....@intel.com>; Liu, Zhiquan <zhiquan....@intel.com>; Daniel Stone <dani...@collabora.com>; Antognolli, Rafael <rafael.antogno...@intel.com>; Emil Velikov <emil.l.veli...@gmail.com>; Chad Versace <chadvers...@chromium.org>; Eric Engestrom <e...@engestrom.ch>; Marathe, Yogesh <yogesh.mara...@intel.com>; Kenneth Graunke <kenn...@whitecape.org>; Kondapally, Kalyan <kalyan.kondapa...@intel.com>; Rainer Hochecker <fernetme...@online.de>; ML mesa-dev <mesa-dev@lists.freedesktop.org>; Nicolai Hähnle <nicolai.haeh...@amd.com>; Varad Gautam <varad.gau...@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...@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...@intel.com> > Reported-by: Li, Guangli <guangli...@intel.com> > Tested-by: Marathe, Yogesh <yogesh.mara...@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@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
