[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
Mon Aug 7 05:24:30 UTC 2017
This can be dropped. I'm running with egl patch alone and things seem fine.
Zhongmin, please comment if you don’t think so.
> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf
> Of Marathe, Yogesh
> Sent: Friday, August 4, 2017 9:18 PM
>
> 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
> _______________________________________________
> 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