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

[Wu, Zhongmin]

Hi Emil and Yogesh
Thank you for your comments,  and thanks Yogesh for giving the detailed explanations 


And according to the document of Android below (https://source.android.com/devices/graphics/arch-bq-gralloc):

Recent Android devices support the sync framework, which enables the system to do nifty things when combined with hardware components that can manipulate graphics data asynchronously. For example, a producer can submit a series of OpenGL ES drawing commands and then enqueue the output buffer before rendering completes. The buffer is accompanied by a fence that signals when the contents are ready.


I think the things is very clear, that is if the rendering is completed already when we call queueBuffer() in mesa ?   If not, we should queue the buffer with a fence which will be signaled when the buffer is ready.



-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov at gmail.com] 
Sent: Tuesday, July 11, 2017 1:18 
To: Marathe, Yogesh <yogesh.marathe at intel.com>
Cc: Wu, Zhongmin <zhongmin.wu at intel.com>; 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

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