[Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync fence for Android OS v4.2

[Rafael Antognolli]

On Wed, Aug 02, 2017 at 02:56:34PM +0100, Emil Velikov wrote:
> On 1 August 2017 at 16:29, Rafael Antognolli
> <rafael.antognolli at intel.com> wrote:
> > On Mon, Jul 31, 2017 at 09:58:24AM -0700, Marathe, Yogesh wrote:
> >> Rafael,  Tomasz,
> >>
> >> > -----Original Message-----
> >> > From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf
> >> > Of Rafael Antognolli
> >> > Sent: Tuesday, July 25, 2017 9:39 PM
> >> > To: Wu, Zhongmin <zhongmin.wu at intel.com>
> >> > Cc: Gao, Shuo <shuo.gao at intel.com>; Liu, Zhiquan <zhiquan.liu at intel.com>;
> >> > Daniel Stone <daniels at collabora.com>; emil.l.velikov at gmail.com; Eric
> >> > Engestrom <eric at engestrom.ch>; Marathe, Yogesh
> >> > <yogesh.marathe at intel.com>; tfiga at chromium.org; Kondapally, Kalyan
> >> > <kalyan.kondapally at intel.com>; mesa-dev at lists.freedesktop.org; Varad
> >> > Gautam <varad.gautam at collabora.com>
> >> > Subject: Re: [Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync fence
> >> > for Android OS v4.2
> >> >
> >> > On Tue, Jul 25, 2017 at 10:07:23AM +0800, Zhongmin Wu wrote:
> >> > > Before we queued the buffer with a invalid fence (-1), it will make
> >> > > some benchmarks failed to test such as flatland.
> >> > >
> >> > > Now we get the out fence during the flushing buffer and then pass it
> >> > > to SurfaceFlinger in eglSwapbuffer function.
> >> > >
> >> > > 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 breaked into two patches.
> >> > >
> >> > > v4.2: a) Add a deinit interface for surface to clear the out fence
> >> >
> >> > Hi Zhongmin,
> >> >
> >> > The patch is indeed looking better. I agree with Tomasz, it would be good to
> >> > have a way for the platform to inform whether it is interested in fences or not.
> >> > What about some flag that you pass to dri2_surf_init? (I'm not sure that's the
> >> > best place, though).
> >> >
> >>
> >> I would like to take it forward from here for remaining review comments, Zhongmin agrees.
> >>
> >> I added 'enable_out_fence' bool to dri2_surf_init() as a parameter and all platforms except
> >> android pass this as false. Based on  'enable_out_fence'  stored with surface,
> >> 'dri2_surf_get/update_fence_fd()' has a check before it calls create_fence_fd(). Is this the
> >> right expectation here?
> >
> > Sounds good to me, although I would need to see the patch. Also please make
> > sure that the dri2 fence extension is supported, assuming you are trying to
> > enable this.
> >
> I've let glmark2 spin (egl+x11) during lunch and it showed no perf. difference.
> 
> Admittedly may not be the best of benchmarks for this use case, even
> though it pushes 200-2000fps, while the new code is in the
> eglSwapBuffers path.
> I got the feeling that one is micro optimising then there's no need for it.
> 
> Rafael, have you tried any test/benchmark on your end?

Hi Emil,

Are you referring to checking whether the platform requested the fence fd or
not? If so, I was thinking about making it optional mainly to avoid
creating extra fds, but I didn't think it would have an impact on performance.

Anyway, I didn't run any benchmark on this.

Rafael