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

[Marathe, Yogesh]

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?

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

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

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?

> >>  - don't introduce unused variables (in make_current)

Done.

> >>  - the create fd for the old display surface (in make_current) seems
> >> bogus

Done.

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

> >
> > Ok. This sounds good to me, will also check commit message while pushing
> next version.
> 
> Also please don't drop people from CC. Thanks in advance.
> 
> Best regards,
> Tomasz