[Mesa-dev] [PATCH v6.1] egl: Allow creation of per surface out fence
Marathe, Yogesh
yogesh.marathe at intel.com
Wed Aug 23 15:03:46 UTC 2017
> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga at chromium.org]
> Sent: Wednesday, August 23, 2017 8:17 PM
> To: Marathe, Yogesh <yogesh.marathe at intel.com>
> Cc: ML mesa-dev <mesa-dev at lists.freedesktop.org>; Emil Velikov
> <emil.l.velikov at gmail.com>; Gao, Shuo <shuo.gao at intel.com>; Liu, Zhiquan
> <zhiquan.liu at intel.com>; Daniel Stone <daniels at collabora.com>; Nicolai
> Hähnle <nicolai.haehnle at amd.com>; Antognolli, Rafael
> <rafael.antognolli at intel.com>; Eric Engestrom <eric at engestrom.ch>; Kenneth
> Graunke <kenneth at whitecape.org>; Rainer Hochecker
> <fernetmenta at online.de>; Kondapally, Kalyan <kalyan.kondapally at intel.com>;
> Timothy Arceri <tarceri at itsqueeze.com>; Varad Gautam
> <varad.gautam at collabora.com>; Wu, Zhongmin <zhongmin.wu at intel.com>
> Subject: Re: [PATCH v6.1] egl: Allow creation of per surface out fence
>
> Hi Yogesh,
>
> Sorry for being late with review. Please see some comments inline.
>
No problem.
> On Fri, Aug 18, 2017 at 7:08 PM, <yogesh.marathe at intel.com> wrote:
> > From: Zhongmin Wu <zhongmin.wu at intel.com>
> >
> > Add plumbing to allow creation of per display surface out fence.
> >
> > Currently enabled only on android, since the system expects a valid fd
> > in ANativeWindow::{queue,cancel}Buffer. We pass a fd of -1 with which
> > native applications such as flatland fail. The patch enables explicit
> > sync on android and fixes one of the functional issue for apps or
> > buffer consumers which depend upon fence and its timestamp.
> >
> > 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 broken into two patches.
> >
> > v4.2: a) Add a deinit interface for surface to clear the out fence
> >
> > v5: a) Add enable_out_fence to init, platform sets it true or
> > false
> > b) Change get fd to update fd and check for fence
> > c) Commit description updated
> >
> > v6: a) Heading and commit description updated
> > b) enable_out_fence is set only if fence is supported
> > c) Review comments on function names
> > d) Test with standalone patch, resolves the bug
> >
> > v6.1 a) Check for old display fence reverted back
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101655
> >
> > Signed-off-by: Zhongmin Wu <zhongmin.wu at intel.com>
> > Signed-off-by: Yogesh Marathe <yogesh.marathe at intel.com>
> > ---
> > src/egl/drivers/dri2/egl_dri2.c | 69 +++++++++++++++++++++++++++++
> > src/egl/drivers/dri2/egl_dri2.h | 9 ++++
> > src/egl/drivers/dri2/platform_android.c | 29 ++++++------
> > 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, 104 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/egl/drivers/dri2/egl_dri2.c
> > b/src/egl/drivers/dri2/egl_dri2.c index ed79e0d..04d0332 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.c
> > +++ b/src/egl/drivers/dri2/egl_dri2.c
> > @@ -1354,6 +1354,44 @@ dri2_destroy_context(_EGLDriver *drv,
> _EGLDisplay *disp, _EGLContext *ctx)
> > return EGL_TRUE;
> > }
> >
> > +EGLBoolean
> > +dri2_init_surface(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
> > + _EGLConfig *conf, const EGLint *attrib_list, EGLBoolean
> > +enable_out_fence) {
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > + struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> > +
> > + dri2_surf->out_fence_fd = -1;
> > + if (dri2_dpy->fence && dri2_dpy->fence->base.version >= 2 &&
> > + dri2_dpy->fence->get_capabilities &&
> > + (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) &
> > + __DRI_FENCE_CAP_NATIVE_FD)) {
> > + dri2_surf->enable_out_fence = enable_out_fence;
> > + }
>
> nit: It might not change anything in practice, but it would be more logical if the
> code always initialized enable_out_fence to some value.
> So maybe let's add dri2_surf->enable_out_fence = 0; above the if.
>
Ok.
> > +
> > + return _eglInitSurface(surf, dpy, type, conf, attrib_list); }
> > +
> > +static void
> > +dri2_surface_set_out_fence_fd( _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_deinit_surface(_EGLSurface *surf) {
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > +
> > + dri2_surface_set_out_fence_fd(surf, -1);
> > + dri2_surf->enable_out_fence = false; }
> > +
> > static EGLBoolean
> > dri2_destroy_surface(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface
> > *surf) { @@ -1365,6 +1403,27 @@ dri2_destroy_surface(_EGLDriver *drv,
> > _EGLDisplay *dpy, _EGLSurface *surf)
> > return dri2_dpy->vtbl->destroy_surface(drv, dpy, surf); }
> >
> > +static void
> > +dri2_surf_update_fence_fd(_EGLContext *ctx,
> > + _EGLDisplay *dpy, _EGLSurface *surf) {
> > + __DRIcontext *dri_ctx = dri2_egl_context(ctx)->dri_context;
> > + struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> > + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> > + int fence_fd = -1;
> > + void *fence;
> > +
> > + if (dri2_surf->enable_out_fence) {
> > + fence = dri2_dpy->fence->create_fence_fd(dri_ctx, -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_surface_set_out_fence_fd(surf, fence_fd);
>
> We don't need to call this if enable_out_fence is 0. Actually, We can invert the if
> above and return early if !enable_out_fence.
>
Right.
> > +}
> > +
> > /**
> > * Called via eglMakeCurrent(), drv->API.MakeCurrent().
> > */
> > @@ -1401,6 +1460,8 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay
> > *disp, _EGLSurface *dsurf,
> >
> > if (old_ctx) {
> > __DRIcontext *old_cctx =
> > dri2_egl_context(old_ctx)->dri_context;
> > + if (old_dsurf)
> > + dri2_surf_update_fence_fd(old_ctx, disp, old_dsurf);
> > dri2_dpy->core->unbindContext(old_cctx);
> > }
> >
> > @@ -1539,6 +1600,10 @@ 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_update_fence_fd(ctx, dpy, surf);
> > return dri2_dpy->vtbl->swap_buffers(drv, dpy, surf); }
> >
> > @@ -1548,6 +1613,10 @@ 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_update_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 f471561..f4f47af 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.h
> > +++ b/src/egl/drivers/dri2/egl_dri2.h
> > @@ -328,6 +328,8 @@ struct dri2_egl_surface
> > __DRIimage *front;
> > unsigned int visual;
> > #endif
> > + int out_fence_fd;
> > + bool enable_out_fence;
>
> Since this is declared as EGLBoolean in dri2_init_surface(), let's use the same
> type here too.
>
Sounds good.
> > };
> >
> > struct dri2_egl_config
> > @@ -456,4 +458,11 @@ dri2_set_WL_bind_wayland_display(_EGLDriver *drv,
> > _EGLDisplay *disp) void dri2_display_destroy(_EGLDisplay *disp);
> >
> > +EGLBoolean
> > +dri2_init_surface(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
> > + _EGLConfig *conf, const EGLint *attrib_list, EGLBoolean
> > +enable_out_fence);
>
> ^ EGLBoolean used for enable_out_fence here.
>
> With these fixed (and no changes other than that), feel free to add:
>
> Reviewed-by: Tomasz Figa <tfiga at chromium.org>
Well, since this isn't merged yet, I think I should push v6.2 with final comments
from you and Emil. Will add your Rb there. Thanks.
>
> Best regards,
> Tomasz
More information about the mesa-dev
mailing list