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

Tue Jul 11 17:18:38 UTC 2017

> -----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.

> >> 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.

> 
> 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