[Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS

Fri Aug 4 12:55:12 UTC 2017

On 3 August 2017 at 17:55, Marathe, Yogesh <yogesh.marathe at intel.com> wrote:
> Adding folks who were CCed for earlier versions.
>
> Hi Emil, few doubts and comments below.
>
>> -----Original Message-----
>> From: Tomasz Figa [mailto:tfiga at chromium.org]
>> Sent: Thursday, August 3, 2017 7:19 PM
>> To: Marathe, Yogesh <yogesh.marathe at intel.com>
>> Cc: Emil Velikov <emil.l.velikov at gmail.com>; Antognolli, Rafael
>> <rafael.antognolli at intel.com>; ML mesa-dev <mesa-
>> dev at lists.freedesktop.org>; Wu, Zhongmin <zhongmin.wu at intel.com>
>> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence
>> for Android OS
>>
>> On Thu, Aug 3, 2017 at 10:11 PM, Marathe, Yogesh
>> <yogesh.marathe at intel.com> wrote:
>> > Emil,
>> >
>> >> -----Original Message-----
>> >> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>> >> Sent: Thursday, August 3, 2017 4:06 PM
>> >> To: Marathe, Yogesh <yogesh.marathe at intel.com>
>> >> Cc: ML mesa-dev <mesa-dev at lists.freedesktop.org>; Wu, Zhongmin
>> >> <zhongmin.wu at intel.com>
>> >> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a
>> >> sync fence for Android OS
>> >>
>> >> On 2 August 2017 at 20:01,  <yogesh.marathe at intel.com> wrote:
>> >> > From: Zhongmin Wu <zhongmin.wu at intel.com>
>> >> >
>> >> > Before we queued the buffer with a invalid fence (-1), it causes
>> >> > benchmarks such as flatland to fail. This patch enables explicit
>> >> > sync feature on android.
>> >> >
>> >> > Now we get the out fence during the flushing buffer and then pass
>> >> > it to SurfaceFlinger in eglSwapbuffer function.
>> >> >
>> >> > v2: a) Also implement the fence in cancelBuffer.
>> >> >     b) The last sync fence is stored in drawable object
>> >> >        rather than brw context.
>> >> >     c) format clear.
>> >> >
>> >> > v3: a) Save the last fence fd in DRI Context object.
>> >> >     b) Return the last fence if the batch buffer is empty and
>> >> >        nothing to be flushed when _intel_batchbuffer_flush_fence
>> >> >     c) Add the new interface in vbtl to set the retrieve fence
>> >> >
>> >> > v3.1 a) close fd in the new vbtl interface on none Android platform
>> >> >
>> >> > v4: a) The last fence is saved in brw context.
>> >> >     b) The retrieve fd is for all the platform but not just Android
>> >> >     c) Add a uniform dri2 interface to initialize the surface.
>> >> >
>> >> > v4.1: a) make some changes of variable name.
>> >> >       b) the patch is broken into two patches.
>> >> >
>> >> > v4.2: a) Add a deinit interface for surface to clear the out fence
>> >> >
>> >> > v5: a) Add enable_out_fence to init, platform sets it true or
>> >> >        false
>> >> >     b) Change get fd to update fd and check for fence
>> >> >     c) Commit description updated
>> >> >
>> >> Seems like most suggestions in last version did not get addressed.
>> >> Please comment if you disagree (it can be that we've
>> >> misread/misremember
>> >> something) before posting another version.
>> >>
>> >> To reiterate:
>> >>  - add blank line between variable declarations and code
>
> Done.
>
>> >>  - use more consistent function names
>
> Do you want any specific functioned to be renamed?
>
s/dri2_surf_init/dri2_init_surface/
s/dri2_surf_deinit/dri2_fini_surface/
s/dri2_surface_set_out_fence/dri2_surface_set_out_fence_fd/

>> >>  - comment above queueBuffer needs fixing
>
> It reads as below now.
>    /* Queue the buffer with stored out fence fd. The ANativeWindow or buffer
>     * consumer may choose to wait for the fence to signal before accessing
>     * it. If fence fd value is -1, buffer can be accessed by consumer
>     * immediately. Consumer or application shouldn't rely on timestamp
>     * associated with fence if the fence fd is -1.
>     *
>     * Ownership of fd is transferred to consumer after queueBuffer and the
>     * consumer is responsible for closing it. Caller must not use the fd
>     * after passing it to queueBuffer.
>     */
>
> Looks ok?
>
A lot better than I was thinking.

>> >>  - version check (2+) the fence extension, calling .create_fence_fd()
>> >> only when
>> >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
>
> The check looks like below now, this is in dri2_surf_update_fence_fd() before create_fence_fd is called.
>
> if (dri2_surf->enable_out_fence && dri2_dpy->fence) {
>        if(__DRI_FENCE_CAP_NATIVE_FD | dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen)) {
>           //create_fence_fd call
>        }
> }
>
Close but no cigar.

if (dri2_surf->enable_out_fence && dri2_dpy->fence &&
    dri2_dpy->fence->base.version >= 2 && dri2_dpy->fence->get_capabilities) {

   if (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) &
__DRI_FENCE_CAP_NATIVE_FD) {
        //create_fence_fd call
   }
}

> Overall, if I further go ahead and check, actually get_capabilities() ultimately returns based on
> has_exec_fence which depends on  I915_PARAM_HAS_EXEC_FENCE. This is always set to
> true for i915 in kernel drv unless forced to false!! I'm not sure if that inner check of get_capabilities
> still makes sense. Isn't the first one sufficient?
>
Not sure what you mean with "first one", but consider the following example:
 - old kernel which does not support (or has force disabled)
I915_PARAM_HAS_EXEC_FENCE.
 - new userspace which unconditionally advertises the fence v2 extension
IIRC one may tweak that things to only conditionally advertise it, but
IMHO it's not worth the hassle.

Even then, Mesa can produce 20 DRI drivers (used by the EGL module) so
focusing on one doesn't quite work.

>> >>  - don't introduce unused variables (in make_current)
>
> Done.
>
>> >>  - the create fd for the old display surface (in make_current) seems
>> >> bogus
>
> Done.
>
Did you drop it all together or changed to use some other surface?
Would be nice to hear the reason why it was added - perhaps I'm
missing something.

I think that we want a fence/fd for the new draw surface. Since
otherwise one won't get created up until the first SwapBuffers call.

>> >>
>> >
>> > I think the patch has gone through headline changes, so it's bit
>> > difficult to track back. I tried to address Rafael's comments (latest
>> > before v5) assuming others were taken care of, after
>> > 4 versions. Doesn’t seem to be the case. No problem, thanks for
>> > re-iterating.  I'll work on fixing these and discuss as required.
>> >
>> >> Also, please change the commit summary message to reflect reality.
>> >> I'm thinking of the following, although you can come with something better.
>> >>
>> >> "egl: allow creation of per surface out fence
>> >>
>> >> Add plumbing to allow creation of per display surface out fence.
>> >>
>> >> Currently enabled only on Android, since the system expects a valid
>> >> fd in ANativeWindow::{queue,cancel}Buffer.
>> >> Without it tools such as flatland will fail, since they cannot get
>> >> the timestamp as we currently pass an fd of -1."
>> >>
>
> Done.
>
> I have taken care of things you've mentioned, so will push v6 based on answers
> above.
>
Please cover the make_current question before doing forward.

Thanks
Emil