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

Tue Jul 11 15:23:37 UTC 2017

Hi Zhongmin,

On Tue, Jul 11, 2017 at 11:02 AM, Wu, Zhongmin <zhongmin.wu at intel.com> wrote:
> By the way,
>
> For cancelBuffer, sorry I forget such function, thanks for notice. It should also pass the same fence fd as the queuebuffer.
>
> And Yogesh, you mentioned the gallium,   is it another platform supported by mesa ?  I am sorry I have no idea about this,  could you please help to check this ?
>
> I think we can co-work with mesa team to work out an acceptable fix which can meet the requirement of Android without any break on other platforms.

One thing needs clarifying here. Release fences from EGL are _not_ a
requirement. It is an optional feature. Android compliance suites pass
fully without Android sync fence support in Mesa at all.

Other than that, it's been taking long enough and I agree that we
should finally wire both acquire and release fence support in EGL and
related drivers. Otherwise we can forget about getting good user
experience on Android.

On a technical side, the EGL change needs to take into account that
not all drivers support fences and so it needs to have a fallback to
old behavior for those which don't.

Other than that, correct me if I'm wrong, but could we just use the
DRI2 fence extension instead of adding some custom callbacks? I can
see that a normal client request to create a sync fence would end up
calling dri2_dpy->fence->create_fence_fd() (if it's present) [1].
Could we do the same?

[1] https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/egl_dri2.c#n2772

+ Kristian, Chad and Dominik who have been looking into sync fence
integration with our EGL drivers.

Best regards,
Tomasz

>
> -----Original Message-----
> From: Wu, Zhongmin
> Sent: Tuesday, July 11, 2017 8:40
> To: 'Emil Velikov' <emil.l.velikov at gmail.com>; Marathe, Yogesh <yogesh.marathe 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 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