[Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync fence for Android OS v4.2
Rafael Antognolli
rafael.antognolli at intel.com
Tue Aug 1 15:29:01 UTC 2017
On Mon, Jul 31, 2017 at 09:58:24AM -0700, Marathe, Yogesh wrote:
> Rafael, Tomasz,
>
> > -----Original Message-----
> > From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf
> > Of Rafael Antognolli
> > Sent: Tuesday, July 25, 2017 9:39 PM
> > 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>; emil.l.velikov at gmail.com; Eric
> > Engestrom <eric at engestrom.ch>; Marathe, Yogesh
> > <yogesh.marathe at intel.com>; tfiga at chromium.org; Kondapally, Kalyan
> > <kalyan.kondapally at intel.com>; mesa-dev at lists.freedesktop.org; Varad
> > Gautam <varad.gautam at collabora.com>
> > Subject: Re: [Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync fence
> > for Android OS v4.2
> >
> > On Tue, Jul 25, 2017 at 10:07:23AM +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.
> > >
> > > 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
> > >
> > > 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.
> > >
> > > v4.1: a) make some changes of variable name.
> > > b) the patch is breaked into two patches.
> > >
> > > v4.2: a) Add a deinit interface for surface to clear the out fence
> >
> > Hi Zhongmin,
> >
> > The patch is indeed looking better. I agree with Tomasz, it would be good to
> > have a way for the platform to inform whether it is interested in fences or not.
> > What about some flag that you pass to dri2_surf_init? (I'm not sure that's the
> > best place, though).
> >
>
> I would like to take it forward from here for remaining review comments, Zhongmin agrees.
>
> I added 'enable_out_fence' bool to dri2_surf_init() as a parameter and all platforms except
> android pass this as false. Based on 'enable_out_fence' stored with surface,
> 'dri2_surf_get/update_fence_fd()' has a check before it calls create_fence_fd(). Is this the
> right expectation here?
Sounds good to me, although I would need to see the patch. Also please make
sure that the dri2 fence extension is supported, assuming you are trying to
enable this.
> Please let me know if 'enable_out_fence' can be changed for a better name. I've already
> implemented other review other comments Rafael mentioned and will include them
> in v4.3 along with this.
I also don't have any better name in mind.
Thanks,
Rafael
> > Please see other comments below.
> >
> > > Change-Id: Ided54d2e193cde73a6f0feb36ac1c0056e4958f2
> > > Signed-off-by: Zhongmin Wu <zhongmin.wu at intel.com>
> > > ---
> > > src/egl/drivers/dri2/egl_dri2.c | 51 +++++++++++++++++++++++++++
> > > src/egl/drivers/dri2/egl_dri2.h | 8 +++++
> > > src/egl/drivers/dri2/platform_android.c | 12 ++++---
> > > src/egl/drivers/dri2/platform_drm.c | 3 +-
> > > src/egl/drivers/dri2/platform_surfaceless.c | 3 +-
> > > src/egl/drivers/dri2/platform_wayland.c | 3 +-
> > > src/egl/drivers/dri2/platform_x11.c | 3 +-
> > > src/egl/drivers/dri2/platform_x11_dri3.c | 3 +-
> > > 8 files changed, 77 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/egl/drivers/dri2/egl_dri2.c
> > > b/src/egl/drivers/dri2/egl_dri2.c index 020a0bc..ffd3a8a 100644
> > > --- a/src/egl/drivers/dri2/egl_dri2.c
> > > +++ b/src/egl/drivers/dri2/egl_dri2.c
> > > @@ -1307,6 +1307,32 @@ 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->out_fence_fd = -1;
> > > + return _eglInitSurface(surf, dpy, type, conf, attrib_list); }
> > > +
> > > +static void
> > > +dri2_surface_set_out_fence( _EGLSurface *surf, int fence_fd) {
> > > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > > + if (dri2_surf->out_fence_fd >=0)
> > > + close(dri2_surf->out_fence_fd);
> > > +
> > > + dri2_surf->out_fence_fd = fence_fd; }
> > > +
> > > +void
> > > +dri2_surf_deinit(_EGLSurface *surf)
> > > +{
> > > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > > + dri2_surface_set_out_fence(surf, -1); }
> > > +
> > > static EGLBoolean
> > > dri2_destroy_surface(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface
> > > *surf) { @@ -1318,6 +1344,22 @@ dri2_destroy_surface(_EGLDriver *drv,
> > > _EGLDisplay *dpy, _EGLSurface *surf)
> > > return dri2_dpy->vtbl->destroy_surface(drv, dpy, surf); }
> > >
> > > +static void
> > > +dri2_surf_get_fence_fd(_EGLContext *ctx,
> >
> > Maybe it's just me but every time I read this code I get confused by the name of
> > this function, since it says "get_fence_fd" but doesn't really return anything.
> > How about dri2_surf_update_fence_fd?
> >
> > > + _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);
> >
> > Before calling any of the dri2 fence extension, shouldn't we check whether it is
> > available? I think you had these checks in earlier versions of this patch, but
> > maybe it got lost along the way.
> >
> > > + 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_out_fence(surf, fence_fd); }
> > > +
> > > /**
> > > * Called via eglMakeCurrent(), drv->API.MakeCurrent().
> > > */
> > > @@ -1352,8 +1394,11 @@ 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);
> > > dri2_dpy->core->unbindContext(old_cctx);
> > > }
> > >
> > > @@ -1490,6 +1535,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 +1547,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..4ea3adb 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 out_fence_fd;
> > > };
> > >
> > > struct dri2_egl_config
> > > @@ -446,4 +447,11 @@ 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);
> > > +
> > > +void
> > > +dri2_surf_deinit(_EGLSurface *surf);
> > > +
> > > #endif /* EGL_DRI2_INCLUDED */
> > > diff --git a/src/egl/drivers/dri2/platform_android.c
> > > b/src/egl/drivers/dri2/platform_android.c
> > > index cc2e4a6..4112a0a 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->out_fence_fd;
> > > + dri2_surf->out_fence_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->out_fence_fd;
> > > + dri2_surf->out_fence_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) {
> > > @@ -488,6 +491,7 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay
> > > *disp, _EGLSurface *surf)
> > >
> > > dri2_dpy->core->destroyDrawable(dri2_surf->dri_drawable);
> > >
> > > + dri2_surf_deinit(surf);
> > > free(dri2_surf);
> > >
> > > return EGL_TRUE;
> > > diff --git a/src/egl/drivers/dri2/platform_drm.c
> > > b/src/egl/drivers/dri2/platform_drm.c
> > > index 8b0562c..0af93d1 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) {
> > > @@ -204,6 +204,7 @@ dri2_drm_destroy_surface(_EGLDriver *drv,
> > _EGLDisplay *disp, _EGLSurface *surf)
> > > dri2_surf->dri_buffers[i]);
> > > }
> > >
> > > + dri2_surf_deinit(surf);
> > > free(surf);
> > >
> > > return EGL_TRUE;
> > > diff --git a/src/egl/drivers/dri2/platform_surfaceless.c
> > > b/src/egl/drivers/dri2/platform_surfaceless.c
> > > index 0eb3fb7..46c95c4 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, @@ -165,6 +165,7 @@
> > > surfaceless_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp,
> > > _EGLSurface *sur
> > >
> > > dri2_dpy->core->destroyDrawable(dri2_surf->dri_drawable);
> > >
> > > + dri2_surf_deinit(surf);
> > > free(dri2_surf);
> > > return EGL_TRUE;
> > > }
> > > diff --git a/src/egl/drivers/dri2/platform_wayland.c
> > > b/src/egl/drivers/dri2/platform_wayland.c
> > > index f746f0b..2122c5d 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) {
> > > @@ -286,6 +286,7 @@ dri2_wl_destroy_surface(_EGLDriver *drv,
> > _EGLDisplay *disp, _EGLSurface *surf)
> > > wl_proxy_wrapper_destroy(dri2_surf->wl_dpy_wrapper);
> > > wl_event_queue_destroy(dri2_surf->wl_queue);
> > >
> > > + dri2_surf_deinit(surf);
> > > free(surf);
> > >
> > > return EGL_TRUE;
> > > diff --git a/src/egl/drivers/dri2/platform_x11.c
> > > b/src/egl/drivers/dri2/platform_x11.c
> > > index 74d3a16..37f47cb 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;
> > > @@ -390,6 +390,7 @@ dri2_x11_destroy_surface(_EGLDriver *drv,
> > _EGLDisplay *disp, _EGLSurface *surf)
> > > if (surf->Type == EGL_PBUFFER_BIT)
> > > xcb_free_pixmap (dri2_dpy->conn, dri2_surf->drawable);
> > >
> > > + dri2_surf_deinit(surf);
> > > free(surf);
> > >
> > > return EGL_TRUE;
> > > diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c
> > > b/src/egl/drivers/dri2/platform_x11_dri3.c
> > > index 3148f49..5f6f7e5 100644
> > > --- a/src/egl/drivers/dri2/platform_x11_dri3.c
> > > +++ b/src/egl/drivers/dri2/platform_x11_dri3.c
> > > @@ -145,6 +145,7 @@ dri3_destroy_surface(_EGLDriver *drv, _EGLDisplay
> > > *disp, _EGLSurface *surf)
> > >
> > > loader_dri3_drawable_fini(&dri3_surf->loader_drawable);
> > >
> > > + dri2_surf_deinit(surf);
> > > free(surf);
> > >
> > > return EGL_TRUE;
> > > @@ -183,7 +184,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) {
> > > --
> > > 1.7.9.5
> > >
> > _______________________________________________
> > 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