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

[Tomasz Figa]

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.

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

Best regards,
Tomasz