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

Tue Jul 11 15:25:02 UTC 2017

-chuanbo.weng at intel.com (bounces)
+"Gao, Shuo" <shuo.gao at intel.com> (forgot to add originally, sorry)

On Wed, Jul 12, 2017 at 12:23 AM, Tomasz Figa <tfiga at chromium.org> wrote:
> 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