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

[Tomasz Figa]

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
>>  - use more consistent function names
>>  - comment above queueBuffer needs fixing
>>  - version check (2+) the fence extension, calling .create_fence_fd() only when
>> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
>>  - don't introduce unused variables (in make_current)
>>  - the create fd for the old display surface (in make_current) seems bogus
>>
>
> 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."
>>
>
> 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