[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.

[Tomasz Figa]

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.

Best regards,
Tomasz