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

[Tomasz Figa]

Hi Yogesh,

On Wed, Jul 12, 2017 at 2:18 AM, Marathe, Yogesh
<yogesh.marathe at intel.com> wrote:
>> -----Original Message-----
>> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf
>> Of Tomasz Figa
>> Sent: Tuesday, July 11, 2017 9:59 PM
>> To: Marathe, Yogesh <yogesh.marathe at intel.com>
>> Cc: Gao, Shuo <shuo.gao at intel.com>; Liu, Zhiquan <zhiquan.liu at intel.com>;
>> Kondapally, Kalyan <kalyan.kondapally at intel.com>; Chad Versace
>> <chadversary at chromium.org>; Eric Engestrom <eric at engestrom.ch>; Emil
>> Velikov <emil.l.velikov at gmail.com>; Wu, Zhongmin
>> <zhongmin.wu at intel.com>; Kenneth Graunke <kenneth at whitecape.org>; Rob
>> Clark <robclark at freedesktop.org>; Widawsky, Benjamin
>> <benjamin.widawsky at intel.com>; ML mesa-dev <mesa-
>> dev at lists.freedesktop.org>; Kristian H . Kristensen
>> <hoegsberg at chromium.org>; Timothy Arceri <tarceri at itsqueeze.com>
>> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] i965:
>> Queue the buffer with a sync fence for Android OS
>>
>> Hi Yogesh,
>>
>> On Wed, Jul 12, 2017 at 1:09 AM, Marathe, Yogesh
>> <yogesh.marathe at intel.com> wrote:
>> > Hello Figa, Few caveats on that approach
>>
>> (I'm Tomasz, by the way)
>
> My bad Tomasz.

No problem. :)

>
>> >> Queue the buffer with a sync fence for Android OS
>> >>
>> >> Now for real, sorry guys... (Seriously gmail why you do this to me.)
>> >>
>> >> -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?
>> >
>> > May be here we need to look at complete sequence eglCreateSyncKHR ->
>> > _eglCreateSync -> dri2_create_sync, as eglCreateSyncKHR  seems entry
>> > point and its doing attrib/type checks before reaching
>> > dri2_create_sync(). Also, dri2_create_sync is static, can't be called
>> > directly, there needs to be an entry point / interface.
>> >
>>
>> I think you misunderstood my suggestion. I didn't mean dri2_create_sync(), but
>> rather using the DRI2 fence extension directly, just as dri2_create_sync() does.
>> You can access dri2_egl_display from Android EGL code and in fact it already
>> uses other extensions like this.
>
> Sorry, I'm still searching around. To try this out, can you please specify, which
> functions did you mean by DRI2 fence extension? An example within EGL code
> would help. Please note we need to call these from platform_android.c finally.

I meant using dri2_dpy->fence->create_fence_fd() directly, if the
available (i.e. dri2_dpy->fence and dri2_dpy->fence->create_fence_fd
are non-NULL) or falling back to -1 if not.

Best regards,
Tomasz

>
>>
>> Best regards,
>> Tomasz
>>
>> >> >
>> >> > [1]
>> >> > https://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/eg
>> >> > l_d
>> >> > ri2.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
>> >> _______________________________________________
>> >> mesa-dev mailing list
>> >> mesa-dev at lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev