[Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync fence for Android OS v4.2
Marathe, Yogesh
yogesh.marathe at intel.com
Tue Jul 25 14:30:25 UTC 2017
Emil,
> -----Original Message-----
> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
> Sent: Tuesday, July 25, 2017 7:46 PM
> To: Wu, Zhongmin <zhongmin.wu at intel.com>
> Cc: ML mesa-dev <mesa-dev at lists.freedesktop.org>; Kondapally, Kalyan
> <kalyan.kondapally at intel.com>; Marathe, Yogesh
> <yogesh.marathe at intel.com>; Antognolli, Rafael
> <rafael.antognolli at intel.com>; Gao, Shuo <shuo.gao at intel.com>; Tomasz Figa
> <tfiga at chromium.org>; Chris Wilson <chris at chris-wilson.co.uk>; Eric
> Engestrom <eric at engestrom.ch>; Rob Herring <robh at kernel.org>; Daniel
> Stone <daniels at collabora.com>; Varad Gautam
> <varad.gautam at collabora.com>; Liu, Zhiquan <zhiquan.liu at intel.com>; Frank
> Binns <frank.binns at imgtec.com>; Brendan King <Brendan.King at imgtec.com>;
> Martin Peres <martin.peres at linux.intel.com>
> Subject: Re: [PATCH 2/2] i965: Queue the buffer with a sync fence for Android
> OS v4.2
>
> Hi Zhongmin,
>
> Thanks you for the update. There's a couple of important comments -
> dri2_make_current + droid_window_enqueue_buffer.
> The rest is just nitpiks.
>
> Tomasz, hats down for the immense help and guidance.
>
> On the potential performance hit (due to the extra fence), I think we could do
> some tests before adding extra infra.
> No obvious benchmarks come to mind - any suggestions?
>
Sorry to jump in, flatland is the one native application on android that tests this
explicitly. It gives time required to render one frame of particular resolution without
other services running. It’s a native app that comes with aosp. And we found this
issue just because of that.
App info - https://android.googlesource.com/platform/frameworks/native/+/master/cmds/flatland/README.txt
Bug - https://bugs.freedesktop.org/show_bug.cgi?id=101655
I already tested this patch set with android and I can see scores not being that great.
May be this is the one we can use to profile this or I can continue to profile based on
guidance here.
> On 25 July 2017 at 03:07, 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
> >
> > 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
> >
> > 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,
> nit: s/dri2_surf_init/dri2_init_surface/
>
> > + _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)
> nit: s/dri2_surface_set_out_fence/dri2_surface_set_out_fence_fd/
>
> > +{
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > + if (dri2_surf->out_fence_fd >=0)
> nit: space between = and 0
>
> > + close(dri2_surf->out_fence_fd);
> > +
> > + dri2_surf->out_fence_fd = fence_fd; }
> > +
> > +void
> > +dri2_surf_deinit(_EGLSurface *surf)
> > +{
> nit: s/dri2_surf_deinit/dri2_fini_surface/
>
> > + 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,
> > + _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);
> Add blank line between declarations and code.
>
> > + 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;
> Unused variable?
>
> > 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);
> Are you sure we should be using the old surface and not the new one?
> Can you elaborate why?
>
> > 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();
> Blank line.
>
> > + 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();
> Blank line.
>
> > + 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);
> >
> The comment above the function needs updating.
>
> > @@ -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;
> Blank line.
>
> > + dri2_surf->out_fence_fd = -1;
> > + ret = dri2_surf->window->cancelBuffer(dri2_surf->window,
> > + dri2_surf->buffer,
> > + fence_fd);
>
>
> Thanks
> Emil
More information about the mesa-dev
mailing list