[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 06:22:25 UTC 2017
> -----Original Message-----
> From: Wu, Zhongmin
> Sent: Monday, August 7, 2017 11:27 AM
> To: Marathe, Yogesh <yogesh.marathe at intel.com>; Tomasz Figa
> <tfiga at chromium.org>
> Cc: Gao, Shuo <shuo.gao at intel.com>; Antognolli, Rafael
> <rafael.antognolli at intel.com>; Emil Velikov <emil.l.velikov at gmail.com>;
> Kenneth Graunke <kenneth at whitecape.org>; ML mesa-dev <mesa-
> dev at lists.freedesktop.org>; Kondapally, Kalyan
> <kalyan.kondapally at intel.com>; Timothy Arceri <tarceri at itsqueeze.com>
> 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:
> http://oorja.iind.intel.com/mediawiki/index.php/Flatland
> can you also try the flatland on this page.
>
> For AOSP flatland, yes, the EGL patch may solve the issue.
That should suffice.
>
> However, I met one case that the batch buffer is empty just at the swapbuffer
> (glfush is just called before that), then you might not get a fence fd. So I had to
> record the last fence fd.
>
> I am not sure, you had better try it on the board If you can get a valid fd on any
> situation.
I think aosp flatland is more relevant here. I haven’t face the issue you have
mentioned so far.
>
>
>
>
>
> -----Original Message-----
> From: Marathe, Yogesh
> Sent: Monday, August 7, 2017 13:25
> To: Tomasz Figa <tfiga at chromium.org>; Wu, Zhongmin
> <zhongmin.wu at intel.com>
> Cc: Gao, Shuo <shuo.gao at intel.com>; Antognolli, Rafael
> <rafael.antognolli at intel.com>; Emil Velikov <emil.l.velikov at gmail.com>;
> Kenneth Graunke <kenneth at whitecape.org>; ML mesa-dev <mesa-
> dev at lists.freedesktop.org>; Kondapally, Kalyan
> <kalyan.kondapally at intel.com>; Timothy Arceri <tarceri at itsqueeze.com>
> 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.
>
> 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