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

[Wu, Zhongmin]

Hi Tomasz

Thanks,  but I am afraid we have to use the last fence from the last buffer flushing , According to my understanding:

If the glFlush was called before swapbuffer ,    there would be no new fence after that since the batch buffer may be flushed out, no batch flushing, no fence generated.

====
You can check the function "_intel_batchbuffer_flush_fence",  at the very beginning, it will check the content of the buffer, if it is empty, it will return and do nothing.
=====

So, as the same reason,  it may not work if we use dri2_dpy->fence->create_fence_fd,   the process is below.

Swapbuffer (==> flush batch buffer) ==>  create_fence_fd(===> flush batch buffer and get fd)

You can see, the second buffer flushing may get nothing (not to mention  the first buffer flushing may get nothing either)

-----Original Message-----
From: Tomasz Figa [mailto:tfiga at chromium.org] 
Sent: Wednesday, July 12, 2017 19:24 
To: Wu, Zhongmin <zhongmin.wu at intel.com>
Cc: Emil Velikov <emil.l.velikov at gmail.com>; Liu, Zhiquan <zhiquan.liu at intel.com>; ML mesa-dev <mesa-dev at lists.freedesktop.org>; Chad Versace <chadversary at chromium.org>; Eric Engestrom <eric at engestrom.ch>; Marathe, Yogesh <yogesh.marathe at intel.com>; Rob Clark <robclark at freedesktop.org>; Kenneth Graunke <kenneth at whitecape.org>; Widawsky, Benjamin <benjamin.widawsky at intel.com>; Kondapally, Kalyan <kalyan.kondapally at intel.com>; 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 Zhongmin,

On Wed, Jul 12, 2017 at 9:55 AM, Wu, Zhongmin <zhongmin.wu at intel.com> wrote:
> Thanks very much for Emil and Tomasz's suggestions.
>
> We can see that the out fence needed in queue buffer is get from batch buffer flushing.
>
> Now we  may flush bath buffer at below places:
>
> 1.   Create sync     ===> flush buffer and get out fence from driver
> 2.    Do glFLush()   ====>    flush buffer  ( not ask for out fence from driver)
> 3.    swapbuffer     ====>  flush buffer ( not ask for  out fence from drvier,   queue buffer with fd -1 )
> ...
>
>
> So,   in my opinion,  we don't need to get the out fence with help of other paths in swapbuffer. we just need to save the last out fence in the latest batch buffer flushing, and give it to Android with queue buffer in each swapbuffer.
>
> ( for example,   if glflush is just  called before swapbuffer,   the following buffer flushing will returned directly as the batch buffer is empty now, we can use the out fence got in glflush, and using create sync API may still not get the fence. )

Do we really need to use the same fence? If glFlush() was called just before eglSwapBuffers(), there were no commands inserted in between and the new fence would just be inserted directly after the one used for glFlush(). I don't think it's an issue. (And the client is actually doing something strange, because there is no need to call
glFlush() directly before eglSwapBuffers().)

>
> But you are right, we need to change the method in the patch, the patch is ill-considered.  As Emil said,  the canclebuffer is missed.
> We can't save the latest fence fd in the context object, as the context is reset yet before calling canclebuffer.  Then it is not good to add the call back in __DRI2flushExtensionRec,   We do need a better design for this.

How about my suggestion to use the DRI2 flush extension (dri2_dpy->fence->create_fence_fd)? I think the following in droid_swap_buffers could work:

   if (dri2_dpy->fence && dri2_dpy->fence->create_fence_fd)
      fence_fd = dri2_dpy->fence->create_fence_fd(ctx, -1);

I think you might also need to bypass flush drawable, as AFAICT it would defeat the purpose of out-fence by waiting synchronously for the GPU to finish.

Best regards,
Tomasz

>
> -----Original Message-----
> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
> Sent: Wednesday, July 12, 2017 1:41
> To: Tomasz Figa <tfiga at chromium.org>
> Cc: Wu, Zhongmin <zhongmin.wu at intel.com>; Marathe, Yogesh 
> <yogesh.marathe 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>; 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>; Kristian H . Kristensen 
> <hoegsberg at chromium.org>; Chad Versace <chadversary at chromium.org>; 
> dbehr at chromium.org
> Subject: Re: [Mesa-dev] [EGL android: accquire fence implementation] 
> i965: Queue the buffer with a sync fence for Android OS
>
> On 11 July 2017 at 16:23, 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.
>>
> Right, I'm not trying to say otherwise.
>
> The strange part, IMHO, is that now flatland has a hard requirement on both fences, where the [developer-side of the] documentation does not say anything about this.
> This sounds a bit backwards. I believe documentation update is in order?
>
> FWIW I was under the impression that EGL_ANDROID_native_fence_sync can be used in flatland. Although as Rob mentioned... not sure if the extension is available since the EGL meta seems to block/strip it out.
>
>
>> 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?
>>
> Reusing existing API would be ideal.
>
> If not, Zhongmin/Yogesh please note:
>  - when extending the interface, the version number must be bumped
>  - user should check the version and the function pointer prior to 
> use, falling back to the old scheme
>  - get_retrive_fd [barring the typo - retrieve], should have at least 
> the fd ownership documented
>
> Thanks
> Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev