[Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS

[Emil Velikov]

On 10 July 2017 at 15:26, Marathe, Yogesh <yogesh.marathe at intel.com> wrote:
> Hello Emil, My two cents since I too spent some time on this.
>
>> -----Original Message-----
>> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf
>> Of Emil Velikov
>> Sent: Monday, July 10, 2017 4:41 PM
>> To: Wu, Zhongmin <zhongmin.wu at intel.com>
>> Cc: Widawsky, Benjamin <benjamin.widawsky at intel.com>; Liu, Zhiquan
>> <zhiquan.liu at intel.com>; Eric Engestrom <eric at engestrom.ch>; Rob Clark
>> <robclark at freedesktop.org>; 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>; Timothy Arceri <tarceri at itsqueeze.com>; Chuanbo
>> Weng <chuanbo.weng at intel.com>
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
>> Queue the buffer with a sync fence for Android OS
>>
>> Hi Zhongmin Wu,
>>
>> Above all, a bit of a disclaimer: I'm by no means an expert on the topic so take
>> the following with a pinch of salt.
>>
>> On 10 July 2017 at 03:11, Zhongmin Wu <zhongmin.wu at intel.com> 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.
>> >
>> Having a closer look it seems that the issue can be summarised as follows:
>>  - flatland intercepts/interacts ANativeWindow::{de,}queueBuffer (how about
>> ANativeWindow::cancelBuffer?)
>>  - the program expects that a sync fd is available for both dequeue and queue
>>
>> At the same time:
>>  - the ANativeWindow documentation does _not_ state such requirement
>>  - even if it did, that will be somewhat wrong, since
>> ANativeWindow::queueBuffer is called by eglSwapBuffers() Where the latter
>> documentation clearly states - "... performs an implicit flush ... glFlush ...
>> vgFlush"
>>
>> My take is that if flatland/Android framework does want an explicit sync point it
>> should insert one with the EGL API.
>> There could be alternative solutions, but the proposed patch seems wrong
>> IMHO.
>
> In fact, I could work this around in producer  (Surface::queueBuffer) by ignoring the (-1)
> passed and by creating a sync using egl APIs. I see two problems with that.
>
> - Before getting a fd using eglDupNativeFenceFDANDROID(), you need a glFlush(),
>    this costs additional cycles for each queueBuffer transaction on each BufferItem and
>    I believe fd is also signaled due to this. (so I don’t know what we'll get by waiting on
>    that fd on consumer side).
> - AFAIK, the whole idea of explicit sync revolves around being able to pass fds created
>   by driver between processes and this one breaks that chain. If we work this around in
>   upper layers, explicit sync feature will have to be fixed for every other lib that may use
>   lib mesa underneath.
>
> For these reasons, I still believe we should fix it here. Of course, you and Rob have very
> valid points on cancelBuffer and about not breaking gallium respectively, those need to
> be taken care of.
>
What I'm saying is - seems like the app/framework does something silly
or at least undocumented.
Fixing things in Mesa may be the right thing to do, but without more
information, its everyone's guess who's got it wrong.

As Rob asked earlier - can we get an a simple test case or some pseudo
code illustrating the whole thing?

Thanks
Emil