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

[Emil Velikov]

On 2 August 2017 at 18:29, Rafael Antognolli
<rafael.antognolli at intel.com> wrote:
> 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.
>
Sure, fds are finite resource, but we're "wasting" one per surface ;-)

Nobody mentioned why they'd want to avoid the fence, so I've naively
assumed that it was performance.
Now I see your concern, thanks for the clarification.

-Emil