[Mesa-dev] [EGL android: accquire fence implementation 1/2] i965: Return the last fence if the batch buffer is empty and nothing to be flushed when _intel_batchbuffer_flush_fence.
Marathe, Yogesh
yogesh.marathe at intel.com
Fri Aug 4 15:48:20 UTC 2017
Tomasz,
> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga at chromium.org]
> Sent: Friday, August 4, 2017 3:48 PM
> To: Marathe, Yogesh <yogesh.marathe at intel.com>
> Cc: Emil Velikov <emil.l.velikov at gmail.com>; Wu, Zhongmin
> <zhongmin.wu at intel.com>; Gao, Shuo <shuo.gao at intel.com>; Antognolli,
> Rafael <rafael.antognolli at intel.com>; Timothy Arceri
> <tarceri at itsqueeze.com>; Kenneth Graunke <kenneth at whitecape.org>;
> Kondapally, Kalyan <kalyan.kondapally at intel.com>; ML mesa-dev <mesa-
> dev at lists.freedesktop.org>
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2] i965:
> Return the last fence if the batch buffer is empty and nothing to be flushed when
> _intel_batchbuffer_flush_fence.
>
> Hi Yogesh,
>
> On Fri, Aug 4, 2017 at 7:12 PM, Marathe, Yogesh <yogesh.marathe at intel.com>
> wrote:
> > Hi Emil,
> >
> >> -----Original Message-----
> >> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
> >> Behalf Of Emil Velikov
> >> Sent: Tuesday, July 25, 2017 8:19 PM
> >> To: Wu, Zhongmin <zhongmin.wu at intel.com>
> >> Cc: Gao, Shuo <shuo.gao at intel.com>; Antognolli, Rafael
> >> <rafael.antognolli at intel.com>; Timothy Arceri
> >> <tarceri at itsqueeze.com>; Marathe, Yogesh <yogesh.marathe at intel.com>;
> >> Tomasz Figa <tfiga at chromium.org>; Kenneth Graunke
> >> <kenneth at whitecape.org>; Kondapally, Kalyan
> >> <kalyan.kondapally at intel.com>; ML mesa-dev <mesa-
> >> dev at lists.freedesktop.org>
> >> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation 1/2]
> i965:
> >> Return the last fence if the batch buffer is empty and nothing to be
> >> flushed when _intel_batchbuffer_flush_fence.
> >>
> >> Hi Zhongmin,
> >>
> >> Is the issue resolved by the EGL patch alone? Worth sticking with that for
> now?
> >>
> >> I think this patch will cause some noticeable overhead - see below for details.
> >>
> >>
> >> On 21 July 2017 at 04:08, Zhongmin Wu <zhongmin.wu at intel.com> wrote:
> >> > Always save the last fence in the brw context when flushing buffer.
> >> > If the buffer is nothing to be flushed, then return the last fence
> >> > when asked for.
> >> >
> >> > Change-Id: Ic47035bcd1a27e402609afd9e2d1e3972548b97d
> >> > Signed-off-by: Zhongmin Wu <zhongmin.wu at intel.com>
> >> > ---
> >> > src/mesa/drivers/dri/i965/brw_context.c | 5 +++++
> >> > src/mesa/drivers/dri/i965/brw_context.h | 1 +
> >> > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 16 ++++++++++++++--
> >> > 3 files changed, 20 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> >> > b/src/mesa/drivers/dri/i965/brw_context.c
> >> > index 5433f90..ed0b056 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> >> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> >> > @@ -1086,6 +1086,8 @@ brwCreateContext(gl_api api,
> >> > ctx->VertexProgram._MaintainTnlProgram = true;
> >> > ctx->FragmentProgram._MaintainTexEnvProgram = true;
> >> >
> >> > + brw->out_fence_fd = -1;
> >> > +
> >> > brw_draw_init( brw );
> >> >
> >> > if ((flags & __DRI_CTX_FLAG_DEBUG) != 0) { @@ -1169,6 +1171,9
> >> > @@ intelDestroyContext(__DRIcontext * driContextPriv)
> >> > brw->throttle_batch[1] = NULL;
> >> > brw->throttle_batch[0] = NULL;
> >> >
> >> > + if (brw->out_fence_fd >= 0)
> >> > + close(brw->out_fence_fd);
> >> > +
> >> > driDestroyOptionCache(&brw->optionCache);
> >> >
> >> > /* free the Mesa context */
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> >> > b/src/mesa/drivers/dri/i965/brw_context.h
> >> > index dc4bc8f..692ea2c 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> >> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> >> > @@ -1217,6 +1217,7 @@ struct brw_context
> >> >
> >> > __DRIcontext *driContext;
> >> > struct intel_screen *screen;
> >> > + int out_fence_fd;
> >> > };
> >> >
> >> > /* brw_clear.c */
> >> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> > index 62d2fe8..d342e5d 100644
> >> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> >> > @@ -648,9 +648,18 @@ 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->out_fence_fd >= 0) {
> >> > + close(brw->out_fence_fd);
> >> > + brw->out_fence_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);
> >> execbuffer() creates an out fence if the "out_fence_fd" pointer is non-NULL.
> >> Hence with this patch we'will create a fence for each
> >> _intel_batchbuffer_flush_fence() invocation...
> >>
> >> Not sure how costly that will be though :-\
> >>
> >
> > I see this results into 1 get_unused_fd_flags() + 1 sync_file_create()
> > and operations to store out fd in kernel for return arg. I doubt it
> > will be very costly, the ioctl
> > DRM_IOCTL_I915_GEM_EXECBUFFER2 or
> DRM_IOCTL_I915_GEM_EXECBUFFER2_WR
> > was anyways there, so nothing huge additional on ioctl front.
> >
> > Nonetheless, what other option do we have? This may sound absurd but
> > is there a way / ioctl to call sync_file_create directly from mesa
> > UMD and store fds instead of creating at every execbuffer()? We also
> > need a way to associate those stored fds to buffers then. I know I'm
> > adding more ioctls but intension is, if we can do this in some
> > init() we'll save these operations during execbuffer().
> >
> > IMHO, this is separate discussion, let this patch enable functionality
> > first and then we can work on making it light.
>
> The question was whether the EGL patch alone solves the Flatland issue. We
> need it answered first before we continue with further discussion.
Sorry, I assumed it won't, I tested flatland again with egl patch alone and it worked!
Will check overall stability of system and confirm if we can drop this by Monday
(shouldn’t break anything else).
>
> Best regards,
> Tomasz
More information about the mesa-dev
mailing list