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

[Emil Velikov]

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?

Thanks
Emil