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

Fri Aug 4 03:21:40 UTC 2017

Tomasz,

> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga at chromium.org]
> Sent: Friday, August 4, 2017 6:52 AM
> To: Marathe, Yogesh <yogesh.marathe at intel.com>
> Cc: Emil Velikov <emil.l.velikov at gmail.com>; Gao, Shuo
> <shuo.gao at intel.com>; Liu, Zhiquan <zhiquan.liu at intel.com>; Daniel Stone
> <daniels at collabora.com>; Nicolai Hähnle <nicolai.haehnle at amd.com>;
> Antognolli, Rafael <rafael.antognolli at intel.com>; Eric Engestrom
> <eric at engestrom.ch>; Wu, Zhongmin <zhongmin.wu at intel.com>; Kenneth
> Graunke <kenneth at whitecape.org>; Kondapally, Kalyan
> <kalyan.kondapally at intel.com>; Rainer Hochecker <fernetmenta at online.de>;
> ML mesa-dev <mesa-dev at lists.freedesktop.org>; Timothy Arceri
> <tarceri at itsqueeze.com>; Varad Gautam <varad.gautam at collabora.com>
> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence
> for Android OS
> 
> Hi Yogesh,
> 
> On Fri, Aug 4, 2017 at 1:55 AM, 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?
> >
> >> >>  - 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?
> 
> Sounds okay to me, but let's wait for Emil's ack.
> 
> >
> >> >>  - 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)) {
> 
> This doesn't make any sense, because non-zero OR whatever is always true. Did
> you by any chance meant to use AND instead? Also please just extend the
> condition of the first if, instead of nesting another under it for no reason.
> 

Right. It must be '&', thanks for pointing out. 

On the nesting, I want to check dri2_dpy->fence is valid first before dri2_dpy->fence->(anything())
can be called, so I believe that nesting can still be there. Rafael had that review comment.
Do you still want to combine conditions in a single 'if'?

> >           //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?
> 
> i965 is not the only driver in Mesa...
> 
> Moreover, there are users of kernels older that current upstream release, which
> might not have support for I915_PARAM_HAS_EXEC_FENCE.
> 

Ok, understood, then we have to keep it.

> Best regards,
> Tomasz